[BUG] git stash incorrectly showing submodule branch instead of superproject branch

[BUG] git stash incorrectly showing submodule branch instead of superproject branch

[Stuart MacDonald]

Bug report inline below. I am willing to provide further information but I am not subscribed to this list.




Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.

What did you do before the bug happened? (Steps to reproduce your issue)

I am working on a UI that includes the hardware's SDK as a submodule.
Both the UI and the SDK are our own code. A colleague is working on the
SDK.

- clone out a project that has a submodule
- set up submodule properly
- create feature branch 'feature_foo'
- move submodule to colleague's feature branch 'feature_sdk_foo'
- develop for a while

- in superproject discover bug, fix bug
- create new superproject branch to contain bug fix 'bugfix_bar'
- leave submodule branch as is; I'm intending to return to it momentarily
- commit fix via 'git add --patch file1 file2 ...'
- save bugfix debugging via 'git stash push -m "debugging" file1 file2 ...'

What did you expect to happen? (Expected behavior)

'git stash list' shows
stash@{0}: On bugfix_bar: debugging

What happened instead? (Actual behavior)

'git stash list' shows
stash@{0}: On feature_sdk_foo: debugging

What's different between what you expected and what actually happened?

The "On <branch>" says the wrong branch; specifically it's the
submodule's currently checked out branch. I have a number of branches on
the go, and need to rely on stash telling me the branch I was on when
pushing to determine when I no longer need the debugging.
The said branch doesn't even exist in the superproject.

Anything else you want to add:

Using Git For Windows on Windows 10. Using the "git bash" command line
git, not the UI git.

I'm pretty sure this used to work correctly, but it's been several years
since my last repo with a submodule (circa 2021).

The Git For Windows bash PS1 prompt lists the current branch I'm on in the
superproject, so the error is quite visible.

Reading the 'git stash' documentation didn't shed any light.
Googling didn't reveal anyone encountering this problem.
Searching git@vger.kernel.org didn't reveal anyone reporting this.

Please review the rest of the bug report below.
You can delete any lines you don't wish to share.


[System Info]
git version:
git version 2.49.0.windows.1
cpu: x86_64
built from commit: cca1f38702730b35f52c29efd62864b85e85ddcc
sizeof-long: 4
sizeof-size_t: 8
shell-path: D:/git-sdk-64-build-installers/usr/bin/sh
feature: fsmonitor--daemon
libcurl: 8.12.1
OpenSSL: OpenSSL 3.2.4 11 Feb 2025
zlib: 1.3.1
uname: Windows 10.0 26100
compiler info: gnuc: 14.2
libc info: no libc information available
$SHELL (typically, interactive shell): C:\Program Files\Git\usr\bin\bash.exe


[Enabled Hooks]
CONFIDENTIALITY NOTICE: The contents of this email, including any attachments, may contain private and confidential information intended to be reviewed only by the individual(s) or organization to whom it is addressed and may be legally protected from disclosure. If you are not the intended recipient or an authorized representative of the intended recipient, please be notified that any review, distribution, copying, saving or disclosure is strictly prohibited. If you have received this email in error, please immediately notify the sender by return email and delete this email from your system, including from the deleted items folder. Thank you for your cooperation.

Re [BUG] git stash incorrectly showing submodule branch instead of superproject branch

[K Jayatheerth]

Thank you for reporting the bug, Stuart.

This is genuinely one of the most interesting Git bugs I’ve seen in a while 
I was able to reproduce it with the following minimal steps:

    mkdir sdk && cd sdk
    git init
    echo "SDK file" > sdk.txt
    git add sdk.txt
    git commit -m "Initial commit in SDK"
    cd ..

    mkdir ui && cd ui
    git init
    git -c protocol.file.allow=always submodule add ../sdk
    git commit -m "Add SDK as submodule"

    git checkout -b feature_foo        # in main repo
    cd sdk
    git checkout -b feature_sdk_foo    # in submodule
    cd ..
    git checkout -b bugfix_bar         # still in main repo

    echo "Bugfix content" > fix.txt
    git add fix.txt
    git stash push -m "debugging"

    git stash list

After this, the stash message shows:
    stash@{0}: On feature_sdk_foo: debugging

Which clearly leaks the submodule’s branch name into the superproject’s stash label.

This is indeed a `git stash` problem 
I verified that:
- Branches were unchanged after stashing
- Submodule state remained untouched
- `git status` correctly reported the superproject branch
- Yet the stash commit was labeled with the submodule’s branch name

So the stash mechanism seems to be pulling `HEAD` information from the submodule 
context by mistake, even when the stash is purely for the superproject.

Also confirming that this is *not Windows-specific* — I reproduced it on Fedora as well.

The historical detail you shared (that this used to work fine around 2021) is helpful. 
I'll dig into the stash code paths to see if I can isolate a regression or misbehavior.

 
-Jayatheerth

Re: Re [BUG] git stash incorrectly showing submodule branch instead of superproject branch

[Stuart MacDonald]

> This is genuinely one of the most interesting Git bugs I’ve seen in a while

Thanks. I think. :-)

> The historical detail you shared (that this used to work fine around 2021) is helpful.

I said I _think_ it used to work; I am not 100% on this.
I can say 100% that
- I've been stashing for a long time; 2016 or before
- the repo with the submodules was in the Apr 2021 - Apr 2022 window
- I don't recall seeing this bug at that time, BUT, I may have just not stashed anything in a way that triggered it

So it feels like a regression but I am not 100% certain.
CONFIDENTIALITY NOTICE: The contents of this email, including any attachments, may contain private and confidential information intended to be reviewed only by the individual(s) or organization to whom it is addressed and may be legally protected from disclosure. If you are not the intended recipient or an authorized representative of the intended recipient, please be notified that any review, distribution, copying, saving or disclosure is strictly prohibited. If you have received this email in error, please immediately notify the sender by return email and delete this email from your system, including from the deleted items folder. Thank you for your cooperation.

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

[K Jayatheerth]

When creating a stash, Git builds the stash commit message using the current
branch name of the superproject. However, in repositories that include
submodules, the stash message may incorrectly show the submodule branch
instead of the superproject.

This happens because `branch_name` is obtained via `refs_resolve_ref_unsafe()`,
which returns a pointer to a static buffer. Later calls to the same function
(e.g., during commit creation or submodule reference resolution) overwrite
this buffer, causing the `branch_name` used in the final stash message to be
incorrect.

Reported-by: Stuart <smacdonald@kaimaging.com>
Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
---
 builtin/stash.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index cfbd92852a..6a375a3430 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1377,6 +1377,7 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b
 	struct strbuf msg = STRBUF_INIT;
 	struct strbuf commit_tree_label = STRBUF_INIT;
 	struct strbuf untracked_files = STRBUF_INIT;
+	char *branch_name_buf = NULL;
 
 	prepare_fallback_ident("git stash", "git@stash");
 
@@ -1404,11 +1405,20 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b
 
 	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);
-	head_short_sha1 = repo_find_unique_abbrev(the_repository,
-						  &head_commit->object.oid,
-						  DEFAULT_ABBREV);
+
+	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)";
+					    
+		head_short_sha1 = repo_find_unique_abbrev(the_repository,
+							&head_commit->object.oid,
+							DEFAULT_ABBREV);					    
 	strbuf_addf(&msg, "%s: %s ", branch_name, head_short_sha1);
 	pp_commit_easy(CMIT_FMT_ONELINE, head_commit, &msg);
 
-- 
2.49.GIT

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

[JAYATHEERTH K]

Changing this I found results to be correct

I think you can use this patch if you want a solution on an immediate basis,
testing and merging this into the master usually takes some time.

After testing with similar use case listed above

jayatheerth@fedora:~/Documents/code/test$ # Clean up previous test directories
rm -rf ~/Documents/code/test/ui ~/Documents/code/test/sdk
mkdir -p ~/Documents/code/test && cd ~/Documents/code/test

# Step 1: Create the submodule repo
mkdir sdk && cd sdk
git init
echo "SDK file" > sdk.txt
git add sdk.txt
git commit -m "Initial commit in SDK"
git stash listk stash messagextsh itbmodule add ../sdk
hint: Using 'master' as the name for the initial branch. This default
branch name
hint: is subject to change. To configure the initial branch name to use in all
hint: of your new repositories, which will suppress this warning, call:
hint:
hint:   git config --global init.defaultBranch <name>
hint:
hint: Names commonly chosen instead of 'master' are 'main', 'trunk' and
hint: 'development'. The just-created branch can be renamed via this command:
hint:
hint:   git branch -m <name>
hint:
hint: Disable this message with "git config set advice.defaultBranchName false"
Initialized empty Git repository in
/home/jayatheerth/Documents/code/test/sdk/.git/
[master (root-commit) 033e72f] Initial commit in SDK
 1 file changed, 1 insertion(+)
 create mode 100644 sdk.txt
hint: Using 'master' as the name for the initial branch. This default
branch name
hint: is subject to change. To configure the initial branch name to use in all
hint: of your new repositories, which will suppress this warning, call:
hint:
hint:   git config --global init.defaultBranch <name>
hint:
hint: Names commonly chosen instead of 'master' are 'main', 'trunk' and
hint: 'development'. The just-created branch can be renamed via this command:
hint:
hint:   git branch -m <name>
hint:
hint: Disable this message with "git config set advice.defaultBranchName false"
Initialized empty Git repository in
/home/jayatheerth/Documents/code/test/ui/.git/
Cloning into '/home/jayatheerth/Documents/code/test/ui/sdk'...
done.
[master (root-commit) 870c575] Add SDK as submodule
 2 files changed, 4 insertions(+)
 create mode 100644 .gitmodules
 create mode 160000 sdk
Switched to a new branch 'feature_foo'
Switched to a new branch 'feature_sdk_foo'
Switched to a new branch 'bugfix_bar'
Saved working directory and index state On bugfix_bar: debugging
stash@{0}: On bugfix_bar: debugging

I found it to be on the bugfix_bar correctly as intended.

-Jayatheerth

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

[Stuart MacDonald]

> Changing this I found results to be correct

The patch looks reasonable to me.

> I think you can use this patch if you want a solution on an immediate basis

I would if I knew how to update/rebuild Git For Window's git.

For now, I'll count this as fixed and try to remember to check it in the future.

Thanks!
...Stu
CONFIDENTIALITY NOTICE: The contents of this email, including any attachments, may contain private and confidential information intended to be reviewed only by the individual(s) or organization to whom it is addressed and may be legally protected from disclosure. If you are not the intended recipient or an authorized representative of the intended recipient, please be notified that any review, distribution, copying, saving or disclosure is strictly prohibited. If you have received this email in error, please immediately notify the sender by return email and delete this email from your system, including from the deleted items folder. Thank you for your cooperation.

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

[Junio C Hamano]

K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:

> @@ -1377,6 +1377,7 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b
>  	struct strbuf msg = STRBUF_INIT;
>  	struct strbuf commit_tree_label = STRBUF_INIT;
>  	struct strbuf untracked_files = STRBUF_INIT;
> +	char *branch_name_buf = NULL;
>  
>  	prepare_fallback_ident("git stash", "git@stash");
>  
> @@ -1404,11 +1405,20 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b
>  
>  	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);
> -	head_short_sha1 = repo_find_unique_abbrev(the_repository,
> -						  &head_commit->object.oid,
> -						  DEFAULT_ABBREV);
> +
> +	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)";
> +					    
> +		head_short_sha1 = repo_find_unique_abbrev(the_repository,
> +							&head_commit->object.oid,
> +							DEFAULT_ABBREV);					    
>  	strbuf_addf(&msg, "%s: %s ", branch_name, head_short_sha1);
>  	pp_commit_easy(CMIT_FMT_ONELINE, head_commit, &msg);

Funny indentation, and branch_name_buf needs to be free'ed after use
but other than that, nice digging!

Thanks.

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

[Eric Sunshine]

On Mon, May 12, 2025 at 1:50 PM Junio C Hamano <gitster@pobox.com> wrote:
> K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:
> > @@ -1404,11 +1405,20 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b
> > +     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)";
> > +
> > +             head_short_sha1 = repo_find_unique_abbrev(the_repository,
> > +                                                     &head_commit->object.oid,
> > +                                                     DEFAULT_ABBREV);
> >       strbuf_addf(&msg, "%s: %s ", branch_name, head_short_sha1);
> >       pp_commit_easy(CMIT_FMT_ONELINE, head_commit, &msg);
>
> Funny indentation, and branch_name_buf needs to be free'ed after use
> but other than that, nice digging!

This change should also be accompanied by a new test to verify the
fixed behavior, right?

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

[JAYATHEERTH K]

On Tue, May 13, 2025 at 12:24 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Mon, May 12, 2025 at 1:50 PM Junio C Hamano <gitster@pobox.com> wrote:
> > K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:
> > > @@ -1404,11 +1405,20 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b
> > > +     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)";
> > > +
> > > +             head_short_sha1 = repo_find_unique_abbrev(the_repository,
> > > +                                                     &head_commit->object.oid,
> > > +                                                     DEFAULT_ABBREV);
> > >       strbuf_addf(&msg, "%s: %s ", branch_name, head_short_sha1);
> > >       pp_commit_easy(CMIT_FMT_ONELINE, head_commit, &msg);
> >
> > Funny indentation, and branch_name_buf needs to be free'ed after use
> > but other than that, nice digging!
>
> This change should also be accompanied by a new test to verify the
> fixed behavior, right?

I will be writing a patch series after checking CI and include tests,
the only reason I sent this patch was
if the bug reporter needed an immediate fix.
Will send a new patch with test case and improved indentation

Thanks for pointing it out

-Jayatheerth

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

[Junio C Hamano]

JAYATHEERTH K <jayatheerthkulkarni2005@gmail.com> writes:

> On Tue, May 13, 2025 at 12:24 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>>
>> On Mon, May 12, 2025 at 1:50 PM Junio C Hamano <gitster@pobox.com> wrote:
>> > K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:
>> > > @@ -1404,11 +1405,20 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b
>> > > +     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)";
>> > > +
>> > > +             head_short_sha1 = repo_find_unique_abbrev(the_repository,
>> > > +                                                     &head_commit->object.oid,
>> > > +                                                     DEFAULT_ABBREV);
>> > >       strbuf_addf(&msg, "%s: %s ", branch_name, head_short_sha1);
>> > >       pp_commit_easy(CMIT_FMT_ONELINE, head_commit, &msg);
>> >
>> > Funny indentation, and branch_name_buf needs to be free'ed after use
>> > but other than that, nice digging!
>>
>> This change should also be accompanied by a new test to verify the
>> fixed behavior, right?
>
> I will be writing a patch series after checking CI and include tests,
> the only reason I sent this patch was
> if the bug reporter needed an immediate fix.
> Will send a new patch with test case and improved indentation

Don't forget to plug the leak, too ;-)

> Thanks for pointing it out

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

[K Jayatheerth]

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>
---
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), 
 					     "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)";
+					    
 	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 &&
+		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 &&
+		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 &&
+		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
-- 
2.49.GIT

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

[René Scharfe]

Am 08.06.25 um 08:35 schrieb K Jayatheerth:
> 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.

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

>  					     "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);
	}

>  	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'.

> +		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.

> +		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

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

[K Jayatheerth]

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>
---

All good points,
Removed multiple if else blocks and kept them 
within a single if/else block.

Removed the white space at the branch_ref = refs_r... line

removed the spaces in the test case.
Thank you for the feedback.

 builtin/stash.c  | 13 ++++++++++---
 t/t3903-stash.sh | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index cfbd92852a..c71e20a8cd 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,16 @@ 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),
 					     "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)";
+					    
 	head_short_sha1 = repo_find_unique_abbrev(the_repository,
 						  &head_commit->object.oid,
 						  DEFAULT_ABBREV);
@@ -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;
 }
 
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 74666ff3e4..b606827a73 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1592,4 +1592,38 @@ test_expect_success 'stash apply reports a locked index' '
 	)
 '
 
+test_expect_success 'stash reflog message uses superproject branch, not submodule branch' '
+	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"
+	) &&
+
+	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 .. &&
+
+		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
-- 
2.49.GIT

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

[Junio C Hamano]

K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:

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

> @@ -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).

>  	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?

> @@ -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.

> +	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".

> +	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 &&

> +		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.

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

[JAYATHEERTH K]

> > +
>
> 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 --
>

Thank you, I needed something like this, I think I'm going to steal
this to other projects too : )

> where $against is HEAD (or an empty tree object while preparing for
> an initial commit).
>
> >       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?

No, that's just a random idea, I get the point I will remove that in
the new patch

>
> > @@ -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.
>

Works, I will use this as is, I think this sounds good.

> > +     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".
>

Ok will remove all the -q tests

> > +     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 &&
>

True.

> > +             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.

Will send a patch soon
Thank you again

- Jayatheerth

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

[K Jayatheerth]

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>
---
1. Used the hook to remove trailing whitespaces
2. Removed the else clause 
3. Changed the test title to the suggested one 
4. Avoided --quiet Flags in Tests
5. Used git -C sub ... instead of cd sub && ... && cd ...

 builtin/stash.c  | 12 +++++++++---
 t/t3903-stash.sh | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index cfbd92852a..4215ee781c 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,15 @@ 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),
 					     "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);
+	}
+					    
 	head_short_sha1 = repo_find_unique_abbrev(the_repository,
 						  &head_commit->object.oid,
 						  DEFAULT_ABBREV);
@@ -1495,6 +1500,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..5678223cfb 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1592,4 +1592,36 @@ test_expect_success 'stash apply reports a locked index' '
 	)
 '
 
+test_expect_success 'submodules does not affect the branch recorded in stash message' '
+	git init sub_project &&
+	(
+		cd sub_project &&
+		echo "Initial content in sub_project" >sub_file.txt &&
+		git add sub_file.txt &&
+		git commit -m "Initial commit in sub_project"
+	) &&
+
+	git init main_project &&
+	(
+		cd main_project &&
+		echo "Initial content in main_project" >main_file.txt &&
+		git add main_file.txt &&
+		git commit -m "Initial commit in main_project" &&
+
+		git -c protocol.file.allow=always submodule add ../sub_project sub &&
+		git commit -m "Added submodule sub_project" &&
+
+		git checkout -b feature_main &&
+		git -C sub checkout -b feature_sub &&
+
+		git checkout -b work_branch &&
+		echo "Important work to be stashed" >work_item.txt &&
+		git add work_item.txt &&
+		git stash push -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
-- 
2.49.GIT

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

[Junio C Hamano]

K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:

> 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>
> ---
> 1. Used the hook to remove trailing whitespaces

Huh?  The pre-commit hook trick I showed was to detect and prevent
you from creating such a commit; it does not remove them for you.
Your trailing whitespaces still remain in the patch just fine ;-)

> @@ -1401,11 +1402,15 @@ 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),

which causes

    .git/rebase-apply/patch:29: trailing whitespace.

    .git/rebase-apply/patch:39: trailing whitespace.

    warning: 2 lines applied after fixing whitespace errors.
    Applying: stash: fix incorrect branch name in stash message

Other than that, looing good.

Thanks, will queue.