Page MenuHomeFreeBSD

Remove history.immutable from .arcconfig
ClosedPublic

Authored by arichardson on Jan 5 2021, 11:50 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Oct 21, 1:09 PM
Unknown Object (File)
Sun, Oct 20, 6:15 PM
Unknown Object (File)
Sun, Oct 20, 1:25 PM
Unknown Object (File)
Sep 27 2024, 7:29 PM
Unknown Object (File)
Sep 27 2024, 5:26 PM
Unknown Object (File)
Sep 23 2024, 8:47 AM
Unknown Object (File)
Sep 17 2024, 9:18 PM
Unknown Object (File)
Sep 10 2024, 7:23 AM

Details

Summary

The history.immutable setting prevents arcanist from updating
the commit messages with the Differential URL and therefore
makes updating patches awkward with a rebase workflow.

Test Plan

arc diff --create HEAD^ adds the metadata now.

Diff Detail

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

Event Timeline

I use git arc stage to update the commit messages which adds the URL and Reviewed by using the format of FreeBSD's commit template instead of the format arcanist prefers. I think this will be a better fix (unlike arc land you can git arc stage an entire patch series and then do a single git push).

The reason I want this is so that arc diff HEAD^ adds the URL to the commit message. Then I can just add more commits and do arc diff HEAD^^ too update the review.

Never used arc land, but LLVM has patched their phabricator instance so that arc patch --nobranch gives me a sane commit message.

Workaround: arc set-config --local history.immutable false. However that won't work if you have a git worktree :( To work around that you need to patch arcanist since upstream doesn't want any contributions (https://secure.phabricator.com/D21438)

Workaround: arc set-config --local history.immutable false. However that won't work if you have a git worktree :( To work around that you need to patch arcanist since upstream doesn't want any contributions (https://secure.phabricator.com/D21438)

I haven't had time to check this, but if we really need to patch arc, we can patch it locally in ports.

Workaround: arc set-config --local history.immutable false. However that won't work if you have a git worktree :( To work around that you need to patch arcanist since upstream doesn't want any contributions (https://secure.phabricator.com/D21438)

I haven't had time to check this, but if we really need to patch arc, we can patch it locally in ports.

If we want FreeBSD cross-development to become a fully supported flow -- e.g., cross build, cross debug, emulate in Qemu, etc -- then we need to avoid patches being added in ports builds, since those won't be available on a non-FreeBSD development system (e.g., on Linux, Mac OS X, etc). Unless we also maintain and make available adapted versions there, and I'm not sure we want to be in that business...?

arichardson edited the test plan for this revision. (Show Details)

If we want FreeBSD cross-development to become a fully supported flow -- e.g., cross build, cross debug, emulate in Qemu, etc -- then we need to avoid patches being added in ports builds, since those won't be available on a non-FreeBSD development system (e.g., on Linux, Mac OS X, etc). Unless we also maintain and make available adapted versions there, and I'm not sure we want to be in that business...?

We don't want to be in the business of providing Windows / mac OS installers for patched Phabricator, indeed. However, I think it's fine to tell folks to either use our fork of arc (obtaining from GitHub etc.), or accept the caveats such as lack of support for worktrees.

ping? Can we drop the immutable setting so that arc diff adds the Differential Revision: metadata to commit messages automatically?

Personally, I'd rather just drop this here than provide a forked repo :(

This revision is now accepted and ready to land.Apr 10 2021, 8:38 PM

After reading https://secure.phabricator.com/book/phabricator/article/arcanist_new_project/ I think this is the right move. And I'm going to drop this in ports repository, too.

This revision now requires review to proceed.Apr 13 2021, 11:35 AM
This revision was not accepted when it landed; it landed in state Needs Review.Apr 13 2021, 11:39 AM
This revision was automatically updated to reflect the committed changes.