Re: [PATCH] branch: segfault fixes and validation

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

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> branch_get() can return NULL (so far on detached HEAD only)...

Do you anticipate any other cases where the API call should validly
return NULL?  I offhand do not, ...

> but some
> code paths in builtin/branch.c cannot deal with that and cause
> segfaults. Fix it.
>
> While at there, make sure to bail out when the user gives 2 or more
> arguments, but only the first one is processed.
>
> Reported-by: Per Cederqvist <cederp@opera.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/branch.c  | 20 ++++++++++++++++++++
>  t/t3200-branch.sh | 21 +++++++++++++++++++++
>  2 files changed, 41 insertions(+)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 6371bf9..c1d688e 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -889,6 +889,13 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  	} else if (new_upstream) {
>  		struct branch *branch = branch_get(argv[0]);
>  
> +		if (argc > 1)
> +			die(_("too many branches to set new upstream"));
> +
> +		if (!branch)
> +			die(_("could not figure out the branch name from '%s'"),
> +			    argc == 1 ? argv[0] : "HEAD");

... and find this "could not figure out" very unfriendly to the
user.  Is it a bug in the implementation, silly Git failing to find
what branch the user meant?  What recourse does the user have at
this point?

Or is it a user error to ask Git to operate on the branch pointed at
by HEAD, when HEAD does not refer to any branch?  If that is the
case, then the message should say that there is no current branch to
operate on because the user is on a detached HEAD.  That would point
the user in the right direction to correct the user error, no?

Of course, argv[0] may not be HEAD and in that case you may have to
say "no such branch %s" % argv[0] or something.  The point is that
"could not figure out" feels a bit too lazy.

> @@ -901,6 +908,13 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  		struct branch *branch = branch_get(argv[0]);
>  		struct strbuf buf = STRBUF_INIT;
>  
> +		if (argc > 1)
> +			die(_("too many branches to unset upstream"));
> +
> +		if (!branch)
> +			die(_("could not figure out the branch name from '%s'"),
> +			    argc == 1 ? argv[0] : "HEAD");

Likewise.

> @@ -916,6 +930,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  		int branch_existed = 0, remote_tracking = 0;
>  		struct strbuf buf = STRBUF_INIT;
>  
> +		if (!strcmp(argv[0], "HEAD"))
> +			die(_("it does not make sense to create 'HEAD' manually"));
> +
> +		if (!branch)
> +			die(_("could not figure out the branch name from '%s'"), argv[0]);

Likewise.

Thanks.