Bug 27847 – bugzilla-tool post-commits trunk..head errors out

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Eric Seidel (no email)]

bugzilla-tool post-commits trunk..head errors out

bugzilla-tool post-commits trunk..head
Skipping fe5ba4cc272372cbd6c14261081e8d0b856cde87: No bug id found in commit log or specified with --bug-id.
fatal: Not a valid object name ^20daa58114c8c3829861a53d405c117352d9ca4f
Traceback (most recent call last):
  File "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/bugzilla-tool", line 668, in <module>
    main()
  File "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/bugzilla-tool", line 665, in main
    return tool.main()
  File "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/bugzilla-tool", line 660, in main
    return command_object.execute(command_options, command_args, self)
  File "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/bugzilla-tool", line 431, in execute
    commit_message = tool.scm().commit_message_for_local_commit(commit_id)
  File "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/modules/scm.py", line 380, in commit_message_for_local_commit
    commit_lines = self.run_command(['git', 'cat-file', 'commit', commit_id]).splitlines()
  File "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/modules/scm.py", line 108, in run_command
    raise ScriptError('Failed to run "%s"  exit_code: %d  cwd: %s' % (args, exit_code, cwd))
modules.scm.ScriptError: Failed to run "['git', 'cat-file', 'commit', '^20daa58114c8c3829861a53d405c117352d9ca4f']"  exit_code: 128  cwd: None

Note this is because of our git rev-parse usage:

git rev-parse --revs-only trunk..HEAD
fe5ba4cc272372cbd6c14261081e8d0b856cde87
^20daa58114c8c3829861a53d405c117352d9ca4f

We really want rev-list (most of the time):

git rev-list trunk..HEAD
fe5ba4cc272372cbd6c14261081e8d0b856cde87
7a3f6b9a5aabf51fe0d54688805d0fd0e9dc657c
4c929dac07dd08bf2328b755a1054189e158478f



I propose that we define our bugzilla-tool post-commits behavior as follows:

1.  0 args means "rev-list trunk..HEAD"
2.  1 arg, "COMMIT" means cherry pick, aka "rev-list COMMIT^..COMMIT"
3.  2 args "A B" means cherry-pick those 2
4.  a ".." range arg "A..B" means "rev-list A..B" (theoretically we could support multiple ranges, but that seems gratuitous)
5.  a "..." range arg is an error (never really makes sense for post-commits"


This mostly means replacing our current rev-parse usage with rev-list, and adding a default case for 0 args, and making ... and error.  Also, we have to detect the lack of .. and modify what we pass to rev-list a little.

Does this sound sane?

[Evan Martin]

More generally, something like

revs_to_upload = []
for arg in argv:
  if '..' in arg:
    args = `git rev-list arg`.splitlines()
  else:
    args = [arg]
  revs_to_upload += args

covers cases 2 through 4 without special-casing.

[David Kilzer (:ddkilzer)]

(In reply to comment #0)
> I propose that we define our bugzilla-tool post-commits behavior as follows:
> 
> 1.  0 args means "rev-list trunk..HEAD"
> 2.  1 arg, "COMMIT" means cherry pick, aka "rev-list COMMIT^..COMMIT"
> 3.  2 args "A B" means cherry-pick those 2
> 4.  a ".." range arg "A..B" means "rev-list A..B" (theoretically we could
> support multiple ranges, but that seems gratuitous)
> 5.  a "..." range arg is an error (never really makes sense for post-commits"

I agree with this functionality.  Are there any git commands that behave the same way?  Doesn't git-rev-parse support all common forms of specifying ranges in git, or do we have to "help" it for items 1 and 2 by pre-processing the args?

[David Kilzer (:ddkilzer)]

(In reply to comment #1)
> More generally, something like
> 
> revs_to_upload = []
> for arg in argv:
>   if '..' in arg:
>     args = `git rev-list arg`.splitlines()
>   else:
>     args = [arg]
>   revs_to_upload += args
> 
> covers cases 2 through 4 without special-casing.

I like  this.  Then only case 1 and 5 need special handling.

[Eric Seidel (no email)]

I think we're defining new behavior here.  "git show" expects to cherry pick what is passed it.  But it doesn't accept ranges.  "git diff" will take a single commit passed to it and turn it into "COMMIT..HEAD" (which is what git-send-bugzilla used to do).  So our behavior is different and will need some custom code (at least as far as I can tell).

Also, as Evan could tell you, "..." are treated differently for different commands.  I think "..." makes no sense for us, and anyone passing it to post-commits likely meant ".." and did so in error, so I think we should error out.

For sanity sake, we could print 1-line descriptions of what we're about to upload before we do so.  We could even ask for confirmation, although that's probably unnecessary.

[Eric Seidel (no email)]

Created attachment 33832 [details]
First attempt


---
 3 files changed, 36 insertions(+), 23 deletions(-)

[David Kilzer (:ddkilzer)]

Comment on attachment 33832 [details]
First attempt


> +    # This function supports the following argument formats:
> +    # no args : rev-list trunk..HEAD
> +    # A..B    : rev-list A..B
> +    # A...B   : error!
> +    # A B     : [A, B]  (different from git diff, which would use "rev-list A..B")
> +    def commit_ids_from_commitish_arguments(self, args):
> +        if not len(args):
> +            args.append('trunk..HEAD')
> +
> +        commit_ids = []
> +        for commitish in args:
> +            if '...' in commitish:
> +                error("'...' is not supported (found in %s). Did you mean '..'?" % commitish)
> +            elif '..' in commitish:
> +                commit_ids += self.run_command(['git', 'rev-list', commitish]).splitlines()
> +            else:
> +                commit_ids.append(commitish)
> +        return commit_ids

I think individual commits should still be passed through git rev-parse:

> +            else:
> +                commit_ids += self.run_command(['git', 'rev-parse', '--revs-only', commitish]).splitlines()

This allows you to specify a branch name or a tag name that turns into a single commit.

r=me with this change. Everything else looks good.

[Eric Seidel (no email)]

Created attachment 33904 [details]
Revised per review comments


---
 3 files changed, 37 insertions(+), 23 deletions(-)

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebKitTools/ChangeLog
	M	WebKitTools/Scripts/bugzilla-tool
	M	WebKitTools/Scripts/modules/scm.py
Committed r46644
	M	WebKitTools/ChangeLog
	M	WebKitTools/Scripts/modules/scm.py
	M	WebKitTools/Scripts/bugzilla-tool
r46644 = 759067c20868401e074f16c80c192f60c0d33e61 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46644

[David Kilzer (:ddkilzer)]

Comment on attachment 33904 [details]
Revised per review comments

r=me

[Eric Seidel (no email)]

I was mostly just testing to make sure post-commits still worked after the suggested edits. ;)  Thanks for the second review!