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

From: Junio C Hamano <gitster@pobox.com>
Date: May 28, 2024, 18:26 UTC
Message-ID: <xmqqfru14wtp.fsf@gitster.g>
In-Reply-To: <xmqqo78p4xam.fsf@gitster.g>

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

Show 10 quoted lines

> 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.