{"thread":{"id":"58536","subject":"[PATCH 0/6] rebase --update-refs: smooth out some rough edges","startedAt":"2022-09-30T14:10:28Z","lastAt":"2022-10-09T17:30:17Z","messageCount":20,"participants":["SZEDER Gábor","Ævar Arnfjörð Bjarmason","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"463953","messageId":"20220930140948.80367-1-szeder.dev@gmail.com","threadId":"58536","inReplyTo":null,"subject":"[PATCH 0/6] rebase --update-refs: smooth out some rough edges","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-09-30T14:09:42Z","receivedAt":"2022-09-30T14:10:28Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"A couple of cleanups and fixes to a new 'git rebase --update-refs' feature\nintroduced in the current release cycle.\n\n\nSZEDER Gábor (6):\n  t2407-worktree-heads.sh: remove outdated loop\n  t3404-rebase-interactive: mark a test with REFFILES prereq\n  rebase -i: emphasize that 'update-ref' expects a fully-qualified ref\n  sequencer: avoid empty lines after 'update-ref' instructions\n  sequencer: duplicate the result of resolve_ref_unsafe()\n  sequencer: fail early if invalid ref is given to 'update-ref'\n    instruction\n\n rebase-interactive.c          |  6 ++--\n sequencer.c                   | 28 ++++++++++++---\n t/t2407-worktree-heads.sh     |  8 ++---\n t/t3404-rebase-interactive.sh | 64 ++++++++++++++++++++++++++++++++++-\n 4 files changed, 91 insertions(+), 15 deletions(-)\n\n-- \n2.38.0.rc2.542.g9b62912f7f\n\n"},{"id":"463954","messageId":"20220930140948.80367-2-szeder.dev@gmail.com","threadId":"58536","inReplyTo":"20220930140948.80367-1-szeder.dev@gmail.com","subject":"[PATCH 1/6] t2407-worktree-heads.sh: remove outdated loop","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-09-30T14:09:43Z","receivedAt":"2022-09-30T14:10:30Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The test 'refuse to overwrite during rebase with --update-refs'\ninitially [1] used two branches 'fake-3' and 'fake-4', and to run the\nsame git operation and check on both it used a 'for i in 3 4' loop.\nThis test was then soon updated [2] to operate only on a single\nbranch.  This change made that loop unnecessary, but it forgot to\nremove it.\n\n(For future work, note the misleading error message in this test: it\nsays \"cannot force update the branch <branch> checked out at\n<worktree>\", but the branch in question is not, in fact, checked out\nin the named worktree (it's not checked out anywhere actually!); it\ncan't be updated because it's kind-of \"locked\" by an 'update-ref'\ninstruction of the ongoing rebase operation.)\n\n[1] aa7f2fd150 (branch: consider refs under 'update-refs', 2022-07-19)\n[2] 89fc0b53fd (rebase: update refs from 'update-ref' commands,\n                2022-07-19)\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/t2407-worktree-heads.sh | 8 ++------\n 1 file changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh\nindex 019a40df2c..126f0d4956 100755\n--- a/t/t2407-worktree-heads.sh\n+++ b/t/t2407-worktree-heads.sh\n@@ -87,12 +87,8 @@ test_expect_success !SANITIZE_LEAK 'refuse to overwrite: worktree in rebase with\n \tgit branch -f can-be-updated wt-3 &&\n \ttest_must_fail git -C wt-3 rebase --update-refs conflict-3 &&\n \n-\tfor i in 3 4\n-\tdo\n-\t\ttest_must_fail git branch -f can-be-updated HEAD 2>err &&\n-\t\tgrep \"cannot force update the branch '\\''can-be-updated'\\'' checked out at.*wt-3\" err ||\n-\t\t\treturn 1\n-\tdone\n+\ttest_must_fail git branch -f can-be-updated HEAD 2>err &&\n+\tgrep \"cannot force update the branch '\\''can-be-updated'\\'' checked out at.*wt-3\" err\n '\n \n test_expect_success !SANITIZE_LEAK 'refuse to fetch over ref: checked out' '\n-- \n2.38.0.rc2.542.g9b62912f7f\n\n"},{"id":"463955","messageId":"20220930140948.80367-3-szeder.dev@gmail.com","threadId":"58536","inReplyTo":"20220930140948.80367-1-szeder.dev@gmail.com","subject":"[PATCH 2/6] t3404-rebase-interactive: mark a test with REFFILES prereq","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-09-30T14:09:44Z","receivedAt":"2022-09-30T14:10:35Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The test '--update-refs: check failed ref update' added in b3b1a21d1a\n(sequencer: rewrite update-refs as user edits todo list, 2022-07-19)\ndirectly modifies the contents of a ref file, so mark this test with\nthe REFFILES prereq.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/t3404-rebase-interactive.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 688b01e3eb..7f0df58628 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1964,7 +1964,7 @@ test_expect_success 'respect user edits to update-ref steps' '\n \ttest_cmp_rev HEAD refs/heads/no-conflict-branch\n '\n \n-test_expect_success '--update-refs: check failed ref update' '\n+test_expect_success REFFILES '--update-refs: check failed ref update' '\n \tgit checkout -B update-refs-error no-conflict-branch &&\n \tgit branch -f base HEAD~4 &&\n \tgit branch -f first HEAD~3 &&\n-- \n2.38.0.rc2.542.g9b62912f7f\n\n"},{"id":"463956","messageId":"20220930140948.80367-5-szeder.dev@gmail.com","threadId":"58536","inReplyTo":"20220930140948.80367-1-szeder.dev@gmail.com","subject":"[PATCH 4/6] sequencer: avoid empty lines after 'update-ref' instructions","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-09-30T14:09:46Z","receivedAt":"2022-09-30T14:10:38Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"When the sequencer generates a todo list for 'git rebase\n--update-refs', it always inserts an empty line after each\n'update-ref' instruction and after each comment line about checked out\nrefs.  These empty lines are unnecessary, distracting, and waste\nvaluable vertical screen real estate, especially when multiple refs\npoint to the same commit:\n\n  pick 29a79f8 two\n  pick 74bf293 three\n  # Ref refs/heads/branch3 checked out at '/tmp/test/WT'\n\n  update-ref refs/heads/branch2\n\n  update-ref refs/heads/branch1\n\n  pick 5f59b82 four\n\nEliminate those empty lines.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n sequencer.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex d26ede83c4..fba92c90b1 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5945,12 +5945,12 @@ static int add_decorations_to_list(const struct commit *commit,\n \t\t/* If the branch is checked out, then leave a comment instead. */\n \t\tif ((path = branch_checked_out(decoration->name))) {\n \t\t\titem->command = TODO_COMMENT;\n-\t\t\tstrbuf_addf(ctx->buf, \"# Ref %s checked out at '%s'\\n\",\n+\t\t\tstrbuf_addf(ctx->buf, \"# Ref %s checked out at '%s'\",\n \t\t\t\t    decoration->name, path);\n \t\t} else {\n \t\t\tstruct string_list_item *sti;\n \t\t\titem->command = TODO_UPDATE_REF;\n-\t\t\tstrbuf_addf(ctx->buf, \"%s\\n\", decoration->name);\n+\t\t\tstrbuf_addstr(ctx->buf, decoration->name);\n \n \t\t\tsti = string_list_insert(&ctx->refs_to_oids,\n \t\t\t\t\t\t decoration->name);\n-- \n2.38.0.rc2.542.g9b62912f7f\n\n"},{"id":"463957","messageId":"20220930140948.80367-4-szeder.dev@gmail.com","threadId":"58536","inReplyTo":"20220930140948.80367-1-szeder.dev@gmail.com","subject":"[PATCH 3/6] rebase -i: emphasize that 'update-ref' expects a fully-qualified ref","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-09-30T14:09:45Z","receivedAt":"2022-09-30T14:10:44Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The argument of the 'update-ref' instruction is treated as a fully\nqualified ref, and there is no DWIMery to interpret 'update-ref\nfeature' to create/update the branch named \"feature\", i.e. the ref\n\"refs/heads/feature\".  Update the help text appended to the todo list\nto make this clearer.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n rebase-interactive.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/rebase-interactive.c b/rebase-interactive.c\nindex 7407c59319..7c6288f997 100644\n--- a/rebase-interactive.c\n+++ b/rebase-interactive.c\n@@ -57,9 +57,9 @@ void append_todo_help(int command_count,\n \"        create a merge commit using the original merge commit's\\n\"\n \"        message (or the oneline, if no original merge commit was\\n\"\n \"        specified); use -c <commit> to reword the commit message\\n\"\n-\"u, update-ref <ref> = track a placeholder for the <ref> to be updated\\n\"\n-\"                      to this position in the new commits. The <ref> is\\n\"\n-\"                      updated at the end of the rebase\\n\"\n+\"u, update-ref <ref> = track a placeholder for the fully-qualified <ref> to\\n\"\n+\"                      be updated to this position in the new commits. The\\n\"\n+\"                      <ref> is updated at the end of the rebase\\n\"\n \"\\n\"\n \"These lines can be re-ordered; they are executed from top to bottom.\\n\");\n \tunsigned edit_todo = !(shortrevisions && shortonto);\n-- \n2.38.0.rc2.542.g9b62912f7f\n\n"},{"id":"463958","messageId":"20220930140948.80367-6-szeder.dev@gmail.com","threadId":"58536","inReplyTo":"20220930140948.80367-1-szeder.dev@gmail.com","subject":"[PATCH 5/6] sequencer: duplicate the result of resolve_ref_unsafe()","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-09-30T14:09:47Z","receivedAt":"2022-09-30T14:10:47Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"When 'git rebase -i --update-refs' generates the todo list for the\nrebased commit range, an 'update-ref' instruction is inserted for each\nref that points to one of those commits, except for the rebased branch\n(because at the end of the rebase it will be updated anyway) and any\nrefs that are checked out in a worktree; for the latter a \"Ref <ref>\nchecked out at '<worktree>'\" comment is added.  One of these comments\ncan be missing under some circumstances: if the oldest commit with a\nref pointing to it has multiple refs pointing to it, and at least one\nof those refs is checked out in a worktree, and one of them (but not\nthe first) is checked out in the worktree associated with the last\ndirectory entry in '.git/worktrees'.\n\nThe culprit is the add_decorations_to_list() function, which calls\nresolve_ref_unsafe() to figure out the refname of the rebased branch.\nHowever, resolve_ref_unsafe() can (and in this case does) return a\npointer to a static buffer.  Alas, add_decorations_to_list() holds on\nthat static buffer until the end of the function, using its contents\nto compare refnames with the rebased branch, while it also calls a\nfunction that invokes refs_resolve_ref_unsafe() internally [1], and\nwhich overwrites the content of that static buffer, and messes up\nsubsequent refname comparisons.\n\nUse xstrdup_or_null() to keep a copy of resolve_ref_unsafe()'s return\nvalue for the duration of add_decorations_to_list().\n\n[1] #0  refs_resolve_ref_unsafe (refs=0x555555a23d00,\n        refname=0x555555928523 \"HEAD\", resolve_flags=0, oid=0x555555a23c78,\n        flags=0x7fffffffc0cc) at refs.c:1781\n    #1  0x000055555587a1d9 in add_head_info (wt=0x555555a23c50) at worktree.c:33\n    #2  0x000055555587a446 in get_linked_worktree (id=0x555555a19c43 \"WorkTree\")\n        at worktree.c:91\n    #3  0x000055555587a628 in get_worktrees () at worktree.c:135\n    #4  0x00005555556a7676 in prepare_checked_out_branches () at branch.c:385\n    #5  0x00005555556a7a76 in branch_checked_out (\n        refname=0x555555a12e9c \"refs/heads/branch2\") at branch.c:446\n    #6  0x0000555555824f55 in add_decorations_to_list (commit=0x5555559efd08,\n        ctx=0x7fffffffc3e0) at sequencer.c:5946\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n sequencer.c                   |  5 +++--\n t/t3404-rebase-interactive.sh | 34 ++++++++++++++++++++++++++++++++++\n 2 files changed, 37 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex fba92c90b1..f1732f88f3 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5917,10 +5917,10 @@ static int add_decorations_to_list(const struct commit *commit,\n \t\t\t\t   struct todo_add_branch_context *ctx)\n {\n \tconst struct name_decoration *decoration = get_name_decoration(&commit->object);\n-\tconst char *head_ref = resolve_ref_unsafe(\"HEAD\",\n+\tconst char *head_ref = xstrdup_or_null(resolve_ref_unsafe(\"HEAD\",\n \t\t\t\t\t\t  RESOLVE_REF_READING,\n \t\t\t\t\t\t  NULL,\n-\t\t\t\t\t\t  NULL);\n+\t\t\t\t\t\t  NULL));\n \n \twhile (decoration) {\n \t\tstruct todo_item *item;\n@@ -5965,6 +5965,7 @@ static int add_decorations_to_list(const struct commit *commit,\n \t\tdecoration = decoration->next;\n \t}\n \n+\tfree((char *)head_ref);\n \treturn 0;\n }\n \ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 7f0df58628..2e081b3914 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -2016,6 +2016,40 @@ test_expect_success REFFILES '--update-refs: check failed ref update' '\n \ttest_cmp expect err.trimmed\n '\n \n+test_expect_success 'what should I call this?!' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit_bulk --message=\"%s\" 4 &&\n+\t\tgit branch branch1 HEAD^ &&\n+\t\tgit branch branch2 HEAD^ &&\n+\t\tgit branch branch3 HEAD^ &&\n+\n+\t\tgit worktree add WorkTree branch2 &&\n+\t\tgit worktree list --porcelain >out &&\n+\t\twtpath=$(sed -n -e \"s%^worktree \\(.*/WorkTree\\)%\\1%p\" out) &&\n+\n+\t\tcat >expect <<-EOF &&\n+\t\tpick $(git log -1 --format=%h HEAD^^) 2\n+\t\tpick $(git log -1 --format=%h HEAD^) 3\n+\t\tupdate-ref refs/heads/branch3\n+\t\t# Ref refs/heads/branch2 checked out at $SQ$wtpath$SQ\n+\t\tupdate-ref refs/heads/branch1\n+\t\tpick $(git log -1 --format=%h HEAD) 4\n+\t\tEOF\n+\n+\t\twrite_script fake-editor.sh <<-\\EOF &&\n+\t\tsed -n -e \"/^$/q;p\" \"$1\"\n+\t\texit 1\n+\t\tEOF\n+\t\ttest_set_editor \"$(pwd)/fake-editor.sh\" &&\n+\t\ttest_must_fail git rebase -i --update-refs HEAD^^^ >actual &&\n+\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n # This must be the last test in this file\n test_expect_success '$EDITOR and friends are unchanged' '\n \ttest_editor_unchanged\n-- \n2.38.0.rc2.542.g9b62912f7f\n\n"},{"id":"463959","messageId":"20220930140948.80367-7-szeder.dev@gmail.com","threadId":"58536","inReplyTo":"20220930140948.80367-1-szeder.dev@gmail.com","subject":"[PATCH 6/6] sequencer: fail early if invalid ref is given to 'update-ref' instruction","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-09-30T14:09:48Z","receivedAt":"2022-09-30T14:10:51Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Users can add their own 'update-ref <ref>' instructions to the rebase\ntodo list, which also gives them the possibility to specify an invalid\nref as argument.  Now, while git does catch any invalid ref and errors\nout, it does so at the very end of the rebase process, when the\ninvalid ref causes the transaction updating all involved refs to fail,\nleaving users on their own to figure out where each of those refs\nshould point now and to update them themselves.\n\nLet's do better, and catch invalid refs early on by calling\ncheck_refname_format() for the argument of each 'update-ref'\ninstruction while parsing the todo file.  This way 'git rebase' would\nerror out right after the user finished editing the todo file, and\nwould show the same generic advice to rectify the situation that is\nshown e.g.  after an unknown instruction or a missing argument for a\n'pick' instruction, etc.\n\nFurthermore, require that all refs given to 'update-ref' instructions\nlive under the \"refs/\" hierarchy.  The argument of the 'update-ref'\ninstruction is treated as a fully qualified ref, so if the todo list\nwere to contain the 'update-ref foo' instruction, then 'git rebase'\nwould happily create the ref file '.git/foo' containing the\nappropriate object id.  This is most likely not what the user wanted\nand will cause confusion.  I assume it's much more probable that some\nusers simply forgot about the \"refs/heads/\" prefix than that they have\na use-case for using 'git rebase' to create/update a ref outside the\n\"refs/\" hierarchy.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n sequencer.c                   | 19 ++++++++++++++++++-\n t/t3404-rebase-interactive.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 46 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex f1732f88f3..ababfa6352 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2522,7 +2522,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n \t\t\t     command_to_string(item->command));\n \n \tif (item->command == TODO_EXEC || item->command == TODO_LABEL ||\n-\t    item->command == TODO_RESET || item->command == TODO_UPDATE_REF) {\n+\t    item->command == TODO_RESET) {\n \t\titem->commit = NULL;\n \t\titem->arg_offset = bol - buf;\n \t\titem->arg_len = (int)(eol - bol);\n@@ -2556,6 +2556,23 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n \t\t}\n \t}\n \n+\tif (item->command == TODO_UPDATE_REF) {\n+\t\tstruct strbuf ref = STRBUF_INIT;\n+\t\tint ret = 0;\n+\n+\t\titem->commit = NULL;\n+\t\titem->arg_offset = bol - buf;\n+\t\titem->arg_len = (int)(eol - bol);\n+\n+\t\tstrbuf_add(&ref, bol, item->arg_len);\n+\t\tif (!starts_with(ref.buf, \"refs/\") ||\n+\t\t    check_refname_format(ref.buf, 0))\n+\t\t\tret = error(_(\"invalid ref for update-ref instruction: %s\"), ref.buf);\n+\n+\t\tstrbuf_release(&ref);\n+\t\treturn ret;\n+\t}\n+\n \tend_of_object_name = (char *) bol + strcspn(bol, \" \\t\\n\");\n \tsaved = *end_of_object_name;\n \t*end_of_object_name = '\\0';\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 2e081b3914..b97f1e8b31 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1964,6 +1964,34 @@ test_expect_success 'respect user edits to update-ref steps' '\n \ttest_cmp_rev HEAD refs/heads/no-conflict-branch\n '\n \n+test_expect_success 'update-refs with invalid refs' '\n+\tcat >fake-todo-4 <<-EOF &&\n+\tupdate-ref refs/heads/foo..bar\n+\tupdate-ref refs/heads/foo.lock\n+\tupdate-ref foo\n+\tupdate-ref foo/bar\n+\tpick $(git rev-parse HEAD)\n+\tEOF\n+\tcat >expect.err <<-EOF &&\n+\terror: invalid ref for update-ref instruction: refs/heads/foo..bar\n+\terror: invalid line 1: update-ref refs/heads/foo..bar\n+\terror: invalid ref for update-ref instruction: refs/heads/foo.lock\n+\terror: invalid line 2: update-ref refs/heads/foo.lock\n+\terror: invalid ref for update-ref instruction: foo\n+\terror: invalid line 3: update-ref foo\n+\terror: invalid ref for update-ref instruction: foo/bar\n+\terror: invalid line 4: update-ref foo/bar\n+\tYou can fix this with ${SQ}git rebase --edit-todo${SQ} and then run ${SQ}git rebase --continue${SQ}.\n+\tOr you can abort the rebase with ${SQ}git rebase --abort${SQ}.\n+\tEOF\n+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n+\t(\n+\t\tset_replace_editor fake-todo-4 &&\n+\t\ttest_must_fail git rebase -i HEAD^ 2>err\n+\t) &&\n+\ttest_cmp expect.err err\n+'\n+\n test_expect_success REFFILES '--update-refs: check failed ref update' '\n \tgit checkout -B update-refs-error no-conflict-branch &&\n \tgit branch -f base HEAD~4 &&\n-- \n2.38.0.rc2.542.g9b62912f7f\n\n"},{"id":"463961","messageId":"220930.86mtagvmit.gmgdl@evledraar.gmail.com","threadId":"58536","inReplyTo":"20220930140948.80367-6-szeder.dev@gmail.com","subject":"Re: [PATCH 5/6] sequencer: duplicate the result of resolve_ref_unsafe()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-30T16:45:12Z","receivedAt":"2022-09-30T16:45:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Sep 30 2022, SZEDER Gábor wrote:\n\n> When 'git rebase -i --update-refs' generates the todo list for the\n> rebased commit range, an 'update-ref' instruction is inserted for each\n> ref that points to one of those commits, except for the rebased branch\n> (because at the end of the rebase it will be updated anyway) and any\n> refs that are checked out in a worktree; for the latter a \"Ref <ref>\n> checked out at '<worktree>'\" comment is added.  One of these comments\n> can be missing under some circumstances: if the oldest commit with a\n> ref pointing to it has multiple refs pointing to it, and at least one\n> of those refs is checked out in a worktree, and one of them (but not\n> the first) is checked out in the worktree associated with the last\n> directory entry in '.git/worktrees'.\n>\n> The culprit is the add_decorations_to_list() function, which calls\n> resolve_ref_unsafe() to figure out the refname of the rebased branch.\n> However, resolve_ref_unsafe() can (and in this case does) return a\n> pointer to a static buffer.  Alas, add_decorations_to_list() holds on\n> that static buffer until the end of the function, using its contents\n> to compare refnames with the rebased branch, while it also calls a\n> function that invokes refs_resolve_ref_unsafe() internally [1], and\n> which overwrites the content of that static buffer, and messes up\n> subsequent refname comparisons.\n\nGood catch...\n\n> Use xstrdup_or_null() to keep a copy of resolve_ref_unsafe()'s return\n> value for the duration of add_decorations_to_list().\n\n...and this makes sense...\n\n>  \tconst struct name_decoration *decoration = get_name_decoration(&commit->object);\n> -\tconst char *head_ref = resolve_ref_unsafe(\"HEAD\",\n> +\tconst char *head_ref = xstrdup_or_null(resolve_ref_unsafe(\"HEAD\",\n\n...but let's change this to a \"char *\" then...\n\n>  \t\t\t\t\t\t  RESOLVE_REF_READING,\n>  \t\t\t\t\t\t  NULL,\n> -\t\t\t\t\t\t  NULL);\n> +\t\t\t\t\t\t  NULL));\n>  \n>  \twhile (decoration) {\n>  \t\tstruct todo_item *item;\n> @@ -5965,6 +5965,7 @@ static int add_decorations_to_list(const struct commit *commit,\n>  \t\tdecoration = decoration->next;\n>  \t}\n>  \n> +\tfree((char *)head_ref);\n\n...so we don't need this cast...?\n"},{"id":"463962","messageId":"220930.86ill4vman.gmgdl@evledraar.gmail.com","threadId":"58536","inReplyTo":"20220930140948.80367-3-szeder.dev@gmail.com","subject":"Re: [PATCH 2/6] t3404-rebase-interactive: mark a test with REFFILES prereq","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-30T16:46:49Z","receivedAt":"2022-09-30T16:50:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Sep 30 2022, SZEDER Gábor wrote:\n\n> The test '--update-refs: check failed ref update' added in b3b1a21d1a\n> (sequencer: rewrite update-refs as user edits todo list, 2022-07-19)\n> directly modifies the contents of a ref file, so mark this test with\n> the REFFILES prereq.\n>\n> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n> ---\n>  t/t3404-rebase-interactive.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 688b01e3eb..7f0df58628 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -1964,7 +1964,7 @@ test_expect_success 'respect user edits to update-ref steps' '\n>  \ttest_cmp_rev HEAD refs/heads/no-conflict-branch\n>  '\n>  \n> -test_expect_success '--update-refs: check failed ref update' '\n> +test_expect_success REFFILES '--update-refs: check failed ref update' '\n>  \tgit checkout -B update-refs-error no-conflict-branch &&\n>  \tgit branch -f base HEAD~4 &&\n>  \tgit branch -f first HEAD~3 &&\n\nWe had various tests that depended on .git/refs/* for a good reason, and\nsome that didn't.\n\nI may be missing something, but in this case this seems to be a \"no good\nreason\" case. I.e. the fix seems to just be:\n\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 688b01e3eb6..f1c021a7f7b 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1994,8 +1994,9 @@ test_expect_success '--update-refs: check failed ref update' '\n \t# At this point, the values of first, second, and third are\n \t# recorded in the update-refs file. We will force-update the\n \t# \"second\" ref, but \"git branch -f\" will not work because of\n-\t# the lock in the update-refs file.\n-\tgit rev-parse third >.git/refs/heads/second &&\n+\t# the lock in the update-refs file, so we need to use\n+\t# \"update-ref\".\n+\tgit update-ref refs/heads/second third &&\n \n \ttest_must_fail git rebase --continue 2>err &&\n \tgrep \"update_ref failed for ref '\\''refs/heads/second'\\''\" err &&\n\nAs the comment notes if you try that with \"git branch\" you'll get an\nerror, even with --force, but update-ref works just fine...\n"},{"id":"463963","messageId":"220930.86edvsvm7q.gmgdl@evledraar.gmail.com","threadId":"58536","inReplyTo":"20220930140948.80367-6-szeder.dev@gmail.com","subject":"Re: [PATCH 5/6] sequencer: duplicate the result of resolve_ref_unsafe()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-30T16:51:04Z","receivedAt":"2022-09-30T16:52:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Sep 30 2022, SZEDER Gábor wrote:\n\n> When 'git rebase -i --update-refs' generates the todo list for the\n> rebased commit range, an 'update-ref' instruction is inserted for each\n> ref that points to one of those commits, except for the rebased branch\n> (because at the end of the rebase it will be updated anyway) and any\n> refs that are checked out in a worktree; for the latter a \"Ref <ref>\n> checked out at '<worktree>'\" comment is added.  One of these comments\n> can be missing under some circumstances: if the oldest commit with a\n> ref pointing to it has multiple refs pointing to it, and at least one\n> of those refs is checked out in a worktree, and one of them (but not\n> the first) is checked out in the worktree associated with the last\n> directory entry in '.git/worktrees'.\n>\n> The culprit is the add_decorations_to_list() function, which calls\n> resolve_ref_unsafe() to figure out the refname of the rebased branch.\n> However, resolve_ref_unsafe() can (and in this case does) return a\n> pointer to a static buffer.  Alas, add_decorations_to_list() holds on\n> that static buffer until the end of the function, using its contents\n> to compare refnames with the rebased branch, while it also calls a\n> function that invokes refs_resolve_ref_unsafe() internally [1], and\n> which overwrites the content of that static buffer, and messes up\n> subsequent refname comparisons.\n>\n> Use xstrdup_or_null() to keep a copy of resolve_ref_unsafe()'s return\n> value for the duration of add_decorations_to_list().\n>\n> [1] #0  refs_resolve_ref_unsafe (refs=0x555555a23d00,\n>         refname=0x555555928523 \"HEAD\", resolve_flags=0, oid=0x555555a23c78,\n>         flags=0x7fffffffc0cc) at refs.c:1781\n>     #1  0x000055555587a1d9 in add_head_info (wt=0x555555a23c50) at worktree.c:33\n>     #2  0x000055555587a446 in get_linked_worktree (id=0x555555a19c43 \"WorkTree\")\n>         at worktree.c:91\n>     #3  0x000055555587a628 in get_worktrees () at worktree.c:135\n>     #4  0x00005555556a7676 in prepare_checked_out_branches () at branch.c:385\n>     #5  0x00005555556a7a76 in branch_checked_out (\n>         refname=0x555555a12e9c \"refs/heads/branch2\") at branch.c:446\n>     #6  0x0000555555824f55 in add_decorations_to_list (commit=0x5555559efd08,\n>         ctx=0x7fffffffc3e0) at sequencer.c:5946\n>\n> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n> ---\n>  sequencer.c                   |  5 +++--\n>  t/t3404-rebase-interactive.sh | 34 ++++++++++++++++++++++++++++++++++\n>  2 files changed, 37 insertions(+), 2 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index fba92c90b1..f1732f88f3 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -5917,10 +5917,10 @@ static int add_decorations_to_list(const struct commit *commit,\n>  \t\t\t\t   struct todo_add_branch_context *ctx)\n>  {\n>  \tconst struct name_decoration *decoration = get_name_decoration(&commit->object);\n> -\tconst char *head_ref = resolve_ref_unsafe(\"HEAD\",\n> +\tconst char *head_ref = xstrdup_or_null(resolve_ref_unsafe(\"HEAD\",\n>  \t\t\t\t\t\t  RESOLVE_REF_READING,\n>  \t\t\t\t\t\t  NULL,\n> -\t\t\t\t\t\t  NULL);\n> +\t\t\t\t\t\t  NULL));\n>  \n>  \twhile (decoration) {\n>  \t\tstruct todo_item *item;\n> @@ -5965,6 +5965,7 @@ static int add_decorations_to_list(const struct commit *commit,\n>  \t\tdecoration = decoration->next;\n>  \t}\n>  \n> +\tfree((char *)head_ref);\n>  \treturn 0;\n>  }\n>  \n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 7f0df58628..2e081b3914 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -2016,6 +2016,40 @@ test_expect_success REFFILES '--update-refs: check failed ref update' '\n>  \ttest_cmp expect err.trimmed\n>  '\n>  \n> +test_expect_success 'what should I call this?!' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\ttest_commit_bulk --message=\"%s\" 4 &&\n> +\t\tgit branch branch1 HEAD^ &&\n> +\t\tgit branch branch2 HEAD^ &&\n> +\t\tgit branch branch3 HEAD^ &&\n> +\n> +\t\tgit worktree add WorkTree branch2 &&\n> +\t\tgit worktree list --porcelain >out &&\n> +\t\twtpath=$(sed -n -e \"s%^worktree \\(.*/WorkTree\\)%\\1%p\" out) &&\n> +\n> +\t\tcat >expect <<-EOF &&\n> +\t\tpick $(git log -1 --format=%h HEAD^^) 2\n> +\t\tpick $(git log -1 --format=%h HEAD^) 3\n> +\t\tupdate-ref refs/heads/branch3\n> +\t\t# Ref refs/heads/branch2 checked out at $SQ$wtpath$SQ\n> +\t\tupdate-ref refs/heads/branch1\n> +\t\tpick $(git log -1 --format=%h HEAD) 4\n\nWe can let this pass, but FWIW these are all cases where we'll lose \"git\nlog\"'s exit code. So:\n\n\tfirst=$(git log ...) &&\n        [...]\n        cat [...]\n        pick $first\n        [...]\n\nIf we want to catch that.\n"},{"id":"463966","messageId":"fc887b02-191e-912a-bd15-1dc882ddf9dc@github.com","threadId":"58536","inReplyTo":"20220930140948.80367-5-szeder.dev@gmail.com","subject":"Re: [PATCH 4/6] sequencer: avoid empty lines after 'update-ref' instructions","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-30T17:18:30Z","receivedAt":"2022-09-30T17:18:37Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/30/2022 10:09 AM, SZEDER Gábor wrote:\n> When the sequencer generates a todo list for 'git rebase\n> --update-refs', it always inserts an empty line after each\n> 'update-ref' instruction and after each comment line about checked out\n> refs.  These empty lines are unnecessary, distracting, and waste\n> valuable vertical screen real estate, especially when multiple refs\n> point to the same commit:\n> \n>   pick 29a79f8 two\n>   pick 74bf293 three\n>   # Ref refs/heads/branch3 checked out at '/tmp/test/WT'\n> \n>   update-ref refs/heads/branch2\n> \n>   update-ref refs/heads/branch1\n> \n>   pick 5f59b82 four\n> \n> Eliminate those empty lines.\n\nAfter this change, I think the end result is this:\n\n  pick 29a79f8 two\n  pick 74bf293 three\n  # Ref refs/heads/branch3 checked out at '/tmp/test/WT'\n  update-ref refs/heads/branch2\n  update-ref refs/heads/branch1\n  pick 5f59b82 four\n\nSpecifically, there is no whitespace break between the last\n'update-ref' command and the next 'pick' command. Since the\n'update-ref' commands likely correspond to chunks of changes,\nit would be nice to still have that whitespace remain. I\nagree that the whitespace between the comments and previous\n'update-ref' commands is wasteful, but I'd rather leave the\nwasteful (and usually rare) whitespace than lose the helpful\nwhitespace.\n\nThere is precedent for this kind of whitespace when using\n'--rebase-merges'.\n\nIf you can find a nice way to add a line of whitespace between\nthese lines and the next instruction type, then I fully\nsupport this change.\n\nThanks,\n-Stolee\n"},{"id":"463967","messageId":"220930.86a66gvkse.gmgdl@evledraar.gmail.com","threadId":"58536","inReplyTo":"20220930140948.80367-7-szeder.dev@gmail.com","subject":"Re: [PATCH 6/6] sequencer: fail early if invalid ref is given to 'update-ref' instruction","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-30T17:09:04Z","receivedAt":"2022-09-30T17:23:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Sep 30 2022, SZEDER Gábor wrote:\n\n> +\tif (item->command == TODO_UPDATE_REF) {\n> +\t\tstruct strbuf ref = STRBUF_INIT;\n> +\t\tint ret = 0;\n> +\n> +\t\titem->commit = NULL;\n> +\t\titem->arg_offset = bol - buf;\n> +\t\titem->arg_len = (int)(eol - bol);\n> +\n> +\t\tstrbuf_add(&ref, bol, item->arg_len);\n\nJust a nit and maybe not worth it, but we've done this allocation dance\njust because..\n\n> +\t\tif (!starts_with(ref.buf, \"refs/\") ||\n> +\t\t    check_refname_format(ref.buf, 0))\n\n...there isn't such a thing as checkn_refname_format() taking a \"size_t\nlen\" or whatever.\n\nSo maybe not worth it, but if we do the equivalent of:\n\t\n\tstatic checkn_refname_format(const char *refname, size_t len, unsigned int flags)\n\t{\n\t        struct strbuf ref = STRBUF_INIT;\n\t        int ret;\n\t\n\t\tstrbuf_add(&ref, refname, len);\n\t        ret = check_refname_format(ref,buf, flags);\n\t        strbuf_release(&ref);\n\t\n\t\treturn ret;\n\t}\n\nThis caller could just (untested):\n\n\tif (!starts_with(bol, \"refs/\") ||\n\t    checkn_refname_format(bol, eol - bol, 0))\n\t\treturn error(_(\"...%.*s\", item->arg_len, bol));\n\nWhich saves us the copy in case the \"starts_with\" test is all we need.\n\nEven without such a helper, maybe:\n\n\tint bad;\n\n        [...]\n\tbad = (!starts_with(ref.buf, \"refs/\") ||\n               check_refname_format(ref.buf, 0));\n        strbuf_release(&buf);\n        if (bad)\n\t\treturn error(_(\"...%.*s\", item->arg_len, bol));\n\treturn 0;\n\nWould make it clearer that the strbuf is just for the use of\ncheck_refname_format().\n\nWhat you have already is also fine, this just sent me down a rabbit hole\nof re-learning that most of the string duplication we do for\ncheck_refname_format() could be avoided if it was slightly less stupid,\ni.e. accepted a \"len\" and \"prefix\" (i.e. \"pretend your refname argument\nstarted with 'refs/heads/'\", or whatever).\n\n> +\t\t\tret = error(_(\"invalid ref for update-ref instruction: %s\"), ref.buf);\n> +\n> +\t\tstrbuf_release(&ref);\n> +\t\treturn ret;\n> +\t}\n> +\n>  \tend_of_object_name = (char *) bol + strcspn(bol, \" \\t\\n\");\n>  \tsaved = *end_of_object_name;\n>  \t*end_of_object_name = '\\0';\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 2e081b3914..b97f1e8b31 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -1964,6 +1964,34 @@ test_expect_success 'respect user edits to update-ref steps' '\n>  \ttest_cmp_rev HEAD refs/heads/no-conflict-branch\n>  '\n>  \n> +test_expect_success 'update-refs with invalid refs' '\n> +\tcat >fake-todo-4 <<-EOF &&\n> +\tupdate-ref refs/heads/foo..bar\n> +\tupdate-ref refs/heads/foo.lock\n> +\tupdate-ref foo\n> +\tupdate-ref foo/bar\n> +\tpick $(git rev-parse HEAD)\n\nAnother potentially hidden segfault/exit code for \"git\"\n"},{"id":"463968","messageId":"1711157b-8f04-008b-1672-f1fbac3c3e81@github.com","threadId":"58536","inReplyTo":"20220930140948.80367-6-szeder.dev@gmail.com","subject":"Re: [PATCH 5/6] sequencer: duplicate the result of resolve_ref_unsafe()","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-30T17:23:27Z","receivedAt":"2022-09-30T17:23:35Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/30/2022 10:09 AM, SZEDER Gábor wrote:\n> When 'git rebase -i --update-refs' generates the todo list for the\n> rebased commit range, an 'update-ref' instruction is inserted for each\n> ref that points to one of those commits, except for the rebased branch\n> (because at the end of the rebase it will be updated anyway) and any\n> refs that are checked out in a worktree; for the latter a \"Ref <ref>\n> checked out at '<worktree>'\" comment is added.  One of these comments\n> can be missing under some circumstances: if the oldest commit with a\n> ref pointing to it has multiple refs pointing to it, and at least one\n> of those refs is checked out in a worktree, and one of them (but not\n> the first) is checked out in the worktree associated with the last\n> directory entry in '.git/worktrees'.\n> \n> The culprit is the add_decorations_to_list() function, which calls\n> resolve_ref_unsafe() to figure out the refname of the rebased branch.\n> However, resolve_ref_unsafe() can (and in this case does) return a\n> pointer to a static buffer.  Alas, add_decorations_to_list() holds on\n> that static buffer until the end of the function, using its contents\n> to compare refnames with the rebased branch, while it also calls a\n> function that invokes refs_resolve_ref_unsafe() internally [1], and\n> which overwrites the content of that static buffer, and messes up\n> subsequent refname comparisons.\n\nGood catch!\n\n> -\tconst char *head_ref = resolve_ref_unsafe(\"HEAD\",\n> +\tconst char *head_ref = xstrdup_or_null(resolve_ref_unsafe(\"HEAD\",\n>  \t\t\t\t\t\t  RESOLVE_REF_READING,\n>  \t\t\t\t\t\t  NULL,\n> -\t\t\t\t\t\t  NULL);\n> +\t\t\t\t\t\t  NULL));\n\nMoving to a 'char *' matches our typical pattern of \"I am responsible\nfor freeing this or passing that responsibility to someone else.\"\n\n> +test_expect_success 'what should I call this?!' '\n\nPerhaps 'with multiple refs, correctly report worktree' ?\n\nThanks,\n-Stolee\n"},{"id":"463969","messageId":"05c5076b-eb78-5381-4eba-863a84190755@github.com","threadId":"58536","inReplyTo":"20220930140948.80367-1-szeder.dev@gmail.com","subject":"Re: [PATCH 0/6] rebase --update-refs: smooth out some rough edges","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-30T17:29:58Z","receivedAt":"2022-09-30T17:30:42Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/30/2022 10:09 AM, SZEDER Gábor wrote:\n> A couple of cleanups and fixes to a new 'git rebase --update-refs' feature\n> introduced in the current release cycle.\n\nThank you for examining the feature closely. I think most of these\nare clearly improvements.\n\nI'm less sold on patch 4 which removes whitespace a bit too\naggressively. I agree that the situation with multiple refs pointing\nto the same commit is somewhat wasteful, but I also think it is\nimportant to use whitespace to highlight the different groups of\ncommits. It provides a nice break for the reader to quickly find the\ncommit they are looking for within each group. I'm not sure how to\nadd a whitespace break only at the transition point between the\nupdate-refs commands and other commands, but it would be nice to have.\n\nOtherwise, everything looks good.\n\nJunio: I don't think any of this needs to rush into v2.38.0, since\nthey are mostly cosmetic changes and helping users get out of bad\nsituations.\n\nThanks,\n-Stolee\n"},{"id":"464041","messageId":"20221001163105.GA7659@szeder.dev","threadId":"58536","inReplyTo":"05c5076b-eb78-5381-4eba-863a84190755@github.com","subject":"Re: [PATCH 0/6] rebase --update-refs: smooth out some rough edges","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-10-01T16:31:05Z","receivedAt":"2022-10-01T16:31:18Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Sep 30, 2022 at 01:29:58PM -0400, Derrick Stolee wrote:\n> On 9/30/2022 10:09 AM, SZEDER Gábor wrote:\n> > A couple of cleanups and fixes to a new 'git rebase --update-refs' feature\n> > introduced in the current release cycle.\n\n> Junio: I don't think any of this needs to rush into v2.38.0, since\n> they are mostly cosmetic changes and helping users get out of bad\n> situations.\n\nI do think that we shouldn't have a release where an 'update-ref'\ninstruction can update anything outside the refs/ hierarchy, so we\ndon't have to worry about potential backwards incompatibility when we\nrestrict it later or add DWIMery.\n\n\n"},{"id":"464042","messageId":"xmqqsfk75vtv.fsf@gitster.g","threadId":"58536","inReplyTo":"20221001163105.GA7659@szeder.dev","subject":"Re: [PATCH 0/6] rebase --update-refs: smooth out some rough edges","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-01T16:53:48Z","receivedAt":"2022-10-01T16:54:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n> On Fri, Sep 30, 2022 at 01:29:58PM -0400, Derrick Stolee wrote:\n>> On 9/30/2022 10:09 AM, SZEDER Gábor wrote:\n>> > A couple of cleanups and fixes to a new 'git rebase --update-refs' feature\n>> > introduced in the current release cycle.\n>\n>> Junio: I don't think any of this needs to rush into v2.38.0, since\n>> they are mostly cosmetic changes and helping users get out of bad\n>> situations.\n>\n> I do think that we shouldn't have a release where an 'update-ref'\n> instruction can update anything outside the refs/ hierarchy, so we\n> don't have to worry about potential backwards incompatibility when we\n> restrict it later or add DWIMery.\nThe thing is, the changes we did relative to v2.37 in order to have\nthe update-refs feature had some time to cook to show to those who\nuse pre-release versions that even with these changes, the other\nparts of the system, especially \"rebase\", weren't negatively\naffected with unintended bugs, at least in the use cases they have.\nThis 6-patch series right off the press that are _intended_ to touch\nonly the update-refs feature had none.  It simply is too late in the\ncycle.\n\nSo I doubt that it is safer to ship the release with them than\nwithout.  They could be in the zero-th batch in the next cycle,\nthough.\n\nThanks.\n"},{"id":"464244","messageId":"xmqqwn9eqkw8.fsf@gitster.g","threadId":"58536","inReplyTo":"220930.86ill4vman.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 2/6] t3404-rebase-interactive: mark a test with REFFILES prereq","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-05T16:45:43Z","receivedAt":"2022-10-05T16:45:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> On Fri, Sep 30 2022, SZEDER Gábor wrote:\n>\n>> The test '--update-refs: check failed ref update' added in b3b1a21d1a\n>> (sequencer: rewrite update-refs as user edits todo list, 2022-07-19)\n>> directly modifies the contents of a ref file, so mark this test with\n>> the REFFILES prereq.\n>> ...\n>  \t# At this point, the values of first, second, and third are\n>  \t# recorded in the update-refs file. We will force-update the\n>  \t# \"second\" ref, but \"git branch -f\" will not work because of\n> -\t# the lock in the update-refs file.\n> -\tgit rev-parse third >.git/refs/heads/second &&\n> +\t# the lock in the update-refs file, so we need to use\n> +\t# \"update-ref\".\n> +\tgit update-ref refs/heads/second third &&\n>  \n>  \ttest_must_fail git rebase --continue 2>err &&\n>  \tgrep \"update_ref failed for ref '\\''refs/heads/second'\\''\" err &&\n>\n> As the comment notes if you try that with \"git branch\" you'll get an\n> error, even with --force, but update-ref works just fine...\n\nAh, I had exactly the same thought but was stopped from sending the\nsuggestion to use \"git update-ref\" right away by \"because of the\nlock in the update-refs file\" that did not immediately click.  What\nis happening is \"git branch\" and any Porcelain commands that call\nthe now slightly misnamed branch_checked_out() function to see if an\noperation is allowed to touch a branch would refrain from touching\nthis ref, but \"update-ref\" as a plumbing is deliberately designed to\nbe usable to bypass the check [*].  The --update-refs series added\ncode to branch_checked_out() to consider any refs being rewritten\nas \"checked out hence no touch\".\n\nEven though being listed in the update-refs file may count as a\nmoral equivalent to having a \"lock in the update-refs file\", because\nwrite_update_refs_state() mentions no \"lock\", the proposed log\nmessage was confusing and I think that was why it did not\n\"immediately click\" at least for me.\n\nIf this works with the update-ref plumbing, we should do so instead\nof adding REFFILES prerequisite.\n\n\n[Footnote]\n\n * Allowing more flexibility to the plumbing is OK, but those\n   scripting around plumbing commands should get the same support as\n   those writing built-in and making internal calls to functions\n   like branch_checked_out(), in order for them to be able to write\n   their Porcelain and offer the same safety to their users.  The\n   function certainly should be exposed to plumbing users, and\n   possibly many other internal-only features.\n\n   The trend in the past ten or so years has been \"Porcelain first\"\n   and then \"Porcelain only\", which sadly made the composability of\n   Git plumbing commands much less useful than pre-made Porcelain\n   commands.\n"},{"id":"464245","messageId":"xmqqsfk2qkqb.fsf@gitster.g","threadId":"58536","inReplyTo":"fc887b02-191e-912a-bd15-1dc882ddf9dc@github.com","subject":"Re: [PATCH 4/6] sequencer: avoid empty lines after 'update-ref' instructions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-05T16:49:16Z","receivedAt":"2022-10-05T16:49:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <derrickstolee@github.com> writes:\n\n>>   pick 29a79f8 two\n>>   pick 74bf293 three\n>>   # Ref refs/heads/branch3 checked out at '/tmp/test/WT'\n>> \n>>   update-ref refs/heads/branch2\n>> \n>>   update-ref refs/heads/branch1\n>> \n>>   pick 5f59b82 four\n>> \n>> Eliminate those empty lines.\n>\n> After this change, I think the end result is this:\n>\n>   pick 29a79f8 two\n>   pick 74bf293 three\n>   # Ref refs/heads/branch3 checked out at '/tmp/test/WT'\n>   update-ref refs/heads/branch2\n>   update-ref refs/heads/branch1\n>   pick 5f59b82 four\n>\n> Specifically, there is no whitespace break between the last\n> 'update-ref' command and the next 'pick' command. Since the\n> 'update-ref' commands likely correspond to chunks of changes,\n> it would be nice to still have that whitespace remain. I\n> agree that the whitespace between the comments and previous\n> 'update-ref' commands is wasteful, but I'd rather leave the\n> wasteful (and usually rare) whitespace than lose the helpful\n> whitespace.\n>\n> There is precedent for this kind of whitespace when using\n> '--rebase-merges'.\n>\n> If you can find a nice way to add a line of whitespace between\n> these lines and the next instruction type, then I fully\n> support this change.\n\nSounds like it is sensible to omit this step for now from this\nseries.  Thanks.\n"},{"id":"464246","messageId":"xmqqo7uqqjjn.fsf@gitster.g","threadId":"58536","inReplyTo":"05c5076b-eb78-5381-4eba-863a84190755@github.com","subject":"Re: [PATCH 0/6] rebase --update-refs: smooth out some rough edges","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-05T17:14:52Z","receivedAt":"2022-10-05T17:15:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <derrickstolee@github.com> writes:\n\n> On 9/30/2022 10:09 AM, SZEDER Gábor wrote:\n>> A couple of cleanups and fixes to a new 'git rebase --update-refs' feature\n>> introduced in the current release cycle.\n>\n> Thank you for examining the feature closely. I think most of these\n> are clearly improvements.\n>\n> I'm less sold on patch 4 which removes whitespace a bit too\n> aggressively. I agree that the situation with multiple refs pointing\n> to the same commit is somewhat wasteful, but I also think it is\n> important to use whitespace to highlight the different groups of\n> commits. It provides a nice break for the reader to quickly find the\n> commit they are looking for within each group. I'm not sure how to\n> add a whitespace break only at the transition point between the\n> update-refs commands and other commands, but it would be nice to have.\n>\n> Otherwise, everything looks good.\n>\n> Junio: I don't think any of this needs to rush into v2.38.0, since\n> they are mostly cosmetic changes and helping users get out of bad\n> situations.\n\nI earlier agreed with the last paragraph, but after having read the\nseries again, I tend to agree with the paragraphs before that.\n\nThanks.\n"},{"id":"464457","messageId":"20221009172306.GA2052@szeder.dev","threadId":"58536","inReplyTo":"1711157b-8f04-008b-1672-f1fbac3c3e81@github.com","subject":"Re: [PATCH 5/6] sequencer: duplicate the result of resolve_ref_unsafe()","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-10-09T17:23:06Z","receivedAt":"2022-10-09T17:30:17Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Sep 30, 2022 at 01:23:27PM -0400, Derrick Stolee wrote:\n> > -\tconst char *head_ref = resolve_ref_unsafe(\"HEAD\",\n> > +\tconst char *head_ref = xstrdup_or_null(resolve_ref_unsafe(\"HEAD\",\n> >  \t\t\t\t\t\t  RESOLVE_REF_READING,\n> >  \t\t\t\t\t\t  NULL,\n> > -\t\t\t\t\t\t  NULL);\n> > +\t\t\t\t\t\t  NULL));\n> \n> Moving to a 'char *' matches our typical pattern of \"I am responsible\n> for freeing this or passing that responsibility to someone else.\"\n\nDo we really have such a misleading pattern?\n\nTo me 'char*', as opposed to 'const char*', means that \"I'll modify\nthe buffer where this pointer points to, and I must be on the lookout \nfor when and how that happens\".\n\n"}]}