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

From: Patrick Steinhardt <ps@pks.im>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org,
	Christian Couder <christian.couder@gmail.com>,
	Jonathan Tan <jonathantanmy@google.com>,
	Elijah Newren <newren@gmail.com>
Subject: Re: [PATCH v3 1/7] fetch: increase test coverage of fetches
Date: Thu, 3 Mar 2022 07:51:09 +0100	[thread overview]
Message-ID: <YiBlXSrSy1iajd9r@ncase> (raw)
In-Reply-To: <xmqqmti7bkf2.fsf@gitster.g>

[-- Attachment #1: Type: text/plain, Size: 2006 bytes --]

On Wed, Mar 02, 2022 at 10:49:05PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> >> >> +				# 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.

Fair enough, thanks for clarifying the steps. Does it make sense to also
change indentantion of the heredocs as per your review of v2 or should I
just keep that as-is now?

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]