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

From: René Scharfe <l.s.r@web.de>
Date: Jun 8, 2025, 13:11 UTC
Message-ID: <f46443ac-eb7f-47db-8f4b-a06384e6fde5@web.de>
In-Reply-To: <20250608063537.233243-1-jayatheerthkulkarni2005@gmail.com>

Am 08.06.25 um 08:35 schrieb K Jayatheerth:

Show 11 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.

Makes sense.

Show 30 quoted lines

> 
> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
> ---
> This this patch was long due, and submodules patchs had a design choices 
> Refined this patch Added tests and freed the leak as intended.
> 
>  builtin/stash.c  | 19 +++++++++++++++----
>  t/t3903-stash.sh | 39 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 54 insertions(+), 4 deletions(-)
> 
> diff --git a/builtin/stash.c b/builtin/stash.c
> index cfbd92852a..13606efb12 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -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,20 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b
>  		ret = 1;
>  		goto done;
>  	}
> -
> -	branch_ref = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
> +	
> +	branch_ref = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),

This part just adds whitespace at the end of the lines.  Please don't.

Show 14 quoted lines

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

Looks complicated.

The if/else can be avoided by assigning both variables in the same block, or in the same statement even. You can see this pattern in the code base by searching for "to_free".

And I'm not sure the tmp variable is providing much benefit.

	if (flags & REF_ISSYMREF) {
		if (skip_prefix(branch_ref, "refs/heads/", &branch_name))
			branch_name = branch_name_buf = xstrdup(branch_name);
	}

Show 24 quoted lines

>  	head_short_sha1 = repo_find_unique_abbrev(the_repository,
>  						  &head_commit->object.oid,
>  						  DEFAULT_ABBREV);
> @@ -1495,6 +1505,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;
>  }
>  
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 74666ff3e4..5d5aac8b15 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -1592,4 +1592,43 @@ test_expect_success 'stash apply reports a locked index' '
>  	)
>  '
>  
> +test_expect_success 'stash reflog message uses superproject branch, not submodule branch' '
> +	git init main_project &&
> +	(
> +		cd main_project &&
> +		echo "Initial content in main_project" > main_file.txt &&

Documentation/CodingGuidelines recommends: "Redirection operators should be written with space before, but no space after them.", i.e., write '>main_file.txt' instead of '> main_file.txt'.

Show 8 quoted lines

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

Same here.

Show 17 quoted lines

> +		git add sub_file.txt &&
> +		git commit -q -m "Initial commit in sub_project"
> +	) &&> +
> +	(
> +		cd 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
> +		) &&
> +
> +		git checkout -q -b work_branch &&
> +
> +		echo "Important work to be stashed" > work_item.txt &&

Same.

> +		git add work_item.txt &&
> +		git stash push -q -m "custom stash for work_branch" &&
> +
> +		git stash list > ../actual_stash_list.txt &&

And here.

> +		grep "On work_branch: custom stash for work_branch" ../actual_stash_list.txt
> +	)

You could avoid spawning one subshell by building sub_project first and then doing all the work in main_project without interruption afterwards.

> +'
> +
>  test_done