Re: [PATCH v3 1/7] fetch: increase test coverage of fetches

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 3, 2022, 06:49 UTC
Message-ID: <xmqqmti7bkf2.fsf@gitster.g>
In-Reply-To: <YiBjjV6Kw2KHsMxq@ncase>

Patrick Steinhardt <ps@pks.im> writes:

Show 28 quoted lines

>> >> +				# would cause us to die immediately.
>> >> +				git update-ref refs/tags/tag1/nested $B
>> >> +				exit \$!
>> >> +			fi
>> >> +		done
>> >> +	EOF
>> >
>> > I think I've reviewed the previous round of these patches in
>> > detail.  I by mistake sent a comment for this step in v2, but I
>> > think the same puzzlement exists in this round, too.
>> 
>> Namely:
>> 
>> I have been wondering if we need to do this from the hook?  If we
>> have this ref before we start "fetch", would it have the same
>> effect, or "fetch" notices that this interfering ref exists and
>> removes it to make room for storing refs/tags/tag1, making the whole
>> thing fail to fail?
>> 
>> > +				exit \$!
>> 
>> In any case, "exit 0" or "exit \$?" would be understandable, but
>> exit with "$!", which is ...?  The process ID of the most recent
>> background command?  Puzzled.
>
> Oof, this was supposed to be `exit \$?`, thanks for catching this. But
> your above comment is right: we can indeed just create the D/F conflict
> outside of the hook and thus avoid the hook script altogether. Thanks!

I see.

As that shell does not send anything to background, at the point of the reference $! would yield an empty string, and "exit" is equivalent to "exit $?", it is doing the right thing, I presume.

The topic has been in 'next' for a while, so if you are inclined to fix it up, please send an incremental patch. If you do "exit" it would be a one-liner change, or if you use a different "cause D/F conflict outside the hook" approach, the change may become a bit more involved.

Thanks.