Closed Bug 1987220 Opened 1 year ago Closed 7 days ago

`moz-phab patch --apply-to here` should apply to the original base revision and rebase, rather than applying the raw diff on HEAD

Categories

(Conduit :: moz-phab, enhancement, P2)

Desktop
All
enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Gijs, Assigned: padenot)

References

Details

Attachments

(2 files)

Lando appears to "just" apply diffs to git HEAD.

It would be nice if, just like the abilities moz-phab patch recently grew / is growing, it would instead apply to the base rev that phab advertises, and tries to rebase (and perhaps fall back to the current logic if that fails due to not finding the base rev, or something).

This would allow more patches to land without developers having to manually rebase and resubmit (and then additionally encountering a "no reviewer has approved the current patch" warning, and sometimes a "pending builds" one, too!).

https://github.com/padenot/review/commit/93529e640f952fe4fcce30ab0224c83e060992c5 does some of that, but has some shortcomings, such as the inability to update the "base" revision for a particular commit (or at least I haven't found an endpoint to do so).

So this breaks whenever the original commit of a patch stack was something that didn't exist on the canonical repo, e.g. you've created a commit, then later decided it wasn't useful, so abandoned, it, etc.

Maybe we can be a bit pragmatic and switch to an earlier base, loosely based on dates instead of hashes.

Type: defect → enhancement
Priority: -- → P2

moz-phab patch used to apply a patch directly wherever --apply-to
pointed, trusting that commit to be valid. It sometimes isn't: the
diff's base commit can be missing locally, or exist only as part of
another, unlanded patch stack. This is unconditional -- it happens for
every patch invocation, regardless of --apply-to (or its default).

The patch now always applies at the diff's original base commit first,
then rebases onto whatever --apply-to names as the target:

  • If the diff's base commit is public (landed), use it as-is.
  • Otherwise (not found at all, or found but not public -- eg. only
    reachable from another, unlanded stack, so fetching from upstream
    will never find it), fall back to the most recent
    autoland-to-mozilla-central merge that landed at or before the
    patch's own timestamp (the later of the diff's creation date and the
    revision's last-modified date).

Implemented for git and jj (which delegates to its colocated git
backend); hg gets an equivalent phase- and revset-based implementation
but is lightly tested here.

Assignee: nobody → padenot
Status: NEW → ASSIGNED

I didn't notice this bug when I completed this on the Lando side in Bug 2047605. Morphing the bug to cover the moz-phab use case instead.

Component: Lando → moz-phab
See Also: → 2047605
Summary: When landing a phabricator patch, lando should apply to the base revision and rebase, rather than applying the raw diff on HEAD → `moz-phab patch --apply-to here` should apply to the original base revision and rebase, rather than applying the raw diff on HEAD

Rather than always resolving a public base and rebasing, first try
applying the patch directly at the target -- this is cheap and avoids
rebuilds from files only touched by a rebase. Only if that direct
application fails (eg. the diff's base commit is missing locally, or
belongs to another, unlanded patch stack) do we fall back to
resolving a public base and rebasing onto the target.

A failed direct attempt needs no special cleanup: retrying just calls
before_patch again at the resolved base, same as running patch
twice in a row already does -- the existing branch/bookmark-name
collision handling picks a fresh name, and the abandoned commits are
left for the usual VCS-specific garbage collection.

Attachment #9621750 - Attachment description: Bug 1987220 - Always apply at a resolved public base, then rebase to target, with a twist. r?sheehan → Bug 1987220 - Always apply at a resolved public base, then rebase to target, with a twist. DOT_GITHUB_OVERRIDE r?sheehan
See Also: → 2068637

Authored by https://github.com/padenot
https://github.com/mozilla-conduit/review/commit/cc5359ee646a654d44ec55d9ad1d7c46a3de8f96
[main] Bug 1987220 - Always apply at a resolved public base, then rebase to target, with a twist. DOT_GITHUB_OVERRIDE r=sheehan

Authored by https://github.com/padenot
https://github.com/mozilla-conduit/review/commit/59a6520debf87c284a1686d9e346e4f7cf710279
[main] Bug 1987220 - try applying at the target before falling back to the rebase strategy. r=sheehan

Status: ASSIGNED → RESOLVED
Closed: 7 days ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: