Skip to content
This repository has been archived by the owner on Sep 11, 2020. It is now read-only.

Hotfix/propose git check ref format #1173

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

thecjharries
Copy link

This is in response to #1172.

I spent the better part of the last two days attempting to PR a fix in place. I don't know Go or the project well enough to accomplish that so I split out the work I did in the hopes that it might be useful. This can be run at the beginning or end of existing ReferenceName pipelines to ensure the names are up to snuff.

I read the contribution docs before sitting down to code but focused on signing off on my commits and missed the format. I've squashed everything to address that. There's not much lost; these two files were, by and large, copypasta.

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>
Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Found a better way to move forward

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Update RN usage

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Tidy the file

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Replace struct instantiation w/fnc call

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Update original reference_tests

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Add basic Name test

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Bolster the tests w/ a String quick

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Expose HasPrefix directly

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Drop HasPrefix tests into the existing architecture

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Add IsNotEmpty for branches

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Test IsNotEmpty

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Update branch to handle new Struct

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Update refspec with NewRN

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Update config

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Update advrefs

Updated a NewRefName more

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Update advrefs

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Update report_status_test

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Finally hit the wall that comes with a messy backend

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Revert to master for a different solution

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Create first check method and test it

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Stub out necessary processing fncs

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Flesh out ActionOptions

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Create consistent naming

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Template all the things (regex is the best)

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Fill out regex

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Draft matching pairs

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Add Error messages

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Finish Trailing Lock condition

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Add forward slash check

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Finalize double dots

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Finalize excluded characters

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Finalize leading slashes

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Add consecutive slashes

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Add extra test case for specific event

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Add trailing dot case

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Add at-open-brace

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Add Skip tests

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Add RefSpecPattern functionality

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Done mucking around with reflection

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Build full handle runner

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Stylize the Enum values

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Add some default building tools

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>

Add some basic docs

Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>
Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>
Signed-off-by: CJ Harries <cj@wizardsoftheweb.pro>
@mcuadros
Copy link
Contributor

mcuadros commented Jul 1, 2019

@thecjharries wonderful contribution.

The validator is following strictly this rules: git-check-ref-format?

Can you move this to a package called plumbing/ref-validator or something similar? and place document the rules (mainly copy paste from the original git documentation) the rules?

And then make it available though a method Refname.Validate() error?

PatternOnlyAtSign = regexp.MustCompile(`^@$`)
)

type CheckRefOptions struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

CheckRefOptions/ValidatorOptions

}

type ActionOptions struct {

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove empty line

ActionOptions ActionOptions
}

func NewCheckRefOptions(default_value bool) *CheckRefOptions {
Copy link
Contributor

Choose a reason for hiding this comment

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

s/default_value/default and please read https://golang.org/doc/effective_go.html#mixed-caps

}
}

func NewActionOptions(default_value ActionChoice) *ActionOptions {
Copy link
Contributor

Choose a reason for hiding this comment

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

same

}
}

func NewRefNameChecker(
Copy link
Contributor

Choose a reason for hiding this comment

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

NewValidator

}

// This runs everything
func (v *RefNameChecker) CheckRefName() error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having the ref to validate on the receiver, send it to this function as argument, also rename the function to Validate(ref ReferenceName)

@@ -0,0 +1,388 @@
package plumbing
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename the file to *_test.go https://golang.org/doc/code.html#Testing

}
}

// func (s *ReferenceValidationSuite) TestCheckRefName(c *C) {
Copy link
Contributor

Choose a reason for hiding this comment

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

leftover

return nil
}

// This runs everything
Copy link
Contributor

Choose a reason for hiding this comment

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

Please read carefully https://blog.golang.org/godoc-documenting-go-code, to fix the documentation in general

@thecjharries
Copy link
Author

Absolutely! I really appreciate you taking the time to go through the code like that. That standard link will be very useful.

I want to make sure I set the package up correctly. plumbing/revlist looks like a good example. Should I mimic that?

It might be a few days before I have time to sit down and fully digest the docs to do this correctly. As I said, I really appreciate your feedback!

@mcuadros
Copy link
Contributor

You can takea lookg to https://github.com/src-d/go-git/tree/master/plumbing/protocol/packp/sideband, but in general you can take a look to any package.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants