git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 5/6] sequencer: duplicate the result of resolve_ref_unsafe()

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Sep 30, 2022, 16:51 UTC
Message-ID
<220930.86edvsvm7q.gmgdl@evledraar.gmail.com>
In-Reply-To
<20220930140948.80367-6-szeder.dev@gmail.com>
On Fri, Sep 30 2022, SZEDER Gábor wrote:
Show 98 quoted lines
> When 'git rebase -i --update-refs' generates the todo list for the
> rebased commit range, an 'update-ref' instruction is inserted for each
> ref that points to one of those commits, except for the rebased branch
> (because at the end of the rebase it will be updated anyway) and any
> refs that are checked out in a worktree; for the latter a "Ref <ref>
> checked out at '<worktree>'" comment is added.  One of these comments
> can be missing under some circumstances: if the oldest commit with a
> ref pointing to it has multiple refs pointing to it, and at least one
> of those refs is checked out in a worktree, and one of them (but not
> the first) is checked out in the worktree associated with the last
> directory entry in '.git/worktrees'.
>
> The culprit is the add_decorations_to_list() function, which calls
> resolve_ref_unsafe() to figure out the refname of the rebased branch.
> However, resolve_ref_unsafe() can (and in this case does) return a
> pointer to a static buffer.  Alas, add_decorations_to_list() holds on
> that static buffer until the end of the function, using its contents
> to compare refnames with the rebased branch, while it also calls a
> function that invokes refs_resolve_ref_unsafe() internally [1], and
> which overwrites the content of that static buffer, and messes up
> subsequent refname comparisons.
>
> Use xstrdup_or_null() to keep a copy of resolve_ref_unsafe()'s return
> value for the duration of add_decorations_to_list().
>
> [1] #0  refs_resolve_ref_unsafe (refs=0x555555a23d00,
>         refname=0x555555928523 "HEAD", resolve_flags=0, oid=0x555555a23c78,
>         flags=0x7fffffffc0cc) at refs.c:1781
>     #1  0x000055555587a1d9 in add_head_info (wt=0x555555a23c50) at worktree.c:33
>     #2  0x000055555587a446 in get_linked_worktree (id=0x555555a19c43 "WorkTree")
>         at worktree.c:91
>     #3  0x000055555587a628 in get_worktrees () at worktree.c:135
>     #4  0x00005555556a7676 in prepare_checked_out_branches () at branch.c:385
>     #5  0x00005555556a7a76 in branch_checked_out (
>         refname=0x555555a12e9c "refs/heads/branch2") at branch.c:446
>     #6  0x0000555555824f55 in add_decorations_to_list (commit=0x5555559efd08,
>         ctx=0x7fffffffc3e0) at sequencer.c:5946
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  sequencer.c                   |  5 +++--
>  t/t3404-rebase-interactive.sh | 34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index fba92c90b1..f1732f88f3 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5917,10 +5917,10 @@ static int add_decorations_to_list(const struct commit *commit,
>  				   struct todo_add_branch_context *ctx)
>  {
>  	const struct name_decoration *decoration = get_name_decoration(&commit->object);
> -	const char *head_ref = resolve_ref_unsafe("HEAD",
> +	const char *head_ref = xstrdup_or_null(resolve_ref_unsafe("HEAD",
>  						  RESOLVE_REF_READING,
>  						  NULL,
> -						  NULL);
> +						  NULL));
>  
>  	while (decoration) {
>  		struct todo_item *item;
> @@ -5965,6 +5965,7 @@ static int add_decorations_to_list(const struct commit *commit,
>  		decoration = decoration->next;
>  	}
>  
> +	free((char *)head_ref);
>  	return 0;
>  }
>  
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 7f0df58628..2e081b3914 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -2016,6 +2016,40 @@ test_expect_success REFFILES '--update-refs: check failed ref update' '
>  	test_cmp expect err.trimmed
>  '
>  
> +test_expect_success 'what should I call this?!' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		test_commit_bulk --message="%s" 4 &&
> +		git branch branch1 HEAD^ &&
> +		git branch branch2 HEAD^ &&
> +		git branch branch3 HEAD^ &&
> +
> +		git worktree add WorkTree branch2 &&
> +		git worktree list --porcelain >out &&
> +		wtpath=$(sed -n -e "s%^worktree \(.*/WorkTree\)%\1%p" out) &&
> +
> +		cat >expect <<-EOF &&
> +		pick $(git log -1 --format=%h HEAD^^) 2
> +		pick $(git log -1 --format=%h HEAD^) 3
> +		update-ref refs/heads/branch3
> +		# Ref refs/heads/branch2 checked out at $SQ$wtpath$SQ
> +		update-ref refs/heads/branch1
> +		pick $(git log -1 --format=%h HEAD) 4

We can let this pass, but FWIW these are all cases where we'll lose "git log"'s exit code. So:

	first=$(git log ...) &&
        [...]
        cat [...]
        pick $first
        [...]
If we want to catch that.
Previous: Ævar Arnfjörð BjarmasonNext: Derrick Stolee
Message 12 of 20 in “rebase --update-refs: smooth out some rough edges”
  1. 0/6 rebase --update-refs: smooth out some rough edgesSZEDER Gábor, Sep 30, 2022
  2. 1/6 t2407-worktree-heads.sh: remove outdated loopSZEDER Gábor, Sep 30, 2022
  3. 2/6 t3404-rebase-interactive: mark a test with REFFILES prereqSZEDER Gábor, Sep 30, 2022
  4. Ævar Arnfjörð BjarmasonSep 30, 2022
  5. Junio C HamanoOct 5, 2022
  6. 4/6 sequencer: avoid empty lines after 'update-ref' instructionsSZEDER Gábor, Sep 30, 2022
  7. Derrick StoleeSep 30, 2022
  8. Junio C HamanoOct 5, 2022
  9. 3/6 rebase -i: emphasize that 'update-ref' expects a fully-qualified refSZEDER Gábor, Sep 30, 2022
  10. 5/6 sequencer: duplicate the result of resolve_ref_unsafe()SZEDER Gábor, Sep 30, 2022
  11. Ævar Arnfjörð BjarmasonSep 30, 2022
  12. Ævar Arnfjörð BjarmasonSep 30, 2022
  13. Derrick StoleeSep 30, 2022
  14. SZEDER GáborOct 9, 2022
  15. 6/6 sequencer: fail early if invalid ref is given to 'update-ref' instructionSZEDER Gábor, Sep 30, 2022
  16. Ævar Arnfjörð BjarmasonSep 30, 2022
  17. Derrick StoleeSep 30, 2022
  18. SZEDER GáborOct 1, 2022
  19. Junio C HamanoOct 1, 2022
  20. Junio C HamanoOct 5, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.