[PATCH 0/2] rebase: stricter label lookups

[PATCH 0/2] rebase: stricter label lookups

[Phillip Wood via GitGitGadget]

If a rebase label does not exist and a branch or tag named refs/rewritten/
does exist it will use that instead of returning an error.

Phillip Wood (2):
  sequencer: unify label lookup
  sequencer: tighten label lookups

 sequencer.c              | 53 +++++++++++++++++++++-------------------
 t/t3430-rebase-merges.sh | 17 +++++++++++++
 2 files changed, 45 insertions(+), 25 deletions(-)


base-commit: 3b08839926fcc7cc48cf4c759737c1a71af430c1
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1414%2Fphillipwood%2Fsequencer-label-read-refs-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1414/phillipwood/sequencer-label-read-refs-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1414
-- 
gitgitgadget

[PATCH 1/2] sequencer: unify label lookup

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The arguments to the `reset` and `merge` commands may be a label created
with a `label` command or an arbitrary commit name. The `merge` command
uses the lookup_label() function to lookup its arguments but `reset` has
a slightly different version of that function in do_reset(). Reduce this
code duplication by calling lookup_label() from do_reset() as well.

This change improves the behavior of `reset` when the argument is a
tree.  Previously `reset` would accept a tree only for the rebase to
fail with

       update_ref failed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object da5497437fd67ca928333aab79c4b4b55036ea66 to branch 'HEAD'

Using lookup_label() means do_reset() will now error out straight away
if its argument is not a commit.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c              | 49 ++++++++++++++++++++--------------------
 t/t3430-rebase-merges.sh |  8 +++++++
 2 files changed, 33 insertions(+), 24 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index e658df7e8ff..21f5032df0d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3696,6 +3696,26 @@ static const char *reflog_message(struct replay_opts *opts,
 	return buf.buf;
 }
 
+static struct commit *lookup_label(const char *label, int len,
+				   struct strbuf *buf)
+{
+	struct commit *commit;
+
+	strbuf_reset(buf);
+	strbuf_addf(buf, "refs/rewritten/%.*s", len, label);
+	commit = lookup_commit_reference_by_name(buf->buf);
+	if (!commit) {
+		/* fall back to non-rewritten ref or commit */
+		strbuf_splice(buf, 0, strlen("refs/rewritten/"), "", 0);
+		commit = lookup_commit_reference_by_name(buf->buf);
+	}
+
+	if (!commit)
+		error(_("could not resolve '%s'"), buf->buf);
+
+	return commit;
+}
+
 static int do_reset(struct repository *r,
 		    const char *name, int len,
 		    struct replay_opts *opts)
@@ -3727,6 +3747,7 @@ static int do_reset(struct repository *r,
 		oidcpy(&oid, &opts->squash_onto);
 	} else {
 		int i;
+		struct commit *commit;
 
 		/* Determine the length of the label */
 		for (i = 0; i < len; i++)
@@ -3734,12 +3755,12 @@ static int do_reset(struct repository *r,
 				break;
 		len = i;
 
-		strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
-		if (get_oid(ref_name.buf, &oid) &&
-		    get_oid(ref_name.buf + strlen("refs/rewritten/"), &oid)) {
-			ret = error(_("could not read '%s'"), ref_name.buf);
+		commit = lookup_label(name, len, &ref_name);
+		if (!commit) {
+			ret = -1;
 			goto cleanup;
 		}
+		oid = commit->object.oid;
 	}
 
 	setup_unpack_trees_porcelain(&unpack_tree_opts, "reset");
@@ -3786,26 +3807,6 @@ cleanup:
 	return ret;
 }
 
-static struct commit *lookup_label(const char *label, int len,
-				   struct strbuf *buf)
-{
-	struct commit *commit;
-
-	strbuf_reset(buf);
-	strbuf_addf(buf, "refs/rewritten/%.*s", len, label);
-	commit = lookup_commit_reference_by_name(buf->buf);
-	if (!commit) {
-		/* fall back to non-rewritten ref or commit */
-		strbuf_splice(buf, 0, strlen("refs/rewritten/"), "", 0);
-		commit = lookup_commit_reference_by_name(buf->buf);
-	}
-
-	if (!commit)
-		error(_("could not resolve '%s'"), buf->buf);
-
-	return commit;
-}
-
 static int do_merge(struct repository *r,
 		    struct commit *commit,
 		    const char *arg, int arg_len,
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index f351701fec2..fbbc4439bfe 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -138,6 +138,14 @@ test_expect_success '`reset` refuses to overwrite untracked files' '
 	git rebase --abort
 '
 
+test_expect_success '`reset` rejects trees' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	test_must_fail env GIT_SEQUENCE_EDITOR="echo reset A^{tree} >" \
+		git rebase -i B C >out 2>err &&
+	grep "object .* is a tree" err &&
+	test_must_be_empty out
+'
+
 test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout -b conflicting-merge A &&
-- 
gitgitgadget

Re: [PATCH 1/2] sequencer: unify label lookup

[Johannes Schindelin]

Hi Phillip,

On Thu, 10 Nov 2022, Phillip Wood via GitGitGadget wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> The arguments to the `reset` and `merge` commands may be a label created
> with a `label` command or an arbitrary commit name. The `merge` command
> uses the lookup_label() function to lookup its arguments but `reset` has
> a slightly different version of that function in do_reset(). Reduce this
> code duplication by calling lookup_label() from do_reset() as well.
>
> This change improves the behavior of `reset` when the argument is a
> tree.  Previously `reset` would accept a tree only for the rebase to
> fail with
>
>        update_ref failed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object da5497437fd67ca928333aab79c4b4b55036ea66 to branch 'HEAD'
>
> Using lookup_label() means do_reset() will now error out straight away
> if its argument is not a commit.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---

Very nice.

From reading the patch, I get the impression that this also addresses
https://github.com/gitgitgadget/git/issues/199, i.e. allowing `reset
<tag>`.

Am I right?
Dscho

P.S.: FWIW here is my ACK for both patches.

>  sequencer.c              | 49 ++++++++++++++++++++--------------------
>  t/t3430-rebase-merges.sh |  8 +++++++
>  2 files changed, 33 insertions(+), 24 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index e658df7e8ff..21f5032df0d 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3696,6 +3696,26 @@ static const char *reflog_message(struct replay_opts *opts,
>  	return buf.buf;
>  }
>
> +static struct commit *lookup_label(const char *label, int len,
> +				   struct strbuf *buf)
> +{
> +	struct commit *commit;
> +
> +	strbuf_reset(buf);
> +	strbuf_addf(buf, "refs/rewritten/%.*s", len, label);
> +	commit = lookup_commit_reference_by_name(buf->buf);
> +	if (!commit) {
> +		/* fall back to non-rewritten ref or commit */
> +		strbuf_splice(buf, 0, strlen("refs/rewritten/"), "", 0);
> +		commit = lookup_commit_reference_by_name(buf->buf);
> +	}
> +
> +	if (!commit)
> +		error(_("could not resolve '%s'"), buf->buf);
> +
> +	return commit;
> +}
> +
>  static int do_reset(struct repository *r,
>  		    const char *name, int len,
>  		    struct replay_opts *opts)
> @@ -3727,6 +3747,7 @@ static int do_reset(struct repository *r,
>  		oidcpy(&oid, &opts->squash_onto);
>  	} else {
>  		int i;
> +		struct commit *commit;
>
>  		/* Determine the length of the label */
>  		for (i = 0; i < len; i++)
> @@ -3734,12 +3755,12 @@ static int do_reset(struct repository *r,
>  				break;
>  		len = i;
>
> -		strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
> -		if (get_oid(ref_name.buf, &oid) &&
> -		    get_oid(ref_name.buf + strlen("refs/rewritten/"), &oid)) {
> -			ret = error(_("could not read '%s'"), ref_name.buf);
> +		commit = lookup_label(name, len, &ref_name);
> +		if (!commit) {
> +			ret = -1;
>  			goto cleanup;
>  		}
> +		oid = commit->object.oid;
>  	}
>
>  	setup_unpack_trees_porcelain(&unpack_tree_opts, "reset");
> @@ -3786,26 +3807,6 @@ cleanup:
>  	return ret;
>  }
>
> -static struct commit *lookup_label(const char *label, int len,
> -				   struct strbuf *buf)
> -{
> -	struct commit *commit;
> -
> -	strbuf_reset(buf);
> -	strbuf_addf(buf, "refs/rewritten/%.*s", len, label);
> -	commit = lookup_commit_reference_by_name(buf->buf);
> -	if (!commit) {
> -		/* fall back to non-rewritten ref or commit */
> -		strbuf_splice(buf, 0, strlen("refs/rewritten/"), "", 0);
> -		commit = lookup_commit_reference_by_name(buf->buf);
> -	}
> -
> -	if (!commit)
> -		error(_("could not resolve '%s'"), buf->buf);
> -
> -	return commit;
> -}
> -
>  static int do_merge(struct repository *r,
>  		    struct commit *commit,
>  		    const char *arg, int arg_len,
> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> index f351701fec2..fbbc4439bfe 100755
> --- a/t/t3430-rebase-merges.sh
> +++ b/t/t3430-rebase-merges.sh
> @@ -138,6 +138,14 @@ test_expect_success '`reset` refuses to overwrite untracked files' '
>  	git rebase --abort
>  '
>
> +test_expect_success '`reset` rejects trees' '
> +	test_when_finished "test_might_fail git rebase --abort" &&
> +	test_must_fail env GIT_SEQUENCE_EDITOR="echo reset A^{tree} >" \
> +		git rebase -i B C >out 2>err &&
> +	grep "object .* is a tree" err &&
> +	test_must_be_empty out
> +'
> +
>  test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' '
>  	test_when_finished "test_might_fail git rebase --abort" &&
>  	git checkout -b conflicting-merge A &&
> --
> gitgitgadget
>
>

Re: [PATCH 1/2] sequencer: unify label lookup

[Phillip Wood]

Hi Dscho

On 18/11/2022 13:40, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Thu, 10 Nov 2022, Phillip Wood via GitGitGadget wrote:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> The arguments to the `reset` and `merge` commands may be a label created
>> with a `label` command or an arbitrary commit name. The `merge` command
>> uses the lookup_label() function to lookup its arguments but `reset` has
>> a slightly different version of that function in do_reset(). Reduce this
>> code duplication by calling lookup_label() from do_reset() as well.
>>
>> This change improves the behavior of `reset` when the argument is a
>> tree.  Previously `reset` would accept a tree only for the rebase to
>> fail with
>>
>>         update_ref failed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object da5497437fd67ca928333aab79c4b4b55036ea66 to branch 'HEAD'
>>
>> Using lookup_label() means do_reset() will now error out straight away
>> if its argument is not a commit.
>>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
> 
> Very nice.
> 
>  From reading the patch, I get the impression that this also addresses
> https://github.com/gitgitgadget/git/issues/199, i.e. allowing `reset
> <tag>`.
> 
> Am I right?
> Dscho

I think so, the tag will be dereferenced by lookup_commit_reference()

Best Wishes

Phillip

> P.S.: FWIW here is my ACK for both patches.
> 
>>   sequencer.c              | 49 ++++++++++++++++++++--------------------
>>   t/t3430-rebase-merges.sh |  8 +++++++
>>   2 files changed, 33 insertions(+), 24 deletions(-)
>>
>> diff --git a/sequencer.c b/sequencer.c
>> index e658df7e8ff..21f5032df0d 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -3696,6 +3696,26 @@ static const char *reflog_message(struct replay_opts *opts,
>>   	return buf.buf;
>>   }
>>
>> +static struct commit *lookup_label(const char *label, int len,
>> +				   struct strbuf *buf)
>> +{
>> +	struct commit *commit;
>> +
>> +	strbuf_reset(buf);
>> +	strbuf_addf(buf, "refs/rewritten/%.*s", len, label);
>> +	commit = lookup_commit_reference_by_name(buf->buf);
>> +	if (!commit) {
>> +		/* fall back to non-rewritten ref or commit */
>> +		strbuf_splice(buf, 0, strlen("refs/rewritten/"), "", 0);
>> +		commit = lookup_commit_reference_by_name(buf->buf);
>> +	}
>> +
>> +	if (!commit)
>> +		error(_("could not resolve '%s'"), buf->buf);
>> +
>> +	return commit;
>> +}
>> +
>>   static int do_reset(struct repository *r,
>>   		    const char *name, int len,
>>   		    struct replay_opts *opts)
>> @@ -3727,6 +3747,7 @@ static int do_reset(struct repository *r,
>>   		oidcpy(&oid, &opts->squash_onto);
>>   	} else {
>>   		int i;
>> +		struct commit *commit;
>>
>>   		/* Determine the length of the label */
>>   		for (i = 0; i < len; i++)
>> @@ -3734,12 +3755,12 @@ static int do_reset(struct repository *r,
>>   				break;
>>   		len = i;
>>
>> -		strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
>> -		if (get_oid(ref_name.buf, &oid) &&
>> -		    get_oid(ref_name.buf + strlen("refs/rewritten/"), &oid)) {
>> -			ret = error(_("could not read '%s'"), ref_name.buf);
>> +		commit = lookup_label(name, len, &ref_name);
>> +		if (!commit) {
>> +			ret = -1;
>>   			goto cleanup;
>>   		}
>> +		oid = commit->object.oid;
>>   	}
>>
>>   	setup_unpack_trees_porcelain(&unpack_tree_opts, "reset");
>> @@ -3786,26 +3807,6 @@ cleanup:
>>   	return ret;
>>   }
>>
>> -static struct commit *lookup_label(const char *label, int len,
>> -				   struct strbuf *buf)
>> -{
>> -	struct commit *commit;
>> -
>> -	strbuf_reset(buf);
>> -	strbuf_addf(buf, "refs/rewritten/%.*s", len, label);
>> -	commit = lookup_commit_reference_by_name(buf->buf);
>> -	if (!commit) {
>> -		/* fall back to non-rewritten ref or commit */
>> -		strbuf_splice(buf, 0, strlen("refs/rewritten/"), "", 0);
>> -		commit = lookup_commit_reference_by_name(buf->buf);
>> -	}
>> -
>> -	if (!commit)
>> -		error(_("could not resolve '%s'"), buf->buf);
>> -
>> -	return commit;
>> -}
>> -
>>   static int do_merge(struct repository *r,
>>   		    struct commit *commit,
>>   		    const char *arg, int arg_len,
>> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
>> index f351701fec2..fbbc4439bfe 100755
>> --- a/t/t3430-rebase-merges.sh
>> +++ b/t/t3430-rebase-merges.sh
>> @@ -138,6 +138,14 @@ test_expect_success '`reset` refuses to overwrite untracked files' '
>>   	git rebase --abort
>>   '
>>
>> +test_expect_success '`reset` rejects trees' '
>> +	test_when_finished "test_might_fail git rebase --abort" &&
>> +	test_must_fail env GIT_SEQUENCE_EDITOR="echo reset A^{tree} >" \
>> +		git rebase -i B C >out 2>err &&
>> +	grep "object .* is a tree" err &&
>> +	test_must_be_empty out
>> +'
>> +
>>   test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' '
>>   	test_when_finished "test_might_fail git rebase --abort" &&
>>   	git checkout -b conflicting-merge A &&
>> --
>> gitgitgadget
>>
>>

[PATCH 2/2] sequencer: tighten label lookups

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The `label` command creates a ref refs/rewritten/<label> that the
`reset` and `merge` commands resolve by calling lookup_label(). That
uses lookup_commit_reference_by_name() to look up the label ref. As
lookup_commit_reference_by_name() uses the dwim rules when looking up
the label it will look for a branch named
refs/heads/refs/rewritten/<label> and return that instead of an error if
the branch exists and the label does not. Fix this by using read_ref()
followed by lookup_commit_object() when looking up labels.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c              | 14 ++++++++------
 t/t3430-rebase-merges.sh |  9 +++++++++
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 21f5032df0d..927da04e709 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3696,15 +3696,17 @@ static const char *reflog_message(struct replay_opts *opts,
 	return buf.buf;
 }
 
-static struct commit *lookup_label(const char *label, int len,
-				   struct strbuf *buf)
+static struct commit *lookup_label(struct repository *r, const char *label,
+				   int len, struct strbuf *buf)
 {
 	struct commit *commit;
+	struct object_id oid;
 
 	strbuf_reset(buf);
 	strbuf_addf(buf, "refs/rewritten/%.*s", len, label);
-	commit = lookup_commit_reference_by_name(buf->buf);
-	if (!commit) {
+	if (!read_ref(buf->buf, &oid)) {
+		commit = lookup_commit_object(r, &oid);
+	} else {
 		/* fall back to non-rewritten ref or commit */
 		strbuf_splice(buf, 0, strlen("refs/rewritten/"), "", 0);
 		commit = lookup_commit_reference_by_name(buf->buf);
@@ -3755,7 +3757,7 @@ static int do_reset(struct repository *r,
 				break;
 		len = i;
 
-		commit = lookup_label(name, len, &ref_name);
+		commit = lookup_label(r, name, len, &ref_name);
 		if (!commit) {
 			ret = -1;
 			goto cleanup;
@@ -3854,7 +3856,7 @@ static int do_merge(struct repository *r,
 		k = strcspn(p, " \t\n");
 		if (!k)
 			continue;
-		merge_commit = lookup_label(p, k, &ref_name);
+		merge_commit = lookup_label(r, p, k, &ref_name);
 		if (!merge_commit) {
 			ret = error(_("unable to parse '%.*s'"), k, p);
 			goto leave_merge;
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index fbbc4439bfe..fa2a06c19f0 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -146,6 +146,15 @@ test_expect_success '`reset` rejects trees' '
 	test_must_be_empty out
 '
 
+test_expect_success '`reset` only looks for labels under refs/rewritten/' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	git branch refs/rewritten/my-label A &&
+	test_must_fail env GIT_SEQUENCE_EDITOR="echo reset my-label >" \
+		git rebase -i B C >out 2>err &&
+	grep "could not resolve ${SQ}my-label${SQ}" err &&
+	test_must_be_empty out
+'
+
 test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout -b conflicting-merge A &&
-- 
gitgitgadget

Re: [PATCH 0/2] rebase: stricter label lookups

[Taylor Blau]

On Thu, Nov 10, 2022 at 04:43:39PM +0000, Phillip Wood via GitGitGadget wrote:
> If a rebase label does not exist and a branch or tag named refs/rewritten/
> does exist it will use that instead of returning an error.
>
> Phillip Wood (2):
>   sequencer: unify label lookup
>   sequencer: tighten label lookups
>
>  sequencer.c              | 53 +++++++++++++++++++++-------------------
>  t/t3430-rebase-merges.sh | 17 +++++++++++++
>  2 files changed, 45 insertions(+), 25 deletions(-)

Both were a very pleasant read. Thanks for working on this, will queue.

Thanks,
Taylor