Skip to content

More sanitized branch options#2665

Open
Zombiefleischer wants to merge 6 commits intojesseduffield:masterfrom
Zombiefleischer:more-sanitized-branch-options
Open

More sanitized branch options#2665
Zombiefleischer wants to merge 6 commits intojesseduffield:masterfrom
Zombiefleischer:more-sanitized-branch-options

Conversation

@Zombiefleischer
Copy link

@Zombiefleischer Zombiefleischer commented May 23, 2023

  • PR Description
    Added more sanitizing to branch names:

    • leading and trailing spaces will be removed
    • whitespaces and special chars (like *, \, etc.) will be replaced with an replacement char defined in the config (default: "-")
  • Please check if the PR fulfills these requirements

  • Cheatsheets are up-to-date (run go run scripts/cheatsheet/main.go generate)
  • Code has been formatted (see here)
  • Tests have been added/updated (see here for the integration test guide)
  • Text is internationalised (see here)
  • Docs (specifically docs/Config.md) have been updated if necessary
  • You've read through your own file changes for silly mistakes etc

Closes #2663

git *commands.GitCommand,
contexts *context.ContextTree,
model *types.Model,
config *config.UserConfig,
Copy link
Owner

Choose a reason for hiding this comment

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

no need to pass this in because the config already lives in HelperCommon, so you can use self.c.UserConfig

Copy link
Owner

@jesseduffield jesseduffield left a comment

Choose a reason for hiding this comment

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

left a comment. Also, be sure to update the docs/Config.md doc

@Zombiefleischer Zombiefleischer changed the title WIP: More sanitized branch options More sanitized branch options May 24, 2023
@Zombiefleischer Zombiefleischer marked this pull request as ready for review May 24, 2023 09:42
Copy link
Owner

@jesseduffield jesseduffield left a comment

Choose a reason for hiding this comment

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

Left another comment. Also, could we add an integration test for this? I.e. start a lazygit session, create a branch, observe that the created branch has the expected name.

return strings.Replace(input, " ", "-", -1)
func (self *RefsHelper) sanitizedBranchName(input string) string {
reg1 := regexp.MustCompile(`^-+`)
reg2 := regexp.MustCompile(`\.|\/\.|\.\.|~|\^|:|\/$|\.lock$|\.lock\/|\\|\*|\s|^\s*$|\.$|\[|\]$`)
Copy link
Owner

Choose a reason for hiding this comment

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

This regex feels like overkill to me. I would just have a slice of characters that we want to replace with hyphens one-to-one. The .lock thing is too presumptuous so let's remove that.

Copy link
Author

Choose a reason for hiding this comment

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

I copied this regex from vscode, but I can remove that part.

@jesseduffield
Copy link
Owner

Sorry it's taken me so long to review this. I've taken a closer look and now understand why the original regex had .lock: git's docs explicitly call that out as invalid. I'm happy to use the vscode regex in that case, though the code can be cleaner i.e.:

func (self *RefsHelper) sanitizedBranchName(input string) string {
	leadingDashes := regexp.MustCompile(`^-+`)
	// See https://git-scm.com/docs/git-check-ref-format for the list of invalid strings.
	invalidStrings := regexp.MustCompile(`\.|\/\.|\.\.|~|\^|:|\/$|\.lock$|\.lock\/|\\|\*|\s|^\s*$|\.$|\[|\]$`)

	result := strings.TrimSpace(input)
	result = leadingDashes.ReplaceAllString(result, "")
	result = invalidStrings.ReplaceAllString(result, self.c.UserConfig.Gui.BranchWhitespaceChar)

	return result
}

Unfortunately the code has diverged quite a lot from the main branch. Would you still be open to continuing with this @Zombiefleischer ? If not, @raulhammerl offered to pick it up in the issue

Copy link
Owner

@jesseduffield jesseduffield left a comment

Choose a reason for hiding this comment

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

marking as changes requested for my own bookkeeping

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.

Branch Whitespace Char Option

2 participants