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 <[email protected]>

Found a better way to move forward

Signed-off-by: CJ Harries <[email protected]>

Update RN usage

Signed-off-by: CJ Harries <[email protected]>

Tidy the file

Signed-off-by: CJ Harries <[email protected]>

Replace struct instantiation w/fnc call

Signed-off-by: CJ Harries <[email protected]>

Update original reference_tests

Signed-off-by: CJ Harries <[email protected]>

Add basic Name test

Signed-off-by: CJ Harries <[email protected]>

Bolster the tests w/ a String quick

Signed-off-by: CJ Harries <[email protected]>

Expose HasPrefix directly

Signed-off-by: CJ Harries <[email protected]>

Drop HasPrefix tests into the existing architecture

Signed-off-by: CJ Harries <[email protected]>

Add IsNotEmpty for branches

Signed-off-by: CJ Harries <[email protected]>

Test IsNotEmpty

Signed-off-by: CJ Harries <[email protected]>

Update branch to handle new Struct

Signed-off-by: CJ Harries <[email protected]>

Update refspec with NewRN

Signed-off-by: CJ Harries <[email protected]>

Update config

Signed-off-by: CJ Harries <[email protected]>

Update advrefs

Updated a NewRefName more

Signed-off-by: CJ Harries <[email protected]>

Update advrefs

Signed-off-by: CJ Harries <[email protected]>

Update report_status_test

Signed-off-by: CJ Harries <[email protected]>

Finally hit the wall that comes with a messy backend

Signed-off-by: CJ Harries <[email protected]>

Revert to master for a different solution

Signed-off-by: CJ Harries <[email protected]>

Create first check method and test it

Signed-off-by: CJ Harries <[email protected]>

Stub out necessary processing fncs

Signed-off-by: CJ Harries <[email protected]>

Flesh out ActionOptions

Signed-off-by: CJ Harries <[email protected]>

Create consistent naming

Signed-off-by: CJ Harries <[email protected]>

Template all the things (regex is the best)

Signed-off-by: CJ Harries <[email protected]>

Fill out regex

Signed-off-by: CJ Harries <[email protected]>

Draft matching pairs

Signed-off-by: CJ Harries <[email protected]>

Add Error messages

Signed-off-by: CJ Harries <[email protected]>

Finish Trailing Lock condition

Signed-off-by: CJ Harries <[email protected]>

Add forward slash check

Signed-off-by: CJ Harries <[email protected]>

Finalize double dots

Signed-off-by: CJ Harries <[email protected]>

Finalize excluded characters

Signed-off-by: CJ Harries <[email protected]>

Finalize leading slashes

Signed-off-by: CJ Harries <[email protected]>

Add consecutive slashes

Signed-off-by: CJ Harries <[email protected]>

Add extra test case for specific event

Signed-off-by: CJ Harries <[email protected]>

Add trailing dot case

Signed-off-by: CJ Harries <[email protected]>

Add at-open-brace

Signed-off-by: CJ Harries <[email protected]>

Add Skip tests

Signed-off-by: CJ Harries <[email protected]>

Add RefSpecPattern functionality

Signed-off-by: CJ Harries <[email protected]>

Done mucking around with reflection

Signed-off-by: CJ Harries <[email protected]>

Build full handle runner

Signed-off-by: CJ Harries <[email protected]>

Stylize the Enum values

Signed-off-by: CJ Harries <[email protected]>

Add some default building tools

Signed-off-by: CJ Harries <[email protected]>

Add some basic docs

Signed-off-by: CJ Harries <[email protected]>
@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.

2 participants