Open Bug 1505607 Opened 6 years ago Updated 5 years ago

Using `arc patch` to apply a patch messes with timestamps and forces you to do a full rebuild, wasting an hour + machine usability

Categories

(Conduit :: Phabricator, defect, P3)

defect

Tracking

(Not tracked)

People

(Reporter: Gijs, Unassigned)

References

Details

(Keywords: conduit-triaged)

STR: 1. update to m-c of today (070757a0160c for me, but any later revision will also work) 2. run ./mach build (potentially with a clobber beforehand) 3. run e.g. arc patch D11145 --nobranch This outputs: $ arc patch D11145 --nobranch Updating to the revision's base commit 7371 files updated, 0 files merged, 357 files removed, 0 files unresolved Created and checked out bookmark arcpatch-D11145. OKAY Successfully committed patch. 4. run ./mach build faster (as the patch only touches JS files) ER: It runs in a few seconds AR: It yells at you about CLOBBER. If you decide to touch CLOBBER (given the state of the tree has basically not changed) and re-run, it'll run ./configure, wasting a bunch of time, but then basically work. If you're less lucky and want to now make binary changes and run ./mach build binaries, it does basically a full rebuild, and takes however long a full clobber build (or inc build with lots of changes) normally takes. I can't really understate how disruptive and painful this is. Please can we ask the arc people to improve their tool or write a better one. `hg pull` can pull in a revision that has a different base rev without updating the working dir, and then `hg rebase` can rebase it, again without messing with clobber/build state. `arc` should be doing the same.
Yeah this is unfortunate. It's tricky because, as a VCS-agnostic tool, Arcanist (and Phabricator generally) doesn't deal with real commits; it basically updates the working copy to the revision's parent and applies a patch. Could you see if "--skip-dependencies" shows the same behaviour? In this case Arcanist shouldn't be trying to update the local copy and, ergo, should leave CLOBBER alone. In theory, it should also leave the working copy alone if Arcanist can't find the revision's parent locally, or if the parent is the currently checked-out commit. If that doesn't work, we could try working around the problem in our upcoming `moz-phab patch` feature (which will be useful for other reasons). As an aside, I'm surprised I haven't heard more comments about this... maybe others are just putting up with the rebuilds? (which, to be clear, they shouldn't have to)
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Mark Côté [:mcote] from comment #1) > Yeah this is unfortunate. It's tricky because, as a VCS-agnostic tool, > Arcanist (and Phabricator generally) doesn't deal with real commits; it > basically updates the working copy to the revision's parent and applies a > patch. I bow to your knowledge of arc, but I'm still confused. A hg imported commit that you export/import is effectively just a patch with some commit information (rev id, which gets trashed anyway, parent, commit message). arc has all that info (otherwise, it couldn't update to the rev's parent, or create a commit message itself), so why can't it do it "properly" using `hg import` APIs that parent the new rev at its original parent, without updating the working dir to it? I realize arc has to build on top of various VCS's, but it also needs to know the VCS to update to the revision in question, right? Does the git implementation not have this problem? It looks like `git apply --index --cached` should behave the 'right' way in git land. > Could you see if "--skip-dependencies" shows the same behaviour? In this > case Arcanist shouldn't be trying to update the local copy and, ergo, should > leave CLOBBER alone. In theory, it should also leave the working copy alone > if Arcanist can't find the revision's parent locally, or if the parent is > the currently checked-out commit. So as established, $ arc patch --diff 32795 --nobranch Updating to the revision's base commit 11998 files updated, 0 files merged, 3169 files removed, 0 files unresolved Created and checked out bookmark arcpatch-D11145. OKAY Successfully committed patch. Touches clobber. (Note that it also brings up your chosen merge/conflict editor because by now this is an old diff and other stuff landed since then). Running: $ arc patch --diff 32795 --nobranch --skip-dependencies Updating to the revision's base commit 11998 files updated, 0 files merged, 3169 files removed, 0 files unresolved Created and checked out bookmark arcpatch-D11145. OKAY Successfully committed patch. Also touches clobber. :-( > If that doesn't work, we could try working around the problem in our > upcoming `moz-phab patch` feature (which will be useful for other reasons). > > As an aside, I'm surprised I haven't heard more comments about this... maybe > others are just putting up with the rebuilds? (which, to be clear, they > shouldn't have to) I'm not sure a lot of people apply patches with `arc patch` at all. It's also most noticeable if the difference between your working directory and the patch crosses a CLOBBER touch - otherwise the build will be unnecessarily longer, but that might not stand out to people enough to notice.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mcote)
Well that's unfortunate. :( I can't speak much more as to what Arcanist is doing. When we start on the "moz-phab patch" feature we'll bear this in mind and try to do something more intelligent. I'll put this as a dependency so we don't forget about it.
Blocks: 1503903
Flags: needinfo?(mcote)
Keywords: conduit-triaged
You want to test the patch. Please check if `--nocommit` will solve the issue. ``` --nocommit Supports: git, hg Normally under git/hg, if the patch is successful, the changes are committed to the working copy. This flag prevents the commit. ```
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Piotr Zalewa [:zalun] from comment #4) > You want to test the patch. Please check if `--nocommit` will solve the > issue. > > ``` > --nocommit > Supports: git, hg > Normally under git/hg, if the patch is successful, the changes > are committed to the working copy. This flag prevents the > commit. > ``` This seems to help, though I'm worried that it just tries to apply the diff against the working dir, which will fail for anything that needs rebasing...
Flags: needinfo?(gijskruitbosch+bugs)
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.