[v3,1/7] rebase: fix incompatible options error message
diff mbox series

Message ID 20181122044841.20993-2-newren@gmail.com
State New
Headers show
Series
  • Reimplement rebase --merge via interactive machinery
Related show

Commit Message

Elijah Newren Nov. 22, 2018, 4:48 a.m. UTC
In commit f57696802c30 ("rebase: really just passthru the `git am`
options", 2018-11-14), the handling of `git am` options was simplified
dramatically (and an option parsing bug was fixed), but it introduced
a small regression in the error message shown when options only
understood by separate backends were used:

$ git rebase --keep --ignore-whitespace
fatal: error: cannot combine interactive options (--interactive, --exec,
--rebase-merges, --preserve-merges, --keep-empty, --root + --onto) with
am options (.git/rebase-apply/applying)

$ git rebase --merge --ignore-whitespace
fatal: error: cannot combine merge options (--merge, --strategy,
--strategy-option) with am options (.git/rebase-apply/applying)

Note that in both cases, the list of "am options" is
".git/rebase-apply/applying", which makes no sense.  Since the lists of
backend-specific options is documented pretty thoroughly in the rebase
man page (in the "Incompatible Options" section, with multiple links
throughout the document), and since I expect this list to change over
time, just simplify the error message.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/rebase.c     | 11 ++++-------
 git-legacy-rebase.sh |  4 ++--
 2 files changed, 6 insertions(+), 9 deletions(-)

Comments

Johannes Schindelin Nov. 28, 2018, 8:28 a.m. UTC | #1
Hi Elijah,

On Wed, 21 Nov 2018, Elijah Newren wrote:

> In commit f57696802c30 ("rebase: really just passthru the `git am`
> options", 2018-11-14), the handling of `git am` options was simplified
> dramatically (and an option parsing bug was fixed), but it introduced
> a small regression in the error message shown when options only
> understood by separate backends were used:
> 
> $ git rebase --keep --ignore-whitespace
> fatal: error: cannot combine interactive options (--interactive, --exec,
> --rebase-merges, --preserve-merges, --keep-empty, --root + --onto) with
> am options (.git/rebase-apply/applying)
> 
> $ git rebase --merge --ignore-whitespace
> fatal: error: cannot combine merge options (--merge, --strategy,
> --strategy-option) with am options (.git/rebase-apply/applying)
> 
> Note that in both cases, the list of "am options" is
> ".git/rebase-apply/applying", which makes no sense.  Since the lists of
> backend-specific options is documented pretty thoroughly in the rebase
> man page (in the "Incompatible Options" section, with multiple links
> throughout the document), and since I expect this list to change over
> time, just simplify the error message.
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---

This patch is obviously good.

Given that you embedded it in the patch series that makes the sequencer
the work horse also for the `merge` backend of `git rebase` in addition to
the `interactive` one, may I assume that you intend this patch for post
v2.20.0?

Ciao,
Dscho

>  builtin/rebase.c     | 11 ++++-------
>  git-legacy-rebase.sh |  4 ++--
>  2 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 5b3e5baec8..5ece134ae6 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1202,14 +1202,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  				break;
>  
>  		if (is_interactive(&options) && i >= 0)
> -			die(_("error: cannot combine interactive options "
> -			      "(--interactive, --exec, --rebase-merges, "
> -			      "--preserve-merges, --keep-empty, --root + "
> -			      "--onto) with am options (%s)"), buf.buf);
> +			die(_("error: cannot combine am options "
> +			      "with interactive options"));
>  		if (options.type == REBASE_MERGE && i >= 0)
> -			die(_("error: cannot combine merge options (--merge, "
> -			      "--strategy, --strategy-option) with am options "
> -			      "(%s)"), buf.buf);
> +			die(_("error: cannot combine am options "
> +			      "with merge options "));
>  	}
>  
>  	if (options.signoff) {
> diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
> index b97ffdc9dd..0a747eb76c 100755
> --- a/git-legacy-rebase.sh
> +++ b/git-legacy-rebase.sh
> @@ -508,13 +508,13 @@ if test -n "$git_am_opt"; then
>  	then
>  		if test -n "$incompatible_opts"
>  		then
> -			die "$(gettext "error: cannot combine interactive options (--interactive, --exec, --rebase-merges, --preserve-merges, --keep-empty, --root + --onto) with am options ($incompatible_opts)")"
> +			die "$(gettext "error: cannot combine am options with interactive options")"
>  		fi
>  	fi
>  	if test -n "$do_merge"; then
>  		if test -n "$incompatible_opts"
>  		then
> -			die "$(gettext "error: cannot combine merge options (--merge, --strategy, --strategy-option) with am options ($incompatible_opts)")"
> +			die "$(gettext "error: cannot combine am options with merge options")"
>  		fi
>  	fi
>  fi
> -- 
> 2.20.0.rc1.7.g58371d377a
> 
>
Elijah Newren Nov. 28, 2018, 3:58 p.m. UTC | #2
On Wed, Nov 28, 2018 at 12:28 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Elijah,
>
> On Wed, 21 Nov 2018, Elijah Newren wrote:
>
> > In commit f57696802c30 ("rebase: really just passthru the `git am`
> > options", 2018-11-14), the handling of `git am` options was simplified
> > dramatically (and an option parsing bug was fixed), but it introduced
> > a small regression in the error message shown when options only
> > understood by separate backends were used:
> >
> > $ git rebase --keep --ignore-whitespace
> > fatal: error: cannot combine interactive options (--interactive, --exec,
> > --rebase-merges, --preserve-merges, --keep-empty, --root + --onto) with
> > am options (.git/rebase-apply/applying)
> >
> > $ git rebase --merge --ignore-whitespace
> > fatal: error: cannot combine merge options (--merge, --strategy,
> > --strategy-option) with am options (.git/rebase-apply/applying)
> >
> > Note that in both cases, the list of "am options" is
> > ".git/rebase-apply/applying", which makes no sense.  Since the lists of
> > backend-specific options is documented pretty thoroughly in the rebase
> > man page (in the "Incompatible Options" section, with multiple links
> > throughout the document), and since I expect this list to change over
> > time, just simplify the error message.
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
>
> This patch is obviously good.
>
> Given that you embedded it in the patch series that makes the sequencer
> the work horse also for the `merge` backend of `git rebase` in addition to
> the `interactive` one, may I assume that you intend this patch for post
> v2.20.0?
>
> Ciao,
> Dscho

I think post v2.20.0 probably makes the most sense.  I was unsure what
the policy was around changing strings late in the cycle, but figured
that the worst case with this regression is someone basically
understands what the message is trying to say but thinks it might be
saying more than they understand and reach out with questions.  In
contrast, if we decide to change the string and some translators don't
have enough time to translate it before 2.20.0 goes out, then someone
who doesn't understand English gets an English error message, which
seems a little worse.

I at least wanted it to be discussed, though, which is why I
highlighted it in the cover letter.
Duy Nguyen Nov. 28, 2018, 4:12 p.m. UTC | #3
On Thu, Nov 22, 2018 at 7:32 PM Elijah Newren <newren@gmail.com> wrote:
>
> In commit f57696802c30 ("rebase: really just passthru the `git am`
> options", 2018-11-14), the handling of `git am` options was simplified
> dramatically (and an option parsing bug was fixed), but it introduced
> a small regression in the error message shown when options only
> understood by separate backends were used:
>
> $ git rebase --keep --ignore-whitespace
> fatal: error: cannot combine interactive options (--interactive, --exec,
> --rebase-merges, --preserve-merges, --keep-empty, --root + --onto) with
> am options (.git/rebase-apply/applying)
>
> $ git rebase --merge --ignore-whitespace
> fatal: error: cannot combine merge options (--merge, --strategy,
> --strategy-option) with am options (.git/rebase-apply/applying)
>
> Note that in both cases, the list of "am options" is
> ".git/rebase-apply/applying", which makes no sense.  Since the lists of
> backend-specific options is documented pretty thoroughly in the rebase
> man page (in the "Incompatible Options" section, with multiple links
> throughout the document), and since I expect this list to change over
> time, just simplify the error message.

Can we simplify it further and remove the "error: " prefix? "fatal:
error: " looks redundant.
Elijah Newren Nov. 28, 2018, 4:31 p.m. UTC | #4
On Wed, Nov 28, 2018 at 8:12 AM Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Thu, Nov 22, 2018 at 7:32 PM Elijah Newren <newren@gmail.com> wrote:
> >
> > In commit f57696802c30 ("rebase: really just passthru the `git am`
> > options", 2018-11-14), the handling of `git am` options was simplified
> > dramatically (and an option parsing bug was fixed), but it introduced
> > a small regression in the error message shown when options only
> > understood by separate backends were used:
> >
> > $ git rebase --keep --ignore-whitespace
> > fatal: error: cannot combine interactive options (--interactive, --exec,
> > --rebase-merges, --preserve-merges, --keep-empty, --root + --onto) with
> > am options (.git/rebase-apply/applying)
> >
> > $ git rebase --merge --ignore-whitespace
> > fatal: error: cannot combine merge options (--merge, --strategy,
> > --strategy-option) with am options (.git/rebase-apply/applying)
> >
> > Note that in both cases, the list of "am options" is
> > ".git/rebase-apply/applying", which makes no sense.  Since the lists of
> > backend-specific options is documented pretty thoroughly in the rebase
> > man page (in the "Incompatible Options" section, with multiple links
> > throughout the document), and since I expect this list to change over
> > time, just simplify the error message.
>
> Can we simplify it further and remove the "error: " prefix? "fatal:
> error: " looks redundant.

Sure, I can do that.  Looks like there are a few other cases that need
fixing as well:
$ git grep error: builtin/rebase.c
builtin/rebase.c:                       die(_("error: cannot combine
interactive options "
builtin/rebase.c:                       die(_("error: cannot combine
merge options (--merge, "
builtin/rebase.c:                       die(_("error: cannot combine
'--preserve-merges' with "
builtin/rebase.c:                       die(_("error: cannot combine
'--rebase-merges' with "
builtin/rebase.c:                       die(_("error: cannot combine
'--rebase-merges' with "

Perhaps, for consistency, I should also change the error message in
the git-legacy-rebase.sh script to use 'fatal' instead of 'error'?:

$ git grep error: *rebase*.sh
git-legacy-rebase.sh:                   die "$(gettext "error: cannot
combine interactive options (--interactive, --exec, --rebase-merges,
--preserve-merges, --keep-empty, --root + --onto) with am options
($incompatible_opts)")"
git-legacy-rebase.sh:                   die "$(gettext "error: cannot
combine merge options (--merge, --strategy, --strategy-option) with am
options ($incompatible_opts)")"
git-legacy-rebase.sh:           die "$(gettext "error: cannot combine
'--signoff' with '--preserve-merges'")"
git-legacy-rebase.sh:           die "$(gettext "error: cannot combine
'--preserve-merges' with '--rebase-merges'")"
git-legacy-rebase.sh:           die "$(gettext "error: cannot combine
'--rebase-merges' with '--strategy-option'")"
git-legacy-rebase.sh:           die "$(gettext "error: cannot combine
'--rebase-merges' with '--strategy'")"

Patch
diff mbox series

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5b3e5baec8..5ece134ae6 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1202,14 +1202,11 @@  int cmd_rebase(int argc, const char **argv, const char *prefix)
 				break;
 
 		if (is_interactive(&options) && i >= 0)
-			die(_("error: cannot combine interactive options "
-			      "(--interactive, --exec, --rebase-merges, "
-			      "--preserve-merges, --keep-empty, --root + "
-			      "--onto) with am options (%s)"), buf.buf);
+			die(_("error: cannot combine am options "
+			      "with interactive options"));
 		if (options.type == REBASE_MERGE && i >= 0)
-			die(_("error: cannot combine merge options (--merge, "
-			      "--strategy, --strategy-option) with am options "
-			      "(%s)"), buf.buf);
+			die(_("error: cannot combine am options "
+			      "with merge options "));
 	}
 
 	if (options.signoff) {
diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
index b97ffdc9dd..0a747eb76c 100755
--- a/git-legacy-rebase.sh
+++ b/git-legacy-rebase.sh
@@ -508,13 +508,13 @@  if test -n "$git_am_opt"; then
 	then
 		if test -n "$incompatible_opts"
 		then
-			die "$(gettext "error: cannot combine interactive options (--interactive, --exec, --rebase-merges, --preserve-merges, --keep-empty, --root + --onto) with am options ($incompatible_opts)")"
+			die "$(gettext "error: cannot combine am options with interactive options")"
 		fi
 	fi
 	if test -n "$do_merge"; then
 		if test -n "$incompatible_opts"
 		then
-			die "$(gettext "error: cannot combine merge options (--merge, --strategy, --strategy-option) with am options ($incompatible_opts)")"
+			die "$(gettext "error: cannot combine am options with merge options")"
 		fi
 	fi
 fi