Repository navigation
Add an update-strategy option for git sources - #1
janwijbrand wants to merge 4 commits into
Conversation
An existing checkout is reconciled with `git merge` and nothing else. So a remote branch that was rebased and force-pushed gets its pre-rebase commits merged back in and the run reports success; when the rewrite conflicts, the checkout is left mid-merge with conflict markers in 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. 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.
| _blocking_status = { | ||
| "merge": ("ahead", "diverged", "dirty"), | ||
| "ff-only": ("dirty",), | ||
| "rebase": ("dirty",), | ||
| "reset": ("dirty",), | ||
| } |
There was a problem hiding this comment.
Can't tell without reading the code that uses it, but this looks messy to me. E.g. I would expect ff-only to refuse diverged. The comment says that the other strategies have custom code for some cases so it's probably handled in there, but then it's not clear to me why these particular cases get singled out for special treatment. I suspect I would go myself for a shared "dirty" rejection (the genuinely universal one), and putting all the rest of the case-handling in each separate update-handler.
tikitu
left a comment
There was a problem hiding this comment.
I could not understand the crucial section, where it applies the various strategies differently based on whether the state is ahead/behind. It feels very likely this is missing some particular combination that isn't handled correctly, just because of how it's structured, but I didn't try to verify that in detail. Less clever and more explicitly-naming-all-alternatives would make me more comfortable, particularly in that spot which is kind of the heart of it all.
Review feedback. The block that applied the strategies worked out `ahead` and `behind` once and then branched on them, which made it hard to see whether every combination was handled. It was not: `reset` returned early when the checkout was not behind, so a checkout that was only ahead kept the local commits `reset` exists to discard. On a deployment host that means a fix committed straight into the checkout survives the next deploy, which is the opposite of what the strategy is for. Dispatch on the strategy first, and let each block decide for itself: * `merge` is unchanged; * `reset` resets unconditionally, which is what "match the remote branch exactly" means, and is a no-op when it already does; * `rebase` hands the whole job to `git rebase`, which already fast-forwards a checkout that is only behind and does nothing when it is up to date, so there is no reason to reimplement part of it here; * `ff-only` is the only one that needs the counts, and only to say how far apart the two have drifted. Also explain why the per-strategy blocking states differ, and correct the comment claiming a `merge` update always refuses local commits. It is checked before the fetch, so it does not catch a divergence that the fetch itself reveals -- which is exactly the case this option exists for.
@tikitu I think we made an improvement on that part of the code. Would you agree? If not, I may need a little assistance. |
| # The historical behaviour, unchanged. `update()` refuses a | ||
| # checkout that is already ahead or diverged before we get here, | ||
| # so in practice this runs on a fast-forward -- except when the | ||
| # divergence only appeared in the fetch above, which is exactly | ||
| # what a rewritten remote branch does. |
There was a problem hiding this comment.
This whole comment is very confusing to me, in a very typically LLM-authored way.
- What is the comment drawing my attention to? It gives a lot of detail, but why do I need to think about that detail?
- Why emphasise that
update()defends a particular invariant, when the next clause says "except in the common case we're concerned with"? (I would complain that we shouldn't rely on the distantupdate()definition, but if I'm understanding right the "except when" clause basically says that this isn't that important.) - What am I supposed to think about this behaviour? Is it semantically correct? Semantically unexpected but we keep it to preserve historical behaviour unchanged? Actually quite bad and a reason to not use this option if you can avoid it?
Much clearer, yes. It's much easier to feel fairly confident that nothing has been missed. |
Upstream runs black from pre-commit and it rewrapped three expressions this branch added: the condition that decides whether to prompt in common.py, and a chained call in git.py and in test_git.py. No behaviour changes.
Upstream puts the contributor's handle on every changelog entry. Add it, and reference the issue that asked for this: fschulze#82 wanted rebase rather than merge as the default. This keeps merge as the default and makes the behaviour an option instead, so it refs rather than closes that issue.
|
This internal PR is now closed and the official upstream PR is open and 3.0.0+md.1 was released. |
An existing checkout is reconciled with
git mergeand nothing else. So aremote branch that was rebased and force-pushed gets its pre-rebase commits
merged back in and the run reports success; when the rewrite conflicts, the
checkout is left mid-merge with conflict markers in 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 withthe commands to recover from it;
rebase-- replay the local commits on top of the rewrite, leaving out thesuperseded 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 checkoutsnobody edits by hand such as deployment hosts and CI.
Alongside it, five smaller fixes that the same situation exposed:
diverged,and one that is only behind as
behind, instead of calling bothahead;which failed with a bare
EOFErrortraceback;dirty, since a branch that is merely ahead has a clean working tree;
git checkoutof arevfails;configured one, instead of moving off it silently.