Re: [PATCH v3 1/3] branch: add '--name-prefix' option

[Junio C Hamano <gitster@pobox.com>]

"VALERI Yoann via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/Documentation/git-branch.adoc b/Documentation/git-branch.adoc
> index c0afddc424..00967fa758 100644
> --- a/Documentation/git-branch.adoc
> +++ b/Documentation/git-branch.adoc
> @@ -17,7 +17,8 @@ git branch [--color[=<when>] | --no-color] [--show-current]
>  	   [(-r|--remotes) | (-a|--all)]
>  	   [--list] [<pattern>...]
>  git branch [--track[=(direct|inherit)] | --no-track] [-f]
> -	   [--recurse-submodules] <branch-name> [<start-point>]
> +	   [--recurse-submodules] [--name-prefix=<token>]
> +           <branch-name> [<start-point>]

The indentation of the last line seems a bit off; the previous line
uses leading HT both in the original and in the updated version, but
the last line uses SP indent.

> @@ -64,6 +65,10 @@ Note that this will create the new branch, but it will not switch the
>  working tree to it; use `git switch <new-branch>` to switch to the
>  new branch.
>  
> +With a `--name-prefix` option, you can add a prefix to the branch to create.
> +This can either a simple name, or a token. Currently, only '@{current}' is
> +managed as token, and will use the current branch name as prefix.

"can either" -> "can either be".
"managed" -> "supported".

> @@ -319,6 +324,10 @@ superproject's "origin/main", but tracks the submodule's "origin/main".
>  	and the object it points at.  _<format>_ is the same as
>  	that of linkgit:git-for-each-ref[1].
>  
> +`--name-prefix <token>`::
> +	A string that will be used as prefix to the name of the new branch to
> +        create. Can be '@{current}' to use the current branch's name.
> +
>  _<branch-name>_::
>  	The name of the branch to create or delete.
>  	The new branch name must pass all checks defined by
> diff --git a/branch.c b/branch.c
> index 243db7d0fc..c24d7ce823 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -365,6 +365,23 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name)
>  	return 0;
>  }
>  
> +void add_branch_prefix(const char *name_prefix,
> +					   const char *current_branch, struct strbuf *buf)

Overly deep indentation here.  Our tab-width is always 8, and you
would align the first "const char" on these two lines.

> +{
> +	int value = 0;

Unused variable?

> +	if (!name_prefix)
> +		return;
> +
> +	if (name_prefix[0] != '@') {
> +		strbuf_addstr(buf, name_prefix);
> +		return;
> +	}
> +
> +	if (strcmp(name_prefix, "@{current}") == 0)
> +		strbuf_addstr(buf, current_branch);
> +}

What happens when we need to support more than the @{current}?  Will
this function grow more parameters and the callers need to prepare
more parameters, even if only one of them may be picked by this
function?  That does not smell like a sound way to make things
maintainable.

> diff --git a/builtin/branch.c b/builtin/branch.c
> index c577b5d20f..58631913c7 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> ...
> +		add_branch_prefix(name_prefix, start_name, &new_branch_name);


Here, `start_name` is passed as `current_branch` to `add_branch_prefix`.
However, `start_name` is the `<start-point>` (e.g., another branch,
a tag, or a commit). If the user runs:

    $ git branch --name-prefix=@{current} new-branch other-branch

the prefix will be `other-branch` instead of the *current* branch
name as advertised in the documentation.  It _may_ be how the
feature was intended to work, but then the name "current" and the
way the documentation describes the token are both misleading.

> +			create_branch(the_repository, new_branch_name.buf, start_name,
> +						  force, 0, reflog, quiet, track, 0);

Overly deep indentation.