Implement command to submit reviews to MozReview using git-cinnabar

RESOLVED FIXED

Status

defect
P2
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: gps, Assigned: gps)

Tracking

Details

Attachments

(3 attachments, 3 obsolete attachments)

Assignee

Description

4 years ago
git-cinnabar is the easiest way to allow Git users to push Firefox commits to MozReview, as it doesn't introduce a server-side dependency to run a Git server nor does it involve complexities with consistent SHA-1 mappings via vcs-sync.

I reckon we implement a Git command that does a `git push` via git-cinnabar and then performs code review foo after the push.

This is essentially reimplementing bits of the Mercurial client extension as a Git command.

This effort is independent of supporting GitHub pull requests, which requires a server-side component to deal with Git.
Assignee

Comment 1

4 years ago
Posted file MozReview Request: bz://1153053/gps (obsolete) —
/r/6863 - git-push-mozreview: git command for pushing to mozreview (bug 1153053)

Pull down this commit:

hg pull -r c801ebdb7ac9206840edd3073a33b8889801a2da https://reviewboard-hg.mozilla.org/version-control-tools/
I tried this git command but it creates a review request without bug number. When trying to publish the review I got Invalid bug ID "<unknown>" error.
(In reply to Kan-Ru Chen [:kanru] from comment #2)
> I tried this git command but it creates a review request without bug number.
> When trying to publish the review I got Invalid bug ID "<unknown>" error.

I think this is because of the missing ircnick part in the reviewid. After construct a reviewid with ircnick part (bz://xxxxxx/ircnick) I successfully published the review request.
https://reviewboard.mozilla.org/r/6863/#review7035

::: git-commands/git-push-mozreview:137
(Diff revision 1)
> +    reviewid = ReviewID(args.reviewid)

Need to set reviewid.user
https://reviewboard.mozilla.org/r/6863/#review8031

::: git-commands/git-push-mozreview:61
(Diff revision 1)
> +        if i > 1 and (len(parents) > 1 or refs):

Should be i >= 1 because i starts from 0
Bug 1153053 - Extract common parts that could be shared by different VCS. r?gps
Bug 1153053 - git-push-mozreview: git command for pushing to mozreview. r?gps
would you give some feedback?
Flags: needinfo?(gps)
Comment on attachment 8616345 [details]
MozReview Request: Bug 1153053 - Extract common parts that could be shared by different VCS. r?gps

Bug 1153053 - Extract common parts that could be shared by different VCS. r?gps
Comment on attachment 8616346 [details]
MozReview Request: Bug 1153053 - git-push-mozreview: git command for pushing to mozreview. r?gps

Bug 1153053 - git-push-mozreview: git command for pushing to mozreview. r?gps
Assignee

Comment 11

4 years ago
Attachment #8590581 - Attachment is obsolete: true
Assignee

Comment 13

4 years ago
https://reviewboard.mozilla.org/r/10431/#review9445

If you are going to refactor things to reusable code, I'd start by pulling pieces out of `doreview`. The protocol pieces will go a long way towards enabling a git command to be implemented.

I worry about trying to rely on too large of a common workflow for both the implementations because there are bound to be some significant differences. For example, auto rewriting/rebasing commits in Git will likely be more difficult than Mercurial. At this juncture, I think it is better to compose each tool's workflow/implementation from lower-level functions than to try and devise a common abstraction today.

But this solution does seem to work. I'll give you that.

::: hgext/reviewboard/clientlib.py:33
(Diff revision 2)
> +class rbclienthelper(object):
> +    def write(self, *args):
> +        pass
> +    def status(self, *args):
> +        pass
> +    def warn(self, *args):
> +        pass
> +    def abort(self, *args):
> +        pass
> +    def getbugzillaauth(self):
> +        return None
> +    def ircnick(self):
> +        return None
> +    def changesets(self):
> +        return []
> +    def precursors(self, changeset):
> +        return None
> +    def reviewid(self):
> +        return None
> +    def reviewstore(self):
> +        return basereviewstore()
> +    def remote(self):
> +        "Return a peerrepository"
> +        return None
> +    def bookmarkcurrent(self):
> +        return None
> +    def branch(self):
> +        return None

Please throw some docstrings on these so we know what they are supposed to do.

Please also throw some abc magic on here so we ensure implementations have the full interface implemented. https://docs.python.org/2/library/abc.html

::: hgext/reviewboard/clientlib.py:93
(Diff revision 2)
> +class basereviewstore(object):

The store is a half-baked concept and only really applied to Mercurial right now. I wouldn't worry about moving it to the common bits.

::: hgext/reviewboard/clientlib.py:182
(Diff revision 2)
> +def doreview(helper):
> +    "Do the work of submitting a review to a remote repo."
> +    assert helper.remote().capable('reviewboard')

What you've done here is move code *and* refactor it. This makes code review difficult because I have to tease out exactly what changed as part of the move. When making these kinds of changes, you should do the refactoring then a move or a move then refactoring. This makes things drastically simpler to grok and easier to look for unwanted changed in behavior. As it stands, I find this diff to be unreviewable.

::: hgext/reviewboard/client.py:122
(Diff revision 2)
> +
> +    def reviewid(self):
> +        return self.repo.reviewid
> +
> +    def reviewstore(self):
> +        return self.repo.reviews
> +
> +    def remote(self):
> +        return self._remote
> +
> +    def bookmarkcurrent(self):
> +        return self.repo._bookmarkcurrent
> +
> +    def branch(self):
> +        return self.repo.dirstate.branch()

For simple attribute lookups like these, @property should be used.
Assignee

Comment 14

4 years ago
https://reviewboard.mozilla.org/r/10433/#review9447

This looks OK. I think the final solution is somewhere between my original (and hacky) version to this one. Let's build the Git implementation on top of lower-level functions instead of rolling everything into doreview().

::: git-commands/git-push-mozreview:55
(Diff revision 2)
> +        res, output = get_output(['git', 'config', '--get', 'bugzilla.username'])
> +        if res:
> +            abort('bugzilla.username config variable must be defined')
> +        bzauth.username = output.strip()
> +
> +        res, output = get_output(['git', 'config', '--get', 'bugzilla.password'])
> +        if res:
> +            abort('bugzilla.password config variable must be defined')
> +        bzauth.password = output.strip()
> +
> +        res, output = get_output(['git', 'config', '--get', 'bugzilla.userid'])
> +        if not res:
> +            bzauth.userid = output.strip()
> +
> +        res, output = get_output(['git', 'config', '--get', 'bugzilla.cookie'])
> +        if not res:
> +            bzauth.cookie = output.strip()
> +        return bzauth

Multiple process invocations on Windows are going to be slow, slow, slow. This is acceptable as a first pass. But this wants to be a single command that dumps all configs and parses for relevant values.
Assignee

Comment 15

4 years ago
Left feedback on reviews.
Flags: needinfo?(gps)
I know some people really want this, but it's a lot of work and would take away from efforts to improve life for those who use our standard VCS (hg).  Furthermore, the GitHub pull request stuff, which not exactly solving this problem, provides an avenue for git usage and should be live soon.  So I'm going to mark this as a P4.  It's on our road map, but fairly far down.
Priority: -- → P4
Doing the ol' priority flipflop here.  Not sure if this is P1, but it seems that this is at least a P2, since the GitHub support won't really help people as much as we initially thought it would.
Priority: P4 → P2
Assignee

Comment 18

4 years ago
And I may take it as a Q4 deliverable.

We more or less decided last week that git-cinnabar will be the supported "pure Git" way of interacting with MozReview. GitHub is its own separate beast. We will *not* stand up a Git server to receive pushes to MozReview - at least not for repos that are canonically hosted in Mercurial (which includes mozilla-central).
Yup, that all makes sense to me.  Someday maybe we can have fancy MozReview support for git-only repos like BMO, but there's no rush there.
Assignee

Comment 20

4 years ago
This is one of my Q4 deliverables.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #8616345 - Attachment is obsolete: true
Attachment #8616346 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Depends on: 1229468
Assignee

Updated

4 years ago
Depends on: 1234413
Assignee

Comment 21

4 years ago
We'll need git-cinnabar present to test Git push to MozReview.

Review commit: https://reviewboard.mozilla.org/r/31127/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31127/
Attachment #8708623 - Flags: review?(smacleod)
Assignee

Comment 22

4 years ago
Comment on attachment 8620014 [details]
MozReview Request: git-mozreview: git command for interacting with MozReview (bug 1153053); r?glandium, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/6863/diff/1-2/
Attachment #8620014 - Attachment description: MozReview Request: git-push-mozreview: git command for pushing to mozreview (bug 1153053) → MozReview Request: git-mozreview: git command for submitting to mozreview (bug 1153053)
https://reviewboard.mozilla.org/r/6863/#review27941

(only looked at the test)

::: git-tests/test-mozreview-submit-basic.t:36
(Diff revision 2)
> +git-cinnabar doesn't appear to like cloning from empty repos; see the review repo

open an issue ;)

::: git-tests/test-mozreview-submit-basic.t:46
(Diff revision 2)
> +  $ git clone hg::${MERCURIAL_URL}test-repo gitrepo
> +  Cloning into 'gitrepo'...
> +  It is recommended that you set "remote.origin.prune" or "fetch.prune" to "true".
> +    git config remote.origin.prune true
> +  or
> +    git config fetch.prune true
> +

You can make the output silent by using `git -c fetch.prune=true clone`
Assignee

Comment 24

4 years ago
Hmmm. I pulled down the git-cinnabar release branch ref update (20de9a0 to 2f4d594) and now the Mercurial SHA-1 doesn't seem to be stored. I'm calling `git push` with remote.%s.cinnabar-data=always. I can confirm from manual Git store poking around that the SHA-1 mapping isn't getting stored (at least not in the tree that the notes/cinnabar ref points to). glandium?
Flags: needinfo?(mh+mozilla)
Assignee

Comment 25

4 years ago
(No urgency on that last request - I'm about to start a 3 day weekend. But I would like to have this landed by end of next week.)
As per irc, you can set cinnabar.data instead of remote.$remote.cinnabar-data. Since that's passed on the command line, it doesn't matter which one you set, and cinnabar.data is, in fact, better in this case. I however fixed remote.$remote.cinnabar-data on the release branch, since this was an unintended regression.
Flags: needinfo?(mh+mozilla)
Attachment #8708623 - Flags: review?(smacleod) → review+
Comment on attachment 8708623 [details]
MozReview Request: testing: install git-cinnabar in testing environment; r=smacleod

https://reviewboard.mozilla.org/r/31127/#review28035
https://reviewboard.mozilla.org/r/6863/#review28277

::: git-commands/git-mozreview-push:56
(Diff revision 2)
> +        for k in ('bugzilla.username', 'bugzilla.apikey'):

Having just used it for the first time, git bz, that several people have those prefs as "bz.apikey" and "bz.username", might as well reuse that.
So, as per IRC discussion, I looked at how to hook things with cinnabar while avoiding to set cinnabar.data=always.

It turns out that mercurial extensions are not loaded without a localrepo, which is the case when cinnabar runs. And cinnabar doesn't have hooks on its own.

However, it now has a "git cinnabar python" command (since 0.3.0, that essentially reexecutes python with the right PYTHONPATH, and inherits everything that git sets (like GIT_EXEC_PATH, GIT_DIR, etc.)), and the python code required to do the equivalent of `git push` is small enough that I think it's fine to use instead of `git push`, and I can easily commit to not break those APIs.

The code is the following:

from binascii import hexlify
from cinnabar.hg import (
    get_repo,
    push,
)
from cinnabar.bundle import PushStore

repo = get_repo('ssh://hg.mozilla.org/try') # this returns a mercurial repo, the result of hg.peer
heads = (hexlify(h) for h in repo.heads())

store = PushStore()
# The arguments to push(), after store, are:
# - a dict of (local_commit, (unused, push -f))
# - hexlified heads of the remote repository
# - a list of branches on the remote repository, to restrict the branches to look
# at locally when negotiating what is in common with the remote repo. An empty
# list makes cinnabar look at all the local (mercurial) branches.
# It returns a cinnabar.dag.gitdag of the git commits that were pushed.
pushed = push(repo, store, {'HEAD': (None, False)}, heads, ())

if pushed:
    for n in pushed.iternodes():
        print n, store.hg_changeset(n)

Ironically, this is faster than `git push` because of all the shortcuts (not supporting bookmarks, not looking at phases, not looking at remote branch names...)

Except for the fact that 0.2.x didn't have the `git cinnabar python` command, this should be compatible with 0.2.x as well.
Assignee

Comment 30

4 years ago
Comment on attachment 8708623 [details]
MozReview Request: testing: install git-cinnabar in testing environment; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31127/diff/1-2/
Attachment #8708623 - Attachment description: MozReview Request: testing: install git-cinnabar in testing environment; r?smacleod → MozReview Request: testing: install git-cinnabar in testing environment; r=smacleod
Assignee

Comment 31

4 years ago
Comment on attachment 8620014 [details]
MozReview Request: git-mozreview: git command for interacting with MozReview (bug 1153053); r?glandium, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/6863/diff/2-3/
Attachment #8620014 - Attachment description: MozReview Request: git-mozreview: git command for submitting to mozreview (bug 1153053) → MozReview Request: git-mozreview: git command for interacting with MozReview (bug 1153053); r?glandium, dminor
Attachment #8620014 - Flags: review?(mh+mozilla)
Attachment #8620014 - Flags: review?(dminor)
Assignee

Comment 32

4 years ago
Comment on attachment 8708623 [details]
MozReview Request: testing: install git-cinnabar in testing environment; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31127/diff/2-3/
Assignee

Comment 33

4 years ago
Comment on attachment 8620014 [details]
MozReview Request: git-mozreview: git command for interacting with MozReview (bug 1153053); r?glandium, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/6863/diff/3-4/
Assignee

Comment 34

4 years ago
https://reviewboard.mozilla.org/r/6863/#review28435

Something I just considered that I have no clue what the behavior of cinnabar is...

What happens when you are pushing a commit to MozReview that has already landed in e.g. central but hasn't been pushed to the review repo yet? Presumably we'll record this commit as part of the push and attempt to flag it for review. This is obviously wrong. In Mercurial, we look at phases to determine what changesets to review. But I'm not sure the best way to do this in Git. Should we be looking at the refs by looking at ancestors of HEAD and stopping at the first commit that has a ref attached?

Once I have the unified Firefox Mercurial repository established later this quarter, I'll likely set up synchronization so Try and the Gecko MozReview repos automatically get its changesets. That way people don't have to push changesets that have already landed. Long term, I'd like MozReview and Try to be "headless repos" based on top of the unified Mercurial repository. Both of these should enable "review what you push" and make the figuring out which commits to turn into reviews problem largely go away.
(In reply to Gregory Szorc [:gps] from comment #34)
> https://reviewboard.mozilla.org/r/6863/#review28435
> 
> Something I just considered that I have no clue what the behavior of
> cinnabar is...
> 
> What happens when you are pushing a commit to MozReview that has already
> landed in e.g. central but hasn't been pushed to the review repo yet?
> Presumably we'll record this commit as part of the push and attempt to flag
> it for review. This is obviously wrong.

Fair point. You can differentiate between the pushed commits already known on a mercurial branch and the pushed commits where the mercurial data had to be created during the push, albeit a little convoluted.

From the mercurial changeset you get from the git commit with store.hg_changeset, get the corresponding git commit again with store.changeset_ref. Commits where the mercurial data had to be created have a type of LazyString, while the others are str. However, that's something I'm going to change in the near future, so it would be better to futureproof the type check. Currently, LazyString is not related to str, but in the future, I want to use a subclass of str with a different name, so checking for type(store.changeset_ref(store.hg_changeset(foo))) != str is probably the best option.
Comment on attachment 8620014 [details]
MozReview Request: git-mozreview: git command for interacting with MozReview (bug 1153053); r?glandium, dminor

https://reviewboard.mozilla.org/r/6863/#review28443

::: git/commands/git-mozreview:146
(Diff revision 4)
> +        args.append('%s~1..%s' % (commits[0], commits[-1]))

You want ^@ instead of ~1, but somehow that doesn't work with .., so you need '^%s^@' % commits[0] and commits[-1].

The difference is that if commits[0] is a merge commit, you'd get the commits from the merged branch as well.

Another option is to to do git log --boundary commits[0]..commits[-1]. In that case commits[0] will have a '-' prefix in the output.

::: git/commands/git-mozreview:204
(Diff revision 4)
> +    remote_name = git_config['mozreview.remote']
> +    key = 'remote.%s.url' % remote_name
> +    if key not in git_config:
> +        print('error: %s is not a defined remote' % remote_name)
> +        return 1

You could have a sensible default here, without requiring a remote at all.

::: git/commands/git-mozreview:212
(Diff revision 4)
> +    if not url.startswith('hg::'):
> +        print('error: we only support pushing to hg:: remotes')
> +        return 1

Check for hg:// as well. It's an alternate form that can end up in people's configuration on windows. Unfortunately, that requires munging. See https://github.com/glandium/git-cinnabar/wiki/Windows-Support for the gory details (and the munge_url function in cinnabar.hg).

::: git/commands/git-mozreview:231
(Diff revision 4)
> +    if url.scheme == 'ssh':
> +        print('error: pushing to MozReview via SSH is not supported')
> +        return 1
> +

I guess some of this logic would be better shared with the mercurial support for mozreview pushes.

::: git/commands/git-mozreview:238
(Diff revision 4)
> +        res, output = get_output(['git', 'rev-list', args.refspec])

If the given refspec is a ref instead of a range, you're going to be rev-listing the whole history. OTOH, passing a range doesn't make sense to me. Push a committish, and negotiation with the server will figure the right range. You then just need to validate the commit exists. You can use git rev-parse --revs-only --validate.

::: git/commands/git-mozreview:263
(Diff revision 4)
> +    # We *always* push HEAD. Selection of what to actually submit for review is
> +    # determined from the command arguments. This could be changed. But for now
> +    # it is the easiest implementation.

O_o see comment above

::: git/commands/git-mozreview:271
(Diff revision 4)
> +        raise AbortError('error performing cinnabar push; please report this bug')

Technically, you can end up with a "Unknown command: python" error from cinnabar if the version is too old. It might be worth making the error message a little more verbose, inviting to make sure cinnabar is up-to-date, etc.

::: git/commands/git-mozreview:496
(Diff revision 4)
> +        url = '%s/autoreview/mozreviewreviewrepos' % args.mercurial_url

This seems to be one of those things that's not supported yet on the prod server, so I'll guess this exposes the root commits?

::: git/hooks/commit-msg-mozreview:23
(Diff revision 4)
> +    This is ported from hgrb.util.genid(). We can't share code easily because
> +    the Python environment from Git hooks isn't well defined.

You could move the hook code into a subcommand of the git mozreview tool, and call that from the hook.

::: git/tests/test-mozreview-commit-selection.t:167
(Diff revision 4)
> +because that's an empty set. We can't use HEAD~2..HEAD because that's 2 commits. Git
> +doesn't appear to provide an intuitive way to select a single commit or a range of
> +commits.)

HEAD^!

It feels to me this could use some more sharing of the mozreview workflow with the mercurial code, but I don't know that code. It just feels wrong to have code that feels VCS-agnostic being in here.
Attachment #8620014 - Flags: review?(mh+mozilla)
Assignee

Comment 37

4 years ago
https://reviewboard.mozilla.org/r/6863/#review28443

> This seems to be one of those things that's not supported yet on the prod server, so I'll guess this exposes the root commits?

Yes. The command is there, just only on the wire protocol. It exposes rev 0's SHA-1. The series in bug 1234413 exposes the web service endpoint.
Assignee

Comment 38

4 years ago
https://reviewboard.mozilla.org/r/6863/#review28443

> You could move the hook code into a subcommand of the git mozreview tool, and call that from the hook.

I initially didn't want to do this because of new process overhead. But the hook can be a shell script and I do like having all the Python code in one place. It even prevents copying of `genid()`.
Assignee

Comment 39

4 years ago
Comment on attachment 8708623 [details]
MozReview Request: testing: install git-cinnabar in testing environment; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31127/diff/3-4/
Assignee

Comment 41

4 years ago
Comment on attachment 8620014 [details]
MozReview Request: git-mozreview: git command for interacting with MozReview (bug 1153053); r?glandium, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/6863/diff/4-5/
Attachment #8620014 - Flags: review?(mh+mozilla)
Assignee

Comment 42

4 years ago
Comment on attachment 8620014 [details]
MozReview Request: git-mozreview: git command for interacting with MozReview (bug 1153053); r?glandium, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/6863/diff/5-6/
Assignee

Comment 43

4 years ago
Comment on attachment 8620014 [details]
MozReview Request: git-mozreview: git command for interacting with MozReview (bug 1153053); r?glandium, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/6863/diff/6-7/
Assignee

Comment 44

4 years ago
Comment on attachment 8620014 [details]
MozReview Request: git-mozreview: git command for interacting with MozReview (bug 1153053); r?glandium, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/6863/diff/7-8/
Comment on attachment 8710750 [details]
MozReview Request: reviewboard: allow fake ids file path to be passed in; r=dminor

https://reviewboard.mozilla.org/r/31859/#review28657
Attachment #8710750 - Flags: review?(dminor) → review+
Attachment #8620014 - Flags: review?(mh+mozilla)
Comment on attachment 8620014 [details]
MozReview Request: git-mozreview: git command for interacting with MozReview (bug 1153053); r?glandium, dminor

https://reviewboard.mozilla.org/r/6863/#review29051

I went through this more thoroughly than last time. I still skipped over reviewboard interaction code. It still feels that some of this code ought to be shared with the corresponding reviewboard interaction code for mercurial, but not knowing that code, that's just that: a feeling.

::: git/cinnabar-debug-push.py:46
(Diff revisions 4 - 8)
> +        ui.readconfig(p, trust=True)

I guess you need that trust=True, and that my change in cinnabar is useless. I might as well remove it.

::: git/cinnabar-debug-push.py:75
(Diff revisions 4 - 8)
> +try:

if __name__ == '__main__':

::: git/commands/git-mozreview:204
(Diff revisions 4 - 8)
>          # lines are surrounded by double quotes.
>          line = line[1:-1]

I didn't pay attention in the previous round, but you don't need to do that if you don't put the double quotes there in the first place in the --format argument.

::: git/commands/git-mozreview:209
(Diff revisions 4 - 8)
> +        commit = commit.lstrip('-')

You actually don't need this because %H doesn't contain the boundary marker. The boundary marker only appears when not using a format string (or when the format string contains %m)

::: git/commands/git-mozreview:265
(Diff revisions 4 - 8)
> -    remote_name = git_config['mozreview.remote']
> +    remote_name = git_config.get('mozreview.remote', 'review')
>      key = 'remote.%s.url' % remote_name
>      if key not in git_config:
> -        print('error: %s is not a defined remote' % remote_name)
> +        raise AbortError('no review remote defined; run `git mozreview configure`')

Why not just push to hg::https://reviewboard-hg.mozilla.org/autoreview if there's no remote setup?

::: git/commands/git-mozreview:266
(Diff revisions 4 - 8)
>      key = 'remote.%s.url' % remote_name

you should check for remote.%s.pushurl first.

::: git/commands/git-mozreview:278
(Diff revisions 4 - 8)
> +        url_s = urlparse.urlunparse(munge_url(url))
>  
>      try:
>          url = urlparse.urlparse(url_s)

why unparse it to reparse it right after?

::: git/commands/git-mozreview:286
(Diff revisions 4 - 8)
> -        print('warning: unknown scheme: %s' % url.scheme)
> +        raise AbortError('unknown scheme: %s' % url.scheme)

s/unknown/unsupported/ ?

::: git/commands/git-mozreview:294
(Diff revisions 4 - 8)
> -    requested_commits = []
> +    push_commit = resolve_commitish(args.commit)
>  
> -    if args.refspec:
> -        res, output = get_output(['git', 'rev-list', args.refspec])
> +    requested_commits = None
> +    if args.review_commit:
> +        requested_commits = [resolve_commitish(args.review_commit)]
> +    elif args.revisions:
> +        res, output = get_output(['git', 'rev-list', '--max-count=100', args.revisions])

Now that I have a better understanding of what you're trying to do, I think this can be simplified both from the user and code perspectives.

Your current code does this:
- expect the user to pick between two options depending if they want one or more commits reviewed.
- get a corresponding list of commits, somehow cut at the 100th.
- push
- get an intersection of that list of commits with the pushed (new) commits
- get info about them.

In the process, you essentially run rev-list/log twice for the same set of commits.

What I think you should do instead is this:
- give the user only _one_ option.
- get a corresponding list of commits, with `git log --format=%H\t%p\t%s --boundary --no-walk $user_input`. If $user_input is a single commit/ref, it will return one sha1. If it's a range, it will return the sha1s of the commits in the range.
- if the corresponding list is larger than 100 commits, throw an error. Arbitrarily cutting at 100 commits is not really nice, better to tell people their range is too big. (can use --max-count=101 and throw if the count is 101)
- push
- get the intersection with the pushed commits
- cross reference with the subjects gotten from the first rev-list, or if there was no list of commits to review from the user, do a `git log --format=%H\t%p\t%s --no-walk [list of all pushed (new) commits]` (using --stdin can be better to pass the list)

::: git/commands/git-mozreview:389
(Diff revisions 4 - 8)
>      git_commits = []
>      hg_changesets = []
>      hg2git = {}
>  
> -    for git_commit, hg_changeset in output['commits']:
> +    for git_commit, hg_changeset, new_data in output['commits']:
>          if git_commit not in review_commits:
>              continue
>  
>          git_commits.append(git_commit)
>          hg_changesets.append(hg_changeset)
>          hg2git[hg_changeset] = git_commit

hg2git = {
    hg_changeset: git_commit
    for git_commit, hg_changeset, new_data in output['commits']
    if git_commit in review_commits
}
hg_changesets = hg2git.keys()
git_commits = hg2git.values()

::: git/commands/git-mozreview:412
(Diff revisions 4 - 8)
>              print('hint: limit reviewed commits by specifying a refspec')

You should give the name of the option to use in that case.

::: git/commands/git-mozreview:653
(Diff revisions 4 - 8)
> +            # Ignore the inline git diff.
> +            if line.startswith('diff --git'):
> +                break

You should add a test for this, using `git commit -v`

::: git/hooks/commit-msg-mozreview:9
(Diff revisions 4 - 8)
> -import os
> +git mozreview commit-msg-hook $@

"$@", with double quotes.

::: git/tests/test-mozreview-commit-selection.t:237
(Diff revisions 4 - 8)
> -  abort: error performing cinnabar push; please report this bug
> +  abort: error performing cinnabar push: Pushing merges is not supported yet

Note that considering your test is not using a fixed version of cinnabar (aiui), this will break when cinnabar starts supporting pushing merges. You /may/ want to handle this by throwing the same error if you detect merges in the set of pushed commits.
Assignee

Comment 47

4 years ago
https://reviewboard.mozilla.org/r/6863/#review29051

There likely is room for consolidation. However, there are just enough parts that are different that I think inventing an abstraction layer would be a lot of work. I'm OK having the implementations mostly independent so we can see what happens. Then we can consolidate later, once we know where it makes sense to do so.

> Why not just push to hg::https://reviewboard-hg.mozilla.org/autoreview if there's no remote setup?

The autoreview repo isn't actually a pushable repo: it exposes a capability saying it lists known review repos. While we could employ server magic to make it pushable, I figured this would confuse tools that keep track of remote refs, as it would be weird to have multiple repos appear to exist at the same URL.
Assignee

Comment 48

4 years ago
https://reviewboard.mozilla.org/r/6863/#review29051

> Now that I have a better understanding of what you're trying to do, I think this can be simplified both from the user and code perspectives.
> 
> Your current code does this:
> - expect the user to pick between two options depending if they want one or more commits reviewed.
> - get a corresponding list of commits, somehow cut at the 100th.
> - push
> - get an intersection of that list of commits with the pushed (new) commits
> - get info about them.
> 
> In the process, you essentially run rev-list/log twice for the same set of commits.
> 
> What I think you should do instead is this:
> - give the user only _one_ option.
> - get a corresponding list of commits, with `git log --format=%H\t%p\t%s --boundary --no-walk $user_input`. If $user_input is a single commit/ref, it will return one sha1. If it's a range, it will return the sha1s of the commits in the range.
> - if the corresponding list is larger than 100 commits, throw an error. Arbitrarily cutting at 100 commits is not really nice, better to tell people their range is too big. (can use --max-count=101 and throw if the count is 101)
> - push
> - get the intersection with the pushed commits
> - cross reference with the subjects gotten from the first rev-list, or if there was no list of commits to review from the user, do a `git log --format=%H\t%p\t%s --no-walk [list of all pushed (new) commits]` (using --stdin can be better to pass the list)

Oh, I didn't know about `--no-walk`. That does appear to be exactly what I want!

> hg2git = {
>     hg_changeset: git_commit
>     for git_commit, hg_changeset, new_data in output['commits']
>     if git_commit in review_commits
> }
> hg_changesets = hg2git.keys()
> git_commits = hg2git.values()

I like the conciseness. We have to use an OrderedDict since we iterate over git_commits later for presenation purposes.
Assignee

Comment 49

4 years ago
Comment on attachment 8708623 [details]
MozReview Request: testing: install git-cinnabar in testing environment; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31127/diff/4-5/
Assignee

Comment 50

4 years ago
Comment on attachment 8710750 [details]
MozReview Request: reviewboard: allow fake ids file path to be passed in; r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31859/diff/1-2/
Attachment #8710750 - Attachment description: MozReview Request: reviewboard: allow fake ids file path to be passed in; r?dminor → MozReview Request: reviewboard: allow fake ids file path to be passed in; r=dminor
Assignee

Comment 51

4 years ago
Comment on attachment 8620014 [details]
MozReview Request: git-mozreview: git command for interacting with MozReview (bug 1153053); r?glandium, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/6863/diff/8-9/
Attachment #8620014 - Flags: review?(mh+mozilla)
Assignee

Comment 52

4 years ago
Comment on attachment 8620014 [details]
MozReview Request: git-mozreview: git command for interacting with MozReview (bug 1153053); r?glandium, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/6863/diff/9-10/
Attachment #8620014 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8620014 [details]
MozReview Request: git-mozreview: git command for interacting with MozReview (bug 1153053); r?glandium, dminor

https://reviewboard.mozilla.org/r/6863/#review29275

::: git/commands/git-mozreview:203
(Diff revisions 8 - 10)
> +        # Stop at merge commits.
>          if len(parents) > 1:
>              break

I think it would be better to explicitly fail here, instead of stopping. If the range the user passed contains a merge, they may have messed up their range, or their tree.

::: git/commands/git-mozreview:259
(Diff revisions 8 - 10)
> -    if key not in git_config:
> +    for key in ('remote.%s.pushurl' % remote_name, 'remote.%s.url' % remote_name):

You could move the formatting in the loop.

for key in ('remote.%s.pushurl', 'remote.%s.url'):
    if key % remote_name in git_config:
        break

::: git/commands/git-mozreview:262
(Diff revisions 8 - 10)
> +    else:

I'm torn here. On one hand, this makes the code simpler. On the other hand, for .. else is an underknown and confusing feature of python.

::: git/commands/git-mozreview:572
(Diff revisions 8 - 10)
> -        print('searching for appropriate review repository...')
> +        ui.write('searching for appropriate review repository...\n')

Why switch those prints to ui.write/ui.warn, but not others?

::: git/commands/git-mozreview:696
(Diff revisions 8 - 10)
>      sp.add_argument(
> -            '-c',
> -            help='commit-ish of single commit to review',
> -            dest='review_commit')
> +            'revisions',
> +            help='commit-ish to review',
> +            nargs='?')

I like the simplicity of the new arguments :)
Assignee

Comment 54

4 years ago
Comment on attachment 8708623 [details]
MozReview Request: testing: install git-cinnabar in testing environment; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31127/diff/5-6/
Assignee

Comment 55

4 years ago
Comment on attachment 8710750 [details]
MozReview Request: reviewboard: allow fake ids file path to be passed in; r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31859/diff/2-3/
Assignee

Comment 56

4 years ago
Comment on attachment 8620014 [details]
MozReview Request: git-mozreview: git command for interacting with MozReview (bug 1153053); r?glandium, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/6863/diff/10-11/
Attachment #8620014 - Attachment description: MozReview Request: git-mozreview: git command for interacting with MozReview (bug 1153053); r?glandium, dminor → MozReview Request: git-mozreview: git command for interacting with MozReview (bug 1153053); r?glandium, dminor, smacleod
Attachment #8620014 - Flags: review?(smacleod)
Assignee

Comment 57

4 years ago
Comment on attachment 8708623 [details]
MozReview Request: testing: install git-cinnabar in testing environment; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31127/diff/6-7/
Assignee

Comment 58

4 years ago
Comment on attachment 8710750 [details]
MozReview Request: reviewboard: allow fake ids file path to be passed in; r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31859/diff/3-4/
Assignee

Comment 59

4 years ago
Comment on attachment 8620014 [details]
MozReview Request: git-mozreview: git command for interacting with MozReview (bug 1153053); r?glandium, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/6863/diff/11-12/
Comment on attachment 8620014 [details]
MozReview Request: git-mozreview: git command for interacting with MozReview (bug 1153053); r?glandium, dminor

https://reviewboard.mozilla.org/r/6863/#review29653

Overall, looks good.

My main convern is whether this will work properly on Windows. I'm guessing the number of people who use git and develop on windows is small, so if it doesn't work, I'm fine with landing it as is. We should just file a bug, add a note in the docs, and add a platform check to the mozreview command which will prevent it from running on windows.

::: docs/mozreview/commits.rst:329
(Diff revision 12)
> +To control which commits are submitted for review, specify a commit-ish

commit-ish?

::: git/cinnabar-debug-push.py:30
(Diff revision 12)
> +

nit: two empty lines before function definition

::: git/cinnabar-debug-push.py:74
(Diff revision 12)
> +

nit: two blank lines

::: git/commands/git-mozreview:239
(Diff revision 12)
> +    if 'bz.username' not in git_config:

Please make a list of keys you expect to find in git_config and iterate on that to check for presence to remove some code duplication here.

::: git/commands/git-mozreview:278
(Diff revision 12)
> +    if url.scheme not in ('http', 'https', 'ssh'):

If we only support http or https, why not combine this code with the following code which disallows ssh.

Also, do we really want to support http as well as https?

::: git/commands/git-mozreview:296
(Diff revision 12)
> +        if len(requested_commits) == 101:

Please define a constant for this, and use > rather than ==.

::: git/commands/git-mozreview:307
(Diff revision 12)
> +    if res.status_code != 200:

Maybe include the status_code in the message in case it is a 500?

::: git/commands/git-mozreview:330
(Diff revision 12)
> +    with tempfile.NamedTemporaryFile('wb') as fh:

Have you tested this on windows? According to [1] you may not be able to use this name to open the file from another process on that platform.

[1] https://docs.python.org/2.7/library/tempfile.html#tempfile.NamedTemporaryFile

::: git/commands/git-mozreview:384
(Diff revision 12)
> +    # Now filter against what is allowed to be reviewed.

Should we abort early here if for some reason review_commits is empty?

::: git/commands/git-mozreview:437
(Diff revision 12)
> +        ui.write('error: non-200 HTTP status code when submitting series\n')

Please output the actual status code.

::: git/commands/git-mozreview:505
(Diff revision 12)
> +                elif 'error' in item:

Please add a final else here to handle something unexpected in an item.
Attachment #8620014 - Flags: review?(dminor)
Assignee

Comment 61

3 years ago
https://reviewboard.mozilla.org/r/6863/#review29653

> commit-ish?

That is Git speak. According to `git help glossary`:

commit-ish (also committish)
           A commit object or an object that can be recursively dereferenced to a commit object. The following are all commit-ishes: a commit object, a tag object that points to a commit object, a
           tag object that points to a tag object that points to a commit object, etc.
Assignee

Comment 62

3 years ago
https://reviewboard.mozilla.org/r/6863/#review29653

> If we only support http or https, why not combine this code with the following code which disallows ssh.
> 
> Also, do we really want to support http as well as https?

We need to support plain text http in the test environment. Production will HTTP 301 http:// to https:// on the load balancer.
Assignee

Comment 63

3 years ago
Comment on attachment 8708623 [details]
MozReview Request: testing: install git-cinnabar in testing environment; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31127/diff/7-8/
Assignee

Comment 64

3 years ago
Comment on attachment 8710750 [details]
MozReview Request: reviewboard: allow fake ids file path to be passed in; r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31859/diff/4-5/
Assignee

Comment 65

3 years ago
Comment on attachment 8620014 [details]
MozReview Request: git-mozreview: git command for interacting with MozReview (bug 1153053); r?glandium, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/6863/diff/12-13/
Attachment #8620014 - Attachment description: MozReview Request: git-mozreview: git command for interacting with MozReview (bug 1153053); r?glandium, dminor, smacleod → MozReview Request: git-mozreview: git command for interacting with MozReview (bug 1153053); r?glandium, dminor
Attachment #8620014 - Flags: review?(smacleod) → review?(dminor)
Comment on attachment 8620014 [details]
MozReview Request: git-mozreview: git command for interacting with MozReview (bug 1153053); r?glandium, dminor

https://reviewboard.mozilla.org/r/6863/#review29977
Attachment #8620014 - Flags: review?(dminor) → review+
Assignee

Comment 67

3 years ago
https://hg.mozilla.org/hgcustom/version-control-tools/rev/77860597587a6b2cd5b961657b0e75f2befce065
git-mozreview: git command for interacting with MozReview (bug 1153053); r=glandium, dminor
Assignee

Comment 68

3 years ago
This has landed.

I'd link to the docs on RTD, but RTD appears to be experiencing an outage of some kind and the docs are failing to generate due to server-side foo.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Assignee

Comment 69

3 years ago
https://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/install-git.html#mozreview-install-git finally generated.

But you won't be able to submit to Git until bug 1244835 is resolved, since Git does all pushing over HTTP.
Assignee

Updated

3 years ago
Duplicate of this bug: 1140576
Assignee

Updated

3 years ago
Blocks: 1246820
Assignee

Updated

3 years ago
Blocks: 1239815
Blocks: 1247171
Blocks: 1247172
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.