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

Thu Jan 23 21:08:26 PST 2020

fhahn planned changes to this revision.
fhahn added a comment.

In D73075#1835629 <https://reviews.llvm.org/D73075#1835629>, @mstorsjo wrote:

> I tested using this for applying one patch (D73139 <https://reviews.llvm.org/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 <https://reviews.llvm.org/D73075#1833933>, @thakis wrote:

> Have you seen D67747 <https://reviews.llvm.org/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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73075/new/

https://reviews.llvm.org/D73075