possible usability issue in rebase -i?

possible usability issue in rebase -i?

[Erik Faye-Lund]

I recently came over a not-overly-helpful error in git rebase -i, when
a line got wrapped by the editor so that a part of the commit-message
was interpreted as a command:

---
$ git rebase -i HEAD~20
<edit file>
Unknown command: .
fatal: ambiguous argument 'Please fix this in the file C:/msysgit/git/.git/rebas
e-merge/git-rebase-todo.': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions
fatal: Not a valid object name Please fix this in the file C:/msysgit/git/.git/r
ebase-merge/git-rebase-todo.
fatal: bad revision 'Please fix this in the file C:/msysgit/git/.git/rebase-merg
e/git-rebase-todo.'

$ git --version
git version 1.6.5.1386.g43a7a.dirty
---

In this particular case, the first character on the new line was '.',
so the first line of the error message makes perfect sense, but the
lines that followed the real error got me pretty confused. Perhaps
this is something that could be cleaned away? I'd think that an
unknown command always should be fatal, and not need to propagate
further. But I might be wrong, as I'm not familiar with the inner
workings of rebase -i.

-- 
Erik "kusma" Faye-Lund

[PATCH] rebase -i: more graceful handling of invalid commands

[Jan Krüger]

Currently, when there is an invalid command, the rest of the line is
still treated as if the command had been valid, i.e. rebase -i attempts
to produce a patch, using the next argument as a SHA1 name. If there is
no next argument or an invalid one, very confusing error messages
appear (the line was '.'; path to git-rebase-todo substituted):

Unknown command: .
fatal: ambiguous argument 'Please fix this in the file $somefile.':
unknown revision or path not in the working tree.
Use '--' to separate paths from revisions
fatal: Not a valid object name Please fix this in the file $somefile.
fatal: bad revision 'Please fix this in the file $somefile.'

Instead, verify the validity of the remaining line and error out earlier
if necessary.

Signed-off-by: Jan Krüger <jk@jk.gs>
---

> I recently came over a not-overly-helpful error in git rebase -i, when
> a line got wrapped by the editor so that a part of the commit-message
> was interpreted as a command:

Here is a suggested fix.

 git-rebase--interactive.sh |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index a1879e3..fdd8eb6 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -416,7 +416,12 @@ do_next () {
 		;;
 	*)
 		warn "Unknown command: $command $sha1 $rest"
-		die_with_patch $sha1 "Please fix this in the file $TODO."
+		if git rev-parse --verify -q "$sha" >/dev/null
+		then
+			die_with_patch $sha1 "Please fix this in the file $TODO."
+		else
+			die "Please fix this in the file $TODO."
+		fi
 		;;
 	esac
 	test -s "$TODO" && return
-- 
1.6.5.rc1

Re: [PATCH] rebase -i: more graceful handling of invalid commands

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 872 bytes --]

Hi,

On Tue, 27 Oct 2009, Jan Krüger wrote:

> Currently, when there is an invalid command, the rest of the line is
> still treated as if the command had been valid, i.e. rebase -i attempts
> to produce a patch, using the next argument as a SHA1 name. If there is
> no next argument or an invalid one, very confusing error messages
> appear (the line was '.'; path to git-rebase-todo substituted):
> 
> Unknown command: .
> fatal: ambiguous argument 'Please fix this in the file $somefile.':
> unknown revision or path not in the working tree.
> Use '--' to separate paths from revisions
> fatal: Not a valid object name Please fix this in the file $somefile.
> fatal: bad revision 'Please fix this in the file $somefile.'
> 
> Instead, verify the validity of the remaining line and error out earlier
> if necessary.
> 
> Signed-off-by: Jan Krüger <jk@jk.gs>

ACK,
Dscho

Re: [PATCH] rebase -i: more graceful handling of invalid commands

[Thomas Rast]

Jan Krüger wrote:
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index a1879e3..fdd8eb6 100755
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -416,7 +416,12 @@ do_next () {
>  		;;
>  	*)
>  		warn "Unknown command: $command $sha1 $rest"
> -		die_with_patch $sha1 "Please fix this in the file $TODO."
> +		if git rev-parse --verify -q "$sha" >/dev/null

I think you need s/sha/sha1/ here?

> +		then
> +			die_with_patch $sha1 "Please fix this in the file $TODO."
> +		else
> +			die "Please fix this in the file $TODO."
> +		fi

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[PATCH v2] rebase -i: more graceful handling of invalid commands

[Jan Krüger]

Currently, when there is an invalid command, the rest of the line is
still treated as if the command had been valid, i.e. rebase -i attempts
to produce a patch, using the next argument as a SHA1 name. If there is
no next argument or an invalid one, very confusing error messages
appear (the line was '.'; path to git-rebase-todo substituted):

Unknown command: .
fatal: ambiguous argument 'Please fix this in the file $somefile.':
unknown revision or path not in the working tree.
Use '--' to separate paths from revisions
fatal: Not a valid object name Please fix this in the file $somefile.
fatal: bad revision 'Please fix this in the file $somefile.'

Instead, verify the validity of the remaining line and error out earlier
if necessary.

Signed-off-by: Jan Krüger <jk@jk.gs>
Acked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---

Thomas Rast wrote:
> I think you need s/sha/sha1/ here?

Of course. For some reason I forgot testing the code path where the
SHA1 is actually valid. Sorry about that.

Dscho's ACK lifted off
<http://article.gmane.org/gmane.comp.version-control.git/131341>.

 git-rebase--interactive.sh |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index a1879e3..fdd8eb6 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -416,7 +416,12 @@ do_next () {
 		;;
 	*)
 		warn "Unknown command: $command $sha1 $rest"
-		die_with_patch $sha1 "Please fix this in the file $TODO."
+		if git rev-parse --verify -q "$sha1" >/dev/null
+		then
+			die_with_patch $sha1 "Please fix this in the file $TODO."
+		else
+			die "Please fix this in the file $TODO."
+		fi
 		;;
 	esac
 	test -s "$TODO" && return
-- 
1.6.5.rc1

Re: [PATCH v2] rebase -i: more graceful handling of invalid commands

[Junio C Hamano]

Thanks.

Re: possible usability issue in rebase -i?

[Baz]

2009/10/27 Erik Faye-Lund <kusmabite@googlemail.com>:
> I recently came over a not-overly-helpful error in git rebase -i, when
> a line got wrapped by the editor so that a part of the commit-message
> was interpreted as a command:
>
> ---
> $ git rebase -i HEAD~20
> <edit file>
> Unknown command: .
> fatal: ambiguous argument 'Please fix this in the file C:/msysgit/git/.git/rebas
> e-merge/git-rebase-todo.': unknown revision or path not in the working tree.
> Use '--' to separate paths from revisions
> fatal: Not a valid object name Please fix this in the file C:/msysgit/git/.git/r
> ebase-merge/git-rebase-todo.
> fatal: bad revision 'Please fix this in the file C:/msysgit/git/.git/rebase-merg
> e/git-rebase-todo.'
>
> $ git --version
> git version 1.6.5.1386.g43a7a.dirty
> ---
>
> In this particular case, the first character on the new line was '.',
> so the first line of the error message makes perfect sense, but the
> lines that followed the real error got me pretty confused. Perhaps
> this is something that could be cleaned away? I'd think that an
> unknown command always should be fatal, and not need to propagate
> further. But I might be wrong, as I'm not familiar with the inner
> workings of rebase -i.

I've got a somewhat related minor usability issue with rebase -i. I
accidentally typed something like 'git rebase -i -z' and got this
message:

error: unknown switch `z'
usage: git-rebase [-i] [options] [--] <upstream> [<branch>]
   or: git-rebase [-i] (--continue | --abort | --skip)

Available options are
    -v, --verbose         display a diffstat of what changed upstream
    --onto ...            rebase onto given branch instead of upstream
    -p, --preserve-merges
                          try to recreate merges instead of ignoring them
    -s, --strategy ...    use the given merge strategy
    -m, --merge           always used (no-op)
    -i, --interactive     always used (no-op)

The last two lines were the surprise. It suggested to me that '-i' and
'-m' were now the defaults for git-rebase - which of course they're
not. A user would not know that this is actually reporting the flags
that work for git-rebase--interactive, especially since that's not
what the command calls itself. I wasn't sure about the best approach
to fixing this - the only comparable commands that pass arbitrary
flags down to an exec'd program make it clear what program is going to
be called (usually git merge) and so interpreting errors is easier.

It seems the intent here was to signal that the flags are different
once a rebase is in progress, but this usage message is shown when
rebase -i -z is called in any state.

Cheers,
Brian
>
> --
> Erik "kusma" Faye-Lund
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: possible usability issue in rebase -i?

[Erik Faye-Lund]

On Tue, Oct 27, 2009 at 4:17 PM, Baz <brian.ewins@gmail.com> wrote:
> I've got a somewhat related minor usability issue with rebase -i. I
> accidentally typed something like 'git rebase -i -z' and got this
> message:
>
> error: unknown switch `z'
> usage: git-rebase [-i] [options] [--] <upstream> [<branch>]
>   or: git-rebase [-i] (--continue | --abort | --skip)
>
> Available options are
>    -v, --verbose         display a diffstat of what changed upstream
>    --onto ...            rebase onto given branch instead of upstream
>    -p, --preserve-merges
>                          try to recreate merges instead of ignoring them
>    -s, --strategy ...    use the given merge strategy
>    -m, --merge           always used (no-op)
>    -i, --interactive     always used (no-op)
>
> The last two lines were the surprise. It suggested to me that '-i' and
> '-m' were now the defaults for git-rebase - which of course they're
> not. A user would not know that this is actually reporting the flags
> that work for git-rebase--interactive, especially since that's not
> what the command calls itself. I wasn't sure about the best approach
> to fixing this - the only comparable commands that pass arbitrary
> flags down to an exec'd program make it clear what program is going to
> be called (usually git merge) and so interpreting errors is easier.
>
> It seems the intent here was to signal that the flags are different
> once a rebase is in progress, but this usage message is shown when
> rebase -i -z is called in any state.

If that is the case, my instinct tells me that this information should
be reflected in the usage-string (instead of the parameter
description). Something like this?

--->8---

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 23ded48..3ed5f94 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -13,15 +13,15 @@
 OPTIONS_KEEPDASHDASH=
 OPTIONS_SPEC="\
 git-rebase [-i] [options] [--] <upstream> [<branch>]
-git-rebase [-i] (--continue | --abort | --skip)
+git-rebase [-i] [-m] (--continue | --abort | --skip)
 --
  Available options are
 v,verbose          display a diffstat of what changed upstream
 onto=              rebase onto given branch instead of upstream
 p,preserve-merges  try to recreate merges instead of ignoring them
 s,strategy=        use the given merge strategy
-m,merge            always used (no-op)
-i,interactive      always used (no-op)
+m,merge            use merging strategies
+i,interactive      interactively edit commits
  Actions:
 continue           continue rebasing process
 abort              abort rebasing process and restore original branch

-- 
Erik "kusma" Faye-Lund

Re: possible usability issue in rebase -i?

[Baz]

2009/10/27 Erik Faye-Lund <kusmabite@googlemail.com>:
> On Tue, Oct 27, 2009 at 4:17 PM, Baz <brian.ewins@gmail.com> wrote:
>> I've got a somewhat related minor usability issue with rebase -i. I
>> accidentally typed something like 'git rebase -i -z' and got this
>> message:
>>
>> error: unknown switch `z'
>> usage: git-rebase [-i] [options] [--] <upstream> [<branch>]
>>   or: git-rebase [-i] (--continue | --abort | --skip)
>>
>> Available options are
>>    -v, --verbose         display a diffstat of what changed upstream
>>    --onto ...            rebase onto given branch instead of upstream
>>    -p, --preserve-merges
>>                          try to recreate merges instead of ignoring them
>>    -s, --strategy ...    use the given merge strategy
>>    -m, --merge           always used (no-op)
>>    -i, --interactive     always used (no-op)
>>
>> The last two lines were the surprise. It suggested to me that '-i' and
>> '-m' were now the defaults for git-rebase - which of course they're
>> not. A user would not know that this is actually reporting the flags
>> that work for git-rebase--interactive, especially since that's not
>> what the command calls itself. I wasn't sure about the best approach
>> to fixing this - the only comparable commands that pass arbitrary
>> flags down to an exec'd program make it clear what program is going to
>> be called (usually git merge) and so interpreting errors is easier.
>>
>> It seems the intent here was to signal that the flags are different
>> once a rebase is in progress, but this usage message is shown when
>> rebase -i -z is called in any state.
>
> If that is the case, my instinct tells me that this information should
> be reflected in the usage-string (instead of the parameter
> description). Something like this?

I'm fine with this way of fixing it, but I'd make a few more changes...

>
> --->8---
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 23ded48..3ed5f94 100755
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -13,15 +13,15 @@
>  OPTIONS_KEEPDASHDASH=
>  OPTIONS_SPEC="\
>  git-rebase [-i] [options] [--] <upstream> [<branch>]

Use the dashless form and be more consistent with the help - and
mention '--root' here, it appears in the
help below:

-git-rebase [-i] [options] [--] <upstream> [<branch>]
+git rebase [--interactive | -i] [options] [--onto <newbase>] [--]
<upstream> [<branch>]
+git rebase [--interactive | -i] [options] --onto <newbase> --root
[--] [<branch>]


> -git-rebase [-i] (--continue | --abort | --skip)
> +git-rebase [-i] [-m] (--continue | --abort | --skip)

Again, dashless. And I'd not mention the useless -i here, the man page
doesn't either:

-git-rebase [-i] (--continue | --abort | --skip)
+git rebase (--continue | --abort | --skip)

>  --
>  Available options are
>  v,verbose          display a diffstat of what changed upstream
>  onto=              rebase onto given branch instead of upstream
>  p,preserve-merges  try to recreate merges instead of ignoring them
>  s,strategy=        use the given merge strategy
> -m,merge            always used (no-op)
> -i,interactive      always used (no-op)
> +m,merge            use merging strategies
> +i,interactive      interactively edit commits

These two items are misplaced in the help (I think). They're not like
abort, continue, skip, but then, the man page doesn't group those
separately either.

+no-verify          override pre-rebase hook from stopping the operation
+root               rebase all reachable commmits up to the root(s)

>  Actions:
>  continue           continue rebasing process
>  abort              abort rebasing process and restore original branch

As above, remove the next two lines after your patch:

-no-verify          override pre-rebase hook from stopping the operation
-root               rebase all reachable commmits up to the root(s)

-Baz

>
> --
> Erik "kusma" Faye-Lund
>

Re: possible usability issue in rebase -i?

[Erik Faye-Lund]

On Tue, Oct 27, 2009 at 10:05 PM, Baz <brian.ewins@gmail.com> wrote:
> I'm fine with this way of fixing it, but I'd make a few more changes...

Feel free to make a patch-series that addresses more issues - I'm not going to.

We make patches of one change at the time in Git. Other (related)
usability issues becomes separate patches, preferably grouped together
in a patch-series. This change would be one patch in such a series.

>>  OPTIONS_SPEC="\
>>  git-rebase [-i] [options] [--] <upstream> [<branch>]
>
> Use the dashless form and be more consistent with the help - and
> mention '--root' here, it appears in the
> help below:
>
> -git-rebase [-i] [options] [--] <upstream> [<branch>]
> +git rebase [--interactive | -i] [options] [--onto <newbase>] [--]
> <upstream> [<branch>]
> +git rebase [--interactive | -i] [options] --onto <newbase> --root
> [--] [<branch>]
>

I'm not sure I follow - aren't dashless options, uhm, dashless? Do you
mean to use the long-form instead of the short-form? I'll assume
that's what you mean for now, since you changed "-i" to "--interactive
| -i".

If so, I'm not 100% convinced it's a clear win: some grep'ing
indicates that both the short and long form are both widely used, with
short-option bein a slight favor:
$ git grep " \[--" | grep -v " \[--\]" | wc -l
    228
$ git grep " \[-[^-]" | wc -l
    243

Also, the usage isn't the only documentation. I think it makes sense
to try to keep the usage short and to the point, there's a list
describing each option (showing the full-name) further down in the
usage-message. And if that's not enough, there's the "git
help"-command.

If I've misunderstood you and you only want the usage-string to match
that of the manpage, perhaps that might be a good idea. I dunno.

>
>> -git-rebase [-i] (--continue | --abort | --skip)
>> +git-rebase [-i] [-m] (--continue | --abort | --skip)
>
> Again, dashless. And I'd not mention the useless -i here, the man page
> doesn't either:
>
> -git-rebase [-i] (--continue | --abort | --skip)
> +git rebase (--continue | --abort | --skip)
>

It was already there, so I didn't consider it, but I guess it makes
sense. Besides, I aimed at not loosing any information while making it
a bit clearer.

> These two items are misplaced in the help (I think). They're not like
> abort, continue, skip, but then, the man page doesn't group those
> separately either.
>
> +no-verify          override pre-rebase hook from stopping the operation
> +root               rebase all reachable commmits up to the root(s)
>

Agree.

>>  Actions:
>>  continue           continue rebasing process
>>  abort              abort rebasing process and restore original branch
>
> As above, remove the next two lines after your patch:
>
> -no-verify          override pre-rebase hook from stopping the operation
> -root               rebase all reachable commmits up to the root(s)

I don't follow this. Are you repeating yourself now? :)

-- 
Erik "kusma" Faye-Lund

Re: possible usability issue in rebase -i?

[Baz]

2009/10/28 Erik Faye-Lund <kusmabite@googlemail.com>:
> On Tue, Oct 27, 2009 at 10:05 PM, Baz <brian.ewins@gmail.com> wrote:
>> I'm fine with this way of fixing it, but I'd make a few more changes...
>
> Feel free to make a patch-series that addresses more issues - I'm not going to.
>

Yep, I wrote one but had to leave the house before sending it. Later today.

> We make patches of one change at the time in Git. Other (related)
> usability issues becomes separate patches, preferably grouped together
> in a patch-series. This change would be one patch in such a series.
>
>>>  OPTIONS_SPEC="\
>>>  git-rebase [-i] [options] [--] <upstream> [<branch>]
>>
>> Use the dashless form and be more consistent with the help - and
>> mention '--root' here, it appears in the
>> help below:
>>
>> -git-rebase [-i] [options] [--] <upstream> [<branch>]
>> +git rebase [--interactive | -i] [options] [--onto <newbase>] [--]
>> <upstream> [<branch>]
>> +git rebase [--interactive | -i] [options] --onto <newbase> --root
>> [--] [<branch>]
>>
>
> I'm not sure I follow - aren't dashless options, uhm, dashless? Do you
> mean to use the long-form instead of the short-form? I'll assume
> that's what you mean for now, since you changed "-i" to "--interactive
> | -i".

No, I just meant 'git rebase' not 'git-rebase'. Sorry, I changed a
couple of things at once.

>
> If so, I'm not 100% convinced it's a clear win: some grep'ing
> indicates that both the short and long form are both widely used, with
> short-option bein a slight favor:
> $ git grep " \[--" | grep -v " \[--\]" | wc -l
>    228
> $ git grep " \[-[^-]" | wc -l
>    243
>
> Also, the usage isn't the only documentation. I think it makes sense
> to try to keep the usage short and to the point, there's a list
> describing each option (showing the full-name) further down in the
> usage-message. And if that's not enough, there's the "git
> help"-command.
>
> If I've misunderstood you and you only want the usage-string to match
> that of the manpage, perhaps that might be a good idea. I dunno.

In the patch I've followed other uses of OPTIONS_SPEC; they're quite
verbose, covering all options, while scripts using USAGE/LONG_USAGE
tend to emit one-liners. As for calling out 'interactive', at the
other extreme its not clear to me why we mention '-i' separately from
'[options]' at all. rebase is already pretty inconsistent here, giving
short or long usage messages depending on whether you passed '-i'. But
I'll take comments on this when I submit the patch, I've no strong
feelings on it.

>
>>
>>> -git-rebase [-i] (--continue | --abort | --skip)
>>> +git-rebase [-i] [-m] (--continue | --abort | --skip)
>>
>> Again, dashless. And I'd not mention the useless -i here, the man page
>> doesn't either:
>>
>> -git-rebase [-i] (--continue | --abort | --skip)
>> +git rebase (--continue | --abort | --skip)
>>
>
> It was already there, so I didn't consider it, but I guess it makes
> sense. Besides, I aimed at not loosing any information while making it
> a bit clearer.
>
>> These two items are misplaced in the help (I think). They're not like
>> abort, continue, skip, but then, the man page doesn't group those
>> separately either.
>>
>> +no-verify          override pre-rebase hook from stopping the operation
>> +root               rebase all reachable commmits up to the root(s)
>>
>
> Agree.
>
>>>  Actions:
>>>  continue           continue rebasing process
>>>  abort              abort rebasing process and restore original branch
>>
>> As above, remove the next two lines after your patch:
>>
>> -no-verify          override pre-rebase hook from stopping the operation
>> -root               rebase all reachable commmits up to the root(s)
>
> I don't follow this. Are you repeating yourself now? :)

Yes :) ... was just finishing off moving those two lines.

Cheers,
Baz

>
> --
> Erik "kusma" Faye-Lund
>

Re: possible usability issue in rebase -i?

[Erik Faye-Lund]

On Wed, Oct 28, 2009 at 3:34 PM, Baz <brian.ewins@gmail.com> wrote:
> 2009/10/28 Erik Faye-Lund <kusmabite@googlemail.com>:
>> I'm not sure I follow - aren't dashless options, uhm, dashless? Do you
>> mean to use the long-form instead of the short-form? I'll assume
>> that's what you mean for now, since you changed "-i" to "--interactive
>> | -i".
>
> No, I just meant 'git rebase' not 'git-rebase'. Sorry, I changed a
> couple of things at once.

Ah, didn't notice that one. I completely agree with you on this.

> tend to emit one-liners. As for calling out 'interactive', at the
> other extreme its not clear to me why we mention '-i' separately from
> '[options]' at all. rebase is already pretty inconsistent here, giving
> short or long usage messages depending on whether you passed '-i'. But
> I'll take comments on this when I submit the patch, I've no strong
> feelings on it.

It's a simple reason why the output is different - this is the usage
for "git rebase -i" (hence it is in git-rebase--interactive.sh). I
guess this distinction would be slightly clearer if we removed the
brackets from the usage like this:

-git-rebase [-i] [whatever]
+git-rebase -i [whatever]


-- 
Erik "kusma" Faye-Lund