Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DOC+TST: gitrepo: Document get_tags() order change from eed2ca6f6 #3715

Merged
merged 2 commits into from Sep 25, 2019

Conversation

kyleam
Copy link
Contributor

@kyleam kyleam commented Sep 25, 2019

Prior to the for_each_ref_() series (gh-3705), get_tags() returned
tags sorted by the committer date.  For annotated tags, this means
they were sorted by the object pointed to rather than the tagger date.
Following gh-3705, the sorted value is by for-each-ref's "creatordate"
field (which exists so that lightweight and annotated tags can be
sorted together), and annotated tags are now sorted based on the
tagger date.

This change seems harmless enough.  No current callers in the code
base rely on annotated tags being sorted by committer date, and it
seems unlikely that outside callers rely on this behavior.  But it
does mean that test_get_tags needs to patch GIT_COMMITTER_DATE when
creating annotated tags so that the tagger date is not affected by the
environment of the test run.

If we really want to preserve the old behavior, note that using
"committerdate" (as originally done by gh-3705's bb17d0ede) is not
correct.  It will sort lightweight tags by their committer date and
leave annotated tags sorted alphabetically at the top of the list.
Instead we'd have to use a more involved approach, such as including
both "committerdate" and the dereferenced "*committerdate" as fields
and sorting by whichever value is non-empty for a given entry.

kyleam added 2 commits Sep 25, 2019
Prior to the for_each_ref_() series (dataladgh-3705), get_tags() returned
tags sorted by the committer date.  For annotated tags, this means
they were sorted by the object pointed to rather than the tagger date.
Following dataladgh-3705, the sorted value is by for-each-ref's "creatordate"
field (which exists so that lightweight and annotated tags can be
sorted together), and annotated tags are now sorted based on the
tagger date.

This change seems harmless enough.  No current callers in the code
base rely on annotated tags being sorted by committer date, and it
seems unlikely that outside callers rely on this behavior.  But it
does mean that test_get_tags needs to patch GIT_COMMITTER_DATE when
creating annotated tags so that the tagger date is not affected by the
environment of the test run.

If we really want to preserve the old behavior, note that using
"committerdate" (as originally done by dataladgh-3705's bb17d0e) is not
correct.  It will sort lightweight tags by their committer date and
leave annotated tags sorted alphabetically at the top of the list.
Instead we'd have to use a more involved approach, such as including
both "committerdate" and the dereferenced "*committerdate" as fields
and sorting by whichever value is non-empty for a given entry.
@codecov
Copy link

@codecov codecov bot commented Sep 25, 2019

Codecov Report

Merging #3715 into master will increase coverage by 0.26%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3715      +/-   ##
==========================================
+ Coverage   82.61%   82.87%   +0.26%     
==========================================
  Files         273      273              
  Lines       35792    35793       +1     
==========================================
+ Hits        29570    29664      +94     
+ Misses       6222     6129      -93
Impacted Files Coverage Δ
datalad/support/gitrepo.py 84.64% <ø> (+0.31%) ⬆️
datalad/support/tests/test_gitrepo.py 99.88% <100%> (ø) ⬆️
datalad/utils.py 64.28% <0%> (+0.1%) ⬆️
datalad/config.py 98.81% <0%> (+0.39%) ⬆️
datalad/support/annexrepo.py 63.32% <0%> (+0.41%) ⬆️
datalad/tests/test_cmd.py 97.29% <0%> (+0.54%) ⬆️
datalad/core/local/create.py 96.18% <0%> (+0.76%) ⬆️
datalad/tests/utils_testrepos.py 93.85% <0%> (+0.87%) ⬆️
datalad/__init__.py 84.21% <0%> (+1.5%) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f541ed1...66142c1. Read the comment docs.

@kyleam
Copy link
Contributor Author

@kyleam kyleam commented Sep 25, 2019

This documents a change in behavior that is already in master and fixes a failing test, so I'll merge it now. If there are objections to that change in behavior, we should of course still discuss it.

@kyleam kyleam merged commit 66142c1 into datalad:master Sep 25, 2019
5 checks passed
@kyleam kyleam deleted the get-tags-date branch Sep 25, 2019
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.

None yet

1 participant