Refactor Git::Lib#rev_parse(revision) and sanitize its input #737
Conversation
| assert_args_are_not_options('rev', revision) | ||
|
|
||
| command('rev-parse', revision) |
There was a problem hiding this comment.
Hey @jcouball, thank you for this project.
I wonder if this command could be adjusted like this:
| assert_args_are_not_options('rev', revision) | |
| command('rev-parse', revision) | |
| command('rev-parse', '--revs-only', '--end-of-options', revision, '--') |
This changes the exception.result.stderr to:
$ git rev-parse --revs-only --end-of-options v3 --
fatal: bad revision 'v3'from:
$ git rev-parse v3
v3
fatal: ambiguous argument 'v3': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'Which left me confused and investigating first time I encountered missing revision name.
If you are up for this change I can prepare a PR for it, WDYT?
There was a problem hiding this comment.
Yes, a PR would be appreciated!
I do have a couple of caveats. I’m not sure if they can be fully addressed, but I’m happy to take the PR either way.
Appending -- at the end of the command seems a bit ambiguous when --revs-only is also used. From what I’ve observed, Git appears to ignore anything after -- in this context — even if you provide a path (existing or not), the output remains the same.
That said, the presence of -- does change the error messaging. As you pointed out, it produces a more helpful error when the input is invalid.
This raises a couple of questions:
- Can we rely on this behavior remaining consistent long-term?
- Is there a risk that combining --revs-only with -- will be considered an error in future Git versions?
There was a problem hiding this comment.
#789 - I think that -- behavior is so deep at core of git that it will never change.
Make the following changes to Git::Lib#rev_parse:
Remove optimizations that try to calculate the resulting SHA instead of calling
git rev-parse. This library should not reimplement git functionality if possible. Git could change and then the assumptions in these optimizations would no longer be valid (e.g. SHAs are 40 digit hex numbers).There is a chance that this could break existing uses if the optimization implementation is already different than the implementation of
git rev-parseValidate that the reference passed in does not look like a command-line option (i.e. starts with a hyphen).