Re: [PATCH v3 00/12] refs: ref storage format migrations

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

> Patrick Steinhardt <ps@pks.im> writes:
>
>>   - Swapped out calls to `remove_path()` to `unlink()`. We do not want
>>     to walk up and remove empty parent directories, even though this is
>>     harmless in practice.
> ...
> Let's find out why this is needed in [10/12].  It may just be a
> simple matter of "let's not bother removing directories as we remove
> loose ref files one by one---we know the whole hierarchy can be
> removed after we are done", in which case I do think it is nicer.

Ah, it is not something as sophisticated like that.  It simply is
wrong to use remove_path() to remove files used by files_backend, as
the helper is designed to work on working tree files.

The reason it is wrong is because "now I removed this path, if the
containing directory has become empty, I need to remove that
directory, and I need to go up recursively doing that" has to stop
somewhere, and for remove_path() that is on the working tree side
the natural place to stop is at the root of the working tree,
i.e. above ".git/" directory.  Of course, when removing extra
directories above a loose ref file, the recursion must stop much
earlier than going up to ".git/", and try_remove_empty_parents()
in files-backend.c is the helper that was designed for the task.

Looking at the difference between the result of applying v2 and v3,
I think this "unlink" thing is only about removing root refs?  So I
agree that a simple unlink() is not just adequate but is absolutely
the right thing to do.  There is no reason for us to go up and remove
empty directories when we remove ORIG_HEAD or other stuff.

Thanks.