Re: [PATCH v2 2/3] builtin/rebase.c: Emit warning when rebasing without a forkpoint
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Sep 4, 2023, 10:16 UTC
- Message-ID
- <d9710161-ddb8-4b0a-9729-6b54cd56427d@gmail.com>
- In-Reply-To
- <xmqqlednuagl.fsf@gitster.g>
On 03/09/2023 05:50, Junio C Hamano wrote:
Show 14 quoted lines
> 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
Show 50 quoted lines
> 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"));
>