Re: [PATCH] replace: fix replacing object with itself

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 16, 2014, 18:59 UTC
Message-ID: <xmqqr3x3ynrm.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CAP8UFD3dZMmGJp1p=ttVUoUkiwh7oYL3gqgsGZ+D8Bd16FiKjQ@mail.gmail.com>

Christian Couder <christian.couder@gmail.com> writes:

Show 14 quoted lines

>> The patch is not wrong per-se, but I wonder how useful this "do not
>> replace itself but all other forms of loops are not checked at all"
>> would be in practice.  If your user did this:
>>
>>         git replace A B ;# pretend as if what is in B is in A
>>         git replace B C ;# pretend as if what is in C is in B
>>         git replace C A ;# pretend as if we have loop
>>         git log C
>>
>> she would not be helped with this patch at all, no?
>
> Yeah, but such loops are much less likely mistakes and are more
> difficult to detect, so I think this patch is at least a good first
> step.

More difficult to notice by humans, hence deserves more help from the tool.

When these two are both mistakes, which one do you think is easier to notice (thusly unlikely to happen)?

	git replace A A
        git replace C A

Of course, the former is immediately obvious to the human who is typing that it is a typo.

The latter would be harder to spot as a mistake as the invoker has to know that there is an existing "pretend as if what is in A is in C" aka "replace A C" done earlier in the repository.

And the cost of detecting such a possible "too deep replace chain" (do not call that a loop---the runtime barfs if you have too deep a replace chain without any loop) wouldn't be too high. You only need to look at existing refs/replace/* refs, their contents, and the two object names that form the proposed new replacement <old,new>.

Even a kludge (read: I am not suggesting that you solve it this way) like "first install the replacement as proposed, then enumerate all the replacement refs and their values and try to see if the runtime check would barf, and if it would, fail the operation and revert the change" would catch a mistake to cause the repository in trouble.