Re: [PATCH v2] push: don't fetch commit object when checking existence

From: Junio C Hamano <gitster@pobox.com>
Date: May 22, 2024, 21:58 UTC
Message-ID: <xmqqbk4x1pc4.fsf@gitster.g>
In-Reply-To: <8f2ebf1b-050f-476a-92d4-dfb06ad04f8d@compton.nu>

Tom Hughes <tom@compton.nu> writes:

Show 7 quoted lines

>>> +	test_must_fail git -C client push 2>err &&
>> We try to overwrite it.  We expect it to fail with "not a fast
>> forward".
>
> Well that is what it would fail with at the moment, but it's not
> what would happen with a non-partial clone - a non-partial clone
> would fail with "fetch first" instead.

Oh, don't get me wrong. I wasn't trying to split hairs between the two error modes and their phrasing. The "fetch-first" from set_ref_status_for_push() is done before we even initiate the transfer to stop the operation, with a cheap check, that will eventually lead to "not a fast forward" error. IOW, in my mind, they are the same errors, just diagnosed at two different places in the code and their messages phrased differently.

> So here we are testing that it's a "fetch first" and rather
> than "not a fast forward".

I think that is being overly specific, but that is fine. As I said, to the end users, these two errors mean the same thing (they would need to fetch first and then integrate their changes before pushing it out again), so it is plausible that we may in the future decide that we want to use the same message. When it happens, this test must change, which may even be a good thing (it makes it clear what the fallout from such a change looks like).

Show 6 quoted lines

>>> +	git -C client rev-list --objects --missing=print "$COMMIT" >objects &&
>>> +	grep "^[?]$COMMIT" objects
>>> +'
>> OK.
>
> and also that it hasn't fetched the new commit.

Yes, and this is a good check that will stand the test of time, even across a change to rephrase the error message.

Thanks.