{"thread":{"id":"58784","subject":"[PATCH 0/2] rebase: stricter label lookups","startedAt":"2022-11-10T16:45:40Z","lastAt":"2022-11-21T09:50:29Z","messageCount":6,"participants":["Phillip Wood via GitGitGadget","Taylor Blau","Johannes Schindelin","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"467067","messageId":"pull.1414.git.1668098622.gitgitgadget@gmail.com","threadId":"58784","inReplyTo":null,"subject":"[PATCH 0/2] rebase: stricter label lookups","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-10T16:43:39Z","receivedAt":"2022-11-10T16:45:40Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"If a rebase label does not exist and a branch or tag named refs/rewritten/\ndoes exist it will use that instead of returning an error.\n\nPhillip Wood (2):\n  sequencer: unify label lookup\n  sequencer: tighten label lookups\n\n sequencer.c              | 53 +++++++++++++++++++++-------------------\n t/t3430-rebase-merges.sh | 17 +++++++++++++\n 2 files changed, 45 insertions(+), 25 deletions(-)\n\n\nbase-commit: 3b08839926fcc7cc48cf4c759737c1a71af430c1\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1414%2Fphillipwood%2Fsequencer-label-read-refs-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1414/phillipwood/sequencer-label-read-refs-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1414\n-- \ngitgitgadget\n"},{"id":"467068","messageId":"ecb81b873a8d72c57fd4812ec86d103927437cf1.1668098622.git.gitgitgadget@gmail.com","threadId":"58784","inReplyTo":"pull.1414.git.1668098622.gitgitgadget@gmail.com","subject":"[PATCH 1/2] sequencer: unify label lookup","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-10T16:43:40Z","receivedAt":"2022-11-10T16:45:43Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nThe arguments to the `reset` and `merge` commands may be a label created\nwith a `label` command or an arbitrary commit name. The `merge` command\nuses the lookup_label() function to lookup its arguments but `reset` has\na slightly different version of that function in do_reset(). Reduce this\ncode duplication by calling lookup_label() from do_reset() as well.\n\nThis change improves the behavior of `reset` when the argument is a\ntree.  Previously `reset` would accept a tree only for the rebase to\nfail with\n\n       update_ref failed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object da5497437fd67ca928333aab79c4b4b55036ea66 to branch 'HEAD'\n\nUsing lookup_label() means do_reset() will now error out straight away\nif its argument is not a commit.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n sequencer.c              | 49 ++++++++++++++++++++--------------------\n t/t3430-rebase-merges.sh |  8 +++++++\n 2 files changed, 33 insertions(+), 24 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex e658df7e8ff..21f5032df0d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3696,6 +3696,26 @@ static const char *reflog_message(struct replay_opts *opts,\n \treturn buf.buf;\n }\n \n+static struct commit *lookup_label(const char *label, int len,\n+\t\t\t\t   struct strbuf *buf)\n+{\n+\tstruct commit *commit;\n+\n+\tstrbuf_reset(buf);\n+\tstrbuf_addf(buf, \"refs/rewritten/%.*s\", len, label);\n+\tcommit = lookup_commit_reference_by_name(buf->buf);\n+\tif (!commit) {\n+\t\t/* fall back to non-rewritten ref or commit */\n+\t\tstrbuf_splice(buf, 0, strlen(\"refs/rewritten/\"), \"\", 0);\n+\t\tcommit = lookup_commit_reference_by_name(buf->buf);\n+\t}\n+\n+\tif (!commit)\n+\t\terror(_(\"could not resolve '%s'\"), buf->buf);\n+\n+\treturn commit;\n+}\n+\n static int do_reset(struct repository *r,\n \t\t    const char *name, int len,\n \t\t    struct replay_opts *opts)\n@@ -3727,6 +3747,7 @@ static int do_reset(struct repository *r,\n \t\toidcpy(&oid, &opts->squash_onto);\n \t} else {\n \t\tint i;\n+\t\tstruct commit *commit;\n \n \t\t/* Determine the length of the label */\n \t\tfor (i = 0; i < len; i++)\n@@ -3734,12 +3755,12 @@ static int do_reset(struct repository *r,\n \t\t\t\tbreak;\n \t\tlen = i;\n \n-\t\tstrbuf_addf(&ref_name, \"refs/rewritten/%.*s\", len, name);\n-\t\tif (get_oid(ref_name.buf, &oid) &&\n-\t\t    get_oid(ref_name.buf + strlen(\"refs/rewritten/\"), &oid)) {\n-\t\t\tret = error(_(\"could not read '%s'\"), ref_name.buf);\n+\t\tcommit = lookup_label(name, len, &ref_name);\n+\t\tif (!commit) {\n+\t\t\tret = -1;\n \t\t\tgoto cleanup;\n \t\t}\n+\t\toid = commit->object.oid;\n \t}\n \n \tsetup_unpack_trees_porcelain(&unpack_tree_opts, \"reset\");\n@@ -3786,26 +3807,6 @@ cleanup:\n \treturn ret;\n }\n \n-static struct commit *lookup_label(const char *label, int len,\n-\t\t\t\t   struct strbuf *buf)\n-{\n-\tstruct commit *commit;\n-\n-\tstrbuf_reset(buf);\n-\tstrbuf_addf(buf, \"refs/rewritten/%.*s\", len, label);\n-\tcommit = lookup_commit_reference_by_name(buf->buf);\n-\tif (!commit) {\n-\t\t/* fall back to non-rewritten ref or commit */\n-\t\tstrbuf_splice(buf, 0, strlen(\"refs/rewritten/\"), \"\", 0);\n-\t\tcommit = lookup_commit_reference_by_name(buf->buf);\n-\t}\n-\n-\tif (!commit)\n-\t\terror(_(\"could not resolve '%s'\"), buf->buf);\n-\n-\treturn commit;\n-}\n-\n static int do_merge(struct repository *r,\n \t\t    struct commit *commit,\n \t\t    const char *arg, int arg_len,\ndiff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\nindex f351701fec2..fbbc4439bfe 100755\n--- a/t/t3430-rebase-merges.sh\n+++ b/t/t3430-rebase-merges.sh\n@@ -138,6 +138,14 @@ test_expect_success '`reset` refuses to overwrite untracked files' '\n \tgit rebase --abort\n '\n \n+test_expect_success '`reset` rejects trees' '\n+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n+\ttest_must_fail env GIT_SEQUENCE_EDITOR=\"echo reset A^{tree} >\" \\\n+\t\tgit rebase -i B C >out 2>err &&\n+\tgrep \"object .* is a tree\" err &&\n+\ttest_must_be_empty out\n+'\n+\n test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' '\n \ttest_when_finished \"test_might_fail git rebase --abort\" &&\n \tgit checkout -b conflicting-merge A &&\n-- \ngitgitgadget\n\n"},{"id":"467069","messageId":"d3a7ed20b9282348a9ebc2490ca70c9e89cf324b.1668098622.git.gitgitgadget@gmail.com","threadId":"58784","inReplyTo":"pull.1414.git.1668098622.gitgitgadget@gmail.com","subject":"[PATCH 2/2] sequencer: tighten label lookups","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-10T16:43:41Z","receivedAt":"2022-11-10T16:45:45Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nThe `label` command creates a ref refs/rewritten/<label> that the\n`reset` and `merge` commands resolve by calling lookup_label(). That\nuses lookup_commit_reference_by_name() to look up the label ref. As\nlookup_commit_reference_by_name() uses the dwim rules when looking up\nthe label it will look for a branch named\nrefs/heads/refs/rewritten/<label> and return that instead of an error if\nthe branch exists and the label does not. Fix this by using read_ref()\nfollowed by lookup_commit_object() when looking up labels.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n sequencer.c              | 14 ++++++++------\n t/t3430-rebase-merges.sh |  9 +++++++++\n 2 files changed, 17 insertions(+), 6 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 21f5032df0d..927da04e709 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3696,15 +3696,17 @@ static const char *reflog_message(struct replay_opts *opts,\n \treturn buf.buf;\n }\n \n-static struct commit *lookup_label(const char *label, int len,\n-\t\t\t\t   struct strbuf *buf)\n+static struct commit *lookup_label(struct repository *r, const char *label,\n+\t\t\t\t   int len, struct strbuf *buf)\n {\n \tstruct commit *commit;\n+\tstruct object_id oid;\n \n \tstrbuf_reset(buf);\n \tstrbuf_addf(buf, \"refs/rewritten/%.*s\", len, label);\n-\tcommit = lookup_commit_reference_by_name(buf->buf);\n-\tif (!commit) {\n+\tif (!read_ref(buf->buf, &oid)) {\n+\t\tcommit = lookup_commit_object(r, &oid);\n+\t} else {\n \t\t/* fall back to non-rewritten ref or commit */\n \t\tstrbuf_splice(buf, 0, strlen(\"refs/rewritten/\"), \"\", 0);\n \t\tcommit = lookup_commit_reference_by_name(buf->buf);\n@@ -3755,7 +3757,7 @@ static int do_reset(struct repository *r,\n \t\t\t\tbreak;\n \t\tlen = i;\n \n-\t\tcommit = lookup_label(name, len, &ref_name);\n+\t\tcommit = lookup_label(r, name, len, &ref_name);\n \t\tif (!commit) {\n \t\t\tret = -1;\n \t\t\tgoto cleanup;\n@@ -3854,7 +3856,7 @@ static int do_merge(struct repository *r,\n \t\tk = strcspn(p, \" \\t\\n\");\n \t\tif (!k)\n \t\t\tcontinue;\n-\t\tmerge_commit = lookup_label(p, k, &ref_name);\n+\t\tmerge_commit = lookup_label(r, p, k, &ref_name);\n \t\tif (!merge_commit) {\n \t\t\tret = error(_(\"unable to parse '%.*s'\"), k, p);\n \t\t\tgoto leave_merge;\ndiff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\nindex fbbc4439bfe..fa2a06c19f0 100755\n--- a/t/t3430-rebase-merges.sh\n+++ b/t/t3430-rebase-merges.sh\n@@ -146,6 +146,15 @@ test_expect_success '`reset` rejects trees' '\n \ttest_must_be_empty out\n '\n \n+test_expect_success '`reset` only looks for labels under refs/rewritten/' '\n+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n+\tgit branch refs/rewritten/my-label A &&\n+\ttest_must_fail env GIT_SEQUENCE_EDITOR=\"echo reset my-label >\" \\\n+\t\tgit rebase -i B C >out 2>err &&\n+\tgrep \"could not resolve ${SQ}my-label${SQ}\" err &&\n+\ttest_must_be_empty out\n+'\n+\n test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' '\n \ttest_when_finished \"test_might_fail git rebase --abort\" &&\n \tgit checkout -b conflicting-merge A &&\n-- \ngitgitgadget\n"},{"id":"467119","messageId":"Y23R7H7X/NEDR/0c@nand.local","threadId":"58784","inReplyTo":"pull.1414.git.1668098622.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] rebase: stricter label lookups","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-11T04:39:08Z","receivedAt":"2022-11-11T04:39:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Nov 10, 2022 at 04:43:39PM +0000, Phillip Wood via GitGitGadget wrote:\n> If a rebase label does not exist and a branch or tag named refs/rewritten/\n> does exist it will use that instead of returning an error.\n>\n> Phillip Wood (2):\n>   sequencer: unify label lookup\n>   sequencer: tighten label lookups\n>\n>  sequencer.c              | 53 +++++++++++++++++++++-------------------\n>  t/t3430-rebase-merges.sh | 17 +++++++++++++\n>  2 files changed, 45 insertions(+), 25 deletions(-)\n\nBoth were a very pleasant read. Thanks for working on this, will queue.\n\nThanks,\nTaylor\n"},{"id":"467521","messageId":"93qo066o-8r35-9q15-rq62-9r398o50p5p6@tzk.qr","threadId":"58784","inReplyTo":"ecb81b873a8d72c57fd4812ec86d103927437cf1.1668098622.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] sequencer: unify label lookup","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-11-18T13:40:04Z","receivedAt":"2022-11-18T13:40:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Thu, 10 Nov 2022, Phillip Wood via GitGitGadget wrote:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> The arguments to the `reset` and `merge` commands may be a label created\n> with a `label` command or an arbitrary commit name. The `merge` command\n> uses the lookup_label() function to lookup its arguments but `reset` has\n> a slightly different version of that function in do_reset(). Reduce this\n> code duplication by calling lookup_label() from do_reset() as well.\n>\n> This change improves the behavior of `reset` when the argument is a\n> tree.  Previously `reset` would accept a tree only for the rebase to\n> fail with\n>\n>        update_ref failed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object da5497437fd67ca928333aab79c4b4b55036ea66 to branch 'HEAD'\n>\n> Using lookup_label() means do_reset() will now error out straight away\n> if its argument is not a commit.\n>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n\nVery nice.\n\nFrom reading the patch, I get the impression that this also addresses\nhttps://github.com/gitgitgadget/git/issues/199, i.e. allowing `reset\n<tag>`.\n\nAm I right?\nDscho\n\nP.S.: FWIW here is my ACK for both patches.\n\n>  sequencer.c              | 49 ++++++++++++++++++++--------------------\n>  t/t3430-rebase-merges.sh |  8 +++++++\n>  2 files changed, 33 insertions(+), 24 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index e658df7e8ff..21f5032df0d 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3696,6 +3696,26 @@ static const char *reflog_message(struct replay_opts *opts,\n>  \treturn buf.buf;\n>  }\n>\n> +static struct commit *lookup_label(const char *label, int len,\n> +\t\t\t\t   struct strbuf *buf)\n> +{\n> +\tstruct commit *commit;\n> +\n> +\tstrbuf_reset(buf);\n> +\tstrbuf_addf(buf, \"refs/rewritten/%.*s\", len, label);\n> +\tcommit = lookup_commit_reference_by_name(buf->buf);\n> +\tif (!commit) {\n> +\t\t/* fall back to non-rewritten ref or commit */\n> +\t\tstrbuf_splice(buf, 0, strlen(\"refs/rewritten/\"), \"\", 0);\n> +\t\tcommit = lookup_commit_reference_by_name(buf->buf);\n> +\t}\n> +\n> +\tif (!commit)\n> +\t\terror(_(\"could not resolve '%s'\"), buf->buf);\n> +\n> +\treturn commit;\n> +}\n> +\n>  static int do_reset(struct repository *r,\n>  \t\t    const char *name, int len,\n>  \t\t    struct replay_opts *opts)\n> @@ -3727,6 +3747,7 @@ static int do_reset(struct repository *r,\n>  \t\toidcpy(&oid, &opts->squash_onto);\n>  \t} else {\n>  \t\tint i;\n> +\t\tstruct commit *commit;\n>\n>  \t\t/* Determine the length of the label */\n>  \t\tfor (i = 0; i < len; i++)\n> @@ -3734,12 +3755,12 @@ static int do_reset(struct repository *r,\n>  \t\t\t\tbreak;\n>  \t\tlen = i;\n>\n> -\t\tstrbuf_addf(&ref_name, \"refs/rewritten/%.*s\", len, name);\n> -\t\tif (get_oid(ref_name.buf, &oid) &&\n> -\t\t    get_oid(ref_name.buf + strlen(\"refs/rewritten/\"), &oid)) {\n> -\t\t\tret = error(_(\"could not read '%s'\"), ref_name.buf);\n> +\t\tcommit = lookup_label(name, len, &ref_name);\n> +\t\tif (!commit) {\n> +\t\t\tret = -1;\n>  \t\t\tgoto cleanup;\n>  \t\t}\n> +\t\toid = commit->object.oid;\n>  \t}\n>\n>  \tsetup_unpack_trees_porcelain(&unpack_tree_opts, \"reset\");\n> @@ -3786,26 +3807,6 @@ cleanup:\n>  \treturn ret;\n>  }\n>\n> -static struct commit *lookup_label(const char *label, int len,\n> -\t\t\t\t   struct strbuf *buf)\n> -{\n> -\tstruct commit *commit;\n> -\n> -\tstrbuf_reset(buf);\n> -\tstrbuf_addf(buf, \"refs/rewritten/%.*s\", len, label);\n> -\tcommit = lookup_commit_reference_by_name(buf->buf);\n> -\tif (!commit) {\n> -\t\t/* fall back to non-rewritten ref or commit */\n> -\t\tstrbuf_splice(buf, 0, strlen(\"refs/rewritten/\"), \"\", 0);\n> -\t\tcommit = lookup_commit_reference_by_name(buf->buf);\n> -\t}\n> -\n> -\tif (!commit)\n> -\t\terror(_(\"could not resolve '%s'\"), buf->buf);\n> -\n> -\treturn commit;\n> -}\n> -\n>  static int do_merge(struct repository *r,\n>  \t\t    struct commit *commit,\n>  \t\t    const char *arg, int arg_len,\n> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\n> index f351701fec2..fbbc4439bfe 100755\n> --- a/t/t3430-rebase-merges.sh\n> +++ b/t/t3430-rebase-merges.sh\n> @@ -138,6 +138,14 @@ test_expect_success '`reset` refuses to overwrite untracked files' '\n>  \tgit rebase --abort\n>  '\n>\n> +test_expect_success '`reset` rejects trees' '\n> +\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n> +\ttest_must_fail env GIT_SEQUENCE_EDITOR=\"echo reset A^{tree} >\" \\\n> +\t\tgit rebase -i B C >out 2>err &&\n> +\tgrep \"object .* is a tree\" err &&\n> +\ttest_must_be_empty out\n> +'\n> +\n>  test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' '\n>  \ttest_when_finished \"test_might_fail git rebase --abort\" &&\n>  \tgit checkout -b conflicting-merge A &&\n> --\n> gitgitgadget\n>\n>\n"},{"id":"467666","messageId":"4220fa3f-fc3f-aff0-fb03-b5dc4b24d087@dunelm.org.uk","threadId":"58784","inReplyTo":"93qo066o-8r35-9q15-rq62-9r398o50p5p6@tzk.qr","subject":"Re: [PATCH 1/2] sequencer: unify label lookup","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-11-21T09:49:57Z","receivedAt":"2022-11-21T09:50:29Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 18/11/2022 13:40, Johannes Schindelin wrote:\n> Hi Phillip,\n> \n> On Thu, 10 Nov 2022, Phillip Wood via GitGitGadget wrote:\n> \n>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>\n>> The arguments to the `reset` and `merge` commands may be a label created\n>> with a `label` command or an arbitrary commit name. The `merge` command\n>> uses the lookup_label() function to lookup its arguments but `reset` has\n>> a slightly different version of that function in do_reset(). Reduce this\n>> code duplication by calling lookup_label() from do_reset() as well.\n>>\n>> This change improves the behavior of `reset` when the argument is a\n>> tree.  Previously `reset` would accept a tree only for the rebase to\n>> fail with\n>>\n>>         update_ref failed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object da5497437fd67ca928333aab79c4b4b55036ea66 to branch 'HEAD'\n>>\n>> Using lookup_label() means do_reset() will now error out straight away\n>> if its argument is not a commit.\n>>\n>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n>> ---\n> \n> Very nice.\n> \n>  From reading the patch, I get the impression that this also addresses\n> https://github.com/gitgitgadget/git/issues/199, i.e. allowing `reset\n> <tag>`.\n> \n> Am I right?\n> Dscho\n\nI think so, the tag will be dereferenced by lookup_commit_reference()\n\nBest Wishes\n\nPhillip\n\n> P.S.: FWIW here is my ACK for both patches.\n> \n>>   sequencer.c              | 49 ++++++++++++++++++++--------------------\n>>   t/t3430-rebase-merges.sh |  8 +++++++\n>>   2 files changed, 33 insertions(+), 24 deletions(-)\n>>\n>> diff --git a/sequencer.c b/sequencer.c\n>> index e658df7e8ff..21f5032df0d 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -3696,6 +3696,26 @@ static const char *reflog_message(struct replay_opts *opts,\n>>   \treturn buf.buf;\n>>   }\n>>\n>> +static struct commit *lookup_label(const char *label, int len,\n>> +\t\t\t\t   struct strbuf *buf)\n>> +{\n>> +\tstruct commit *commit;\n>> +\n>> +\tstrbuf_reset(buf);\n>> +\tstrbuf_addf(buf, \"refs/rewritten/%.*s\", len, label);\n>> +\tcommit = lookup_commit_reference_by_name(buf->buf);\n>> +\tif (!commit) {\n>> +\t\t/* fall back to non-rewritten ref or commit */\n>> +\t\tstrbuf_splice(buf, 0, strlen(\"refs/rewritten/\"), \"\", 0);\n>> +\t\tcommit = lookup_commit_reference_by_name(buf->buf);\n>> +\t}\n>> +\n>> +\tif (!commit)\n>> +\t\terror(_(\"could not resolve '%s'\"), buf->buf);\n>> +\n>> +\treturn commit;\n>> +}\n>> +\n>>   static int do_reset(struct repository *r,\n>>   \t\t    const char *name, int len,\n>>   \t\t    struct replay_opts *opts)\n>> @@ -3727,6 +3747,7 @@ static int do_reset(struct repository *r,\n>>   \t\toidcpy(&oid, &opts->squash_onto);\n>>   \t} else {\n>>   \t\tint i;\n>> +\t\tstruct commit *commit;\n>>\n>>   \t\t/* Determine the length of the label */\n>>   \t\tfor (i = 0; i < len; i++)\n>> @@ -3734,12 +3755,12 @@ static int do_reset(struct repository *r,\n>>   \t\t\t\tbreak;\n>>   \t\tlen = i;\n>>\n>> -\t\tstrbuf_addf(&ref_name, \"refs/rewritten/%.*s\", len, name);\n>> -\t\tif (get_oid(ref_name.buf, &oid) &&\n>> -\t\t    get_oid(ref_name.buf + strlen(\"refs/rewritten/\"), &oid)) {\n>> -\t\t\tret = error(_(\"could not read '%s'\"), ref_name.buf);\n>> +\t\tcommit = lookup_label(name, len, &ref_name);\n>> +\t\tif (!commit) {\n>> +\t\t\tret = -1;\n>>   \t\t\tgoto cleanup;\n>>   \t\t}\n>> +\t\toid = commit->object.oid;\n>>   \t}\n>>\n>>   \tsetup_unpack_trees_porcelain(&unpack_tree_opts, \"reset\");\n>> @@ -3786,26 +3807,6 @@ cleanup:\n>>   \treturn ret;\n>>   }\n>>\n>> -static struct commit *lookup_label(const char *label, int len,\n>> -\t\t\t\t   struct strbuf *buf)\n>> -{\n>> -\tstruct commit *commit;\n>> -\n>> -\tstrbuf_reset(buf);\n>> -\tstrbuf_addf(buf, \"refs/rewritten/%.*s\", len, label);\n>> -\tcommit = lookup_commit_reference_by_name(buf->buf);\n>> -\tif (!commit) {\n>> -\t\t/* fall back to non-rewritten ref or commit */\n>> -\t\tstrbuf_splice(buf, 0, strlen(\"refs/rewritten/\"), \"\", 0);\n>> -\t\tcommit = lookup_commit_reference_by_name(buf->buf);\n>> -\t}\n>> -\n>> -\tif (!commit)\n>> -\t\terror(_(\"could not resolve '%s'\"), buf->buf);\n>> -\n>> -\treturn commit;\n>> -}\n>> -\n>>   static int do_merge(struct repository *r,\n>>   \t\t    struct commit *commit,\n>>   \t\t    const char *arg, int arg_len,\n>> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\n>> index f351701fec2..fbbc4439bfe 100755\n>> --- a/t/t3430-rebase-merges.sh\n>> +++ b/t/t3430-rebase-merges.sh\n>> @@ -138,6 +138,14 @@ test_expect_success '`reset` refuses to overwrite untracked files' '\n>>   \tgit rebase --abort\n>>   '\n>>\n>> +test_expect_success '`reset` rejects trees' '\n>> +\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n>> +\ttest_must_fail env GIT_SEQUENCE_EDITOR=\"echo reset A^{tree} >\" \\\n>> +\t\tgit rebase -i B C >out 2>err &&\n>> +\tgrep \"object .* is a tree\" err &&\n>> +\ttest_must_be_empty out\n>> +'\n>> +\n>>   test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' '\n>>   \ttest_when_finished \"test_might_fail git rebase --abort\" &&\n>>   \tgit checkout -b conflicting-merge A &&\n>> --\n>> gitgitgadget\n>>\n>>\n"}]}