Re: [PATCH v2 2/3] builtin/rebase.c: Emit warning when rebasing without a forkpoint

[Phillip Wood <phillip.wood123@gmail.com>]

On 03/09/2023 05:50, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> If you rewind to lose commits from the branch you are (re)building
>> against, and what was rewound and discarded was part of the work you
>> are building, whether it is on a local branch or on a remote branch
>> that contains what you have already pushed, they will be discarded,
>> it is by design, and it is a known deficiency with the fork-point
>> heuristics.  How the fork-point heuristics breaks down is rather
>> well known ...
> 
> Another tangent, this time very closely related to this topic, is
> that it may be worth warning when the fork-point heuristics chooses
> the base commit that is different from the original upstream,
> regardless of how we ended up using fork-point heuristics.

I think that is a good idea and would help to mitigate the surprise that 
some users have expressed when --fork-point kicks and they didn't know 
about it. I think we may want to compare "branch_base" which holds the 
merge-base of HEAD and upstream with "restrict_revision" to decide when 
to warn.

Best Wishes

Phillip

> Experienced users may not be confused when the heuristics kicks in
> and when it does not (e.g. because they configured, because they
> used the "lazy" form, or because they gave "--fork-point" from the
> command line explicitly), but they still may get surprising results
> if a reflog entry chosen to be used as the base by the heuristics is
> not what they expected to be used, and can lose their work that way.
> Imagine that you pushed your work to the remote that is a shared
> repository, and then continued building on top of it, while others
> rewound the remote branch to eject your work, and your "git fetch"
> updated the remote-tracking branch.  You'll be pretty much in the
> same situation you had in your reproduction recipe that rewound your
> own local branch that you used to build your derived work on and
> would lose your work the same way, if you do not notice that the
> remote branch has been rewound (and the fork-point heuristics chose
> a "wrong" commit from the reflog of your remote-tracking branch.
> 
> Perhaps something along the lines of this (not even compile tested,
> though)...  It might even be useful to show a shortlog between the
> .restrict_revision and .upstream, which is the list of commits that
> is potentially lost, but that might turn out to be excessively loud
> and noisy in the workflow of those who do benefit from the
> fork-point heuristics because their project rewinds branches too
> often and too wildly for them to manually keep track of.  I dunno.
> 
> 
>   builtin/rebase.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git c/builtin/rebase.c w/builtin/rebase.c
> index 50cb85751f..432a97e205 100644
> --- c/builtin/rebase.c
> +++ w/builtin/rebase.c
> @@ -1721,9 +1721,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   	if (keep_base && options.reapply_cherry_picks)
>   		options.upstream = options.onto;
>   
> -	if (options.fork_point > 0)
> +	if (options.fork_point > 0) {
>   		options.restrict_revision =
>   			get_fork_point(options.upstream_name, options.orig_head);
> +		if (options.restrict_revision &&
> +		    options.restrict_revision != options.upstream)
> +			warning(_("fork-point heuristics using %s from the reflog of %s"),
> +				oid_to_hex(&options.restrict_revision->object.oid),
> +				options.upstream_name);
> +	}
>   
>   	if (repo_read_index(the_repository) < 0)
>   		die(_("could not read index"));
>