fix: switch away from git-annex (or stale master) after clone - #415
yarikoptic wants to merge 1 commit into
Conversation
Some origins — notably several `OpenNeuroDatasets` and `nemarDatasets` repos — advertise `git-annex` as their default branch, or keep `master` as default while `main` holds the newer content. `datalad clone` follows the advertised HEAD, so we end up extracting metadata from the wrong branch (e.g. NeMar datasets showing up in the registry without BIDS metadata because `git-annex` was checked out). Add `pick_preferred_branch` / `ensure_preferred_branch_checked_out` in `datalad_tls`: after clone, prefer whichever of `main`/`master` has the more recent commit at origin (or the sole one that exists), check it out, and repoint local `origin/HEAD` so downstream code sees the fix. Wire the fixup in at both clone sites — `process_dataset_url` and `update_ds_clone::reclone_ds` — and swap the update-path branch comparison to use the preferred branch so we reclone when the primary branch changes (e.g. `main` overtakes `master`). Closes: #414 Co-Authored-By: Claude Code 2.1.231 / Claude Opus 4.7 <noreply@anthropic.com>
| return max( | ||
| candidates, | ||
| key=lambda name: datetime.fromisoformat(candidates[name]["last_commit_dt"]), | ||
| ) |
There was a problem hiding this comment.
datetime.fromisoformat cannot parse a Z suffix before Python 3.11, and %(authordate:iso8601-strict) renders UTC commits with exactly that suffix. Any repo whose main/master tip was authored in UTC therefore raises ValueError here. This is what is currently making the test job red (26 failed, 430 passed, 5 errors).
There is no supported environment where this works: setup.cfg declares python_requires = >= 3.9, all three workflows pin 3.9, and the runtime image phusion/baseimage:jammy-1.0.1 carries 3.10.
The consequence is not a mis-ranking but a hard failure in both task paths. In process_dataset_url the exception propagates and the fresh clone is deleted, so the URL is never processed. In chk_url_to_update it lands in the handler that does n_failed_chks += 1, and once that reaches DATALAD_REGISTRY_MAX_FAILED_CHKS_PER_URL the dispatcher's RepoUrl.n_failed_chks < max_failed_chks filter stops selecting the URL for good.
Since nothing outside datalad_tls.py reads last_commit_dt (the API blueprints and templates only test JSONB keys, as in branches ? 'git-annex'), the tidiest fix is in get_origin_branches rather than here, so every stored value is uniformly parseable: either normalize the suffix, or record %(authordate:unix) and compare integers. Worth noting that rows written before the fix keep the Z spelling until their next update, so the column holds both spellings for a while unless they are rewritten deliberately.
Verification
Git's rendering, confirmed with git 2.55.0. Only UTC is affected:
$ git for-each-ref --format='%(refname) [%(authordate:iso8601-strict)]' refs/heads/
refs/heads/main [2020-01-01T00:00:00Z] # commit authored +00:00
refs/heads/main [2020-01-01T00:00:00+02:00] # commit authored +02:00On the project interpreter (3.9.23):
$ python -c "from datetime import datetime; datetime.fromisoformat('2020-01-01T00:00:00Z')"
ValueError: Invalid isoformat string: '2020-01-01T00:00:00Z'And from the PR's own CI run:
E ValueError: Invalid isoformat string: '2026-08-13T14:17:21Z'
datalad_registry/utils/datalad_tls.py:172: ValueError
======= 26 failed, 430 passed, 5 warnings, 5 errors in 112.44s (0:01:52) =======
| ds.repo.call_git( | ||
| [ | ||
| "symbolic-ref", | ||
| "refs/remotes/origin/HEAD", | ||
| f"refs/remotes/origin/{preferred}", | ||
| ] | ||
| ) |
There was a problem hiding this comment.
Worth adding a ds.config.reload() at the end of this function, because without it the PR stops one line short of fixing ds_id for the very repos it targets.
Dataset.id in datalad 0.19.6 is just self.config.get('datalad.dataset.id', None), with no reload of its own. Dataset.config delegates to repo.config, a ConfigManager held by the flyweight repo object and populated during dl.clone, at which point the working tree still held the git-annex branch and so had no .datalad/config. Checking out main here materializes that file on disk, but nothing re-reads it, so the manager keeps answering from its clone-time snapshot and _update_dataset_url_info records ds_id = None again.
To be clear, this is not a regression: these repos already stored ds_id = NULL before the PR, for the same reason. It is simply an opportunity that is now within reach and currently missed. As it stands the PR corrects the branch and the extracted metadata but still records the dataset as not being a DataLad dataset.
There is a second benefit. get_pure_annex_ds_collection_stats classifies a dataset as pure annex (an annex repo that is not a DataLad dataset) with branches ? 'git-annex' AND ds_id IS NULL. Every repo this PR targets has a git-annex branch by construction, so they are all currently miscounted as pure annex in the overview page's collection stats. Reloading the config would correct that statistic on the next update too.
Reproduction (datalad 0.19.6)
An annex source dataset with HEAD pointed at git-annex, mimicking the issue #414 failure mode, then cloned:
branch after clone: git-annex
ds.id right after clone: None # <- also the pre-PR state
.datalad/config on disk: True # after checkout -f -B main origin/main
ds.id after checkout (no reload): None
ds.id after config.reload(): 9ec93277-3124-497c-8eae-d01a69a4fee8
| # Prefer main/master over origin's advertised default (may be `git-annex` | ||
| # or an outdated `master`). See | ||
| # https://github.com/datalad/datalad-registry/issues/414 | ||
| target_branch = pick_preferred_branch( | ||
| current_ds_clone | ||
| ) or get_origin_default_branch(current_ds_clone) |
There was a problem hiding this comment.
The or get_origin_default_branch(...) fallback only fires when neither main nor master exists at origin. Whenever either ref is present, pick_preferred_branch returns a name, the or short-circuits, and origin's advertised default is never consulted at all (get_origin_default_branch is the only caller of git ls-remote --symref origin HEAD).
The choice then rests entirely on refs/remotes/origin/*, the local remote-tracking refs, which the git fetch above does not prune.
| def pick_preferred_branch(ds: Dataset) -> Optional[str]: | ||
| """ | ||
| Return the more-recently-committed of `main`/`master` at origin, or the | ||
| sole one that exists, or `None`. | ||
| """ | ||
| candidates = { | ||
| name: info | ||
| for name, info in get_origin_branches(ds).items() | ||
| if name in _PREFERRED_BRANCH_CANDIDATES | ||
| } | ||
| if not candidates: | ||
| return None | ||
| return max( | ||
| candidates, | ||
| key=lambda name: datetime.fromisoformat(candidates[name]["last_commit_dt"]), | ||
| ) |
There was a problem hiding this comment.
This overrides origin's default even when that default is a legitimate third branch, which is wider than issue #414 asks for.
Take a repo whose real default is dev or trunk but which still carries a leftover main or master. The leftover branch is the one that gets checked out, in process_dataset_url at registration and in reclone_ds on the update path. The registry then tracks a branch the repo owner did not choose, where today it tracks the right one. Worse, that branch never moves, so the entry silently freezes: the fast-forward path finds nothing and the record stays frozen while the real default advances.
| preferred = pick_preferred_branch(ds) | ||
| if preferred is None: | ||
| return |
There was a problem hiding this comment.
@yarikoptic, a question on how to read the strategy in issue #414. It lists:
- see if that is git-annex branch and if so -- switch to other 'main' or 'master'
- if both 'main' and 'master' branches present (one might be checked out) -- check which one is more recent and check that one out
Is the second bullet meant as a subcondition of the first, so that it applies only once the default has been found to be git-annex? Or is it independent, so that a repo whose advertised default is a real main or master should be re-examined too?
The reason it matters: as implemented the switch is unconditional. Whenever origin has a main or a master, the clone is moved onto it, whatever origin advertises as its default, and refs/remotes/origin/HEAD is repointed to match. A repo whose default is dev and which still carries a long-abandoned master ends up checked out on that master.
If the second bullet is a subcondition, the logic should trigger only when origin's advertised default is git-annex, and every other repo should be left on whatever origin advertises.
Demonstration: origin advertises dev, clone ends up on a stale master
A source repo whose default branch is dev (2025), carrying a leftover master last touched in 2019, cloned and then passed through ensure_preferred_branch_checked_out:
origin advertises default : dev
branch after clone : dev
pick_preferred_branch() : master
branch after ensure(...) : master
origin/HEAD now points at : refs/remotes/origin/master
files in working tree : ['.noannex', 'seed.txt']
newer.txt, added on dev, is gone from the working tree, so metadata extraction would run against the 2019 content. Non-UTC author dates were used here so that the Z-suffix parsing problem stays out of the way.
| } | ||
| if not candidates: | ||
| return None | ||
| return max( |
There was a problem hiding this comment.
When main and master point at the same commit, the two dates are equal and max returns the first maximal element, that is, whichever key get_origin_branches yielded first. That happens to be main, since git for-each-ref sorts by refname and dicts preserve insertion order. The right branch wins, but by accident rather than by intent.
The tie is not an exotic case: it is the normal state right after a repo renames master to main and keeps both names pointing at the same commit.
An explicit tie-break, plus a comment saying which branch is meant to win, would keep this from flipping if get_origin_branches ever changes its ordering.
Current behavior
origin branches, in iteration order:
main 25ff4989 2020-01-01T00:00:00+02:00
master 25ff4989 2020-01-01T00:00:00+02:00
tie is real : True
pick_preferred_branch(): main
| @@ -128,14 +134,17 @@ def reclone_ds() -> Dataset: | |||
|
|
|||
| current_ds_clone.repo.call_git(["fetch"]) | |||
There was a problem hiding this comment.
Without --prune, this leaves remote-tracking refs behind for branches that no longer exist at origin, and pick_preferred_branch reads exactly those refs.
Adding --prune here would also stop the branches column from recording branches that have been deleted at origin.
| return None | ||
| return max( | ||
| candidates, | ||
| key=lambda name: datetime.fromisoformat(candidates[name]["last_commit_dt"]), |
There was a problem hiding this comment.
last_commit_dt carries authordate, which is preserved by rebase, git am, cherry-pick, and history imports, so it does not record when a branch last moved. A main whose tip was applied from an old patch can lose to a master abandoned years ago, and the abandoned branch is then the one checked out.
committerdate is the field that answers "which branch moved most recently", and the docstring above already promises that behavior ("more-recently-committed").
The field comes from get_origin_branches, which also feeds the branches column, so exposing committerdate there alongside authordate is probably cleaner than swapping one for the other.
| if preferred is None: | ||
| return | ||
|
|
||
| current_branch = ds.repo.call_git(["symbolic-ref", "--short", "HEAD"]).strip() |
There was a problem hiding this comment.
symbolic-ref --short HEAD exits non-zero on a detached HEAD, which datalad turns into a CommandError.
A top-level datalad clone always leaves HEAD attached, so this is not reachable today at either call site. It is only worth raising because of what an exception costs in this particular spot: in chk_url_to_update it increments n_failed_chks, and once that reaches DATALAD_REGISTRY_MAX_FAILED_CHKS_PER_URL the dispatcher stops selecting the URL for good.
git branch --show-current prints an empty string rather than failing, if you want the guard for free.
…annex` Address the review of #415. `pick_preferred_branch()` now takes the branch that the origin remote advertises as its default and returns it unchanged unless it is `git-annex` or one of `main`/`master`. A dataset whose default branch is one its owner has chosen, e.g. `dev`, is no longer displaced by a leftover `master` lying around at the origin remote, neither in `process_dataset_url()` at registration nor in `reclone_ds()` on the update path. This also keeps `TestUpdateDsClone::test_new_default_branch_at_origin_remote`, and the tests of the other tasks built on the `new-branch` fixtures, passing. `ensure_preferred_branch_checked_out()` runs on a freshly cloned dataset only, so it reads that advertised default off the branch that the clone has just checked out, rather than paying for another `git ls-remote`. The `main`/`master` candidates are now ranked by `committerdate` instead of by the `authordate` carried in `last_commit_dt`. `authordate` is preserved by rebases, `git am`, cherry-picks, and history imports, so it does not say which branch moved last. Reading it as a UNIX timestamp also does away with the `datetime.fromisoformat()` call on a value that `git for-each-ref` renders with a `Z` suffix for a commit authored in UTC, which no supported interpreter of this project, Python 3.9 and 3.10, can parse. That call is the sole cause of the 26 failures and 5 errors in the `test` job. A tie between `main` and `master` now goes to `main` by intent, rather than by the iteration order of `git for-each-ref`, as after a rename that has left both names on the same commit. The config of the clone is reloaded after the switch, so that `Dataset.id`, and with it `RepoUrl.ds_id`, picks up the `.datalad/config` that the switch has just materialized in the working tree. Without it, the very datasets this fix targets keep being recorded with `ds_id = NULL`, and keep being counted as "pure annex" in the collection stats of the overview page. The fetch on the update path is now a `git fetch --prune`, so that neither the branch selection nor the `branches` column keeps reading the remote-tracking refs of branches deleted at the origin remote. That pruning takes with it the remote-tracking ref of the branch that the clone tracks when that branch is gone from the origin remote, e.g. after a rename of the default branch there, upon which `git rev-parse @{u}` starts failing. That failure is now caught and answered with a new clone, which is what the situation calls for. Left uncaught it would propagate out of `chk_url_to_update()` and count toward `DATALAD_REGISTRY_MAX_FAILED_CHKS_PER_URL`, after which the dispatcher stops selecting the URL altogether. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WhJ79EU9AmkXktvMiH853v
…annex` Address the review of #415. `pick_preferred_branch()` now takes the branch that the origin remote advertises as its default and returns it unchanged unless it is `git-annex` or one of `main`/`master`. A dataset whose default branch is one its owner has chosen, e.g. `dev`, is no longer displaced by a leftover `master` lying around at the origin remote, neither in `process_dataset_url()` at registration nor in `reclone_ds()` on the update path. This also keeps `TestUpdateDsClone::test_new_default_branch_at_origin_remote`, and the tests of the other tasks built on the `new-branch` fixtures, passing. `ensure_preferred_branch_checked_out()` runs on a freshly cloned dataset only, so it reads that advertised default off the branch that the clone has just checked out, rather than paying for another `git ls-remote`. The `main`/`master` candidates are now ranked by `committerdate` instead of by the `authordate` carried in `last_commit_dt`. `authordate` is preserved by rebases, `git am`, cherry-picks, and history imports, so it does not say which branch moved last. Reading it as a UNIX timestamp also does away with the `datetime.fromisoformat()` call on a value that `git for-each-ref` renders with a `Z` suffix for a commit authored in UTC, which no supported interpreter of this project, Python 3.9 and 3.10, can parse. That call is the sole cause of the 26 failures and 5 errors in the `test` job. A tie between `main` and `master` now goes to `main` by intent, rather than by the iteration order of `git for-each-ref`, as after a rename that has left both names on the same commit. The config of the clone is reloaded after the switch, so that `Dataset.id`, and with it `RepoUrl.ds_id`, picks up the `.datalad/config` that the switch has just materialized in the working tree. Without it, the very datasets this fix targets keep being recorded with `ds_id = NULL`, and keep being counted as "pure annex" in the collection stats of the overview page. The fetch on the update path is now a `git fetch --prune`, so that neither the branch selection nor the `branches` column keeps reading the remote-tracking refs of branches deleted at the origin remote. That pruning takes with it the remote-tracking ref of the branch that the clone tracks when that branch is gone from the origin remote, e.g. after a rename of the default branch there, upon which `git rev-parse @{u}` starts failing. That failure is now caught and answered with a new clone, which is what the situation calls for. Left uncaught it would propagate out of `chk_url_to_update()` and count toward `DATALAD_REGISTRY_MAX_FAILED_CHKS_PER_URL`, after which the dispatcher stops selecting the URL altogether. `ensure_preferred_branch_checked_out()` reads the branch that the origin remote advertises off the local `refs/remotes/origin/HEAD` that `git clone` records, rather than off the branch that happens to be checked out. A detached HEAD then no longer costs a dataset the protection of its chosen default branch, and the function no longer rests on being called on a fresh clone. It also writes that ref whenever it does not already name the branch to be tracked, so a clone of an origin remote that advertises no default branch gets the ref that `_update_dataset_url_info()` reads. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WhJ79EU9AmkXktvMiH853v
…annex` Address the review of #415. `pick_preferred_branch()` now takes the branch that the origin remote advertises as its default and returns it unchanged unless it is `git-annex` or one of `main`/`master`. A dataset whose default branch is one its owner has chosen, e.g. `dev`, is no longer displaced by a leftover `master` lying around at the origin remote, neither in `process_dataset_url()` at registration nor in `reclone_ds()` on the update path. This also keeps `TestUpdateDsClone::test_new_default_branch_at_origin_remote`, and the tests of the other tasks built on the `new-branch` fixtures, passing. `ensure_preferred_branch_checked_out()` runs on a freshly cloned dataset only, so it reads that advertised default off the branch that the clone has just checked out, rather than paying for another `git ls-remote`. The `main`/`master` candidates are now ranked by `committerdate` instead of by the `authordate` carried in `last_commit_dt`. `authordate` is preserved by rebases, `git am`, cherry-picks, and history imports, so it does not say which branch moved last. Reading it as a UNIX timestamp also does away with the `datetime.fromisoformat()` call on a value that `git for-each-ref` renders with a `Z` suffix for a commit authored in UTC, which no supported interpreter of this project, Python 3.9 and 3.10, can parse. That call is the sole cause of the 26 failures and 5 errors in the `test` job. A tie between `main` and `master` now goes to `main` by intent, rather than by the iteration order of `git for-each-ref`, as after a rename that has left both names on the same commit. The config of the clone is reloaded after the switch, so that `Dataset.id`, and with it `RepoUrl.ds_id`, picks up the `.datalad/config` that the switch has just materialized in the working tree. Without it, the very datasets this fix targets keep being recorded with `ds_id = NULL`, and keep being counted as "pure annex" in the collection stats of the overview page. The fetch on the update path is now a `git fetch --prune`, so that neither the branch selection nor the `branches` column keeps reading the remote-tracking refs of branches deleted at the origin remote. That pruning takes with it the remote-tracking ref of the branch that the clone tracks when that branch is gone from the origin remote, e.g. after a rename of the default branch there, upon which `git rev-parse @{u}` starts failing. That failure is now caught and answered with a new clone, which is what the situation calls for. Left uncaught it would propagate out of `chk_url_to_update()` and count toward `DATALAD_REGISTRY_MAX_FAILED_CHKS_PER_URL`, after which the dispatcher stops selecting the URL altogether. `ensure_preferred_branch_checked_out()` reads the branch that the origin remote advertises off the local `refs/remotes/origin/HEAD` that `git clone` records, rather than off the branch that happens to be checked out. A detached HEAD then no longer costs a dataset the protection of its chosen default branch, and the function no longer rests on being called on a fresh clone. It also writes that ref whenever it does not already name the branch to be tracked, so a clone of an origin remote that advertises no default branch gets the ref that `_update_dataset_url_info()` reads. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WhJ79EU9AmkXktvMiH853v
2 participants
Some origins — notably several
OpenNeuroDatasetsandnemarDatasetsrepos — advertisegit-annexas their default branch, or keepmasteras default whilemainholds the newer content.datalad clonefollows the advertised HEAD, so we end up extracting metadata from the wrong branch (e.g. NeMar datasets showing up in the registry without BIDS metadata becausegit-annexwas checked out).Add
pick_preferred_branch/ensure_preferred_branch_checked_outindatalad_tls: after clone, prefer whichever ofmain/masterhas the more recent commit at origin (or the sole one that exists), check it out, and repoint localorigin/HEADso downstream code sees the fix. Wire the fixup in at both clone sites —process_dataset_urlandupdate_ds_clone::reclone_ds— and swap the update-path branch comparison to use the preferred branch so we reclone when the primary branch changes (e.g.mainovertakesmaster).