1171981 - Autoland should fold imported pullrequests before pushing for review

Status: RESOLVED FIXED

(Conduit Graveyard :: Transplant, defect)

Description

[Dan Minor [:dminor]]

Initially we're planning to review only squashed/folded commits from github to avoid having to deal with history rewriting on the git side.

Comment 1

[Dan Minor [:dminor]]

Attachment: MozReview Request: autoland: fold imported pullrequests (bug 1171981) r?gps

autoland: fold imported pullrequests; (bug 1171981); r?gps

For now we will fold imported pullrequests into a single commit so we don't
have to worry about history editing on the git side. This fits well with the
common github pattern of pushing additional commits for review fixes rather
than amended existing commits.

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 8616005 [details]
MozReview Request: autoland: fold imported pullrequests (bug 1171981) r?gps

https://reviewboard.mozilla.org/r/10387/#review9125

::: autoland/autoland/transplant.py:50
(Diff revision 1)
> +            cmds.append(['hg', 'bookmark', '-f', 'transplant'])

Bookmarks can be fickle. I'd encourage you to do this without bookmarks.

Assuming this function can't be called in parallel, fold ``<base>::`` and the result will be ``tip``.

If this function can be called in parallel, all bets are off: you may want to rewrite this code as a Mercurial command that grabs a repo lock.

Comment 3

[Dan Minor [:dminor]]

Comment on attachment 8616005 [details]
MozReview Request: autoland: fold imported pullrequests (bug 1171981) r?gps

autoland: fold imported pullrequests (bug 1171981); r?gps

For now we will fold imported pullrequests into a single commit so we don't
have to worry about history editing on the git side. This fits well with the
common github pattern of pushing additional commits for review fixes rather
than amended existing commits.

This also removes the use of bookmarks for importing pullrequests.

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8616005 [details]
MozReview Request: autoland: fold imported pullrequests (bug 1171981) r?gps

https://reviewboard.mozilla.org/r/10387/#review9419

::: autoland/autoland/transplant.py:34
(Diff revision 2)
>      cmds = [['hg', 'update', '--clean'],
>              ['hg', 'strip', '--no-backup', '-r', 'draft()'],
>              ['hg', 'pull'],
> -            ['hg', 'update', 'central'],
> +            ['hg', 'update'],]

The behavior of `hg update` without any revision is to update to the tip of the current branch. If there are multiple heads for the current branch (as is often the case when not using branches for development), behavior is wonky. If possible, it's best to always specify an explicit revision to update to.

Also, `hg pull` without an explicit remote or revision is kinda wonky. We should at least pull in a revision and update to it, or a known symbol associated with that remote (e.g. "central" if useing firefoxtree).

::: autoland/autoland/transplant.py:53
(Diff revision 2)
> +                     'folded pullrequest for review'])

Do we not want to preserve the original commit messages? We can get them easily by running:

  $ hg log -r '::. and draft()' -T '{desc}\n'
  
Also, `ancestor(draft())` pulls in all ancestors of all draft commits. That's a wide net. Although it should be fine since all draft changesets are stripped. Use the revset in the command above.

Comment 5

[Dan Minor [:dminor]]

Comment on attachment 8616005 [details]
MozReview Request: autoland: fold imported pullrequests (bug 1171981) r?gps

autoland: fold imported pullrequests (bug 1171981) r?gps

For now we will fold imported pullrequests into a single commit so we don't
have to worry about history editing on the git side. This fits well with the
common github pattern of pushing additional commits for review fixes rather
than amended existing commits.

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 8616005 [details]
MozReview Request: autoland: fold imported pullrequests (bug 1171981) r?gps

https://reviewboard.mozilla.org/r/10387/#review9873

Almost. Ping me continuously to ensure you get speedy review. We want this to land soon!

::: autoland/autoland/transplant.py:89
(Diff revision 3)
>      cmds.append(['hg', '--config', 'bugzilla.userid=%s' % bzuserid,
>                         '--config', 'bugzilla.cookie=%s' % bzcookie,
>                         '--config', 'mozilla.ircnick=%s' % user,
> -                       'push', '--reviewid', str(bugid), '-r', 'transplant',
> +                       'push', '--reviewid', str(bugid), '-c', '.',
>                         'mozreview-push'])

As a follow-up, to avoid worrying about accidental disclosure of these settings, you can set the HGRCPATH environment variable to control the Mercurial config file to load. This config file can define the Bugzilla credentials then it can `%include /etc/mercurial/hgrc` to import the old system default hgrc. This will effectively allow you to overlay additional configs on top of /etc/mercurial/hgrc.

::: autoland/autoland/transplant.py:86
(Diff revision 3)
> +        cmds.append(['hg', 'fold', '-r', '::. and draft()', '-m', desc])

This /may/ blow out command length limitations if commit messages are long. You may want to use `-l` to grab the commit message from a file.

::: autoland/autoland/transplant.py:52
(Diff revision 3)
>          cmds.append(['hg', 'import', commit])

If `hg import` fails, the working directory may be polluted with untracked files. You should throw a `hg purge --all` into the list of commands to ensure you start with a clean working copy. Run this before `hg update -C`.

Comment 7

[Dan Minor [:dminor]]

Comment on attachment 8616005 [details]
MozReview Request: autoland: fold imported pullrequests (bug 1171981) r?gps

autoland: fold imported pullrequests (bug 1171981) r?gps

For now we will fold imported pullrequests into a single commit so we don't
have to worry about history editing on the git side. This fits well with the
common github pattern of pushing additional commits for review fixes rather
than amended existing commits.

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8616005 [details]
MozReview Request: autoland: fold imported pullrequests (bug 1171981) r?gps

https://reviewboard.mozilla.org/r/10387/#review10165

Looks good!