Comment 9 Samuel Mannehed 2025-03-31 10:25:56 CEST

Fixed now. Some cleanup was done and FIXME's were fixed as well. Tested on Fedora 41.

> MUST
> 
> * It must be safe to delete any valid hiveconf folder
The case found in migrate_conf no longer crashes.

Comment 10 Alexander Zeijlon 2025-03-31 14:19:20 CEST

I found a scenario that reproduces the stack trace in comment 0:

> [ /first]
> data1=1
> 
> [/first/second]
> data2=2

Note the leading space before the path on the parent folder definition.

Comment 11 Alexander Zeijlon 2025-03-31 14:34:16 CEST

Verify:
=======

Hivetool no longer crashes when a slightly malformed folder structure is removed from the hive.

The behavior, however, is still a bit weird. When recursively removing the folder structure (see comment 10) by calling delete("/first"), everything but the root folder, " /first" is removed.

This makes it look like we are inconsistent with when to strip white spaces from folder names. Clearly, we are recursively finding "/second" relative to "/first" and "data1=1" relative to " /first". But not " /first" itself.

Comment 12 Alexander Zeijlon 2025-03-31 14:36:49 CEST

I also found that we are not removing comments beginning with ";".

Comment 13 Samuel Mannehed 2025-03-31 15:55:52 CEST

(In reply to Alexander Zeijlon from comment #12)
> I also found that we are not removing comments beginning with ";".

True, now fixed as part of bug 8557.
https://github.com/astrand/hiveconf/blob/master/doc/hive-file-syntax.txt

Comment 14 Samuel Mannehed 2025-03-31 16:08:30 CEST

(In reply to Alexander Zeijlon from comment #10)
> I found a scenario that reproduces the stack trace in comment 0:
> 
> > [ /first]
> > data1=1
> > 
> > [/first/second]
> > data2=2
> 
> Note the leading space before the path on the parent folder definition.

This is the same as the following configuration:
> [ /otherfolder]
> data1=1
> 
> [/first/second]
> data2=2
There is no strict requirement that a top-level folder must exist for a subfolder to be correct.


(In reply to Alexander Zeijlon from comment #11)
> The behavior, however, is still a bit weird. When recursively removing the
> folder structure (see comment 10) by calling delete("/first"), everything
> but the root folder, " /first" is removed.
No, the result of delete("/first", recursive=True) is:
> [ /first]
> data1=1
I.e, only the folder starting with "/first" is removed. I don't think there is anything wrong with that.

Comment 15 Alexander Zeijlon 2025-03-31 17:23:03 CEST

(In reply to Samuel Mannehed from comment #14)
> No, the result of delete("/first", recursive=True) is:
> > [ /first]
> > data1=1
> I.e, only the folder starting with "/first" is removed. I don't think there
> is anything wrong with that.

I realized what I did differently here. I tried looping over all folders in "/" and then deleted them recursively like so:
> for folder in hive.get_folders('/')[1:]:
>     hive.delete(f'/{folder}', recursive=True)

The space is interpreted as a path component, and thus calling hive.delete("/ ", recursive=True) resulted in "data1=1" being removed while "[ /first]" persists in the file.

I'm not sure if his is correct behavior, but at least I think it is not a part of this bug.

Comment 16 Alexander Zeijlon 2025-04-01 14:05:35 CEST

The stack trace in comment 0, occurs when hivetool tries to remove a folder that does no longer exist in the file on disk [1], so in this scenario:

[1] This is not the same as the scenario I assumed had occurred in comment 10.

somefile.hconf:
> [/some]
> [/some/path]
> data = 1
Load the file in python:
> hive = hiveconf.open_hive("somefile.hconf")
Modify somefile.hconf:
> [/some/path]
> data = 1
Try to delete the folder "/some"
> hive.delete("/some", recursive=True)

Doing this reproduces the stack trace, before the fix. After the fix, the run results in all folders and subfolders of "/some" being removed.

Comment 17 Alexander Zeijlon 2025-04-01 14:07:13 CEST

Looks good to me, closing!