Page MenuHomeFreeBSD

git-arc: Add -c flag to patch to commit the change
ClosedPublic

Authored by imp on May 7 2023, 5:34 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Apr 21, 10:42 AM
Unknown Object (File)
Sun, Apr 21, 10:37 AM
Unknown Object (File)
Sun, Apr 21, 10:37 AM
Unknown Object (File)
Sun, Apr 21, 10:37 AM
Unknown Object (File)
Sun, Apr 21, 10:37 AM
Unknown Object (File)
Sun, Apr 21, 10:37 AM
Unknown Object (File)
Fri, Apr 19, 8:42 PM
Unknown Object (File)
Wed, Apr 17, 12:22 PM

Details

Summary

git arc patch -c D1234 will download differential D1234, try to apply it
to the tree, and if successful will ask phab for the title and
summary. It will construct a commit message with that, the reviewers,
and the differential revision. It also tries its best to deduce the
proper 'author' to use for the commit, and warn if it thinks it has made
a bad choice.

Sponsored by: Netflix

Test Plan

Note: this replaces the rather not-good-for-us arc land which screws up too many things.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
imp added reviewers: markj, emaste, jhb, kevans.
imp edited the test plan for this revision. (Show Details)

tweak warning

tools/tools/git/git-arc.sh
137
463

Same comment as below for the gitarc_ prefix.

473
475

Same comment as below for the gitarc_ prefix.

490

The case indentation is wrong, it should be like C switch statements.

492
509

then drop the else?

519

I don't really like this behaviour, it makes it easy to silently commit the wrong thing. Once the phab user in question has a single commit in the tree, won't that be enough for the earlier steps to work in the future? Then, manual intervention would only be needed once per new contributor, which doesn't seem too onerous?

527

I used the gitarc__ prefix for functions which map to verbs, i.e., git arc patch -> gitarc__patch. For helper functions this gitarc_ prefix just seems confusing. I'd suggest something like commit_patch here.

572

Cases should have the same indentation as case, like switch statements in style(9).

602

As a safety belt, we should probably check that the user doesn't already have something staged in the tree, otherwise it'll get committed along with the patch.

tools/tools/git/git-arc.sh
519

I'll make it optional. I hate with a burning passion anything manual if I can mostly avoid it.

572

Emacs for thee win...

602

Ok.

tools/tools/git/git-arc.sh
519

Rather than adding a switch, how about a "does this look right?" prompt.

How does this differ from git arc stage?

In D39992#910666, @des wrote:

How does this differ from git arc stage?

git arc stage cherry-picks a commit from the local repository into main, while adding commit metadata. git arc patch applies a patch from phabricator.

It might make sense for git arc stage to start behaving like git arc patch -c. Though, one sometimes has small local fixups that aren't in phabricator for whatever reason (e.g., tweaks to the commit log message).

I'd be hard pressed to think of a time since I started using git arc that I didn't want my local copy to be staged for commit. That suggest some sort of new command that reverts to last copy in review rather than changing stage's semantics.

Also, I wish we marked the commit early with the review number and that we could update the title and summary fields in git arc update more easily. While I'm asking for ponies

In D39992#910744, @imp wrote:

I'd be hard pressed to think of a time since I started using git arc that I didn't want my local copy to be staged for commit. That suggest some sort of new command that reverts to last copy in review rather than changing stage's semantics.

git arc patch -c can already be used to stage one's own commits, right?

In D39992#910747, @imp wrote:

Also, I wish we marked the commit early with the review number and that we could update the title and summary fields in git arc update more easily. While I'm asking for ponies

I think the reason I didn't do it that way is because the target commit might not be a leaf commit, so a git arc update would require all child commits to be rebased, which can get messy. It could make sense for some workflows though, so it could be added as a non-default behaviour in principle.

tools/tools/git/git-arc.sh
54
tools/tools/git/git-arc.sh
519

I would prefer that for this final case we either abort with an error or use a prompt that lets the person verify the e-mail or enter a new one or abort.

In general having a working version of this would be really nice.

In D39992#910905, @jhb wrote:

In general having a working version of this would be really nice.

Yea.. I have been given a hint on how to get the git metadata, if it was uploaded (git arc for the win) and it will use that first. And it gives us motivation to strongly encourage people to use git arc instead of uploading patches. The -U9999 kludge should die... but I'm getting ahead of myself.

tools/tools/git/git-arc.sh
519

I'll do a yes/no prompt. Beyond that is a git commit --amend --author...

527

Gotcha

Also, git arc submit --add-id could be done with git rebase + edit options... though if you wanted to do that with arbitrary commits on 3 branches that might get fugy... I'd love to do it where it's possible though...

use git arc instead of uploading patches

Really Phabricator should also pick up the metadata if a git patch is uploaded

use git arc instead of uploading patches

Really Phabricator should also pick up the metadata if a git patch is uploaded

I'd love that, but unsure if it does... I'll have to check that out...

In D39992#910946, @imp wrote:

use git arc instead of uploading patches

Really Phabricator should also pick up the metadata if a git patch is uploaded

I'd love that, but unsure if it does... I'll have to check that out...

Sorry, I meant in the sense that it needs to (if it doesn't already), not that I expect it to.

Update to include another place to look for author/email before we start
guessing.

imp marked 12 inline comments as done.May 8 2023, 11:27 PM

Tick off the things I've applied

tools/tools/git/git-arc.sh
519

Please eval latest prompting to see if you think that would work best.

572

This one was deleted, for the win

Catch up on those issues flagged that I hadn't done yet in the last update.

No need to grep -v null if we use ? operator

Looks like arc_call_conduit is a typo. Fixing it manually on my end made everything work.

tools/tools/git/git-arc.sh
546

This fails with ./tools/tools/git/git-arc.sh: arc_call_conduit: not found
Perhaps this is a typo and you meant arc call-conduit analogous to what I see in diff2phid() function?

564

Same here:
This fails with ./tools/tools/git/git-arc.sh: arc_call_conduit: not found
Perhaps this is a typo and you meant arc call-conduit analogous to what I see in diff2phid() function?

This revision now requires changes to proceed.May 9 2023, 1:13 AM

You need the other patches in the series.. the code is right...
It points out that we may need -a to say apply the whole patch train

This revision is now accepted and ready to land.May 9 2023, 2:09 AM

Not sure if this is something that you want to address in this change: long lines are not wrapped when I do git-arc patch -c D40016

https://reviews.freebsd.org/D40016#911170

Not sure if this is something that you want to address in this change: long lines are not wrapped when I do git-arc patch -c D40016

https://reviews.freebsd.org/D40016#911170

Yea.. I'm not sure that should be automatic.. I'm going to punt for now...

Test 2 also passed successfully:

https://reviews.freebsd.org/D40016#911186

We need someone with @FreeBSD.org email to upload a raw diff via UI to test one more case.

Test 2 also passed successfully:

https://reviews.freebsd.org/D40016#911186

We need someone with @FreeBSD.org email to upload a raw diff via UI to test one more case.

40008 maybe. I did a bunch today...

40008 fails:

> sh ./tools/tools/git/git-arc.sh patch -c D40008
 INFO  Base commit is not in local repository; trying to fetch.
Checking patch stand/efi/libefi/eficom.c...
error: stand/efi/libefi/eficom.c: does not exist in index

 Patch Failed!
Usage Exception: Unable to apply patch!
In D39992#910905, @jhb wrote:

In general having a working version of this would be really nice.

+1

tools/tools/git/git-arc.sh
473

authorAddr and authorName should be declared as local vars. (And I don't think there's any other uses of camelCase in this code so would prefer not to add one.)

475
477

The first sentence doesn't quite make sense to me.

480
546

This patch apparently depends on D36583.

564

Missing D36583.

This revision now requires review to proceed.May 9 2023, 3:39 PM
imp marked 8 inline comments as done.

markj's suggestions are good, make them so.

Tick off the things I've applied

tools/tools/git/git-arc.sh
473

yea, too many styles I'm bouncing between... Thanks for catching this one.

477

rewrote it to be less snarky, which made it much clearer.

markj added inline comments.
tools/tools/git/git-arc.sh
584

This bit is duplicated with gitarc__stage() - could we factor it out into a helper function?

This revision is now accepted and ready to land.May 9 2023, 5:05 PM

Looks ok to me, just some nitpicking.

tools/tools/git/git-arc.sh
616