Re: [PATCH] prune: mark rebase autostash and orig-head as reachable

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Junio

On 08/02/2024 18:06, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> Rebase records the oid of HEAD before rebasing and the commit created by
>> "--autostash" in files in the rebase state directory. This means that
>> the autostash commit is never reachable from any ref or reflog and when
>> rebasing a detached HEAD the original HEAD can become unreachable if the
>> user expires HEAD's the reflog while the rebase is running. Fix this by
>> reading the relevant files when marking reachable commits.
> 
> I do not like this kind of special casing in general, but because
> these are our tools' droppings, I am OK to grandfather them in, as
> long as we promise ourselves that we will not add more of these
> ad-hoc "text files" that record object names, loss of which affects
> correctness.  They should, like "git bisect", be using proper
> references to protect these objects instead, of course.

We should definitely do that for future commands

> I agree with you that we might want to add pseudorefs as a starting
> points of reachability traversal, but I suspect it would add
> unnecessary complexity we would rather not want to deal with.
> 
> For example, not GC'ing what is pointed at by lines in FETCH_HEAD is
> OK.  Excluding those objects that are only reachable from an object
> mentioned by a pseudoref, when a new "git fetch" is negotiating with
> a remote what objects need to be sent here, might be disastrous, as
> the pseudoref that said "this object is here and you can safely
> consider everything reachable from it is" will be short-lived and
> can go away anytime, and an auto-gc kicking in at a wrong time ...

I can see that including pseudorefs when "git fetch" is negotiating 
could be problematic but does it use mark_reachable_objects()? Maybe I'm 
missing something as I've only done a quick grep but it only seems to be 
called from builtin/prune.c and builtin/repack.c and prior to 4421474e06 
(Move traversal of reachable objects into a separate library., 
2007-01-06) it seems to have been a static method in builtin-prune.c

Best Wishes

Phillip