K Jayatheerth <jayatheerthkulkarni2005@xxxxxxxxx> 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@xxxxxxxxx>
> ---
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.
