| Message ID | 20230220033224.10400-1-alexhenrie24@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] rebase: add a --rebase-merges=drop option | expand |
Hi Alex On 20/02/2023 03:32, Alex Henrie wrote: > Name the new option "drop" intead of "no" or "false" to avoid confusion > in the future if --rebase-merges grows the ability to truly "rebase" > merge commits by reusing the conflict resolution information from the > original merge commit, and we want to add an option to ignore the > conflict resolution information. > > This option can be used to countermand a previous --rebase-merges > option. I'm a bit confused as to the reason for this change - what's the advantage over just saying --no-rebase-merges which already exists? Best Wishes Phillip > Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> > --- > Documentation/git-rebase.txt | 2 +- > builtin/rebase.c | 2 +- > t/t3430-rebase-merges.sh | 30 ++++++++++++++++++++++++++++++ > 3 files changed, 32 insertions(+), 2 deletions(-) > > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt > index 9a295bcee4..92e90f96aa 100644 > --- a/Documentation/git-rebase.txt > +++ b/Documentation/git-rebase.txt > @@ -528,7 +528,7 @@ have the long commit hash prepended to the format. > See also INCOMPATIBLE OPTIONS below. > > -r:: > ---rebase-merges[=(rebase-cousins|no-rebase-cousins)]:: > +--rebase-merges[=(rebase-cousins|no-rebase-cousins|drop)]:: > By default, a rebase will simply drop merge commits from the todo > list, and put the rebased commits into a single, linear branch. > With `--rebase-merges`, the rebase will instead try to preserve > diff --git a/builtin/rebase.c b/builtin/rebase.c > index 6635f10d52..96c0474379 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -1436,7 +1436,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > if (options.exec.nr) > imply_merge(&options, "--exec"); > > - if (rebase_merges) { > + if (rebase_merges && strcmp("drop", rebase_merges)) { > if (!*rebase_merges) > ; /* default mode; do nothing */ > else if (!strcmp("rebase-cousins", rebase_merges)) > diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh > index fa2a06c19f..861c8405f2 100755 > --- a/t/t3430-rebase-merges.sh > +++ b/t/t3430-rebase-merges.sh > @@ -250,6 +250,36 @@ test_expect_success 'with a branch tip that was cherry-picked already' ' > EOF > ' > > +test_expect_success 'do not rebase merges unless asked' ' > + git checkout -b rebase-merges-default E && > + before="$(git rev-parse --verify HEAD)" && > + test_tick && > + git rebase --rebase-merges C && > + test_cmp_rev HEAD $before && > + test_tick && > + git rebase C && > + test_cmp_graph C.. <<-\EOF > + * B > + * D > + o C > + EOF > +' > + > +test_expect_success 'do not rebase merges when asked to drop them' ' > + git checkout -b rebase-merges-drop E && > + before="$(git rev-parse --verify HEAD)" && > + test_tick && > + git rebase --rebase-merges C && > + test_cmp_rev HEAD $before && > + test_tick && > + git rebase --rebase-merges=drop C && > + test_cmp_graph C.. <<-\EOF > + * B > + * D > + o C > + EOF > +' > + > test_expect_success 'do not rebase cousins unless asked for' ' > git checkout -b cousins main && > before="$(git rev-parse --verify HEAD)" &&
On Mon, Feb 20, 2023 at 2:31 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > > On 20/02/2023 03:32, Alex Henrie wrote: > > Name the new option "drop" intead of "no" or "false" to avoid confusion > in the future if --rebase-merges grows the ability to truly "rebase" > > merge commits by reusing the conflict resolution information from the > > original merge commit, and we want to add an option to ignore the > > conflict resolution information. > > > > This option can be used to countermand a previous --rebase-merges > > option. > > I'm a bit confused as to the reason for this change - what's the > advantage over just saying --no-rebase-merges which already exists? I didn't know that there was a --no-rebase-merges option because there is no documentation about it and no tests for it. I will replace this patch with patches that add the missing documentation and tests. -Alex
Alex Henrie <alexhenrie24@gmail.com> writes: > Name the new option "drop" intead of "no" or "false" to avoid confusion This is traditionally called "flattening the history". Don't we confuse uesrs by introducing a new phrase? rebase-merges is about transplanting the history without flattening, i.e. keeping the mergy commit graph topology. If there are only two kinds of rebase (i.e. keeping the topology which is rebase-merges and the other "flattening" kind) operation, shouldn't the option be called "--no-rebase-merges" instead? --rebase-merges=no is also understandable. > in the future if --rebase-merges grows the ability to truly "rebase" > merge commits by reusing the conflict resolution information from the > original merge commit, and we want to add an option to ignore the > conflict resolution information. I am not sure why such a change "in the future" is not merely a bugfix of the current "--rebase-merges", though. Once it is fixed, is there a reason to make the fixed behaviour only available behind an option?
Hi Junio, On 20/02/2023 21:42, Junio C Hamano wrote: > Alex Henrie <alexhenrie24@gmail.com> writes: > >> Name the new option "drop" intead of "no" or "false" to avoid confusion > This is traditionally called "flattening the history". Don't we > confuse uesrs by introducing a new phrase? While "flatten.." is used on list, we rarely mention it in our man pages, and usually only in a cautionary manner via the rev-list-options.txt under "--show-linear-break". It's not always clear what is meant by 'flattening' and which aspects are included/excluded from the flattened display. I suspect that a recent question on the git-users list [1] originates from the same confusions. Maybe it's something that could be included in the Glossary to supplement the not well known how-to discussion in keep-canonical-history-correct.txt > > rebase-merges is about transplanting the history without flattening, > i.e. keeping the mergy commit graph topology. If there are only two > kinds of rebase (i.e. keeping the topology which is rebase-merges > and the other "flattening" kind) operation, shouldn't the option be > called "--no-rebase-merges" instead? --rebase-merges=no is also > understandable. > >> in the future if --rebase-merges grows the ability to truly "rebase" >> merge commits by reusing the conflict resolution information from the >> original merge commit, and we want to add an option to ignore the >> conflict resolution information. > I am not sure why such a change "in the future" is not merely a > bugfix of the current "--rebase-merges", though. Once it is fixed, > is there a reason to make the fixed behaviour only available behind > an option? [1] https://groups.google.com/d/msgid/git-users/057bd9e2-b20b-4794-b8a0-bc16ede374c1n%40googlegroups.com -- Philip
Philip Oakley <philipoakley@iee.email> writes: > Maybe it's something that could be included in the Glossary to > supplement the not well known how-to discussion in > keep-canonical-history-correct.txt Yeah, "linearizing the history" may be much easier to understand than "flattening". In any case, a canonical reference would be a good first step.
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index 9a295bcee4..92e90f96aa 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -528,7 +528,7 @@ have the long commit hash prepended to the format. See also INCOMPATIBLE OPTIONS below. -r:: ---rebase-merges[=(rebase-cousins|no-rebase-cousins)]:: +--rebase-merges[=(rebase-cousins|no-rebase-cousins|drop)]:: By default, a rebase will simply drop merge commits from the todo list, and put the rebased commits into a single, linear branch. With `--rebase-merges`, the rebase will instead try to preserve diff --git a/builtin/rebase.c b/builtin/rebase.c index 6635f10d52..96c0474379 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1436,7 +1436,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) if (options.exec.nr) imply_merge(&options, "--exec"); - if (rebase_merges) { + if (rebase_merges && strcmp("drop", rebase_merges)) { if (!*rebase_merges) ; /* default mode; do nothing */ else if (!strcmp("rebase-cousins", rebase_merges)) diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index fa2a06c19f..861c8405f2 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -250,6 +250,36 @@ test_expect_success 'with a branch tip that was cherry-picked already' ' EOF ' +test_expect_success 'do not rebase merges unless asked' ' + git checkout -b rebase-merges-default E && + before="$(git rev-parse --verify HEAD)" && + test_tick && + git rebase --rebase-merges C && + test_cmp_rev HEAD $before && + test_tick && + git rebase C && + test_cmp_graph C.. <<-\EOF + * B + * D + o C + EOF +' + +test_expect_success 'do not rebase merges when asked to drop them' ' + git checkout -b rebase-merges-drop E && + before="$(git rev-parse --verify HEAD)" && + test_tick && + git rebase --rebase-merges C && + test_cmp_rev HEAD $before && + test_tick && + git rebase --rebase-merges=drop C && + test_cmp_graph C.. <<-\EOF + * B + * D + o C + EOF +' + test_expect_success 'do not rebase cousins unless asked for' ' git checkout -b cousins main && before="$(git rev-parse --verify HEAD)" &&
Name the new option "drop" intead of "no" or "false" to avoid confusion in the future if --rebase-merges grows the ability to truly "rebase" merge commits by reusing the conflict resolution information from the original merge commit, and we want to add an option to ignore the conflict resolution information. This option can be used to countermand a previous --rebase-merges option. Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> --- Documentation/git-rebase.txt | 2 +- builtin/rebase.c | 2 +- t/t3430-rebase-merges.sh | 30 ++++++++++++++++++++++++++++++ 3 files changed, 32 insertions(+), 2 deletions(-)