Page MenuHomePhorge

Generate more fields in Download Raw Diff
AcceptedPublic

Authored by avivey on Jul 26 2023, 04:43.
Tags
None
Referenced Files
F2310080: D25359.1721381924.diff
Thu, Jul 18, 09:38
Unknown Object (File)
Tue, Jul 9, 12:04
Unknown Object (File)
Mon, Jul 8, 13:48
Unknown Object (File)
Sun, Jul 7, 16:18
Unknown Object (File)
Sun, Jun 30, 15:30
Unknown Object (File)
Sun, Jun 30, 14:10
Unknown Object (File)
Sun, Jun 30, 14:08
Unknown Object (File)
Wed, Jun 26, 09:45

Details

Summary

To make the raw diff more compatible with git am:

  • Add From field (Author name and email from commit message, if available)
  • Add Date (Revision's creation date)
  • Add Subject field (Revision title)

Closes T15249.
Depends on D25358.

Test Plan

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

avivey requested review of this revision.Jul 26 2023, 04:43

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.

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?

@avivey What do you think about this date thing? Note: I completely forgot how computers work. By the way, it works, so... \o/

This revision is now accepted and ready to land.Aug 20 2023, 07:22

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.

hey, @ton - is this useful for you?