[utils] Add initial llvm-patch helper to manage patches.

[fhahn]

Summary

The first version of llvm-patch provides an 'apply' subcommand, that
fetches the latest diff for a given differential ID and applies it to
the working directory, using the commit message from Phabricator.

It requires a valid ~/.arcrc file with your API token, e.g.

{

"hosts": {
  "https://reviews.llvm.org/api/": {
    "token": "cli-thisismyapikey"
  }
}

}

To get your API token, visit https://reviews.llvm.org/conduit/login/.

[merge_guards_bot]

Unit tests: pass. 62027 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[paquette]

llvm/utils/llvm-patch

42

Can we check if .arcrc exists before opening it?

63–64

• If data['result'] is an empty string, this will still return something. Should there also be a check for if data['result'] = ''?
• Is there only data in 'error_info' when data['result'] is empty? Would it make more sense to just check if data['error_info'] has something in it?
• sys.exit(1) on error?

63–65

If data['result'] is an empty string, this will still return something. Should there also be a check for if data['result'] is empty?

78

This will crash if args.ID is not an integer.

83–85

Can we use logging for this?

E.g. https://docs.python.org/3.8/library/logging.html

Might make sense to add a function like log_error_and_exit?

134

Can we verify the format of the ID in the parser itself using an argparse custom type? Then if someone types in some garbage, you can just die + produce a useful message in parse_args().

138–141

Can we use argparse.ArgumentError for this?

[thakis]

Have you seen D67747? Probably makes sense to combine forces there.

[mstorsjo]

I tested using this for applying one patch (D73139), but ran into the following issues:

• The patch that differential.getrawdiff returns had

--- a/lld/test/COFF/comdat-gcc-compatibility.s
+++ b/lld/test/COFF/comdat-gcc-compatibility.s

for a newly added file. (For a newly added file, the git diff should normally say --- /dev/null.) This causes git apply to fail with not finding the file to patch. As the revision has been updated now that it has been pushed, it no longer does that (as it picks a newer diff). (I'm not really sure how the original patch was made and uploaded though, as the "Download raw diff" button on the web (for the last revision before committing it) gives a diff that contains

Index: lld/test/COFF/comdat-gcc-compatibility.s
===================================================================
--- lld/test/COFF/comdat-gcc-compatibility.s
+++ lld/test/COFF/comdat-gcc-compatibility.s

• If I manually worked around this, to get a patch that git apply managed to commit, the newly added file still was missing, as git apply doesn't add it to the index, and the final git commit -a doesn't pick it up unless specifically added
• At this point, I still had to manually word-wrap the commit message to fit normal git standards. (Not sure if arcanist does this automatically either though.)
• I looked into the right conduit calls for fetching the user name and email for getting a proper git author field. Finding the user name seems fairly straightforward using differential.revision.search followed by user.search. That does give the realname of the author, but it doesn't give the email address. A later dig into this indicates that it isn't really doable, https://stackoverflow.com/a/25569692/3115956 saying "Phabricator does not expose email addresses, by design. We consider this information to be private.". Despite this, the email is easily visible if e.g. browsing the mails that it sends out...

[fhahn]

In D73075#1835629, @mstorsjo wrote:

I tested using this for applying one patch (D73139), but ran into the following issues:

• The patch that differential.getrawdiff returns had

--- a/lld/test/COFF/comdat-gcc-compatibility.s
+++ b/lld/test/COFF/comdat-gcc-compatibility.s

for a newly added file. (For a newly added file, the git diff should normally say --- /dev/null.) This causes git apply to fail with not finding the file to patch. As the revision has been updated now that it has been pushed, it no longer does that (as it picks a newer diff). (I'm not really sure how the original patch was made and uploaded though, as the "Download raw diff" button on the web (for the last revision before committing it) gives a diff that contains

Index: lld/test/COFF/comdat-gcc-compatibility.s
===================================================================
--- lld/test/COFF/comdat-gcc-compatibility.s
+++ lld/test/COFF/comdat-gcc-compatibility.s

• If I manually worked around this, to get a patch that git apply managed to commit, the newly added file still was missing, as git apply doesn't add it to the index, and the final git commit -a doesn't pick it up unless specifically added

Thanks for giving it a try and for the feedback. There’s certainly much to improve and those are things that we should definitely handle properly.

• At this point, I still had to manually word-wrap the commit message to fit normal git standards. (Not sure if arcanist does this automatically either though.)

I’m not sure either, but I don’t think so. But if the review is created with arc from a got commit, the line endings will be preserved and applying the patch preserves the format as well. It might make sense to enforce a max line length in the script.

• I looked into the right conduit calls for fetching the user name and email for getting a proper git author field. Finding the user name seems fairly straightforward using differential.revision.search followed by user.search. That does give the realname of the author, but it doesn't give the email address. A later dig into this indicates that it isn't really doable, https://stackoverflow.com/a/25569692/3115956 saying "Phabricator does not expose email addresses, by design. We consider this information to be private.". Despite this, the email is easily visible if e.g. browsing the mails that it sends out...

That’s a bit unfortunate. Maybe it could still warn if there’s a mismatch between committer name and the author on phab.

In D73075#1833933, @thakis wrote:

Have you seen D67747? Probably makes sense to combine forces there.

No, thanks for the link! After a quick look it seems like it handles the upload side of things. I’ll take a closer look!

Thanks for all the comments! For now I’ll wait a bit to see if there are any objections to adding such a helper script in general.

[mstorsjo]

In D73075#1837867, @fhahn wrote:

That’s a bit unfortunate. Maybe it could still warn if there’s a mismatch between committer name and the author on phab.

Yeah, and maybe also fetch and print the original author name and maybe prepopulate the author field like "Real Name <mail@missing>" or something such, with a note to look up the proper email address.

I guess there's two cases where this tool would be used:

• Applying someone else's patch. In that case getting as much info as possible and a reminder to look up what to set in the author field is best
• Applying one's own patch. (This isn't something I'd ever do myself though.) In that case, if the author seems to be oneself, it's best to leave the author field in whichever form it's set up in the local git config.

[fhahn]

I finally had some time to address the outstanding comments. But given that there seems to be no consensus on llvm-dev that people actually want this, I won't try to push this any further for now.

In the current state, it works well enough for me so that I use it exclusively to fetch patches from Phab. The only weakness is that it does not support resolving conflicts as of yet.

In case anyone is interested in using the script, it is available on Github https://github.com/fhahn/llvm-project/blob/llvm-patch-script/llvm/utils/llvm-patch

[fhahn]

llvm/utils/llvm-patch

63–64

changed it to use data.get('result', '') and then checking the length

78

validated now during argparse, as suggested.

llvm/utils/llvm-patch

+#!/usr/bin/env python3
+
+"""
+This script uses the Phabricator API to manage patches. It is currently
+work-in-progress, please double-check the results before pushing changes.
+
+It requires a valid ~/.arcrc file with your API token, e.g.
+
+{
+  "hosts": {
+    "https://reviews.llvm.org/api/": {
+      "token": "cli-thisismyapikey"
+    }
+  }
+}
+
+To get your API token, visit https://reviews.llvm.org/conduit/login/.
+
+TODO:
+  * Support uploading patches.
+  * Better error handling.
+  * Python2 support?
+  * Use a better way to find raw diff IDs instead of using querydiffs.
+  * Support applying a stack of patches.
+"""
+
+import argparse
+import json
+import os.path
+import os
+import subprocess
+import sys
+import tempfile
+import re
+import urllib.parse
+import urllib.request
+import logging
+import shlex
+
+
+def log_error_and_exit(msg):
+    logging.error(msg)
+    sys.exit(1)
+
+
+def get_api_token():
+    """Get the Phabricator API token for reviews.llvm.org from ~/.arcrc."""
+
+    try:
+        with open(os.path.expanduser(os.path.join('~', '.arcrc')), 'rt') as f:
+            d = json.loads(f.read())
+            if 'hosts' not in d:
+                log_error_and_exit('No hosts in ~/.arcrc found')
+
+            d = d['hosts']
+            if 'https://reviews.llvm.org/api/' not in d:
+                log_error_and_exit(
+                    'No entry for "https://reviews.llvm.org/api/"'
+                    ' found in ~/.arcrc')
+            return d['https://reviews.llvm.org/api/']['token']
+    except BaseException:
+        log_error_and_exit('~/.arcrc not found')
+
+
+def api_request(endpoint, values):
+    values['api.token'] = get_api_token()
+    data = urllib.parse.urlencode(values)
+    data = data.encode('ascii')  # data should be bytes
+    req = urllib.request.Request(
+        'https://reviews.llvm.org/api/' + endpoint, data)
+    with urllib.request.urlopen(req) as response:
+        data = json.loads(response.read())
+        result = data.get('result', '')
+        if len(result) == 0:
+            log_error_and_exit(
+                'Missing or empty response, error info : {}'.format(
+                    data['error_info']))
+        return result
+
+
+def should_strip_line(s):
+    return s.startswith('Summary:') or s.startswith(
+        'Subscribers:') or s.startswith('Tags:')
+
+
+def apply_patch(args):
+    """
+    Fetches the latest diff for a given differential ID and applies it to the
+    current working directory.
+    """
+    revision_id = int(args.ID)
+
+    # Make sure there are no uncommitted changes.
+    ret = subprocess.call(['git', 'diff-index', '--quiet', 'HEAD'])
+    if ret != 0:
+        log_error_and_exit(
+            'There are uncommitted changes in the repository!\n'
+            'Can only apply a patch to a clean working directory.')
+
+    # Get list of diffs for the requested revision.
+    logging.info('Fetching information for D{}.'.format(revision_id))
+    revision_query = api_request('differential.query', {
+        'ids[0]': revision_id
+    })
+    if len(revision_query) != 1:
+        log_error_and_exit(
+            'Expected a single result for revision, but got {} results!'.format(
+                len(revision_query)))
+    if int(revision_query[0]['id']) != revision_id:
+        log_error_and_exit(
+            'Got revision result for unexpected ID (expected {} but got {})!'.format(
+                revision_id, revision_query[0]['id']))
+
+    diffs = revision_query[0]['diffs']
+    diff_id = diffs[0]
+    for i in range(1, len(diffs)):
+        if int(diffs[i - 1]) <= int(diffs[i]):
+            log_error_and_exit('Unexpected order of diff ids.')
+
+    # Fetch raw diff for latest diff id.
+    logging.info('Fetching diff with ID {}.'.format(diff_id))
+    rawdiff = api_request('differential.getrawdiff', {
+        'diffID': diff_id
+    })
+
+    # Get commit message and strip unnecessary lines (Summary:, Subscribers:
+    # and Tags" ).
+    commit_msg = api_request('differential.getcommitmessage', {
+        'revision_id': revision_id})
+    commit_msg = '\n'.join(
+        s for s in commit_msg.splitlines() if not should_strip_line(s))
+
+    # Write diff to file.
+    diff_file, path = tempfile.mkstemp(prefix='phabdiff', text=True)
+    with open(path, 'wt') as diff_file:
+        diff_file.write(rawdiff)
+
+    logging.info('Applying diff to working directory.')
+    with open(path, 'rt') as diff_file:
+        subprocess.check_call(
+            ['git', 'apply', '-3', '--intent-to-add'], stdin=diff_file)
+    os.remove(path)
+
+    # Commit all changed files.
+    subprocess.run(['git', 'commit', '-a', '--file=-'],
+                   input=commit_msg.encode(), check=True)
+
+    git_user_name = subprocess.run(['git',
+                                    'config',
+                                    '--get',
+                                    'user.name'],
+                                   check=True,
+                                   capture_output=True).stdout.decode().strip()
+    logging.info('Checking if git user name matches revision author')
+    revision_author = api_request('user.query',
+                                  {'phids[0]': revision_query[0]['authorPHID']})
+    author_name = revision_author[0]['realName']
+    if git_user_name != author_name:
+        logging.warning(
+            'Git user does not match revision author ({} != {})!'.format(
+                git_user_name, author_name))
+
+
+PHAB_ID_RE = re.compile(r'^D(\d+)$')
+
+
+def phabricator_id(s):
+    m = PHAB_ID_RE.match(s)
+    if not m:
+        msg = "%r is not a Phabricator patch ID (must start with D followed by an integer)" % s
+        raise argparse.ArgumentTypeError(msg)
+    return int(m.group(1))
+
+
+def main():
+    logging.basicConfig(format='%(levelname)s:%(message)s', level=logging.INFO)
+
+    parser = argparse.ArgumentParser()
+    subparsers = parser.add_subparsers(
+        title='subcommands')
+
+    apply_parser = subparsers.add_parser(
+        'apply',
+        help='Fetch the latest diff for the given differential ID from '
+             'reviews.llvm.org and apply it to the working directory.')
+    apply_parser.add_argument(
+        'ID',
+        help='Differential ID, e.g. D5678',
+        type=phabricator_id)
+    apply_parser.set_defaults(func=apply_patch)
+
+    args = parser.parse_args()
+    if not hasattr(args, 'func'):
+        log_error_and_exit('llvm-patch: A subcommand is required')
+
+    args.func(args)
+    logging.warning('This script is work-in-progress. Double-check results '
+                    'before pushing changes!')
+
+
+if __name__ == "__main__":
+    main()