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
Fix 6310 Provide ability to sort/order branches and tags #8427
Conversation
b051ec4
to
1fa75cd
Compare
Codecov Report
@@ Coverage Diff @@
## master #8427 +/- ##
==========================================
+ Coverage 53.00% 53.11% +0.10%
==========================================
Files 877 881 +4
Lines 62128 62366 +238
Branches 11332 11351 +19
==========================================
+ Hits 32934 33127 +193
- Misses 26533 26567 +34
- Partials 2661 2672 +11
Flags with carried forward coverage won't be shown. Click here to find out more. |
@msftbot merge in 48 hours |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brief, review, brief testing. Some comments, considered blocking auto merge
GitUI/BranchTreePanel/ContextMenu/GitRefsSortOrderContextMenuItem.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
git show-ref do not have --sort option
I cannot trigger the use of --sort to GetRefsCmd() with sort though
@msftbot forget what I just told you |
df6b42f
to
8f1acd5
Compare
Outstanding: resolution of the context menu placement for the Tags node. Judging by incoming PRs looks like @ivangrek has significantly better understanding of this part 😁. |
8f1acd5
to
e0dddf2
Compare
Champion! You're clearly a better tester than I am 🙇♂️ No, this is definitely something I didn't anticipate, and now I need to think of it more. |
6a00136
to
04617c5
Compare
@gerhardol I'd appreciate your re-review, I believe your points are addressed. |
04617c5
to
e7e35a2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mostly looked at GitModule changes, a few comments only.
Briefly looked at the rest, briefly tested.
I suggest it is merged, but not deep enough review to formally approve.
{ sortBy != GitRefsSortBy.Default, GetSortCriteria(tags, sortBy, sortOrder), string.Empty }, | ||
format, | ||
{ branches, "refs/heads/", string.Empty }, | ||
{ branches && tags, "refs/remotes/", string.Empty }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related issue #6044, this PR shows more clearly the special handling for remotes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for pointing me to this. This API can do with quite a bit of improvement, but I didn't want to change to much in the context of this PR. I will do a follow up improvement later.
// Else | ||
// format = %(*objectname) %(*refname) // i.e. info from a referenced commit | ||
// End | ||
format = @"--format=""%(if)%(authordate)%(then)%(objectname) %(refname)%(else)%(*objectname) %(*refname)%(end)"""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes #8301 replacing PR #8309
This assumes the comment in #8301 (comment) is OK: non-dereferenced tags are not used in the application
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, we can certainly look at this as a follow up 👍
8b67585
to
cc22182
Compare
Taking this as an approval. Feel free to cancel the auto-merge, if it isn't so. |
Hello @RussKie! Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:
If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest it is merged, but not deep enough review to formally approve.
Taking this as an approval. Feel free to cancel the auto-merge, if it isn't so.
Approval to merge, but incomplete review, focusing on GetRefs() that I found to be important.
More of browsing the remaining, I could understand what I looked at and no obvious problems.
There are a few discussion points still, can be discussed separately.
Approving with the limitations in the review stated above.
Restore the functionality (albeit improved) removed in 9c7c76f. Allow to either defer sorting to git (i.e. sort by Default) which is either implicit, or configured via `git config branch.sort`. Alternatively expose the following sort by options: - authordate - committerdate - creatordate - taggerdate - refname - objectsize - upstream For user-specified sort, allow to select ascending or descending order (default: desc). Git v2.19+ is required to facilitate sort by default, hence bump in the min version. Update the recommended version to 2.28.0 while we are at it. The sort configuration applies to both the left panel and the branches dropdown in the toolstrip. Fixes gitextensions#6310 Fixes gitextensions#8301 Resolves gitextensions#7472 Closes gitextensions#4869 Closes gitextensions#8309
cc22182
to
a0f8eea
Compare
@RussKie Thank you!!!! ❤️ Been missing this a lot! Only question I have is: is there a way to sort alphanumerically, but case insensitively ? Might be possible already, but I haven't figured out how to do it. |
I don't know.
Git is a Linux tool, so it is generally case sensitive. And as you can see
from the PR diff or command log (F12) the sort command can be quite
intricate.
That said, I haven't considered or investigated case sensitivity because
for me it was never an issue...
|
Related: #8508 |
Fixes #6310
Fixes #7456
Fixes #8301
Fixes #8427
Resolves #7472
Closes #4869
Closes #8309 as obsolete
Proposed changes
Restore the functionality (albeit improved) removed in 9c7c76f.
Allow to either defer sorting to git (i.e. sort by Default) which is either implicit, or configured via
git config branch.sort
.Alternatively expose the following sort by options:
For user-specified sort, allow to select ascending or descending order (default: desc).
Git v2.19+ is required to facilitate the above, hence bump in the min version.
The sort configuration applies to both the left panel and the branches dropdown in the toolstrip.
Screenshots
After
Settings:
The left panel:
Test methodology
✒️ I contribute this code under The Developer Certificate of Origin.