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

feat(revsets): add merges() and allow branches() to filter branch names #1130

Merged
merged 3 commits into from
Dec 31, 2023

Conversation

claytonrcarter
Copy link
Collaborator

@claytonrcarter claytonrcarter commented Nov 8, 2023

  • I have found merges() to be useful, in particular on public commits.
  • for branches(<pattern>) see Tell `sync` to ignore certain branches #1117
  • branches(<pattern>) needs more work; it's currently mush slower that I would expect, and it seems to not match some branchs (at least it didn't match all the branches I expected when I used it to push these recent PRs) Edit: I believe that this is fixed.

@claytonrcarter
Copy link
Collaborator Author

claytonrcarter commented Nov 9, 2023

branches() needs more work; it's currently mush slower that I would expect, and it seems to not match some branchs (at least it didn't match all the branches I expected when I used it to push these recent PRs)

For example, on my copy of the git-branchless repo, I have approx 1600 commits and 7 branches, as follows:

⋮
◆ 5871499 2h (ᐅ master) Change 'suggest' to 'discuss'
┣━┓
┃ ◯ 2bd4827 5s (crc/submit-dry-run) feat(submit): add --dry-run flag
┣━┓
┃ ◯ 306e9b4 5s (crc/fix-status-spaces, local) fix(status): rudimentary support for filenames with spaces
┣━┓
┃ ◯ 9661224 5s (crc/sync-resolve-stack) feat(sync): resolve revset arguments to their entire stacks
┣━┓
┃ ⊘ 2 omitted commits
┃ ⋮
┃ ◯ 620a061 5s (move-fixup-on-disk) temp(on disk fixup): Try to fix abandoned branches
⋮
⊘ 1 omitted commit
⋮
◯ fa5342c 5s (crc/revsets) feat(revset): allow branches() to accept a text pattern

Running git sl 'branches(crc)' takes around 3s to run, and seems to process every commit in the repo. Also, git submit 'branches(crc)' --dry-run doesn't match/report the crc/fix-status-spaces branch, which I'm currently assuming has something to do w/ the other branch that points to that commit. (Hmm, on second thought, perhaps that's an issue w/ the implementation of #1129?) Edit: nope, that was a separate issue #1131

@claytonrcarter claytonrcarter force-pushed the crc/revsets branch 2 times, most recently from fa5342c to d324481 Compare November 9, 2023 03:05
@claytonrcarter
Copy link
Collaborator Author

Running git sl 'branches(crc)' takes around 3s to run, and seems to process every commit in the repo.

This was my misunderstanding of how revset Matchers work. I hadn't connected that they work on all visible commits, and there is currently no way to restrict that. (This is the first revset feature – that I can tell – that would make any sense to operate on anything other than all visible commits, because we know that branches are (probably) a subset of visible commits.)

git-branchless-revset/src/builtins.rs Outdated Show resolved Hide resolved
git-branchless-revset/src/builtins.rs Show resolved Hide resolved
git-branchless-revset/src/builtins.rs Outdated Show resolved Hide resolved
@claytonrcarter claytonrcarter marked this pull request as draft November 13, 2023 01:56
claytonrcarter added a commit to claytonrcarter/git-branchless that referenced this pull request Nov 13, 2023
claytonrcarter added a commit to claytonrcarter/git-branchless that referenced this pull request Nov 14, 2023
claytonrcarter added a commit to claytonrcarter/git-branchless that referenced this pull request Nov 20, 2023
claytonrcarter added a commit to claytonrcarter/git-branchless that referenced this pull request Nov 20, 2023
claytonrcarter added a commit to claytonrcarter/git-branchless that referenced this pull request Dec 23, 2023
@claytonrcarter claytonrcarter force-pushed the crc/revsets branch 2 times, most recently from c196623 to 466a6f0 Compare December 25, 2023 11:50
claytonrcarter added a commit to claytonrcarter/git-branchless that referenced this pull request Dec 25, 2023
@claytonrcarter claytonrcarter marked this pull request as ready for review December 26, 2023 13:30
@claytonrcarter
Copy link
Collaborator Author

Sorry, I lost track of this. As I recall, I have addressed the review as best I could figure out; see my comments from 11/13. I think I just forgot to un-draft this after doing so. Thank you!

claytonrcarter added a commit to claytonrcarter/git-branchless that referenced this pull request Dec 26, 2023
git-branchless-revset/src/builtins.rs Outdated Show resolved Hide resolved
git-branchless-revset/src/builtins.rs Outdated Show resolved Hide resolved
git-branchless-revset/src/builtins.rs Outdated Show resolved Hide resolved
git-branchless-revset/src/builtins.rs Outdated Show resolved Hide resolved
Copy link
Owner

@arxanas arxanas left a comment

Choose a reason for hiding this comment

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

Looks good pending a few comments!

git-branchless-revset/src/pattern.rs Show resolved Hide resolved
claytonrcarter added a commit to claytonrcarter/git-branchless that referenced this pull request Dec 31, 2023
claytonrcarter added a commit to claytonrcarter/git-branchless that referenced this pull request Dec 31, 2023
claytonrcarter added a commit to claytonrcarter/git-branchless that referenced this pull request Dec 31, 2023
@claytonrcarter claytonrcarter force-pushed the crc/revsets branch 2 times, most recently from fe04365 to 8b7b136 Compare December 31, 2023 13:17
claytonrcarter added a commit to claytonrcarter/git-branchless that referenced this pull request Dec 31, 2023
@claytonrcarter
Copy link
Collaborator Author

Looks good pending a few comments!

Great, thank you!

@claytonrcarter claytonrcarter enabled auto-merge (rebase) December 31, 2023 13:24
@claytonrcarter claytonrcarter merged commit 6e789ba into arxanas:master Dec 31, 2023
13 checks passed
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.

2 participants