[PATCH 0/6] rebase --update-refs: smooth out some rough edges

[PATCH 0/6] rebase --update-refs: smooth out some rough edges

[SZEDER Gábor]

A couple of cleanups and fixes to a new 'git rebase --update-refs' feature
introduced in the current release cycle.


SZEDER Gábor (6):
  t2407-worktree-heads.sh: remove outdated loop
  t3404-rebase-interactive: mark a test with REFFILES prereq
  rebase -i: emphasize that 'update-ref' expects a fully-qualified ref
  sequencer: avoid empty lines after 'update-ref' instructions
  sequencer: duplicate the result of resolve_ref_unsafe()
  sequencer: fail early if invalid ref is given to 'update-ref'
    instruction

 rebase-interactive.c          |  6 ++--
 sequencer.c                   | 28 ++++++++++++---
 t/t2407-worktree-heads.sh     |  8 ++---
 t/t3404-rebase-interactive.sh | 64 ++++++++++++++++++++++++++++++++++-
 4 files changed, 91 insertions(+), 15 deletions(-)

-- 
2.38.0.rc2.542.g9b62912f7f

[PATCH 1/6] t2407-worktree-heads.sh: remove outdated loop

[SZEDER Gábor]

The test 'refuse to overwrite during rebase with --update-refs'
initially [1] used two branches 'fake-3' and 'fake-4', and to run the
same git operation and check on both it used a 'for i in 3 4' loop.
This test was then soon updated [2] to operate only on a single
branch.  This change made that loop unnecessary, but it forgot to
remove it.

(For future work, note the misleading error message in this test: it
says "cannot force update the branch <branch> checked out at
<worktree>", but the branch in question is not, in fact, checked out
in the named worktree (it's not checked out anywhere actually!); it
can't be updated because it's kind-of "locked" by an 'update-ref'
instruction of the ongoing rebase operation.)

[1] aa7f2fd150 (branch: consider refs under 'update-refs', 2022-07-19)
[2] 89fc0b53fd (rebase: update refs from 'update-ref' commands,
                2022-07-19)

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t2407-worktree-heads.sh | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh
index 019a40df2c..126f0d4956 100755
--- a/t/t2407-worktree-heads.sh
+++ b/t/t2407-worktree-heads.sh
@@ -87,12 +87,8 @@ test_expect_success !SANITIZE_LEAK 'refuse to overwrite: worktree in rebase with
 	git branch -f can-be-updated wt-3 &&
 	test_must_fail git -C wt-3 rebase --update-refs conflict-3 &&
 
-	for i in 3 4
-	do
-		test_must_fail git branch -f can-be-updated HEAD 2>err &&
-		grep "cannot force update the branch '\''can-be-updated'\'' checked out at.*wt-3" err ||
-			return 1
-	done
+	test_must_fail git branch -f can-be-updated HEAD 2>err &&
+	grep "cannot force update the branch '\''can-be-updated'\'' checked out at.*wt-3" err
 '
 
 test_expect_success !SANITIZE_LEAK 'refuse to fetch over ref: checked out' '
-- 
2.38.0.rc2.542.g9b62912f7f

[PATCH 2/6] t3404-rebase-interactive: mark a test with REFFILES prereq

[SZEDER Gábor]

The test '--update-refs: check failed ref update' added in b3b1a21d1a
(sequencer: rewrite update-refs as user edits todo list, 2022-07-19)
directly modifies the contents of a ref file, so mark this test with
the REFFILES prereq.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t3404-rebase-interactive.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 688b01e3eb..7f0df58628 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1964,7 +1964,7 @@ test_expect_success 'respect user edits to update-ref steps' '
 	test_cmp_rev HEAD refs/heads/no-conflict-branch
 '
 
-test_expect_success '--update-refs: check failed ref update' '
+test_expect_success REFFILES '--update-refs: check failed ref update' '
 	git checkout -B update-refs-error no-conflict-branch &&
 	git branch -f base HEAD~4 &&
 	git branch -f first HEAD~3 &&
-- 
2.38.0.rc2.542.g9b62912f7f

Re: [PATCH 2/6] t3404-rebase-interactive: mark a test with REFFILES prereq

[Ævar Arnfjörð Bjarmason]

On Fri, Sep 30 2022, SZEDER Gábor wrote:

> The test '--update-refs: check failed ref update' added in b3b1a21d1a
> (sequencer: rewrite update-refs as user edits todo list, 2022-07-19)
> directly modifies the contents of a ref file, so mark this test with
> the REFFILES prereq.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  t/t3404-rebase-interactive.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 688b01e3eb..7f0df58628 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1964,7 +1964,7 @@ test_expect_success 'respect user edits to update-ref steps' '
>  	test_cmp_rev HEAD refs/heads/no-conflict-branch
>  '
>  
> -test_expect_success '--update-refs: check failed ref update' '
> +test_expect_success REFFILES '--update-refs: check failed ref update' '
>  	git checkout -B update-refs-error no-conflict-branch &&
>  	git branch -f base HEAD~4 &&
>  	git branch -f first HEAD~3 &&

We had various tests that depended on .git/refs/* for a good reason, and
some that didn't.

I may be missing something, but in this case this seems to be a "no good
reason" case. I.e. the fix seems to just be:

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 688b01e3eb6..f1c021a7f7b 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1994,8 +1994,9 @@ test_expect_success '--update-refs: check failed ref update' '
 	# At this point, the values of first, second, and third are
 	# recorded in the update-refs file. We will force-update the
 	# "second" ref, but "git branch -f" will not work because of
-	# the lock in the update-refs file.
-	git rev-parse third >.git/refs/heads/second &&
+	# the lock in the update-refs file, so we need to use
+	# "update-ref".
+	git update-ref refs/heads/second third &&
 
 	test_must_fail git rebase --continue 2>err &&
 	grep "update_ref failed for ref '\''refs/heads/second'\''" err &&

As the comment notes if you try that with "git branch" you'll get an
error, even with --force, but update-ref works just fine...

Re: [PATCH 2/6] t3404-rebase-interactive: mark a test with REFFILES prereq

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Fri, Sep 30 2022, SZEDER Gábor wrote:
>
>> The test '--update-refs: check failed ref update' added in b3b1a21d1a
>> (sequencer: rewrite update-refs as user edits todo list, 2022-07-19)
>> directly modifies the contents of a ref file, so mark this test with
>> the REFFILES prereq.
>> ...
>  	# At this point, the values of first, second, and third are
>  	# recorded in the update-refs file. We will force-update the
>  	# "second" ref, but "git branch -f" will not work because of
> -	# the lock in the update-refs file.
> -	git rev-parse third >.git/refs/heads/second &&
> +	# the lock in the update-refs file, so we need to use
> +	# "update-ref".
> +	git update-ref refs/heads/second third &&
>  
>  	test_must_fail git rebase --continue 2>err &&
>  	grep "update_ref failed for ref '\''refs/heads/second'\''" err &&
>
> As the comment notes if you try that with "git branch" you'll get an
> error, even with --force, but update-ref works just fine...

Ah, I had exactly the same thought but was stopped from sending the
suggestion to use "git update-ref" right away by "because of the
lock in the update-refs file" that did not immediately click.  What
is happening is "git branch" and any Porcelain commands that call
the now slightly misnamed branch_checked_out() function to see if an
operation is allowed to touch a branch would refrain from touching
this ref, but "update-ref" as a plumbing is deliberately designed to
be usable to bypass the check [*].  The --update-refs series added
code to branch_checked_out() to consider any refs being rewritten
as "checked out hence no touch".

Even though being listed in the update-refs file may count as a
moral equivalent to having a "lock in the update-refs file", because
write_update_refs_state() mentions no "lock", the proposed log
message was confusing and I think that was why it did not
"immediately click" at least for me.

If this works with the update-ref plumbing, we should do so instead
of adding REFFILES prerequisite.


[Footnote]

 * Allowing more flexibility to the plumbing is OK, but those
   scripting around plumbing commands should get the same support as
   those writing built-in and making internal calls to functions
   like branch_checked_out(), in order for them to be able to write
   their Porcelain and offer the same safety to their users.  The
   function certainly should be exposed to plumbing users, and
   possibly many other internal-only features.

   The trend in the past ten or so years has been "Porcelain first"
   and then "Porcelain only", which sadly made the composability of
   Git plumbing commands much less useful than pre-made Porcelain
   commands.

[PATCH 3/6] rebase -i: emphasize that 'update-ref' expects a fully-qualified ref

[SZEDER Gábor]

The argument of the 'update-ref' instruction is treated as a fully
qualified ref, and there is no DWIMery to interpret 'update-ref
feature' to create/update the branch named "feature", i.e. the ref
"refs/heads/feature".  Update the help text appended to the todo list
to make this clearer.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 rebase-interactive.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/rebase-interactive.c b/rebase-interactive.c
index 7407c59319..7c6288f997 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -57,9 +57,9 @@ void append_todo_help(int command_count,
 "        create a merge commit using the original merge commit's\n"
 "        message (or the oneline, if no original merge commit was\n"
 "        specified); use -c <commit> to reword the commit message\n"
-"u, update-ref <ref> = track a placeholder for the <ref> to be updated\n"
-"                      to this position in the new commits. The <ref> is\n"
-"                      updated at the end of the rebase\n"
+"u, update-ref <ref> = track a placeholder for the fully-qualified <ref> to\n"
+"                      be updated to this position in the new commits. The\n"
+"                      <ref> is updated at the end of the rebase\n"
 "\n"
 "These lines can be re-ordered; they are executed from top to bottom.\n");
 	unsigned edit_todo = !(shortrevisions && shortonto);
-- 
2.38.0.rc2.542.g9b62912f7f

[PATCH 4/6] sequencer: avoid empty lines after 'update-ref' instructions

[SZEDER Gábor]

When the sequencer generates a todo list for 'git rebase
--update-refs', it always inserts an empty line after each
'update-ref' instruction and after each comment line about checked out
refs.  These empty lines are unnecessary, distracting, and waste
valuable vertical screen real estate, especially when multiple refs
point to the same commit:

  pick 29a79f8 two
  pick 74bf293 three
  # Ref refs/heads/branch3 checked out at '/tmp/test/WT'

  update-ref refs/heads/branch2

  update-ref refs/heads/branch1

  pick 5f59b82 four

Eliminate those empty lines.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 sequencer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index d26ede83c4..fba92c90b1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5945,12 +5945,12 @@ static int add_decorations_to_list(const struct commit *commit,
 		/* If the branch is checked out, then leave a comment instead. */
 		if ((path = branch_checked_out(decoration->name))) {
 			item->command = TODO_COMMENT;
-			strbuf_addf(ctx->buf, "# Ref %s checked out at '%s'\n",
+			strbuf_addf(ctx->buf, "# Ref %s checked out at '%s'",
 				    decoration->name, path);
 		} else {
 			struct string_list_item *sti;
 			item->command = TODO_UPDATE_REF;
-			strbuf_addf(ctx->buf, "%s\n", decoration->name);
+			strbuf_addstr(ctx->buf, decoration->name);
 
 			sti = string_list_insert(&ctx->refs_to_oids,
 						 decoration->name);
-- 
2.38.0.rc2.542.g9b62912f7f

Re: [PATCH 4/6] sequencer: avoid empty lines after 'update-ref' instructions

[Derrick Stolee]

On 9/30/2022 10:09 AM, SZEDER Gábor wrote:
> When the sequencer generates a todo list for 'git rebase
> --update-refs', it always inserts an empty line after each
> 'update-ref' instruction and after each comment line about checked out
> refs.  These empty lines are unnecessary, distracting, and waste
> valuable vertical screen real estate, especially when multiple refs
> point to the same commit:
> 
>   pick 29a79f8 two
>   pick 74bf293 three
>   # Ref refs/heads/branch3 checked out at '/tmp/test/WT'
> 
>   update-ref refs/heads/branch2
> 
>   update-ref refs/heads/branch1
> 
>   pick 5f59b82 four
> 
> Eliminate those empty lines.

After this change, I think the end result is this:

  pick 29a79f8 two
  pick 74bf293 three
  # Ref refs/heads/branch3 checked out at '/tmp/test/WT'
  update-ref refs/heads/branch2
  update-ref refs/heads/branch1
  pick 5f59b82 four

Specifically, there is no whitespace break between the last
'update-ref' command and the next 'pick' command. Since the
'update-ref' commands likely correspond to chunks of changes,
it would be nice to still have that whitespace remain. I
agree that the whitespace between the comments and previous
'update-ref' commands is wasteful, but I'd rather leave the
wasteful (and usually rare) whitespace than lose the helpful
whitespace.

There is precedent for this kind of whitespace when using
'--rebase-merges'.

If you can find a nice way to add a line of whitespace between
these lines and the next instruction type, then I fully
support this change.

Thanks,
-Stolee

Re: [PATCH 4/6] sequencer: avoid empty lines after 'update-ref' instructions

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

>>   pick 29a79f8 two
>>   pick 74bf293 three
>>   # Ref refs/heads/branch3 checked out at '/tmp/test/WT'
>> 
>>   update-ref refs/heads/branch2
>> 
>>   update-ref refs/heads/branch1
>> 
>>   pick 5f59b82 four
>> 
>> Eliminate those empty lines.
>
> After this change, I think the end result is this:
>
>   pick 29a79f8 two
>   pick 74bf293 three
>   # Ref refs/heads/branch3 checked out at '/tmp/test/WT'
>   update-ref refs/heads/branch2
>   update-ref refs/heads/branch1
>   pick 5f59b82 four
>
> Specifically, there is no whitespace break between the last
> 'update-ref' command and the next 'pick' command. Since the
> 'update-ref' commands likely correspond to chunks of changes,
> it would be nice to still have that whitespace remain. I
> agree that the whitespace between the comments and previous
> 'update-ref' commands is wasteful, but I'd rather leave the
> wasteful (and usually rare) whitespace than lose the helpful
> whitespace.
>
> There is precedent for this kind of whitespace when using
> '--rebase-merges'.
>
> If you can find a nice way to add a line of whitespace between
> these lines and the next instruction type, then I fully
> support this change.

Sounds like it is sensible to omit this step for now from this
series.  Thanks.

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

[SZEDER Gábor]

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
+		EOF
+
+		write_script fake-editor.sh <<-\EOF &&
+		sed -n -e "/^$/q;p" "$1"
+		exit 1
+		EOF
+		test_set_editor "$(pwd)/fake-editor.sh" &&
+		test_must_fail git rebase -i --update-refs HEAD^^^ >actual &&
+
+		test_cmp expect actual
+	)
+'
+
 # This must be the last test in this file
 test_expect_success '$EDITOR and friends are unchanged' '
 	test_editor_unchanged
-- 
2.38.0.rc2.542.g9b62912f7f

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

[Ævar Arnfjörð Bjarmason]

On Fri, Sep 30 2022, 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...

> Use xstrdup_or_null() to keep a copy of resolve_ref_unsafe()'s return
> value for the duration of add_decorations_to_list().

...and this makes sense...

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

...but let's change this to a "char *" then...

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

...so we don't need this cast...?

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

[Ævar Arnfjörð Bjarmason]

On Fri, Sep 30 2022, 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.
>
> 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.

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

[Derrick Stolee]

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

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

[SZEDER Gábor]

On Fri, Sep 30, 2022 at 01:23:27PM -0400, Derrick Stolee wrote:
> > -	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."

Do we really have such a misleading pattern?

To me 'char*', as opposed to 'const char*', means that "I'll modify
the buffer where this pointer points to, and I must be on the lookout 
for when and how that happens".

[PATCH 6/6] sequencer: fail early if invalid ref is given to 'update-ref' instruction

[SZEDER Gábor]

Users can add their own 'update-ref <ref>' instructions to the rebase
todo list, which also gives them the possibility to specify an invalid
ref as argument.  Now, while git does catch any invalid ref and errors
out, it does so at the very end of the rebase process, when the
invalid ref causes the transaction updating all involved refs to fail,
leaving users on their own to figure out where each of those refs
should point now and to update them themselves.

Let's do better, and catch invalid refs early on by calling
check_refname_format() for the argument of each 'update-ref'
instruction while parsing the todo file.  This way 'git rebase' would
error out right after the user finished editing the todo file, and
would show the same generic advice to rectify the situation that is
shown e.g.  after an unknown instruction or a missing argument for a
'pick' instruction, etc.

Furthermore, require that all refs given to 'update-ref' instructions
live under the "refs/" hierarchy.  The argument of the 'update-ref'
instruction is treated as a fully qualified ref, so if the todo list
were to contain the 'update-ref foo' instruction, then 'git rebase'
would happily create the ref file '.git/foo' containing the
appropriate object id.  This is most likely not what the user wanted
and will cause confusion.  I assume it's much more probable that some
users simply forgot about the "refs/heads/" prefix than that they have
a use-case for using 'git rebase' to create/update a ref outside the
"refs/" hierarchy.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 sequencer.c                   | 19 ++++++++++++++++++-
 t/t3404-rebase-interactive.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index f1732f88f3..ababfa6352 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2522,7 +2522,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
 			     command_to_string(item->command));
 
 	if (item->command == TODO_EXEC || item->command == TODO_LABEL ||
-	    item->command == TODO_RESET || item->command == TODO_UPDATE_REF) {
+	    item->command == TODO_RESET) {
 		item->commit = NULL;
 		item->arg_offset = bol - buf;
 		item->arg_len = (int)(eol - bol);
@@ -2556,6 +2556,23 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
 		}
 	}
 
+	if (item->command == TODO_UPDATE_REF) {
+		struct strbuf ref = STRBUF_INIT;
+		int ret = 0;
+
+		item->commit = NULL;
+		item->arg_offset = bol - buf;
+		item->arg_len = (int)(eol - bol);
+
+		strbuf_add(&ref, bol, item->arg_len);
+		if (!starts_with(ref.buf, "refs/") ||
+		    check_refname_format(ref.buf, 0))
+			ret = error(_("invalid ref for update-ref instruction: %s"), ref.buf);
+
+		strbuf_release(&ref);
+		return ret;
+	}
+
 	end_of_object_name = (char *) bol + strcspn(bol, " \t\n");
 	saved = *end_of_object_name;
 	*end_of_object_name = '\0';
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 2e081b3914..b97f1e8b31 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1964,6 +1964,34 @@ test_expect_success 'respect user edits to update-ref steps' '
 	test_cmp_rev HEAD refs/heads/no-conflict-branch
 '
 
+test_expect_success 'update-refs with invalid refs' '
+	cat >fake-todo-4 <<-EOF &&
+	update-ref refs/heads/foo..bar
+	update-ref refs/heads/foo.lock
+	update-ref foo
+	update-ref foo/bar
+	pick $(git rev-parse HEAD)
+	EOF
+	cat >expect.err <<-EOF &&
+	error: invalid ref for update-ref instruction: refs/heads/foo..bar
+	error: invalid line 1: update-ref refs/heads/foo..bar
+	error: invalid ref for update-ref instruction: refs/heads/foo.lock
+	error: invalid line 2: update-ref refs/heads/foo.lock
+	error: invalid ref for update-ref instruction: foo
+	error: invalid line 3: update-ref foo
+	error: invalid ref for update-ref instruction: foo/bar
+	error: invalid line 4: update-ref foo/bar
+	You can fix this with ${SQ}git rebase --edit-todo${SQ} and then run ${SQ}git rebase --continue${SQ}.
+	Or you can abort the rebase with ${SQ}git rebase --abort${SQ}.
+	EOF
+	test_when_finished "test_might_fail git rebase --abort" &&
+	(
+		set_replace_editor fake-todo-4 &&
+		test_must_fail git rebase -i HEAD^ 2>err
+	) &&
+	test_cmp expect.err err
+'
+
 test_expect_success REFFILES '--update-refs: check failed ref update' '
 	git checkout -B update-refs-error no-conflict-branch &&
 	git branch -f base HEAD~4 &&
-- 
2.38.0.rc2.542.g9b62912f7f

Re: [PATCH 6/6] sequencer: fail early if invalid ref is given to 'update-ref' instruction

[Ævar Arnfjörð Bjarmason]

On Fri, Sep 30 2022, SZEDER Gábor wrote:

> +	if (item->command == TODO_UPDATE_REF) {
> +		struct strbuf ref = STRBUF_INIT;
> +		int ret = 0;
> +
> +		item->commit = NULL;
> +		item->arg_offset = bol - buf;
> +		item->arg_len = (int)(eol - bol);
> +
> +		strbuf_add(&ref, bol, item->arg_len);

Just a nit and maybe not worth it, but we've done this allocation dance
just because..

> +		if (!starts_with(ref.buf, "refs/") ||
> +		    check_refname_format(ref.buf, 0))

...there isn't such a thing as checkn_refname_format() taking a "size_t
len" or whatever.

So maybe not worth it, but if we do the equivalent of:
	
	static checkn_refname_format(const char *refname, size_t len, unsigned int flags)
	{
	        struct strbuf ref = STRBUF_INIT;
	        int ret;
	
		strbuf_add(&ref, refname, len);
	        ret = check_refname_format(ref,buf, flags);
	        strbuf_release(&ref);
	
		return ret;
	}

This caller could just (untested):

	if (!starts_with(bol, "refs/") ||
	    checkn_refname_format(bol, eol - bol, 0))
		return error(_("...%.*s", item->arg_len, bol));

Which saves us the copy in case the "starts_with" test is all we need.

Even without such a helper, maybe:

	int bad;

        [...]
	bad = (!starts_with(ref.buf, "refs/") ||
               check_refname_format(ref.buf, 0));
        strbuf_release(&buf);
        if (bad)
		return error(_("...%.*s", item->arg_len, bol));
	return 0;

Would make it clearer that the strbuf is just for the use of
check_refname_format().

What you have already is also fine, this just sent me down a rabbit hole
of re-learning that most of the string duplication we do for
check_refname_format() could be avoided if it was slightly less stupid,
i.e. accepted a "len" and "prefix" (i.e. "pretend your refname argument
started with 'refs/heads/'", or whatever).

> +			ret = error(_("invalid ref for update-ref instruction: %s"), ref.buf);
> +
> +		strbuf_release(&ref);
> +		return ret;
> +	}
> +
>  	end_of_object_name = (char *) bol + strcspn(bol, " \t\n");
>  	saved = *end_of_object_name;
>  	*end_of_object_name = '\0';
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 2e081b3914..b97f1e8b31 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1964,6 +1964,34 @@ test_expect_success 'respect user edits to update-ref steps' '
>  	test_cmp_rev HEAD refs/heads/no-conflict-branch
>  '
>  
> +test_expect_success 'update-refs with invalid refs' '
> +	cat >fake-todo-4 <<-EOF &&
> +	update-ref refs/heads/foo..bar
> +	update-ref refs/heads/foo.lock
> +	update-ref foo
> +	update-ref foo/bar
> +	pick $(git rev-parse HEAD)

Another potentially hidden segfault/exit code for "git"

Re: [PATCH 0/6] rebase --update-refs: smooth out some rough edges

[Derrick Stolee]

On 9/30/2022 10:09 AM, SZEDER Gábor wrote:
> A couple of cleanups and fixes to a new 'git rebase --update-refs' feature
> introduced in the current release cycle.

Thank you for examining the feature closely. I think most of these
are clearly improvements.

I'm less sold on patch 4 which removes whitespace a bit too
aggressively. I agree that the situation with multiple refs pointing
to the same commit is somewhat wasteful, but I also think it is
important to use whitespace to highlight the different groups of
commits. It provides a nice break for the reader to quickly find the
commit they are looking for within each group. I'm not sure how to
add a whitespace break only at the transition point between the
update-refs commands and other commands, but it would be nice to have.

Otherwise, everything looks good.

Junio: I don't think any of this needs to rush into v2.38.0, since
they are mostly cosmetic changes and helping users get out of bad
situations.

Thanks,
-Stolee

Re: [PATCH 0/6] rebase --update-refs: smooth out some rough edges

[SZEDER Gábor]

On Fri, Sep 30, 2022 at 01:29:58PM -0400, Derrick Stolee wrote:
> On 9/30/2022 10:09 AM, SZEDER Gábor wrote:
> > A couple of cleanups and fixes to a new 'git rebase --update-refs' feature
> > introduced in the current release cycle.

> Junio: I don't think any of this needs to rush into v2.38.0, since
> they are mostly cosmetic changes and helping users get out of bad
> situations.

I do think that we shouldn't have a release where an 'update-ref'
instruction can update anything outside the refs/ hierarchy, so we
don't have to worry about potential backwards incompatibility when we
restrict it later or add DWIMery.

Re: [PATCH 0/6] rebase --update-refs: smooth out some rough edges

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> On Fri, Sep 30, 2022 at 01:29:58PM -0400, Derrick Stolee wrote:
>> On 9/30/2022 10:09 AM, SZEDER Gábor wrote:
>> > A couple of cleanups and fixes to a new 'git rebase --update-refs' feature
>> > introduced in the current release cycle.
>
>> Junio: I don't think any of this needs to rush into v2.38.0, since
>> they are mostly cosmetic changes and helping users get out of bad
>> situations.
>
> I do think that we shouldn't have a release where an 'update-ref'
> instruction can update anything outside the refs/ hierarchy, so we
> don't have to worry about potential backwards incompatibility when we
> restrict it later or add DWIMery.
The thing is, the changes we did relative to v2.37 in order to have
the update-refs feature had some time to cook to show to those who
use pre-release versions that even with these changes, the other
parts of the system, especially "rebase", weren't negatively
affected with unintended bugs, at least in the use cases they have.
This 6-patch series right off the press that are _intended_ to touch
only the update-refs feature had none.  It simply is too late in the
cycle.

So I doubt that it is safer to ship the release with them than
without.  They could be in the zero-th batch in the next cycle,
though.

Thanks.

Re: [PATCH 0/6] rebase --update-refs: smooth out some rough edges

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

> On 9/30/2022 10:09 AM, SZEDER Gábor wrote:
>> A couple of cleanups and fixes to a new 'git rebase --update-refs' feature
>> introduced in the current release cycle.
>
> Thank you for examining the feature closely. I think most of these
> are clearly improvements.
>
> I'm less sold on patch 4 which removes whitespace a bit too
> aggressively. I agree that the situation with multiple refs pointing
> to the same commit is somewhat wasteful, but I also think it is
> important to use whitespace to highlight the different groups of
> commits. It provides a nice break for the reader to quickly find the
> commit they are looking for within each group. I'm not sure how to
> add a whitespace break only at the transition point between the
> update-refs commands and other commands, but it would be nice to have.
>
> Otherwise, everything looks good.
>
> Junio: I don't think any of this needs to rush into v2.38.0, since
> they are mostly cosmetic changes and helping users get out of bad
> situations.

I earlier agreed with the last paragraph, but after having read the
series again, I tend to agree with the paragraphs before that.

Thanks.