Details
- Reviewers
ton valerio.bozzolan - Group Reviewers
O1: Blessed Committers - Maniphest Tasks
- T15249: Generate [From, Subject, Date] fields in download raw .diff URL to support "git am" and friends
Download Raw Diff from revisions with and without commits (created using arc-diff and using "upload patch")
but be sure that your Differential revision has the Repository.
Check that the diff applies cleanly using both git am and GNU patch.
Diff Detail
- Repository
- rP Phorge
- Branch
- more-php8
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 692 Build 692: arc lint + arc unit
Event Timeline
whut
$ phorge (master)> arc patch D25359 INFO Base commit is not in local repository; trying to fetch. Branch name arcpatch-D25359 already exists; trying a new name. Branch name arcpatch-D25359_1 already exists; trying a new name. Created and checked out branch arcpatch-D25359_2. INFO Base commit is not in local repository; trying to fetch. Checking patch src/workflow/ArcanistExportWorkflow.php... error: src/workflow/ArcanistExportWorkflow.php: does not exist in index Checking patch src/parser/ArcanistBundle.php... error: src/parser/ArcanistBundle.php: does not exist in index Patch Failed! Usage Exception: Unable to apply patch!
looks like arc patch tries to patch D25358 first, because this diff "Depends on" that one - but that one is in arcanist, not phorge.
Nice. Let's file a Task for that. We can work with arc patch D25359 --skip-dependencies in the meanwhile
| src/applications/differential/controller/DifferentialRevisionViewController.php | ||
|---|---|---|
| 1087 | Interestingly, if the patch is not attached to any repository, the toGitPatch() is not executed but just toUnifiedDiff() that does not generate these additional fields. I think this is under control so I just expanded the test plan. | |
| src/applications/differential/controller/DifferentialRevisionViewController.php | ||
|---|---|---|
| 1083 | For some reasons from web I'm not able to upload a Diff with a non-empty authorship. Tips? | |
I noticed that Phorge exports the date in this format:
Date: 2023-08-02 23:23:38 (UTC+2)
While git exports the date in this format:
Thu Jun 29 23:23:28 2023 +0200
Is this known? Should we uniform these?
Premising that git am imports this perfectly. But maybe because it's broad in what it accepts, and we could be strict about what we create.
| src/applications/differential/controller/DifferentialRevisionViewController.php | ||
|---|---|---|
| 1083 | If I understand correctly, not exposing the author's email from a normal web "patch upload" is a feature, since we do not expose the author's email if it does not come from a commit. Since only the commit is supposed to be public. But, probably we should parse and import the email, and expose it, if the user is manually uploading that in the patch. Importing the email from the web upload, is probably is OK to be considered only in a follow-up Task. | |
@avivey What do you think about this date thing? Note: I completely forgot how computers work. By the way, it works, so... \o/
IDK - this was code was never explicitly about git am - I think it was about patch, which started in the stone age and predates things like "formal specification".
git am is also a strange animal, having a format that looks like email, but I'm guessing no modern email service will allow you to do something like git format-patch | send-email (ok, maybe whatever replace Pine does that).
Also possibly git am is using whatever version of patch and/or date is installed on the machine. Or not.
I'm guessing "date/time parsing" is mostly a solved problem in the *nix world, and they accept basically anything now:
$ date -u -d 'next fri 8:21z + 2 days 5 hours 2 minutes' Sun 27 Aug 13:23:00 UTC 2023
When you get to the VCS interfaces of Phorge, a lot of the code is in the "this seems to work, and here's a special case for git 1.17.1" level of correctness.
Also, I think the timezone in that call is coming from the user that downloads the patch, which is also "technically incorrect".
In the ideal world, this patch wouldn't be in the path of T15535 - there'll be a git fetch something command, and download raw would remain an obscure feature that's never removed because it doesn't bother anyone.
| src/applications/differential/controller/DifferentialRevisionViewController.php | ||
|---|---|---|
| 1083 | Parsing use email from web-submitted patch is waaay out of scope for this change. Probably falls under T15535. | |