From: Phillip Wood Date: Mon, 04 Sep 2023 10:16:28 GMT Subject: Re: [PATCH v2 2/3] builtin/rebase.c: Emit warning when rebasing without a forkpoint Message-ID: In-Reply-To: On 03/09/2023 05:50, Junio C Hamano wrote: > Junio C Hamano 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")); >