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

ShowOnlyCurrentBranch were not showing commits #10228

Merged

Conversation

gerhardol
Copy link
Member

@gerhardol gerhardol commented Oct 5, 2022

Fixes #10225

Proposed changes

Git Notes must be excluded for --all only with the previous argument "--not --glob=notes --not".
Changed to the easier to understand "--exclude=refs/notes" that could be used independent of the various branch filters (including reflog).
However, "--exclude=refs/notes" only makes sense with --all, so kept there.

Test methodology

Adopted test.
(It is really of the kind to increase test coverage, so quite meaningless.)

Merge strategy

I agree that the maintainer squash merge this PR (if the commit message is clear).


✒️ I contribute this code under The Developer Certificate of Origin.

@ghost ghost assigned gerhardol Oct 5, 2022
@gerhardol
Copy link
Member Author

@mstv should we show stash and notes for current only and branches?
Not done in 3.5

Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've looked at the changes but I can't currently verify the correctness of the changes.

I think we ought to come up with integration tests which would exercise various filters and assert mber of entries in the revision grid.

@gerhardol gerhardol added this to the 4.0.0 milestone Oct 6, 2022
@gerhardol
Copy link
Member Author

I've looked at the changes but I can't currently verify the correctness of the changes.

You will need to read https://git-scm.com/docs/git-log and understand the order and dependency of options.
The handling was quite complicated before when the implementation was split in three modules, should be easier now.

I think we ought to come up with integration tests which would exercise various filters and assert mber of entries in the revision grid.

There are tests, some added in the recent refactoring. What was missing here was that the option was dependent for this option (most options are unrelated). That test was added (will be added for stash too). The option was also changed to be easier to understand from the man page.

I have not used this myself yet, it is therefore this is a draft. (Was too late when creating.)

@gerhardol gerhardol force-pushed the feature/i10225-show-current-branch branch from 96bb739 to 1bcdb5c Compare October 6, 2022 19:27
@gerhardol gerhardol marked this pull request as ready for review October 6, 2022 19:37
@gerhardol
Copy link
Member Author

gerhardol commented Oct 6, 2022

CodeFactor reports an issue due to the added test case generator FilterInfo_NotesStash
Edit: fixed a problem with --exclude does not assume /* as was the case with --glob

@gerhardol gerhardol force-pushed the feature/i10225-show-current-branch branch from 1bcdb5c to 0c5bdd4 Compare October 6, 2022 20:40
@mstv
Copy link
Member

mstv commented Oct 6, 2022

Should we show stash and notes for current only and branches?

I think at least stash@{0} should always be displayed. Stashes are one's own work. (The Show stashes option could be added to the Advanced Filter - only if someone complains.)

I don't use notes (and I am not fond of auxilary commits). Though if one uses them, these commits could be of interest - at least if they are related to the visible branches.

@gerhardol
Copy link
Member Author

I think at least stash@{0} should always be displayed. Stashes are one's own work. (The Show stashes option could be added to the Advanced Filter - only if someone complains.)

Added, filter is weird though...

I don't use notes (and I am not fond of auxilary commits). Though if one uses them, these commits could be of interest - at least if they are related to the visible branches.

These commits could be excluded also for --all, not of interest. The commit they relate are not seen.
image

@gerhardol
Copy link
Member Author

Changed the filter to include stash@{0}, added a test.
I was tempted to make this ref independent of the branch filter, but it takes about 1 ms to resolve this ref for me, so it is dependent of current/filtered branches

--
I have 563 stashes in my GE test repo and to resolve all of them 100 times with --all, limiting output to 100 commits and that requires about 1:38 instead of 48 s, which indicates one stash is resolved in 1 ms. As the tests are repeated, a real scenario without caching could require more time.
So always adding stash@{0} will add 1 ms to --reflog and -all, even if not needed. But the tests and the flow would be simpler.

@mstv
Copy link
Member

mstv commented Oct 7, 2022

The test GitExtensions.UITests.UserControls.RevisionGrid.RevisionGridControlTests.View_reflects_reset_branch_filter... seems to fail consistently.

stash@{0} makes git think this was the HEAD of the Current branch (only).

I think we must create a (set of) test repo(s) and verify that the expected commits are returned by git.

@gerhardol gerhardol force-pushed the feature/i10225-show-current-branch branch from a2e0cab to 0c379e1 Compare October 7, 2022 21:19
@gerhardol
Copy link
Member Author

The test GitExtensions.UITests.UserControls.RevisionGrid.RevisionGridControlTests.View_reflects_reset_branch_filter... seems to fail consistently.

stash@{0} makes git think this was the HEAD of the Current branch (only).

I think we must create a (set of) test repo(s) and verify that the expected commits are returned by git.

I pushed an update. If there were no stashes, the query failed. So back to a convoluted logic to get stash@{0} in the grid for current only...

@mstv
Copy link
Member

mstv commented Oct 7, 2022

When refreshing (or switching between All branches and Filtered branches with empty filter), the result varies randomly:

image

image

image

image

image

@gerhardol
Copy link
Member Author

When refreshing (or switching between All branches and Filtered branches with empty filter),

That is the same...

the result varies randomly:

The Git arguments is the same in this scenario, so it is a different issue
I cannot see why the revisions are processed in a different way.

Git Notes must be excluded for --all
@gerhardol gerhardol force-pushed the feature/i10225-show-current-branch branch from 0c379e1 to db64a50 Compare October 8, 2022 19:31
@gerhardol
Copy link
Member Author

Squashed the changes for "stash@{0}" for current/filtered. That commit could be separated too (I prefer to keep it in this though).

Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working this out

@gerhardol
Copy link
Member Author

Rebase merge this tomorrow

Skip refs/notes (should also be skipped for --all).

FilterInfoTests: proper regex

Remove unneeded imports
@gerhardol gerhardol force-pushed the feature/i10225-show-current-branch branch from 04674c6 to 51fd2c5 Compare October 9, 2022 18:27
@gerhardol gerhardol merged commit 077046c into gitextensions:master Oct 9, 2022
@gerhardol gerhardol deleted the feature/i10225-show-current-branch branch October 9, 2022 19:45
@ghost ghost modified the milestones: 4.0.0, vNext Oct 9, 2022
gerhardol added a commit that referenced this pull request Oct 9, 2022
Skip refs/notes (should also be skipped for --all).

FilterInfoTests: proper regex

Remove unneeded imports

(cherry picked from commit 077046c)
@gerhardol gerhardol modified the milestones: vNext, 4.0.0 Oct 9, 2022
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.

v4 beta "Show current branch only" shows blank page
3 participants