Page MenuHomePhabricator

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

Authored by fhahn on Jan 20 2020, 5:51 PM.

Details

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/.

Diff Detail

Event Timeline

fhahn created this revision.Jan 20 2020, 5:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2020, 5:51 PM

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 added inline comments.Jan 21 2020, 10:58 AM
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?

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

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 planned changes to this revision.Jan 23 2020, 9:07 PM

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.

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.

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 updated this revision to Diff 292750.Sep 18 2020, 4:18 AM
fhahn marked 5 inline comments as done.

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 abandoned this revision.Sep 18 2020, 4:18 AM
fhahn added inline comments.
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.