| Message ID | 20181122044841.20993-2-newren@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Reimplement rebase --merge via interactive machinery | expand |
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 > >
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.
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.
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'")"
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
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(-)