[arc][hg] make `land` not fail for landinng middle commits
AbandonedPublic
Actions

Authored by pieter on Mar 1 2014, 10:27 PM.

Details

Reviewers

epriestley
durham

Group Reviewers

Blessed Reviewers

Summary

I've been playing around with a plain-vanilla hg setup with mutable
history (this is for a non-fb side project). Since I don't really
hg good (or do other things good either), I've mostly stuck with
a rebase-based workflow where I often want to land changesets that
are a child of master but a parent of some other changeset:

o  changeset:   19:e749d4a1566d
|  bookmark:    dude
|  tag:         tip
|  user:        pieter.hooimeijer@gmail.com
|  date:        Sat Mar 01 11:55:43 2014 -0800
|  summary:     yup
|
@  changeset:   18:f4a8fc648307
|  bookmark:    landme
|  user:        pieter.hooimeijer@gmail.com
|  date:        Tue Feb 25 23:16:35 2014 -0800
|  summary:     do stuff
|
o  changeset:   17:b9f15bafb4a3
|  bookmark:    master
|  user:        pieter.hooimeijer@gmail.com
|  date:        Tue Feb 25 21:16:54 2014 -0800
|  summary:     finished this ages ago

...where I want to land landme onto master and leave dude
on top.

Test Plan

spacecat

In a history that looks like ^:

hg update landme
arc land landme

Then observe that none of the three places I 'fixed' yield
mercurial exection errors.

I'm not sure how this might affect other workflows; happy to
take hints on what I might be breaking for others here.

Diff Detail

Repository

rARC Arcanist

Branch

hg_nonmutable

Lint

Lint Passed

Unit

No Test Coverage

Event Timeline

What goes wrong with the workflow as it was before?

src/workflow/ArcanistLandWorkflow.php
706	This line does three things: a) rebases to the destination, b) collapses commits, c) adds the Reviewed By line to the commit message. The condition you added means we don't need to collapse, but I think we still need this line to do (a) and (c). What was wrong with this line before? Were you doing 'arc amend' yourself, and therefore it was n't actually producing a new commit (since the commit message didn't change)?
739	'children(...)' only returns the immediate children of the commit. In your example, children(master) => landme and children(landme) => dude, so unless master and landme shared a child (which would require some sort of merge commit), this does nothing right?

This revision now requires changes to proceed.Mar 3 2014, 6:56 PM

@durham: Thanks for the comments; I'll take a closer look. For max efficiency, is there a specific minimum version of Mercurial that we support here?

src/workflow/ArcanistLandWorkflow.php
706	This line does three things goodcatch Were you doing 'arc amend' yourself, and therefore it was n't actually producing a new commit (since the commit message didn't change)? That's possible; I'll check back to see how to reproduce this. I don't think I've been running `arc amend` on the side, but it's possible I tested on a repo after a previous `arc land` run that failed after this step.
739	Hmm yeah this looks like it shouldn't be necessary.

arc doesn't have a specific minimum supported version of hg. Generally, we just try to implement things in the most backward-compatible way we can, and try harder if a pathway is common/required than if it is uncommon/esoteric/optional. Mostly, some things were just not feasible to implement in a realistic way in older versions of Mercurial. In some cases, we do feature tests and tell the user ("You don't have feature X, upgrade mercurial to Y.Z to use this arc feature.").

(I am generally out of my depth on these more subtle Mercurial workflow questions; what we implement is more or less "the Facebook workflow", which I have no experience with. I believe bookmarks don't see a tremendous amount of use on other Mercurial installs, and you likely have significant latitude to mess around here if that leads to a cleaner or better solution. T3855 has the most recent general discussion of this section of the code.)

@epriestley: Thanks for the context. I will try a newer version and see if that makes things better.

Been a while, but it looks like I should just use a less ancient version of hg.

bluemyself

Revision Contents
Changeset List

Path

Size

src/

workflow/

ArcanistLandWorkflow.php

36 lines

Diff 19917

View Options

src/workflow/ArcanistLandWorkflow.php

[arc][hg] make `land` not fail for landinng middle commitsAbandonedPublicActions