Re: [PATCH] advice: improve hint for diverging branches.

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 5, 2023, 23:20 UTC
Message-ID: <xmqqh6o8p5pl.fsf@gitster.g>
In-Reply-To: <pull.1570.git.git.1693861162353.gitgitgadget@gmail.com>

"Konstantin Pereiaslov via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 7 quoted lines

> From: Konstantin Pereiaslov <perk11@perk11.info>
>
> Added a description of what the offered options will do and for pull
> also offered a 3rd option during a pull - a hard reset.
> This option should be helpful for the new users that accidentally
> committed into the wrong branch which is a scenario I saw very
> often.

cf. Documentation/SubmittingPatches:[[describe-changes]]

> The resulting tooltip looks like this for pull:
>
> hint: Diverging branches can't be fast-forwarded.
> Consider the following options:

We do not give "hint:" prefix to this line???

Show 6 quoted lines

> hint:
> hint: To merge remote changes into your branch:
> hint:   git merge --no-ff
> hint:
> hint: To apply your changes on top of remote changes:
> hint:   git rebase

Hmph, "apply" -> "replay" perhaps?

> hint: To discard your local changes and apply the remote changes:

Here "apply" is definitely a misnomer. Nothing is applied; you just discard your work and adopt (or "accept") the state of the remote as a whole.

> hint:   git reset --hard refs/remotes/upstream/branch-name
> hint:
> hint: Disable this message with "git config advice.diverging false"

OK.

Overall, "... looks like this" should be shown a bit indented so that the example stands out from the text that explains the example.

> There is some danger because it's semi-destructive, but so are
> other options offered if user doesn't know the commands to
> revert back.

Sorry, but I do not quite understand what you want to say here.

Show 8 quoted lines

> @@ -1112,8 +1126,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>  
>  	/* ff-only takes precedence over rebase */
>  	if (opt_ff && !strcmp(opt_ff, "--ff-only")) {
> -		if (divergent)
> -			die_ff_impossible();
> +		if (divergent) {
> +			const char* pull_branch_spec = get_pull_branch(repo, *refspecs);

In this codebase, asterisk sticks to the variable/function identifier, not types.

But more importantly, what guarantees your recomputation using '*refspecs' here will match the result of the logic that computed 'divergent', which certainly would have already known what commit we tried to fast-forward our branch to, and where that commit came from? We shouldn't be computing the same thing twice, and in different ways; that is a sure way to introduce inconsistent results.

Show 7 quoted lines

> +			die_ff_impossible_during_pull(pull_branch_spec);
> +		}
>  		opt_rebase = REBASE_FALSE;
>  	}
>  	/* If no action specified and we can't fast forward, then warn. */
>
> base-commit: d814540bb75bbd2257f9a6bf59661a84fe8cf3cf

Thanks.