From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:47:37
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
From: Jan Krüger <hidden> Date: 2016-06-15 22:47:37
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 <redacted>
---
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(-)
@@ -416,7 +416,12 @@ do_next () {;;*)warn"Unknown command: $command$sha1$rest"-die_with_patch$sha1"Please fix this in the file $TODO."+ifgitrev-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;;esactest-s"$TODO"&&return
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:47:37
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 <redacted>
@@ -416,7 +416,12 @@ do_next () {;;*)warn"Unknown command: $command$sha1$rest"-die_with_patch$sha1"Please fix this in the file $TODO."+ifgitrev-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
From: Jan Krüger <hidden> Date: 2016-06-15 22:47:37
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 <redacted>
Acked-by: Johannes Schindelin <redacted>
---
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(-)
@@ -416,7 +416,12 @@ do_next () {;;*)warn"Unknown command: $command$sha1$rest"-die_with_patch$sha1"Please fix this in the file $TODO."+ifgitrev-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;;esactest-s"$TODO"&&return
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
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:47:37
On Tue, Oct 27, 2009 at 4:17 PM, Baz [off-list ref] 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---
On Tue, Oct 27, 2009 at 4:17 PM, Baz [off-list ref] wrote:
quoted
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...
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>]
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
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:47:37
On Tue, Oct 27, 2009 at 10:05 PM, Baz [off-list ref] 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.
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.
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.
quoted
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
On Tue, Oct 27, 2009 at 10:05 PM, Baz [off-list ref] wrote:
quoted
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.
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.
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.
quoted
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.
quoted
quoted
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
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:47:37
On Wed, Oct 28, 2009 at 3:34 PM, Baz [off-list ref] wrote:
2009/10/28 Erik Faye-Lund [off-list ref]:
quoted
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