Re: [PATCH v3 2/2] push: fix --force-if-includes detached HEAD advice

[Junio C Hamano <gitster@pobox.com>]

Tyler Cipriani <tyler@tylercipriani.com> writes:

> When a --force-if-includes push is rejected due to a detached HEAD
> state where there is no per-branch reflog to consult, the advice is
> misleading:
>
>      ! [rejected] HEAD -> main (remote ref updated since checkout)
>     error: failed to push some refs to '<remote>'
>     hint: Updates were rejected because the tip of the remote-tracking
>     hint: branch has been updated since the last checkout. If you want
>     hint: to integrate the remote changes, use 'git pull' before
>     hint: pushing again. See the 'Note about fast-forwards' in 'git
>     hint: push --help' for details.
>
> But a `git pull` will not fix this rejection. What is required is either
>
> - Specify the expected remote tip with --force-with-lease=<ref>:<expect>
> - Ignore the error with --no-force-if-includes
>
> Add ref->unverifiable to differentiate between a detached HEAD rejection
> vs. a remote update rejection.

Makes sense.

> diff --git a/builtin/push.c b/builtin/push.c
> index 6021b71d66..9676c6241f 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -319,6 +319,12 @@ static const char message_advice_ref_needs_update[] =
>  	   "remote changes, use 'git pull' before pushing again.\n"
>  	   "See the 'Note about fast-forwards' in 'git push --help' for details.");
>  
> +static const char message_advice_ref_unverifiable[] =
> +	N_("Updates were rejected because the tip of the remote-tracking branch\n"
> +	   "cannot be checked against a detached HEAD. If you want to push anyway,\n"
> +	   "specify the expected value with '--force-with-lease=<ref>:<expect>'\n"
> +	   "or use '--no-force-if-includes' to skip this check.");

Good.

> +static void advise_ref_unverifiable(void)
> +{
> +	if (!advice_enabled(ADVICE_PUSH_REF_UNVERIFIABLE) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
> +		return;

Line that is over +100 column wide?

> +	advise(_(message_advice_ref_unverifiable));
> +}

This is a tangent, but on a separate thread we were talking about
consolidating a sequence

    if (advice_enabled(ADVICE_FOO))
	advise(_(message for FOO));

into

    advise_if_enabled(ADVICE_FOO, _(message for FOO));

This is an example of usage that falls outside of the pattern (not a
bad thing; just what those who advocate more use of advise_if_enabled()
need to be aware of).

> diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
> index 0c02151747..fe6af3f41c 100755
> --- a/t/t5533-push-cas.sh
> +++ b/t/t5533-push-cas.sh
> @@ -311,7 +311,8 @@ test_expect_success 'background updates to remote can be mitigated with "--force
>  		git switch main &&
>  		test_commit J &&
>  		git fetch --all &&
> -		test_must_fail git push --force-with-lease --force-if-includes --all
> +		test_must_fail git push --force-with-lease --force-if-includes --all 2>err &&
> +		test_grep "remote ref updated since checkout" err
>  	) &&
>  	git ls-remote dst refs/heads/main >actual.main &&
>  	git ls-remote dst refs/heads/branch >actual.branch &&
> @@ -457,7 +458,9 @@ test_expect_success '"--force-if-includes" should reject forced update from deta
>  		git reset --hard origin/main &&
>  		git switch -c newbranch origin/main &&
>  		git checkout HEAD^ &&
> -		test_must_fail git push --force-if-includes --force-with-lease origin HEAD:main
> +		test_must_fail git push --force-if-includes --force-with-lease origin HEAD:main 2>err &&
> +		test_grep "remote ref unverifiable" err &&
> +		test_grep "no-force-if-includes" err
>  	)
>  '

Great.