Skip to content

Refactor Git::Lib#rev_parse(revision) and sanitize its input #737

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 25, 2024
Merged

Refactor Git::Lib#rev_parse(revision) and sanitize its input #737

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ tree.blobs
tree.subtrees
tree.children # blobs and subtrees

g.revparse('v2.5:Makefile')
g.rev_parse('v2.0.0:README.md')

g.branches # returns Git::Branch objects
g.branches.local
Expand Down
13 changes: 8 additions & 5 deletions lib/git/base.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,14 +634,17 @@ def with_temp_working &blk
# runs git rev-parse to convert the objectish to a full sha
#
# @example
# git.revparse("HEAD^^")
# git.revparse('v2.4^{tree}')
# git.revparse('v2.4:/doc/index.html')
# git.rev_parse("HEAD^^")
# git.rev_parse('v2.4^{tree}')
# git.rev_parse('v2.4:/doc/index.html')
#
def revparse(objectish)
self.lib.revparse(objectish)
def rev_parse(objectish)
self.lib.rev_parse(objectish)
end

# For backwards compatibility
alias revparse rev_parse

def ls_tree(objectish, opts = {})
self.lib.ls_tree(objectish, opts)
end
Expand Down
33 changes: 24 additions & 9 deletions lib/git/lib.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,17 +311,32 @@ def full_log_commits(opts = {})
process_commit_log_data(full_log)
end

def revparse(string)
return string if string =~ /^[A-Fa-f0-9]{40}$/ # passing in a sha - just no-op it
rev = ['head', 'remotes', 'tags'].map do |d|
File.join(@git_dir, 'refs', d, string)
end.find do |path|
File.file?(path)
end
return File.read(rev).chomp if rev
command('rev-parse', string)
# Verify and resolve a Git revision to its full SHA
#
# @see https://git-scm.com/docs/git-rev-parse git-rev-parse
# @see https://git-scm.com/docs/git-rev-parse#_specifying_revisions Valid ways to specify revisions
# @see https://git-scm.com/docs/git-rev-parse#Documentation/git-rev-parse.txt-emltrefnamegtemegemmasterememheadsmasterememrefsheadsmasterem Ref disambiguation rules
#
# @example
# lib.rev_parse('HEAD') # => '9b9b31e704c0b85ffdd8d2af2ded85170a5af87d'
# lib.rev_parse('9b9b31e') # => '9b9b31e704c0b85ffdd8d2af2ded85170a5af87d'
#
# @param revision [String] the revision to resolve
#
# @return [String] the full commit hash
#
# @raise [Git::FailedError] if the revision cannot be resolved
# @raise [ArgumentError] if the revision is a string starting with a hyphen
#
def rev_parse(revision)
assert_args_are_not_options('rev', revision)

command('rev-parse', revision)
Comment on lines +332 to +334
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @jcouball, thank you for this project.

I wonder if this command could be adjusted like this:

Suggested change
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?

Copy link
Member Author

@jcouball jcouball May 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#789 - I think that -- behavior is so deep at core of git that it will never change.

end

# For backwards compatibility with the old method name
alias :revparse :rev_parse

def namerev(string)
command('name-rev', string).split[1]
end
Expand Down
2 changes: 1 addition & 1 deletion lib/git/object.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ def initialize(base, objectish)
end

def sha
@sha ||= @base.lib.revparse(@objectish)
@sha ||= @base.lib.rev_parse(@objectish)
end

def size
Expand Down
4 changes: 2 additions & 2 deletions tests/units/test_branch.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,11 +160,11 @@ def test_branch_update_ref
File.write('foo','rev 2')
git.add('foo')
git.commit('rev 2')
git.branch('testing').update_ref(git.revparse('HEAD'))
git.branch('testing').update_ref(git.rev_parse('HEAD'))

# Expect the call to Branch#update_ref to pass the full ref name for the
# of the testing branch to Lib#update_ref
assert_equal(git.revparse('HEAD'), git.revparse('refs/heads/testing'))
assert_equal(git.rev_parse('HEAD'), git.rev_parse('refs/heads/testing'))
end
end
end
26 changes: 22 additions & 4 deletions tests/units/test_lib.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,10 +174,28 @@ def test_git_ssh_from_environment_is_passed_to_binary
Git::Base.config.git_ssh = saved_git_ssh
end

def test_revparse
assert_equal('1cc8667014381e2788a94777532a788307f38d26', @lib.revparse('1cc8667014381')) # commit
assert_equal('94c827875e2cadb8bc8d4cdd900f19aa9e8634c7', @lib.revparse('1cc8667014381^{tree}')) #tree
assert_equal('ba492c62b6227d7f3507b4dcc6e6d5f13790eabf', @lib.revparse('v2.5:example.txt')) #blob
def test_rev_parse_commit
assert_equal('1cc8667014381e2788a94777532a788307f38d26', @lib.rev_parse('1cc8667014381')) # commit
end

def test_rev_parse_tree
assert_equal('94c827875e2cadb8bc8d4cdd900f19aa9e8634c7', @lib.rev_parse('1cc8667014381^{tree}')) #tree
end

def test_rev_parse_blob
assert_equal('ba492c62b6227d7f3507b4dcc6e6d5f13790eabf', @lib.rev_parse('v2.5:example.txt')) #blob
end

def test_rev_parse_with_bad_revision
assert_raise(ArgumentError) do
@lib.rev_parse('--all')
end
end

def test_rev_parse_with_unknown_revision
assert_raise(Git::FailedError) do
@lib.rev_parse('NOTFOUND')
end
end

def test_object_type
Expand Down
4 changes: 2 additions & 2 deletions tests/units/test_object.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,8 @@ def test_blob_contents
assert(block_called)
end

def test_revparse
sha = @git.revparse('v2.6:example.txt')
def test_rev_parse
sha = @git.rev_parse('v2.6:example.txt')
assert_equal('1f09f2edb9c0d9275d15960771b363ca6940fbe3', sha)
end

Expand Down
Loading