[v2,2/2] rebase: mark --update-refs as requiring the merge backend

Commit Message

Elijah Newren Jan. 20, 2023, 4:56 a.m. UTC

From: Elijah Newren <[e-mail]>

--update-refs is built in terms of the sequencer, which requires the
merge backend.  It was already marked as incompatible with the apply
backend in the git-rebase manual, but the code didn't check for this
incompatibility and warn the user.  Check and warn now.

While at it, fix a typo in t3422...and fix some misleading wording (all
useful options other than --whitespace=fix have long since been
implemented in the merge backend).

Signed-off-by: Elijah Newren <[e-mail]>
---
 builtin/rebase.c                       |  3 +++
 t/t3422-rebase-incompatible-options.sh | 15 ++++++++++-----
 2 files changed, 13 insertions(+), 5 deletions(-)

Comments

Phillip Wood Jan. 20, 2023, 4:46 p.m. UTC | #1

Hi Elijah

Thanks for working on this

On 20/01/2023 04:56, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <[e-mail]>
> 
> --update-refs is built in terms of the sequencer, which requires the
> merge backend.  It was already marked as incompatible with the apply
> backend in the git-rebase manual, but the code didn't check for this
> incompatibility and warn the user.  Check and warn now.

Strictly speaking we die rather than warn but I don't think that 
warrants a re-roll. I just had a quick look to see how easy it would be 
to add the advice Stolee's patch had if the user has set 
rebase.updaterefs but does not pass "--no-update-refs" when using the 
apply backend but it looks a bit fiddly unfortunately as we could die in 
imply_merge() or later on.

Thinking more generally, imply_merge() does a good job of telling the 
user which option is incompatible with "--apply" but if the user passes 
a merge option with "--whitespace=fix" and omits "--apply" then we just 
print a generic message saying "apply options and merge options cannot 
be used together" which isn't terribly helpful to the user (doubly so 
when the merge option come from a config setting).

I've also noticed that "--autosquash" is ignored if we end up using the 
apply backend. That's a separate issue but shares the "this may have 
come from a config setting rather than a command line argument" problem.

All in all I'm not sure if it is friendlier to die when the user has 
rebsase.updaterefs set and they try to rebase with "--whitespace=fix" or 
if it is better just to ignore the config in that case. If we can find a 
way to print some help when we die in that case it would be nicer for 
the user.

Best Wishes

Phillip

> While at it, fix a typo in t3422...and fix some misleading wording (all
> useful options other than --whitespace=fix have long since been
> implemented in the merge backend).
> 
> Signed-off-by: Elijah Newren <[e-mail]>
> ---
>   builtin/rebase.c                       |  3 +++
>   t/t3422-rebase-incompatible-options.sh | 15 ++++++++++-----
>   2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index ace8bd4a41c..e8bcdbf9fcd 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1507,6 +1507,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   		}
>   	}
>   
> +	if (options.update_refs)
> +		imply_merge(&options, "--update-refs");
> +
>   	if (options.type == REBASE_UNSPECIFIED) {
>   		if (!strcmp(options.default_backend, "merge"))
>   			imply_merge(&options, "--merge");
> diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
> index ebcbd79ab8a..d72c863b21b 100755
> --- a/t/t3422-rebase-incompatible-options.sh
> +++ b/t/t3422-rebase-incompatible-options.sh
> @@ -25,11 +25,11 @@ test_expect_success 'setup' '
>   '
>   
>   #
> -# Rebase has lots of useful options like --whitepsace=fix, which are
> -# actually all built in terms of flags to git-am.  Since neither
> -# --merge nor --interactive (nor any options that imply those two) use
> -# git-am, using them together will result in flags like --whitespace=fix
> -# being ignored.  Make sure rebase warns the user and aborts instead.
> +# Rebase has a useful option, --whitespace=fix, which is actually
> +# built in terms of flags to git-am.  Since neither --merge nor
> +# --interactive (nor any options that imply those two) use git-am,
> +# using them together will result in --whitespace=fix being ignored.
> +# Make sure rebase warns the user and aborts instead.
>   #
>   
>   test_rebase_am_only () {
> @@ -60,6 +60,11 @@ test_rebase_am_only () {
>   		test_must_fail git rebase $opt --exec 'true' A
>   	"
>   
> +	test_expect_success "$opt incompatible with --update-refs" "
> +		git checkout B^0 &&
> +		test_must_fail git rebase $opt --update-refs A
> +	"
> +
>   }
>   
>   test_rebase_am_only --whitespace=fix

Elijah Newren Jan. 21, 2023, 1:34 a.m. UTC | #2

On Fri, Jan 20, 2023 at 8:46 AM Phillip Wood <[e-mail]> wrote:
>
> Hi Elijah
>
> Thanks for working on this
>
> On 20/01/2023 04:56, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <[e-mail]>
> >
> > --update-refs is built in terms of the sequencer, which requires the
> > merge backend.  It was already marked as incompatible with the apply
> > backend in the git-rebase manual, but the code didn't check for this
> > incompatibility and warn the user.  Check and warn now.
>
> Strictly speaking we die rather than warn but I don't think that
> warrants a re-roll.

Oh, good catch.  I'm re-rolling anyway, so I might as well fix this.

> I just had a quick look to see how easy it would be
> to add the advice Stolee's patch had if the user has set
> rebase.updaterefs but does not pass "--no-update-refs" when using the
> apply backend but it looks a bit fiddly unfortunately as we could die in
> imply_merge() or later on.

Yeah, and it gets even more finicky than that.  If the user specifies
_any_ merge-specific options on the command line together with an
apply-specific option, then there's no point bringing up
rebase.updaterefs (or rebase.autosquash).  We only want to bring up
those config options if they are the only reasons for getting a
backends-are-incompatible error message.

> Thinking more generally, imply_merge() does a good job of telling the
> user which option is incompatible with "--apply" but if the user passes
> a merge option with "--whitespace=fix" and omits "--apply" then we just
> print a generic message saying "apply options and merge options cannot
> be used together" which isn't terribly helpful to the user (doubly so
> when the merge option come from a config setting).

That's not specific to --whitespace=fix (it also happens with -C, and
in the past happened with other options that used to only work with
the apply backend).  In particular, it's whenever both backends are
implied -- in those cases, we don't try to keep track of which options
implied it and thus only provide a very generic error message.

> I've also noticed that "--autosquash" is ignored if we end up using the
> apply backend. That's a separate issue but shares the "this may have
> come from a config setting rather than a command line argument" problem.

Yeah, Stolee also pointed this one out...and --autosquash was missing
the same incompatible-with-apply-options warnings too.

> All in all I'm not sure if it is friendlier to die when the user has
> rebsase.updaterefs set and they try to rebase with "--whitespace=fix" or
> if it is better just to ignore the config in that case. If we can find a
> way to print some help when we die in that case it would be nicer for
> the user.

I think ignoring it would be worse, as I argued over at [1].  But
another thing to keep in mind is that we can eventually make the
question obsolete by deprecating and eventually removing the apply
backend, as suggested by Junio[2].  That would allow us to remove all
the incompatibility checking and simplify the manual.


[1] https://lore.kernel.org/git/CABPp-BHDhpSVpuaubTP=[e-mail]/
[2] See "longer term goal" of
https://lore.kernel.org/git/[e-mail]/

Patch

diff --git a/builtin/rebase.c b/builtin/rebase.c
index ace8bd4a41c..e8bcdbf9fcd 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1507,6 +1507,9 @@  int cmd_rebase(int argc, const char **argv, const char *prefix)
 		}
 	}
 
+	if (options.update_refs)
+		imply_merge(&options, "--update-refs");
+
 	if (options.type == REBASE_UNSPECIFIED) {
 		if (!strcmp(options.default_backend, "merge"))
 			imply_merge(&options, "--merge");
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index ebcbd79ab8a..d72c863b21b 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -25,11 +25,11 @@  test_expect_success 'setup' '
 '
 
 #
-# Rebase has lots of useful options like --whitepsace=fix, which are
-# actually all built in terms of flags to git-am.  Since neither
-# --merge nor --interactive (nor any options that imply those two) use
-# git-am, using them together will result in flags like --whitespace=fix
-# being ignored.  Make sure rebase warns the user and aborts instead.
+# Rebase has a useful option, --whitespace=fix, which is actually
+# built in terms of flags to git-am.  Since neither --merge nor
+# --interactive (nor any options that imply those two) use git-am,
+# using them together will result in --whitespace=fix being ignored.
+# Make sure rebase warns the user and aborts instead.
 #
 
 test_rebase_am_only () {
@@ -60,6 +60,11 @@  test_rebase_am_only () {
 		test_must_fail git rebase $opt --exec 'true' A
 	"
 
+	test_expect_success "$opt incompatible with --update-refs" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --update-refs A
+	"
+
 }
 
 test_rebase_am_only --whitespace=fix