Fix discover showing different branch trees across worktrees#1755
Fix discover showing different branch trees across worktrees#1755earfman wants to merge 6 commits into
Conversation
|
Thanks for the contrib! pls fix mypy so that we can also validate test coverage (will be posted automatically by codecov once the tests pass) And also, why does PR title/description mention status? AFAICS |
| if os.path.isdir(worktrees_dir): | ||
| for worktree_id in sorted(os.listdir(worktrees_dir)): | ||
| reflog_paths.append(self.get_main_worktree_git_subpath("worktrees", worktree_id, "logs", "HEAD")) | ||
|
|
There was a problem hiding this comment.
Note that depending on machete.worktree.useTopLevelMacheteFile git config key, machete file can be shared b/w worktrees (default), or be stored per-worktree. Still, I think it's reasonable to assume that discovery can happen based on all the reflogs combined, regardless of what worktree it is run from. Worst case, when useTopLevelMacheteFile=false, too many branches will be taken into account in discovery and the user will need to trim down the per-worktree machete file.
Also, when useTopLevelMacheteFile=false, does discover write to the per-worktree machete file (for the worktree where discover is executed)? I'd assume so, but pls verify while you're here
There was a problem hiding this comment.
(okay responded below)
In future, if we deem that discovery should happen per-worktree when useTopLevelMacheteFile=false, we can easily conditionally change the logic to the old one. For now, better not introduce too many moving parts, this is complex enough as it is 😅 worktrees (thanks to wide agent adoption...) only opened another cans of worms, thus adding complexity in multiple places
Credit the contributor, drop the issue number, and remove the 'status' reference since only discover is affected.
Annotate the discover_here() helper's return type as Set[str] (was a bare 'set') to satisfy disallow_any_generics under mypy, so the check suite passes.
|
Thanks @PawelLipski, and thanks for the collaborator invite! Pushed two commits that address everything: mypy — the failure was a bare Release note — credited myself and dropped the issue number, matching the earlier entries.
Per-worktree file question — confirmed: with |
The new test uses linked worktrees (git 2.5+). Guard it with the same skipif the other worktree tests use, so it stops failing on the python 3.6 / git 1.8.0 CI lane.
| os.chdir(worktree) | ||
| from_worktree = discover_here() | ||
|
|
||
| # The two worktrees must agree, and each cross-worktree branch must survive pruning. |
There was a problem hiding this comment.
Also, sanity check - without the change to git.py, are the two discovered layouts different? otherwise the test would be of limited value
|
|
||
| ## New in git-machete 3.44.2 | ||
|
|
||
| - fixed: `git machete discover` no longer produces a different branch tree depending on which worktree it is run from; the fresh-branch recency ranking now aggregates HEAD reflogs across all worktrees rather than only the current one (contributed by @earfman) |
There was a problem hiding this comment.
Actually, pls also include the reporter:
| - fixed: `git machete discover` no longer produces a different branch tree depending on which worktree it is run from; the fresh-branch recency ranking now aggregates HEAD reflogs across all worktrees rather than only the current one (contributed by @earfman) | |
| - fixed: `git machete discover` no longer produces a different branch tree depending on which worktree it is run from; the fresh-branch recency ranking now aggregates HEAD reflogs across all worktrees rather than only the current one (reported by @jasonoura, contributed by @earfman) |
Probably the first time the given issue has a different reporter & implementer, and neither is me (or someone from my team) 😅 are you guys associated in any way?
|
|
||
| # The two worktrees must agree, and each cross-worktree branch must survive pruning. | ||
| assert from_main == from_worktree, (from_main, from_worktree) | ||
| assert "feat_wt" in from_main # checked out only in the linked worktree |
There was a problem hiding this comment.
Yeah, they'd be different, okay
|
Ouch, there'll be no coverage since it's coming from fork, and secrets are not provided by CI to the builds from forks, lemme think how to resolve it... |
|
|
||
| # Raw reflog line: "<old-sha> <new-sha> <name> <email> <unix-ts> <tz>\t<subject>". | ||
| # We read the files directly (rather than N `git reflog show` subprocesses, one per worktree) since | ||
| # `git reflog show` can only report the current worktree's HEAD; the on-disk format is stable. |
There was a problem hiding this comment.
Actually yeah, if it comes to format stability - is it declared anywhere in git docs?
I mean, tests pass for a very wide range of versions (1.8 to the recent 2.5x ones), so the format is effectively stable, but curious if git claims it explicitly
There was a problem hiding this comment.
(maybe irrelevant, see comment at the end of file)
| @@ -1460,22 +1460,56 @@ def get_git_timespec_parsed_to_unix_timestamp(self, date: str) -> int: | |||
| raise UnexpectedMacheteException(f"Cannot parse timespec: `{date}`") | |||
|
|
|||
| def get_latest_checkout_timestamps(self) -> Dict[str, int]: # TODO (#110): default dict with 0 | |||
There was a problem hiding this comment.
Remove this TODO while you're here, it's stale ;p
| # We read the files directly (rather than N `git reflog show` subprocesses, one per worktree) since | ||
| # `git reflog show` can only report the current worktree's HEAD; the on-disk format is stable. | ||
| pattern = re.compile("^checkout: moving from (.+) to (.+)$") | ||
| for reflog_path in reflog_paths: |
There was a problem hiding this comment.
I'd extract reading of reflogs to a lower-level method which would return the reflog entries as some named tuples, and then get_latest_checkout_timestamps would just use these entries without dealing with file format
| # (rather than "first entry wins") is order-independent, so it's correct even though the | ||
| # reflog files are appended oldest-first - the opposite order to `git reflog show`. | ||
| if timestamp > result.get(branch, 0): | ||
| result[branch] = timestamp |
There was a problem hiding this comment.
Also, in fact, would it be very difficult to instead of all this parsing, just run git reflog in each individual directory, and aggregate the results at the end? This is run once per discover (which is a relatively rare operation), and reflog is fast compared to e.g. log (mostly does what you do here anyway)
There was a problem hiding this comment.
This way we won't need to take care of parsing raw reflog data, but instead git reflog output which is easier to control
Fixes #1754.
Problem
git machete discoverproduces a different branch tree depending on which worktree it is run from, even though all worktrees share one.git/machetefile by default (machete.worktree.useTopLevelMacheteFile=true). Runningdiscoverin one worktree can silently drop branches that are visible from another.Root cause
discovertrims its output to the ~10 most-recently-checked-out branches, ranked viaGit.get_latest_checkout_timestamps(). That method parsedgit reflog showforHEAD, butHEAD's reflog is per-worktree (<git-dir>/logs/HEADfor the main worktree,<git-dir>/worktrees/<id>/logs/HEADfor each linked one). So the recency ranking only saw the current worktree's checkouts; branches recently used in other worktrees were treated as stale and pruned, differently depending on where the command ran.Fix
Aggregate every worktree's
HEADreflog (<git-dir>/logs/HEADplusworktrees/*/logs/HEAD), keeping the most recent checkout timestamp per branch. The ranking is now worktree-independent, sodiscoverproduces the same, complete tree regardless of the invoking worktree — keeping the shared-file behaviour (intentional and documented) while making the tree consistent, per the reporter's points 2 and 3.The reflog files are read directly (a single
git reflog showcan only report the current worktree's HEAD), decoded as strict UTF-8 witherrors="surrogateescape"so a non-ASCII committer name can't raise mid-read.Testing
New regression test
test_discover_is_worktree_independentasserts thatdiscoveryields the same managed branches from both the main and a linked worktree (using--checked-out-sincefor a deterministic trigger); it fails without the fix and passes with it. The existing discover/git/worktree suites pass locally. Added aRELEASE_NOTES.mdentry under 3.44.2.