Re: [PATCH v2] stash: fix incorrect branch name in stash message

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 8, 2025, 16:20 UTC
Message-ID: <xmqqo6uyw6h4.fsf@gitster.g>
In-Reply-To: <20250608144542.275836-1-jayatheerthkulkarni2005@gmail.com>

K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:

Show 14 quoted lines

> When creating a stash, Git uses the current branch name
> of the superproject to construct the stash commit message.
> However, in repositories with submodules,
> the message may mistakenly display the submodule branch name instead.
>
> This is because `refs_resolve_ref_unsafe()` returns a pointer to a static buffer.
> Subsequent calls to the same function overwrite the buffer,
> corrupting the originally fetched `branch_name` used for the stash message.
>
> Use `xstrdup()` to duplicate the branch name immediately after resolving it,
> so that later buffer overwrites do not affect the stash message.
>
> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
> ---

Nicely described.

Show 14 quoted lines

> @@ -1372,6 +1372,7 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b
>  	const char *head_short_sha1 = NULL;
>  	const char *branch_ref = NULL;
>  	const char *branch_name = "(no branch)";
> +	char *branch_name_buf = NULL;
>  	struct commit *head_commit = NULL;
>  	struct commit_list *parents = NULL;
>  	struct strbuf msg = STRBUF_INIT;
> @@ -1401,11 +1402,16 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b
>  		ret = 1;
>  		goto done;
>  	}
> -
> +

Addition of trailing whitespace?

You can avoid such mistakes in the future by enabling our sample pre-commit hook, which essentially does

	git diff-index --check --cached $against --

where $against is HEAD (or an empty tree object while preparing for an initial commit).

Show 10 quoted lines

>  	branch_ref = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
>  					     "HEAD", 0, NULL, &flags);
> -	if (flags & REF_ISSYMREF)
> -		skip_prefix(branch_ref, "refs/heads/", &branch_name);
> +
> +	if (flags & REF_ISSYMREF) {
> +		if (skip_prefix(branch_ref, "refs/heads/", &branch_name))
> +			branch_name = branch_name_buf = xstrdup(branch_name);
> +	} else
> +		branch_name = "(no branch)";

Do we need the else clause? The original did not have it and showed the "(no branch)" message without an issue, and I do not see anything is changed by what happens inside the other side of this if statement. Am I missing something?

Show 7 quoted lines

> @@ -1495,6 +1501,7 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b
>  	strbuf_release(&msg);
>  	strbuf_release(&untracked_files);
>  	free_commit_list(parents);
> +	free(branch_name_buf);
>  	return ret;
>  }

Makes sense.

This is a common pattern we use with a variable whose name contains "to_free" (e.g., "branch_name_to_free"), but "branch_name_buf" is pleanty readable and easy to understand what is going on.

> +test_expect_success 'stash reflog message uses superproject branch, not submodule branch' '

The title looks a bit on the overly-long side.  Would

    stash message records the superproject branch

be sufficient? The fact that the stash is implemented as reflog is invidible and irrelevant at this level, so "reflog message" is wasting bytes without adding any useful information.

What we want to make sure is that the message records the current branch name, whether the project has any submodules or not, and from that point of view,

    stash message records the correct branch name

ought to be good, but not quite, because this test is trying to trigger a bug that was present only when there are submodules, so not mentioning superproject/submodule at all would not work well.

Would

    submodules does not affect the branch recorded in stash message

work?  That is the best one I can come up with offhand.

Show 7 quoted lines

> +	git init sub_project &&
> +	(
> +		cd sub_project &&
> +		echo "Initial content in sub_project" >sub_file.txt &&
> +		git add sub_file.txt &&
> +		git commit -q -m "Initial commit in sub_project"
> +	) &&

It is easier to debug the test script if you avoid using --quiet too much. Regular "sh ./t3903-stash.sh" will squelch these output anyway, and they can be seen when the test script is run with "-v".

Show 11 quoted lines

> +	git init main_project &&
> +	(
> +		cd main_project &&
> +		echo "Initial content in main_project" >main_file.txt &&
> +		git add main_file.txt &&
> +		git commit -q -m "Initial commit in main_project" &&
> +
> +		git -c protocol.file.allow=always submodule add --quiet ../sub_project sub &&
> +		git commit -q -m "Added submodule sub_project" &&
> +
> +		git checkout -q -b feature_main &&

> +		cd sub &&
> +		git checkout -q -b feature_sub &&
> +		cd .. &&

These three lines can be written more compactly as:

		git -C sub checkout -b feature_sub &&

Show 11 quoted lines

> +		git checkout -q -b work_branch &&
> +		echo "Important work to be stashed" >work_item.txt &&
> +		git add work_item.txt &&
> +		git stash push -q -m "custom stash for work_branch" &&
> +
> +		git stash list >../actual_stash_list.txt &&
> +		grep "On work_branch: custom stash for work_branch" ../actual_stash_list.txt
> +	)
> +'
> +
>  test_done

Thanks.