Re: [PATCH] fast-import: avoid making replace refs point to themselves

From: Elijah Newren <newren@gmail.com>
Date: Nov 15, 2024, 03:41 UTC
Message-ID: <CABPp-BGOLfeUn6H3cdeMf5tZ1gswoaR51J+PDhMB4akR3jG+LA@mail.gmail.com>
In-Reply-To: <xmqqv7wp483v.fsf@gitster.g>

On Thu, Nov 14, 2024 at 5:08 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 6 quoted lines

>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > I am not sure if a warning is even warranted.  If you decide to
> > replace an object A with the same object A, the result ought to be a
> > no-op.

Do you mean that the result ought to be that there is no replacement? If so, I agree, but that's not always equivalent to a no-op...

Show 12 quoted lines

> >  I wonder if it is makes more sense to
> >
> >  (1) do this unconditionally and silently, and
> >  (2) when we prepare the replace_map, ignore self-referencing ones.
> >
> > instead.  If (2) makes sense entirely depends on the answer of an
> > earlier question (i.e. "is there a reason why self-reference is more
> > common than general loop?").
>
> Forgot to add.  (1) could be done even at a lower layer, i.e. the
> ref API could be told to reject such a replace ref creation/update
> that maps an object name to itself.

Perhaps rejecting the creation or update of a replace ref would make sense at a lower level, but a no-op isn't what I want here -- it'd just result in a different bug. In particular, note that my patch modifies fast-import to issue a delete where it would otherwise issue an update instead of merely rejecting the update. If the repository had a value for the replace ref before fast-import was run, and the replace ref was explicitly named in the fast-import stream, I don't want the replace ref to be left with a pre-fast-import value.