Re: [RFC PATCH 02/15] pull.c: don't feed NULL to strcmp() on get_rebase_fork_point() path

["René Scharfe" <l.s.r@web.de>]

Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason:
> Fix an issue with feeding NULL to strcmp() noted by GCC's -fanalyzer,
> this fixes a bug in 1678b81ecce (pull: teach git pull about --rebase,
> 2015-06-18).
>
> In cmd_pull() we could go through the function without initializing
> the "repo" argument (the -fanalyzer output shows how exactly), we'd
> then call get_rebase_fork_point (), which would in turn call
> get_tracking_branch() with that "NULL" repo argument.
>
> Let's avoid this potential issue by returning NULL in this case, which
> will have get_rebase_fork_point() return -1 in turn.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/pull.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 01155ba67b2..ed8df004028 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -756,14 +756,16 @@ static const char *get_tracking_branch(const char *remote, const char *refspec)
>  		starts_with(spec_src, "remotes/"))
>  		spec_src = "";
>
> -	if (*spec_src) {
> -		if (!strcmp(remote, "."))
> -			merge_branch = mkpath("refs/heads/%s", spec_src);
> -		else
> -			merge_branch = mkpath("refs/remotes/%s/%s", remote, spec_src);
> -	} else
> +	if ((*spec_src && !remote) || !*spec_src) {

This is equivalent to:

+	if (!*spec_src || !remote) {

It's easier to read without the redundant term.

The patch would be even easier to understand as a one-liner that just adds
the missing check, i.e.:

-	if (*spec_src) {
+	if (*spec_src && remote) {

>  		merge_branch = NULL;
> +		goto cleanup;
> +	}
>
> +	if (!strcmp(remote, "."))
> +		merge_branch = mkpath("refs/heads/%s", spec_src);
> +	else
> +		merge_branch = mkpath("refs/remotes/%s/%s", remote, spec_src);
> +cleanup:
>  	refspec_item_clear(&spec);
>  	return merge_branch;
>  }