Page MenuHomePhabricator
Create Task
Maniphest T4333

`arc land` should provide a mechanism to land revisions by other authors while preserving authorship meta-data
Closed, ResolvedPublic
Actions

Description

Currently, arc land does not provide a convenient mechanism to land commits authored by others (e.g., someone who is not authorized to commit to a project; someone who is out of the office) while preserving authorship information. Currently, if I apply a patch with arc patch, then use arc land to land to the repository, authorship meta-data is not preserved. On IRC (https://secure.phabricator.com/chatlog/channel/6/?at=110908), @epriestley suggested that his workflow was something along these lines:

git checkout master
arc patch -nobranch D###
git push

This is simple, but nonintuitive.

Revisions and Commits

rARC Arcanist
D14356rARCa03c6079bb71 Make "arc land" great again

Related Objects

Event Timeline

wotte created this task.Jan 20 2014, 11:28 PM
wotte raised the priority of this task from to Needs Triage.
wotte updated the task description. (Show Details)
wotte added a project: Arcanist.
wotte added a subscriber: wotte.
epriestley triaged this task as Normal priority.Jan 25 2014, 10:22 PM
epriestley added a subscriber: epriestley.

Particularly, arc land should preserve the author at the HEAD of whatever we're landing.

epriestley assigned this task to btrahan.Apr 9 2014, 12:30 AM

After your recent changes, I think arc land works against others' branches, but we should tighten this up:

  • If you have some branch feaure with author X and run arc land on it, the original author X should be retained as the author of the squash/merge commit (you'll be the committer, which is fine).
  • This might already work, or might need us to pass an --author flag somewhere, or might need us to make a separate git commit --amend -C HEAD --author ... call.
  • Check both the --no-ff and --squash (default) strategies.
  • And then maybe do Mercurial too, or don't until someone complains.
qgil added a subscriber: qgil.Apr 25 2014, 7:50 PM
robla added a subscriber: robla.Apr 29 2014, 12:38 AM
YenaHong added a subscriber: YenaHong.May 2 2014, 4:22 AM
chrisbolt added a subscriber: chrisbolt.May 16 2014, 5:56 PM
joshuaspence added a subscriber: joshuaspence.Jun 20 2014, 4:12 PM
cburroughs added a subscriber: cburroughs.Jul 3 2014, 6:43 PM
mattrobenolt added a subscriber: mattrobenolt.Jul 7 2014, 3:04 PM
avivey added a subscriber: avivey.Jul 7 2014, 4:11 PM

Would it make sense to have arc patch always use author information from the revision/diff?

epriestley added a comment.Jul 7 2014, 4:19 PM

It already does, provided we have original author information. (With the caveat that it doesn't preserve timestamps right now.)

shadowhand added a subscriber: shadowhand.EditedJul 9 2014, 2:40 PM

+1 for this, and it is tangentially related to the Land to Github button as well, which should use the author's token (not mine).

edit: Evan makes a point that this would potentially allow me to force changes into a repository that i do not have permission for. Perhaps the suggestion of using the "Signed Off By" (see git cherry-pick -s) would also apply here.

shadowhand added a comment.Jul 9 2014, 2:48 PM

See also T5580

joshuaspence renamed this task from 'arc land' should provide a mechanism to land revisions by other authors while preserving authorship meta-data to `arc land` should provide a mechanism to land revisions by other authors while preserving authorship meta-data.Jul 15 2014, 5:38 AM
branden added a subscriber: branden.Jul 29 2014, 11:26 PM
staticshock added a subscriber: staticshock.Aug 6 2014, 9:04 PM
epriestley added a comment.Aug 25 2014, 1:58 AM

◀ Merged tasks: T3515.

epriestley added a comment.Aug 25 2014, 2:00 AM

See also T4706. In particular, users expect these behaviors:

  • They expect arc land to work no matter who authored the code in the branch.
  • They expect arc land D123 to "just work": figure out which branch is intended, or create a suitable brnach by using arc patch first if necessary.
btrahan removed btrahan as the assignee of this task.Sep 12 2014, 7:58 PM
btrahan added a subscriber: btrahan.

I don't have any plans to pursue this sort of thing at this time. Mainly cuz its really hard for me to work on, but to a lesser degree because I don't think this is particularly important given the other work here.

If any user out there REALLY wants to see this fixed soon-ish, the big hurdle for me is creating the proper test data. So if some user can give me access to their instance that can reproduce this sort of problem with some reproduction steps then let's have at it.

BYK added a subscriber: BYK.Oct 9 2014, 10:59 AM
joshuaspence claimed this task.Jan 8 2015, 6:48 AM
angerman added a subscriber: angerman.Jan 29 2015, 2:12 PM
Funnnny added a subscriber: Funnnny.Mar 16 2015, 5:19 PM
mstyles added a subscriber: mstyles.May 1 2015, 9:24 PM
avivey mentioned this in T8290: Additional workflow to facilitate pull-request.May 21 2015, 10:36 PM
avivey mentioned this in Q189: Author was changed after arc land.Oct 26 2015, 12:52 AM
epriestley added a revision: D14356: Make "arc land" great again.Oct 28 2015, 2:13 AM
epriestley added a comment.Oct 28 2015, 2:26 AM

After D14356, arc land will preserve authorship and author date data. Since arc patch also preserves them, the net effect should be preserved authors.

T182 is also now a nearly-viable alternative for installs that have a substantial need for this.

This doesn't cover arc land D123 but I'm not convinced that's really worth building, especially if T182 is generally deployable.

cburroughs added a comment.Oct 28 2015, 12:46 PM

After D14356, arc land will preserve authorship and author date data. Since arc patch also preserves them, the net effect should be preserved authors.

Does that mean arc patch D123 && arc land will "Just Work" now and the arc patch --nobranch D123 workflow is obsolete?

epriestley added a comment.Oct 28 2015, 1:16 PM

In Git, after D14356, and ignoring any bugs, arc patch D123 && arc land should work (it should already have worked, just not preserved author metadata?).

This workflow is slightly different than arc patch --nobranch D123, and I wouldn't call the --nobranch workflow obsolete, per se. Two particular differences are that arc land will prompt you about landing a revision you don't own, and will prompt you about other revision state problems (like failing or pending tests). So you'll probably need to "Y" through at least one prompt. The --nobranch workflow is a little lower-level.

If you test the changes locally before landing them, arc patch will attempt to reconstruct them on top of the original base commit, so you may miss problems introduced since the change was made. arc patch --nobranch reduces the risk of this. You could arc patch, then rebase or merge, then test, then arc land.

arc patch also does not preserve tracking branches currently, so arc land on your patched branch may select a different target branch than arc land would have on the original branch in the author's working copy. You can select the desired target explicitly with --onto. This is unlikely to do anything surprising, I mention it mostly for completeness.

I don't currently plan to pursue a really tailored arc land-someone-elses-revision workflow, because I believe there are two better workflows available or almost-available: give them commit access, or land from the web UI (T182). I'm open to rethinking this if there are workflows I'm missing or unaware of that would benefit from a hypothetical arc land --on-behalf-of-author D123 with a more tailored workflow (e..g, no "this is someone else's revision" prompt).

The two cases I've personally hit where bringing the change into my local working copy is helpful are centered around first-time contributors to this project:

  1. I want to double-check that the author actually tested it (spoiler: they didn't).
  2. I want to rewrite the change locally and then push unreviewed code.

For (1), getting it merged/rebased on top of master is slightly desirable, and arc patch --nobranch is a little easier than arc patch + git rebase (although conceivably we could introduce arc patch --rebase or whatever).

For (2), I probably shouldn't be doing that anyway, and it seems OK to not have a super tailored workflow for it.

epriestley closed this task as Resolved by committing rARCa03c6079bb71: Make "arc land" great again.Oct 28 2015, 2:05 PM
epriestley added a commit: rARCa03c6079bb71: Make "arc land" great again.
epriestley mentioned this in T9657: Upgrading: arc land flow has been rewritten in git.Oct 28 2015, 2:11 PM
avivey mentioned this in Q189: Author was changed after arc land (Answer 222).Nov 15 2015, 9:40 PM
avivey merged a task: T9922: Allow downloading a revision 'commit', meaning including the metadatas.Dec 7 2015, 7:26 PM
avivey added a subscriber: • thiblahute.
abika mentioned this in T12255: Landing differential from other author does not preserve ownership.Feb 13 2017, 5:41 PM
kugel- added a subscriber: kugel-.Nov 30 2017, 3:37 PM

arc land still prints a warning in this scenario. Is there a way to disable that through .arcconfig? arc land itself doesn't seem to have an option to skip the warning.

kugel- added a comment.Nov 30 2017, 3:41 PM

Also, in case of "arc land --merge", the merge commit is falsely authored by the revision author.

Phabricator · Built by Phacility