Skip to content

Add an update-strategy option for git sources - #223

Open
janwijbrand wants to merge 1 commit into
fschulze:masterfrom
minddistrict:upstream-update-strategy
Open

janwijbrand wants to merge 1 commit into
fschulze:masterfrom
minddistrict:upstream-update-strategy

Conversation

@janwijbrand

Copy link
Copy Markdown

The problem

git_update reconciles an existing checkout with git merge and nothing else.
When the remote branch has been rebased and force-pushed, that merge brings
back the commits the rewrite replaced — and buildout exits 0, so nothing
reports a problem:

mr.developer: Updated 'mypkg' with git.
mr.developer: Switching to branch 'feature'.
*   Merge remote-tracking branch 'remotes/origin/feature' into feature
|\
| * topic: add c      <- post-rebase
| * topic: add b
| * topic: add a
* | topic: add c      <- pre-rebase, brought back
* | topic: add b
* | topic: add a
|/
## feature...origin/feature [ahead 4]

Every commit now exists twice and the branch has silently diverged from its
remote. When the rewrite conflicts instead, the merge fails and the checkout is
left mid-merge, with conflict markers written into the source files.

This matters more than it used to: rebasing topic branches and GitHub's stacked
pull requests both rewrite published branches as a matter of course.

What this adds

An update-strategy option, settable per source or for the whole buildout:

value behaviour on a rewritten remote branch
merge the default, unchanged
ff-only only fast-forwards; reports the divergence and how to recover, and leaves the checkout untouched
rebase replays the local commits on top, leaving out the superseded copies; aborts cleanly on conflict
reset makes the checkout match the remote branch exactly

The default stays merge, so existing users see no change.

rebase leans on git rather than reimplementing it: git rebase already leaves
out commits whose patch is already upstream — which is exactly what the
superseded copies are — and fast-forwards when there is nothing local to
replay. On conflict the strategy runs git rebase --abort, so a failed update
never leaves a half-finished rebase behind.

reset is meant for checkouts nobody edits by hand: deployment hosts, CI and
build images, where "make it match the remote" is the whole requirement.

Refs #82, which asked for rebase rather than merge as the default. This makes
it an option instead, so nothing changes for anyone who does not ask for it.

Also in here

Four smaller things the same investigation turned up, each with its own
changelog entry:

  • A checkout that is both ahead of and behind its remote is now reported as
    diverged rather than ahead, and one that is only behind as behind.
    develop status shows these as X and <. A rewritten remote branch was
    previously indistinguishable from having local commits.
  • No prompt when there is no terminal to answer it. With always-checkout set,
    a non-interactive run used to end in a bare EOFError traceback from
    input(); it now reports which checkout is in the way and what state it is
    in.
  • The message names the state that actually blocked the update, instead of
    calling everything "dirty" — a branch that is merely ahead of its remote has
    a perfectly clean working tree.
  • A failed git checkout of a rev names the revision, instead of a branch
    master that was never involved.

Testing

Eight new tests in test_git.py, one per strategy plus the cases that are easy
to get wrong: that reset discards local commits when the checkout is ahead but
not behind, that a conflicting rebase leaves the checkout exactly as it was,
and that merge still behaves as it always has.

pre-commit run -a passes. The test suite passes locally except the mercurial
and subversion tests, which need those binaries installed — CI does that.

Note

We have been running this internally as 3.0.0+md.1, built from this branch,
while preparing the pull request.

An existing checkout is reconciled with `git merge` and nothing else. So when
the remote branch has been rebased and force-pushed, that merge brings back the
commits the rewrite replaced and the run exits 0, with nothing reporting a
problem. When the rewrite conflicts instead, the checkout is left mid-merge,
with conflict markers written into the source files.

Add `update-strategy`, settable per source or for the whole buildout:

* `merge` -- the default, unchanged, so existing users see no difference;
* `ff-only` -- only fast-forward, and report a rewritten branch together with
  the commands to recover from it;
* `rebase` -- replay the local commits on top of the rewrite, leaving out the
  superseded copies of the commits it replaced, and abort without changing
  anything if one of them conflicts;
* `reset` -- make the checkout match the remote branch exactly, for checkouts
  nobody edits by hand such as deployment hosts and CI.

`rebase` leans on git rather than reimplementing it: `git rebase` already
leaves out commits whose patch is upstream, which is exactly what the
superseded copies are, and fast-forwards when there is nothing local to replay.

Refs fschulze#82, which asked for rebase rather than merge as the default. This makes
it an option instead, so nothing changes for anyone who does not ask for it.

Alongside it, five smaller fixes that the same situation exposed:

* report a checkout that is both ahead of and behind its remote as `diverged`,
  and one that is only behind as `behind`, instead of calling both `ahead`;
* do not prompt "the package is dirty" when there is no terminal to answer it,
  which failed with a bare `EOFError` traceback;
* name the state that actually blocked an update instead of always calling it
  dirty, since a branch that is merely ahead has a clean working tree;
* name the revision, not a branch, when `git checkout` of a `rev` fails;
* warn when a checkout is switched away from a branch that is not the
  configured one, instead of moving off it silently.
@fschulze

Copy link
Copy Markdown
Owner

Sounds sensible and a very quick look at the changes didn't have anything jump out at me.

So +1 from me.

@janwijbrand

Copy link
Copy Markdown
Author

Sounds sensible and a very quick look at the changes didn't have anything jump out at me.

So +1 from me.

Thanks - the next step is to merge it and release it? Can I help in anyway there?

@janwijbrand

Copy link
Copy Markdown
Author

Can I help in anyway there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants