GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Allow the user to select exactly which references to include in the analysis by mhagger · Pull Request #75 · github/git-sizer

Conversation

@mhagger

@mhagger mhagger commented Apr 21, 2021

Copy link
Copy Markdown
Member

It would often be convenient to select exactly which references should be used as starting points when enumerating objects for analysis. We already have --branches, --tags, and --remotes to cause those types of references to be included. But what about

  • Include all references except for those under refs/notes (see issue Don't consider objects under refs/notes/* when looking for "worst" items #1).
  • Include all references except for the special refs/stash reference used for managing stashes.
  • Include all references except for remote references and GitHub's special refs/pulls references.
  • Include all branches and tags but not CI build tags that match refs/tags/build-*.

This PR adds four new options that make it possible to select arbitrary subsets of references:

  • --include PREFIX and --include-regexp PATTERN: specify references that should be included in the analysis.
  • --exclude PREFIX and --exclude-regexp PATTERN: specify references that should not be included in the analysis.

I still need to add tests.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mhagger added 11 commits April 21, 2021 10:15
@mhagger
NegatedBoolValue: rename some local variables for consistency
cbc8706
@mhagger
Split the reference-filtering code into a separate file
65f247d
@mhagger
Make PrefixFilter a little more useful (and add a docstring)
09554d3
@mhagger
IncludeExcludeFilter: new way of selecting references
9183d98
@mhagger
Use a hand-written usage message
4f6bfb5
@mhagger
Allow arbitrary references to be included/excluded by prefix
d8b2507
@mhagger
Allow arbitrary references to be included/excluded by regexp
9ee5241
@mhagger
mainImplementation(): take the command-line arguments as args
99c5f75
@mhagger
Add a --show-refs option, to show what refs are being processed
1865f33 
This will help people figure out the right reference selection
options.
@mhagger
ReferenceFilter: consider only the reference name
55a860f 
That's all we need right now, and it will make the thing easier to
test.
@mhagger
Add some tests of ReferenceFilters and IncludeExcludeFilter
197eb5a
@mhagger
mhagger marked this pull request as ready for review April 23, 2021 09:57
@mhagger

mhagger commented Apr 23, 2021

Copy link
Copy Markdown
Member Author

This could use more integration testing, but it does have unit tests and I've tested it manually, so I think it's GTG.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mhagger added 3 commits April 23, 2021 13:09
@mhagger
Add an integration test of reference selection
d01bfce
@mhagger
Merge master into include-exclude-refs
8d95600
@mhagger
updateRef(): check for errors internally
c7c0d03 
That's a big part of the point of being a test helper :-)
@mhagger

mhagger commented Apr 23, 2021

Copy link
Copy Markdown
Member Author

I added an integration test after all. Wenn schon, denn schon.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mhp
mhp approved these changes Apr 23, 2021
View reviewed changes

@mhp mhp left a comment

Copy link
Copy Markdown

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.

Overall, looks great. I've suggested a couple more test cases, to cover the documented behaviour, but that's all.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread git/ref_filter_test.go
{"refs/heads", "refs/tags/refs/heads", false},
{"refs/stash", "refs/stash", true},
{"refs/stash", "refs/stashy", false},
{"refs/remotes", "refs/remotes/origin/master", true},

@mhp mhp Apr 23, 2021

Copy link
Copy Markdown

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.

You don't appear to have any tests covering prefixes ending with /. Is it worth adding a couple of tests to assert the documented behaviour?

 // * Prefix "refs/foo/" matches "refs/foo/bar" but not "refs/foo" or
 //   "refs/foobar".

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mhagger mhagger Apr 23, 2021

Copy link
Copy Markdown
Member Author

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.

Good point. Done ☑️

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mhagger added 2 commits April 23, 2021 14:14
@mhagger
TestPrefixFilter(): add tests of prefixes ending in /
f9bab83
@mhagger
Reference filtering: improve the usage note
3bcb0cd 
And make the tests more resemble the examples from the usage note.
@mhagger
mhagger merged commit 25d20f9 into master Apr 23, 2021
@mhagger
mhagger deleted the include-exclude-refs branch April 23, 2021 12:29
This was referenced Apr 23, 2021
Don't consider objects under refs/notes/* when looking for "worst" items #1
Closed
Add new convenience options like --no-branches, --stash, etc. #77
Open
Don't show objects that only in Stash #29
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@mhp mhp mhp approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@mhagger @mhp
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.