Re: [PATCH 5/6] sequencer: duplicate the result of resolve_ref_unsafe()
From: Derrick Stolee <hidden>
Date: 2022-09-30 17:23:35
On 9/30/2022 10:09 AM, SZEDER Gábor wrote:
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.
Good catch!
- 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));Moving to a 'char *' matches our typical pattern of "I am responsible for freeing this or passing that responsibility to someone else."
+test_expect_success 'what should I call this?!' '
Perhaps 'with multiple refs, correctly report worktree' ? Thanks, -Stolee