threads / patch / 31949

patchreplace: parse revision argument for -d

Subject: [PATCH] replace: parse revision argument for -d

## tl;dr

22 messages between Oct 26, 2012 and Nov 14, 2012. Diffs are folded; open one to read it.

replies: 21people: 4as markdown or json

Michael J Gruber· Oct 26, 2012, 13:33 UTC · lore

'git replace' parses the revision arguments when it creates replacements (so that a sha1 can be abbreviated, e.g.) but not when deleting replacements.

This sucks.
Make it parse the argument to 'replace -d' in the same way.

Just in case someone lost the replacement object before deleting the replacement, take the argument literally if it can not be resolved to a full sha1.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 builtin/replace.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)
Show changes to builtin/replace.c +10 −5
diff --git a/builtin/replace.c b/builtin/replace.c
index e3aaf70..80e2039 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -46,24 +46,29 @@ typedef int (*each_replace_name_fn)(const char *name, const char *ref,
 
 static int for_each_replace_name(const char **argv, each_replace_name_fn fn)
 {
-	const char **p;
+	const char **p, *q;
 	char ref[PATH_MAX];
 	int had_error = 0;
 	unsigned char sha1[20];
 
 	for (p = argv; *p; p++) {
-		if (snprintf(ref, sizeof(ref), "refs/replace/%s", *p)
+		q = *p;
+		if (get_sha1(q, sha1))
+			warning("Failed to resolve '%s' as a valid ref; taking it literally.", q);
+		else
+			q = sha1_to_hex(sha1);
+		if (snprintf(ref, sizeof(ref), "refs/replace/%s", q)
 					>= sizeof(ref)) {
-			error("replace ref name too long: %.*s...", 50, *p);
+			error("replace ref name too long: %.*s...", 50, q);
 			had_error = 1;
 			continue;
 		}
 		if (read_ref(ref, sha1)) {
-			error("replace ref '%s' not found.", *p);
+			error("replace ref '%s' not found.", q);
 			had_error = 1;
 			continue;
 		}
-		if (fn(*p, ref, sha1))
+		if (fn(q, ref, sha1))
 			had_error = 1;
 	}
 	return had_error;

-- 
1.8.0.370.g8cbad08
Christian Couder· Oct 26, 2012, 15:25 UTC · re: Michael J Gruber · lore

Re: [PATCH] replace: parse revision argument for -d

Hi,

On Fri, Oct 26, 2012 at 3:33 PM, Michael J Gruber <git@drmicha.warpmail.net> wrote:

Show 7 quoted lines
> 'git replace' parses the revision arguments when it creates replacements
> (so that a sha1 can be abbreviated, e.g.) but not when deleting
> replacements.
>
> This sucks.
>
> Make it parse the argument to 'replace -d' in the same way.
Nit: there could be more than one argument to 'replace -d', so perhaps
"each argument" is better.
> Just in case someone lost the replacement object before deleting the
> replacement, take the argument literally if it can not be resolved to a
Here too.
Show 46 quoted lines
> full sha1.
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
>  builtin/replace.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/replace.c b/builtin/replace.c
> index e3aaf70..80e2039 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -46,24 +46,29 @@ typedef int (*each_replace_name_fn)(const char *name, const char *ref,
>
>  static int for_each_replace_name(const char **argv, each_replace_name_fn fn)
>  {
> -       const char **p;
> +       const char **p, *q;
>         char ref[PATH_MAX];
>         int had_error = 0;
>         unsigned char sha1[20];
>
>         for (p = argv; *p; p++) {
> -               if (snprintf(ref, sizeof(ref), "refs/replace/%s", *p)
> +               q = *p;
> +               if (get_sha1(q, sha1))
> +                       warning("Failed to resolve '%s' as a valid ref; taking it literally.", q);
> +               else
> +                       q = sha1_to_hex(sha1);
> +               if (snprintf(ref, sizeof(ref), "refs/replace/%s", q)
>                                         >= sizeof(ref)) {
> -                       error("replace ref name too long: %.*s...", 50, *p);
> +                       error("replace ref name too long: %.*s...", 50, q);
>                         had_error = 1;
>                         continue;
>                 }
>                 if (read_ref(ref, sha1)) {
> -                       error("replace ref '%s' not found.", *p);
> +                       error("replace ref '%s' not found.", q);
>                         had_error = 1;
>                         continue;
>                 }
> -               if (fn(*p, ref, sha1))
> +               if (fn(q, ref, sha1))
>                         had_error = 1;
>         }
>         return had_error;
Looks good to me.

Thanks, Christian.

Jeff King· Oct 29, 2012, 06:58 UTC · re: Michael J Gruber · lore

Re: [PATCH] replace: parse revision argument for -d

On Fri, Oct 26, 2012 at 03:33:27PM +0200, Michael J Gruber wrote:
Show 7 quoted lines
>  	for (p = argv; *p; p++) {
> -		if (snprintf(ref, sizeof(ref), "refs/replace/%s", *p)
> +		q = *p;
> +		if (get_sha1(q, sha1))
> +			warning("Failed to resolve '%s' as a valid ref; taking it literally.", q);
> +		else
> +			q = sha1_to_hex(sha1);

Doesn't get_sha1 already handle this for 40-byte sha1s (and for anything else, it would not work anyway)?

-Peff
Michael J Gruber· Oct 29, 2012, 09:02 UTC · re: Jeff King · lore

Re: [PATCH] replace: parse revision argument for -d

Jeff King venit, vidit, dixit 29.10.2012 07:58:
Show 12 quoted lines
> On Fri, Oct 26, 2012 at 03:33:27PM +0200, Michael J Gruber wrote:
> 
>>  	for (p = argv; *p; p++) {
>> -		if (snprintf(ref, sizeof(ref), "refs/replace/%s", *p)
>> +		q = *p;
>> +		if (get_sha1(q, sha1))
>> +			warning("Failed to resolve '%s' as a valid ref; taking it literally.", q);
>> +		else
>> +			q = sha1_to_hex(sha1);
> 
> Doesn't get_sha1 already handle this for 40-byte sha1s (and for anything
> else, it would not work anyway)?
What is "this"???

So far, "git replace -d <rev>" only accepts a full sha1, because it uses it literally as a ref name "resf/replace/<rev>" without resolving anything.

The patch makes it so that <rev> gets resolved to a sha1 even if it is abbreviated, and then it gets used.

Or do you mean the warning?
Michael
Jeff King· Oct 29, 2012, 09:04 UTC · re: Michael J Gruber · lore

Re: [PATCH] replace: parse revision argument for -d

On Mon, Oct 29, 2012 at 10:02:47AM +0100, Michael J Gruber wrote:
Show 23 quoted lines
> Jeff King venit, vidit, dixit 29.10.2012 07:58:
> > On Fri, Oct 26, 2012 at 03:33:27PM +0200, Michael J Gruber wrote:
> > 
> >>  	for (p = argv; *p; p++) {
> >> -		if (snprintf(ref, sizeof(ref), "refs/replace/%s", *p)
> >> +		q = *p;
> >> +		if (get_sha1(q, sha1))
> >> +			warning("Failed to resolve '%s' as a valid ref; taking it literally.", q);
> >> +		else
> >> +			q = sha1_to_hex(sha1);
> > 
> > Doesn't get_sha1 already handle this for 40-byte sha1s (and for anything
> > else, it would not work anyway)?
> 
> What is "this"???
> 
> So far, "git replace -d <rev>" only accepts a full sha1, because it uses
> it literally as a ref name "resf/replace/<rev>" without resolving anything.
> 
> The patch makes it so that <rev> gets resolved to a sha1 even if it is
> abbreviated, and then it gets used.
> 
> Or do you mean the warning?
Sorry, yeah, I meant the warning and fallback.

If I understand correctly, the fallback will never work unless we are fed a 40-byte sha1. But get_sha1 should always return a 40-byte sha1 without doing any further processing.

-Peff
Michael J Gruber· Oct 29, 2012, 10:08 UTC · re: Jeff King · lore

Re: [PATCH] replace: parse revision argument for -d

Jeff King venit, vidit, dixit 29.10.2012 10:04:
Show 31 quoted lines
> On Mon, Oct 29, 2012 at 10:02:47AM +0100, Michael J Gruber wrote:
> 
>> Jeff King venit, vidit, dixit 29.10.2012 07:58:
>>> On Fri, Oct 26, 2012 at 03:33:27PM +0200, Michael J Gruber wrote:
>>>
>>>>  	for (p = argv; *p; p++) {
>>>> -		if (snprintf(ref, sizeof(ref), "refs/replace/%s", *p)
>>>> +		q = *p;
>>>> +		if (get_sha1(q, sha1))
>>>> +			warning("Failed to resolve '%s' as a valid ref; taking it literally.", q);
>>>> +		else
>>>> +			q = sha1_to_hex(sha1);
>>>
>>> Doesn't get_sha1 already handle this for 40-byte sha1s (and for anything
>>> else, it would not work anyway)?
>>
>> What is "this"???
>>
>> So far, "git replace -d <rev>" only accepts a full sha1, because it uses
>> it literally as a ref name "resf/replace/<rev>" without resolving anything.
>>
>> The patch makes it so that <rev> gets resolved to a sha1 even if it is
>> abbreviated, and then it gets used.
>>
>> Or do you mean the warning?
> 
> Sorry, yeah, I meant the warning and fallback.
> 
> If I understand correctly, the fallback will never work unless we are
> fed a 40-byte sha1. But get_sha1 should always return a 40-byte sha1
> without doing any further processing.

You do understand correctly, and I misunderstood get_sha1(). I does not check for the presence of that sha1 in the object db, so that it succeeds no matter what, that is: if it's valid hex. So I should probably rewrite the error handling: no more need to check for lengths.

Michael
Michael J Gruber· Oct 29, 2012, 13:23 UTC · re: Michael J Gruber · lore

[PATCHv2] replace: parse revision argument for -d

'git replace' parses the revision arguments when it creates replacements (so that a sha1 can be abbreviated, e.g.) but not when deleting replacements.

Make it parse the arguments to 'replace -d' in the same way.
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
v2 has the simplified error check as per Jeff, and a reworded message.
Comes with a free test case, too.
 builtin/replace.c  | 14 ++++++++------
 t/t6050-replace.sh | 11 +++++++++++
 2 files changed, 19 insertions(+), 6 deletions(-)
Show changes to 2 files +19 −6

builtin/replace.c, t/t6050-replace.sh

diff --git a/builtin/replace.c b/builtin/replace.c
index e3aaf70..7b00055 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -46,24 +46,26 @@ typedef int (*each_replace_name_fn)(const char *name, const char *ref,
 
 static int for_each_replace_name(const char **argv, each_replace_name_fn fn)
 {
-	const char **p;
+	const char **p, *q;
 	char ref[PATH_MAX];
 	int had_error = 0;
 	unsigned char sha1[20];
 
 	for (p = argv; *p; p++) {
-		if (snprintf(ref, sizeof(ref), "refs/replace/%s", *p)
-					>= sizeof(ref)) {
-			error("replace ref name too long: %.*s...", 50, *p);
+		q = *p;
+		if (get_sha1(q, sha1)) {
+			error("Failed to resolve '%s' as a valid ref.", q);
 			had_error = 1;
 			continue;
 		}
+		q = sha1_to_hex(sha1);
+		snprintf(ref, sizeof(ref), "refs/replace/%s", q);
 		if (read_ref(ref, sha1)) {
-			error("replace ref '%s' not found.", *p);
+			error("replace ref '%s' not found.", q);
 			had_error = 1;
 			continue;
 		}
-		if (fn(*p, ref, sha1))
+		if (fn(q, ref, sha1))
 			had_error = 1;
 	}
 	return had_error;
diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index 5c87f28..decdc33 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -140,6 +140,17 @@ test_expect_success '"git replace" replacing' '
      test "$HASH2" = "$(git replace)"
 '
 
+test_expect_success '"git replace" resolves sha1' '
+     SHORTHASH2=$(git rev-parse --short=8 $HASH2) &&
+     git replace -d $SHORTHASH2 &&
+     git replace $SHORTHASH2 $R &&
+     git show $HASH2 | grep "O Thor" &&
+     test_must_fail git replace $HASH2 $R &&
+     git replace -f $HASH2 $R &&
+     test_must_fail git replace -f &&
+     test "$HASH2" = "$(git replace)"
+'
+
 # This creates a side branch where the bug in H2
 # does not appear because P2 is created by applying
 # H2 and squashing H5 into it.

-- 
1.8.0.370.g8cbad08
Jeff King· Nov 9, 2012, 16:48 UTC · re: Michael J Gruber · lore

Re: [PATCHv2] replace: parse revision argument for -d

On Mon, Oct 29, 2012 at 02:23:27PM +0100, Michael J Gruber wrote:
Show 10 quoted lines
> 'git replace' parses the revision arguments when it creates replacements
> (so that a sha1 can be abbreviated, e.g.) but not when deleting
> replacements.
> 
> Make it parse the arguments to 'replace -d' in the same way.
> 
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
> v2 has the simplified error check as per Jeff, and a reworded message.
> Comes with a free test case, too.

I noticed this today in my pile of "to look at" patches. Sorry for being slow.

Show 10 quoted lines
>  	for (p = argv; *p; p++) {
> -		if (snprintf(ref, sizeof(ref), "refs/replace/%s", *p)
> -					>= sizeof(ref)) {
> -			error("replace ref name too long: %.*s...", 50, *p);
> +		q = *p;
> +		if (get_sha1(q, sha1)) {
> +			error("Failed to resolve '%s' as a valid ref.", q);
>  			had_error = 1;
>  			continue;
>  		}
Looks reasonable.
Show 5 quoted lines
> +		q = sha1_to_hex(sha1);
> +		snprintf(ref, sizeof(ref), "refs/replace/%s", q);
>  		if (read_ref(ref, sha1)) {
> -			error("replace ref '%s' not found.", *p);
> +			error("replace ref '%s' not found.", q);

I worry a little about assuming that "q", which points to a static internal buffer of sha1_to_hex, is still valid after calling read_ref. We'll end up in resolve_ref, which might need to do considerable work (e.g., loading the whole packed refs file). Just grepping for sha1_to_hex, I don't think it is a problem currently, but it might be worth copying the value (you could even point into the "ref" buffer to avoid dealing with an extra allocation).

Other than that, it looks good to me.
-Peff
Michael J Gruber· Nov 12, 2012, 08:51 UTC · re: Jeff King · lore

Re: [PATCHv2] replace: parse revision argument for -d

Jeff King venit, vidit, dixit 09.11.2012 17:48:
Show 15 quoted lines
> On Mon, Oct 29, 2012 at 02:23:27PM +0100, Michael J Gruber wrote:
> 
>> 'git replace' parses the revision arguments when it creates replacements
>> (so that a sha1 can be abbreviated, e.g.) but not when deleting
>> replacements.
>>
>> Make it parse the arguments to 'replace -d' in the same way.
>>
>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
>> ---
>> v2 has the simplified error check as per Jeff, and a reworded message.
>> Comes with a free test case, too.
> 
> I noticed this today in my pile of "to look at" patches. Sorry for being
> slow.

No problem. This is not urgent, and it takes some effort to look at code amongst all those black-and-white discussions. [It even takes effort to refrain from responding when you words are being twisted around...]

Show 26 quoted lines
>>  	for (p = argv; *p; p++) {
>> -		if (snprintf(ref, sizeof(ref), "refs/replace/%s", *p)
>> -					>= sizeof(ref)) {
>> -			error("replace ref name too long: %.*s...", 50, *p);
>> +		q = *p;
>> +		if (get_sha1(q, sha1)) {
>> +			error("Failed to resolve '%s' as a valid ref.", q);
>>  			had_error = 1;
>>  			continue;
>>  		}
> 
> Looks reasonable.
> 
>> +		q = sha1_to_hex(sha1);
>> +		snprintf(ref, sizeof(ref), "refs/replace/%s", q);
>>  		if (read_ref(ref, sha1)) {
>> -			error("replace ref '%s' not found.", *p);
>> +			error("replace ref '%s' not found.", q);
> 
> I worry a little about assuming that "q", which points to a static
> internal buffer of sha1_to_hex, is still valid after calling read_ref.
> We'll end up in resolve_ref, which might need to do considerable work
> (e.g., loading the whole packed refs file). Just grepping for
> sha1_to_hex, I don't think it is a problem currently, but it might be
> worth copying the value (you could even point into the "ref" buffer to
> avoid dealing with an extra allocation).

We could just leave '*p' as it is (after all, that was the user input), or use 'ref+strlen("refs/replace/")'.

I wasn't aware of the volatile nature of the return value. Thanks for spotting!

Michael
Michael J Gruber· Nov 12, 2012, 14:18 UTC · re: Michael J Gruber · lore

[PATCHv3] replace: parse revision argument for -d

'git replace' parses the revision arguments when it creates replacements (so that a sha1 can be abbreviated, e.g.) but not when deleting replacements.

Make it parse the argument to 'replace -d' in the same way.
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
Notes:
    v3 safeguards the hex buffer against reuse
 builtin/replace.c  | 16 ++++++++++------
 t/t6050-replace.sh | 11 +++++++++++
 2 files changed, 21 insertions(+), 6 deletions(-)
Show changes to 2 files +21 −6

builtin/replace.c, t/t6050-replace.sh

diff --git a/builtin/replace.c b/builtin/replace.c
index e3aaf70..33e6ec3 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -46,24 +46,28 @@ typedef int (*each_replace_name_fn)(const char *name, const char *ref,
 
 static int for_each_replace_name(const char **argv, each_replace_name_fn fn)
 {
-	const char **p;
+	const char **p, *q;
 	char ref[PATH_MAX];
 	int had_error = 0;
 	unsigned char sha1[20];
 
 	for (p = argv; *p; p++) {
-		if (snprintf(ref, sizeof(ref), "refs/replace/%s", *p)
-					>= sizeof(ref)) {
-			error("replace ref name too long: %.*s...", 50, *p);
+		q = *p;
+		if (get_sha1(q, sha1)) {
+			error("Failed to resolve '%s' as a valid ref.", q);
 			had_error = 1;
 			continue;
 		}
+		q = sha1_to_hex(sha1);
+		snprintf(ref, sizeof(ref), "refs/replace/%s", q);
+		/* read_ref() may reuse the buffer */
+		q = ref + strlen("refs/replace/");
 		if (read_ref(ref, sha1)) {
-			error("replace ref '%s' not found.", *p);
+			error("replace ref '%s' not found.", q);
 			had_error = 1;
 			continue;
 		}
-		if (fn(*p, ref, sha1))
+		if (fn(q, ref, sha1))
 			had_error = 1;
 	}
 	return had_error;
diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index 5c87f28..decdc33 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -140,6 +140,17 @@ test_expect_success '"git replace" replacing' '
      test "$HASH2" = "$(git replace)"
 '
 
+test_expect_success '"git replace" resolves sha1' '
+     SHORTHASH2=$(git rev-parse --short=8 $HASH2) &&
+     git replace -d $SHORTHASH2 &&
+     git replace $SHORTHASH2 $R &&
+     git show $HASH2 | grep "O Thor" &&
+     test_must_fail git replace $HASH2 $R &&
+     git replace -f $HASH2 $R &&
+     test_must_fail git replace -f &&
+     test "$HASH2" = "$(git replace)"
+'
+
 # This creates a side branch where the bug in H2
 # does not appear because P2 is created by applying
 # H2 and squashing H5 into it.

-- 
1.8.0.311.gdd08018
Jeff King· Nov 12, 2012, 20:42 UTC · re: Michael J Gruber · lore

Re: [PATCHv3] replace: parse revision argument for -d

On Mon, Nov 12, 2012 at 03:18:02PM +0100, Michael J Gruber wrote:
Show 11 quoted lines
> 'git replace' parses the revision arguments when it creates replacements
> (so that a sha1 can be abbreviated, e.g.) but not when deleting
> replacements.
> 
> Make it parse the argument to 'replace -d' in the same way.
> 
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
> 
> Notes:
>     v3 safeguards the hex buffer against reuse
Thanks, I don't see any other functional problems.
Show 10 quoted lines
> diff --git a/builtin/replace.c b/builtin/replace.c
> index e3aaf70..33e6ec3 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -46,24 +46,28 @@ typedef int (*each_replace_name_fn)(const char *name, const char *ref,
>  
>  static int for_each_replace_name(const char **argv, each_replace_name_fn fn)
>  {
> -	const char **p;
> +	const char **p, *q;

I find this readable today, but I wonder if in six months we will wonder what in the world "q" means. Maybe "short_refname" or something would be appropriate?

-Peff
Michael J Gruber· Nov 13, 2012, 10:25 UTC · re: Jeff King · lore

Re: [PATCHv3] replace: parse revision argument for -d

Jeff King venit, vidit, dixit 12.11.2012 21:42:
Show 30 quoted lines
> On Mon, Nov 12, 2012 at 03:18:02PM +0100, Michael J Gruber wrote:
> 
>> 'git replace' parses the revision arguments when it creates replacements
>> (so that a sha1 can be abbreviated, e.g.) but not when deleting
>> replacements.
>>
>> Make it parse the argument to 'replace -d' in the same way.
>>
>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
>> ---
>>
>> Notes:
>>     v3 safeguards the hex buffer against reuse
> 
> Thanks, I don't see any other functional problems.
> 
>> diff --git a/builtin/replace.c b/builtin/replace.c
>> index e3aaf70..33e6ec3 100644
>> --- a/builtin/replace.c
>> +++ b/builtin/replace.c
>> @@ -46,24 +46,28 @@ typedef int (*each_replace_name_fn)(const char *name, const char *ref,
>>  
>>  static int for_each_replace_name(const char **argv, each_replace_name_fn fn)
>>  {
>> -	const char **p;
>> +	const char **p, *q;
> 
> I find this readable today, but I wonder if in six months we will wonder
> what in the world "q" means. Maybe "short_refname" or something would be
> appropriate?
That would be sooo inappropriate! ;)
Maybe "full_hex"?

I should also do away with the first replacement which really made sense in v1 only.

v4 to follow.
Michael
Michael J Gruber· Nov 13, 2012, 10:34 UTC · re: Michael J Gruber · lore

[PATCHv4] replace: parse revision argument for -d

'git replace' parses the revision arguments when it creates replacements (so that a sha1 can be abbreviated, e.g.) but not when deleting replacements.

Make it parse the argument to 'replace -d' in the same way.
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
Notes:
    v4 names the aux variable more concisely and does away with a superfluous
    assignment.
 builtin/replace.c  | 15 +++++++++------
 t/t6050-replace.sh | 11 +++++++++++
 2 files changed, 20 insertions(+), 6 deletions(-)
Show changes to 2 files +20 −6

builtin/replace.c, t/t6050-replace.sh

diff --git a/builtin/replace.c b/builtin/replace.c
index e3aaf70..398ccd5 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -46,24 +46,27 @@ typedef int (*each_replace_name_fn)(const char *name, const char *ref,
 
 static int for_each_replace_name(const char **argv, each_replace_name_fn fn)
 {
-	const char **p;
+	const char **p, *full_hex;
 	char ref[PATH_MAX];
 	int had_error = 0;
 	unsigned char sha1[20];
 
 	for (p = argv; *p; p++) {
-		if (snprintf(ref, sizeof(ref), "refs/replace/%s", *p)
-					>= sizeof(ref)) {
-			error("replace ref name too long: %.*s...", 50, *p);
+		if (get_sha1(*p, sha1)) {
+			error("Failed to resolve '%s' as a valid ref.", *p);
 			had_error = 1;
 			continue;
 		}
+		full_hex = sha1_to_hex(sha1);
+		snprintf(ref, sizeof(ref), "refs/replace/%s", full_hex);
+		/* read_ref() may reuse the buffer */
+		full_hex = ref + strlen("refs/replace/");
 		if (read_ref(ref, sha1)) {
-			error("replace ref '%s' not found.", *p);
+			error("replace ref '%s' not found.", full_hex);
 			had_error = 1;
 			continue;
 		}
-		if (fn(*p, ref, sha1))
+		if (fn(full_hex, ref, sha1))
 			had_error = 1;
 	}
 	return had_error;
diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index 5c87f28..decdc33 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -140,6 +140,17 @@ test_expect_success '"git replace" replacing' '
      test "$HASH2" = "$(git replace)"
 '
 
+test_expect_success '"git replace" resolves sha1' '
+     SHORTHASH2=$(git rev-parse --short=8 $HASH2) &&
+     git replace -d $SHORTHASH2 &&
+     git replace $SHORTHASH2 $R &&
+     git show $HASH2 | grep "O Thor" &&
+     test_must_fail git replace $HASH2 $R &&
+     git replace -f $HASH2 $R &&
+     test_must_fail git replace -f &&
+     test "$HASH2" = "$(git replace)"
+'
+
 # This creates a side branch where the bug in H2
 # does not appear because P2 is created by applying
 # H2 and squashing H5 into it.

-- 
1.8.0.311.gdd08018
Jeff King· Nov 13, 2012, 13:37 UTC · re: Michael J Gruber · lore

Re: [PATCHv4] replace: parse revision argument for -d

On Tue, Nov 13, 2012 at 11:34:11AM +0100, Michael J Gruber wrote:
Show 12 quoted lines
> 'git replace' parses the revision arguments when it creates replacements
> (so that a sha1 can be abbreviated, e.g.) but not when deleting
> replacements.
> 
> Make it parse the argument to 'replace -d' in the same way.
> 
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
> 
> Notes:
>     v4 names the aux variable more concisely and does away with a superfluous
>     assignment.

Thanks. I agree the name "full_hex" is much better. Patch looks good to me at this point.

-Peff
Michael J Gruber· Nov 13, 2012, 10:30 UTC · re: Michael J Gruber · lore

Notes in format-patch (was: Re: [PATCHv3] replace: parse revision argument for -d)

Michael J Gruber venit, vidit, dixit 12.11.2012 15:18:
Show 16 quoted lines
> 'git replace' parses the revision arguments when it creates replacements
> (so that a sha1 can be abbreviated, e.g.) but not when deleting
> replacements.
> 
> Make it parse the argument to 'replace -d' in the same way.
> 
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
> 
> Notes:
>     v3 safeguards the hex buffer against reuse
>  builtin/replace.c  | 16 ++++++++++------
>  t/t6050-replace.sh | 11 +++++++++++
>  2 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/builtin/replace.c b/builtin/replace.c

By the way - Junio, is that the intented outcome of "format-patch --notes"? I would rather put the newline between the note and the diffstat (and omit the one after the ---) but may have goofed up a rebase:

...
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
Notes:
    v3 safeguards the hex buffer against reuse
 builtin/replace.c  | 16 ++++++++++------
...
Jeff King· Nov 13, 2012, 13:38 UTC · re: Michael J Gruber · lore

Re: Notes in format-patch (was: Re: [PATCHv3] replace: parse revision argument for -d)

On Tue, Nov 13, 2012 at 11:30:19AM +0100, Michael J Gruber wrote:
Show 21 quoted lines
> Michael J Gruber venit, vidit, dixit 12.11.2012 15:18:
> > 'git replace' parses the revision arguments when it creates replacements
> > (so that a sha1 can be abbreviated, e.g.) but not when deleting
> > replacements.
> > 
> > Make it parse the argument to 'replace -d' in the same way.
> > 
> > Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> > ---
> > 
> > Notes:
> >     v3 safeguards the hex buffer against reuse
> >  builtin/replace.c  | 16 ++++++++++------
> >  t/t6050-replace.sh | 11 +++++++++++
> >  2 files changed, 21 insertions(+), 6 deletions(-)
> > 
> > diff --git a/builtin/replace.c b/builtin/replace.c
> 
> By the way - Junio, is that the intented outcome of "format-patch
> --notes"? I would rather put the newline between the note and the
> diffstat (and omit the one after the ---) but may have goofed up a rebase:

I do not know was intended, but the above quoted output is hard to read, and your suggested change looks way better.

-Peff
Junio C Hamano· Nov 13, 2012, 16:29 UTC · re: Michael J Gruber · lore

Re: Notes in format-patch

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 21 quoted lines
> Michael J Gruber venit, vidit, dixit 12.11.2012 15:18:
>> 'git replace' parses the revision arguments when it creates replacements
>> (so that a sha1 can be abbreviated, e.g.) but not when deleting
>> replacements.
>> 
>> Make it parse the argument to 'replace -d' in the same way.
>> 
>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
>> ---
>> 
>> Notes:
>>     v3 safeguards the hex buffer against reuse
>>  builtin/replace.c  | 16 ++++++++++------
>>  t/t6050-replace.sh | 11 +++++++++++
>>  2 files changed, 21 insertions(+), 6 deletions(-)
>> 
>> diff --git a/builtin/replace.c b/builtin/replace.c
>
> By the way - Junio, is that the intented outcome of "format-patch
> --notes"? I would rather put the newline between the note and the
> diffstat...

I do not mind (actually I personally would prefer to see) a blank line between the three-dash and "Notes:", but I agree that we should have a blank line before the diffstat block.

Junio C Hamano· Nov 13, 2012, 16:56 UTC · re: Junio C Hamano · lore

Re: Notes in format-patch

Junio C Hamano <gitster@pobox.com> writes:
Show 27 quoted lines
> Michael J Gruber <git@drmicha.warpmail.net> writes:
>
>> Michael J Gruber venit, vidit, dixit 12.11.2012 15:18:
>>> 'git replace' parses the revision arguments when it creates replacements
>>> (so that a sha1 can be abbreviated, e.g.) but not when deleting
>>> replacements.
>>> 
>>> Make it parse the argument to 'replace -d' in the same way.
>>> 
>>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
>>> ---
>>> 
>>> Notes:
>>>     v3 safeguards the hex buffer against reuse
>>>  builtin/replace.c  | 16 ++++++++++------
>>>  t/t6050-replace.sh | 11 +++++++++++
>>>  2 files changed, 21 insertions(+), 6 deletions(-)
>>> 
>>> diff --git a/builtin/replace.c b/builtin/replace.c
>>
>> By the way - Junio, is that the intented outcome of "format-patch
>> --notes"? I would rather put the newline between the note and the
>> diffstat...
>
> I do not mind (actually I personally would prefer to see) a blank
> line between the three-dash and "Notes:", but I agree that we should
> have a blank line before the diffstat block.

As the topic seems to be already in Peff's next, here is a trivial fix for this in incremental form.

-- >8 --
Subject: format-patch: add a blank line between notes and diffstat

The last line of the note text comes immediately before the diffstat block, making the latter unnecessarily harder to view.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 log-tree.c | 1 +
 1 file changed, 1 insertion(+)
Show changes to diff +1 −0
diff --git i/log-tree.c w/log-tree.c
index 712a22b..9303fd8 100644
--- i/log-tree.c
+++ w/log-tree.c
@@ -683,6 +683,7 @@ void show_log(struct rev_info *opt)
 			opt->shown_dashes = 1;
 		}
 		strbuf_addstr(&msgbuf, ctx.notes_message);
+		strbuf_addch(&msgbuf, '\n');
 	}
 
 	if (opt->show_log_size) {
Junio C Hamano· Nov 13, 2012, 17:26 UTC · re: Junio C Hamano · lore

Re: Notes in format-patch

Junio C Hamano <gitster@pobox.com> writes:
Show 26 quoted lines
> As the topic seems to be already in Peff's next, here is a trivial
> fix for this in incremental form.
>
> -- >8 --
> Subject: format-patch: add a blank line between notes and diffstat
>
> The last line of the note text comes immediately before the diffstat
> block, making the latter unnecessarily harder to view.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  log-tree.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git i/log-tree.c w/log-tree.c
> index 712a22b..9303fd8 100644
> --- i/log-tree.c
> +++ w/log-tree.c
> @@ -683,6 +683,7 @@ void show_log(struct rev_info *opt)
>  			opt->shown_dashes = 1;
>  		}
>  		strbuf_addstr(&msgbuf, ctx.notes_message);
> +		strbuf_addch(&msgbuf, '\n');
>  	}
>  
>  	if (opt->show_log_size) {
... and it is broken X-<.

The blank line should be added before the diffstat, not after the notes message (t3307 shows a case where we give notes without diffstat, and we shouldn't be adding an extra blank line in that case.

Junio C Hamano· Nov 13, 2012, 18:09 UTC · re: Junio C Hamano · lore

Re: Notes in format-patch

Junio C Hamano <gitster@pobox.com> writes:
Show 6 quoted lines
> ... and it is broken X-<.
>
> The blank line should be added before the diffstat, not after the
> notes message (t3307 shows a case where we give notes without
> diffstat, and we shouldn't be adding an extra blank line in that
> case.
Second try.
-- >8 --
Subject: format-patch: add a blank line between notes and diffstat

The last line of the note text comes immediately before the diffstat block, making the latter unnecessarily harder to view.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 log-tree.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)
Show changes to diff +21 −10
diff --git c/log-tree.c w/log-tree.c
index 712a22b..4f86def 100644
--- c/log-tree.c
+++ w/log-tree.c
@@ -727,15 +727,16 @@ int log_tree_diff_flush(struct rev_info *opt)
 	}
 
 	if (opt->loginfo && !opt->no_commit_id) {
-		/* When showing a verbose header (i.e. log message),
-		 * and not in --pretty=oneline format, we would want
-		 * an extra newline between the end of log and the
-		 * output for readability.
-		 */
 		show_log(opt);
 		if ((opt->diffopt.output_format & ~DIFF_FORMAT_NO_OUTPUT) &&
 		    opt->verbose_header &&
 		    opt->commit_format != CMIT_FMT_ONELINE) {
+			/*
+			 * When showing a verbose header (i.e. log message),
+			 * and not in --pretty=oneline format, we would want
+			 * an extra newline between the end of log and the
+			 * diff/diffstat output for readability.
+			 */
 			int pch = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_PATCH;
 			if (opt->diffopt.output_prefix) {
 				struct strbuf *msg = NULL;
@@ -743,11 +744,21 @@ int log_tree_diff_flush(struct rev_info *opt)
 					opt->diffopt.output_prefix_data);
 				fwrite(msg->buf, msg->len, 1, stdout);
 			}
-			if (!opt->shown_dashes) {
-				if ((pch & opt->diffopt.output_format) == pch)
-					printf("---");
-				putchar('\n');
-			}
+
+			/*
+			 * We may have shown three-dashes line early
+			 * between notes and the log message, in which
+			 * case we only want a blank line after the
+			 * notes without (an extra) three-dashes line.
+			 * Otherwise, we show the three-dashes line if
+			 * we are showing the patch with diffstat, but
+			 * in that case, there is no extra blank line
+			 * after the three-dashes line.
+			 */
+			if (!opt->shown_dashes &&
+			    (pch & opt->diffopt.output_format) == pch)
+				printf("---");
+			putchar('\n');
 		}
 	}
 	diff_flush(&opt->diffopt);
Michael J Gruber· Nov 14, 2012, 09:17 UTC · re: Junio C Hamano · lore

Re: Notes in format-patch

Junio C Hamano venit, vidit, dixit 13.11.2012 19:09:
Show 22 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> ... and it is broken X-<.
>>
>> The blank line should be added before the diffstat, not after the
>> notes message (t3307 shows a case where we give notes without
>> diffstat, and we shouldn't be adding an extra blank line in that
>> case.
> 
> Second try.
> 
> -- >8 --
> Subject: format-patch: add a blank line between notes and diffstat
> 
> The last line of the note text comes immediately before the diffstat
> block, making the latter unnecessarily harder to view.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> 
>  log-tree.c | 31 +++++++++++++++++++++----------
>  1 file changed, 21 insertions(+), 10 deletions(-)

Thanks, that patch works. I'm curious, though, where the empty line between the --- and your diffstat comes from. Do you have an empty note? I'm not getting any (origin/next+your patch).

The fact that we don't usually have that empty line was the reason why I preferred to have no empty line between the --- and the "Note:".

Michael
Junio C Hamano· Nov 14, 2012, 13:15 UTC · re: Michael J Gruber · lore

Re: Notes in format-patch

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 26 quoted lines
> Junio C Hamano venit, vidit, dixit 13.11.2012 19:09:
>> Junio C Hamano <gitster@pobox.com> writes:
>> 
>>> ... and it is broken X-<.
>>>
>>> The blank line should be added before the diffstat, not after the
>>> notes message (t3307 shows a case where we give notes without
>>> diffstat, and we shouldn't be adding an extra blank line in that
>>> case.
>> 
>> Second try.
>> 
>> -- >8 --
>> Subject: format-patch: add a blank line between notes and diffstat
>> 
>> The last line of the note text comes immediately before the diffstat
>> block, making the latter unnecessarily harder to view.
>> 
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>> 
>>  log-tree.c | 31 +++++++++++++++++++++----------
>>  1 file changed, 21 insertions(+), 10 deletions(-)
>
> Thanks, that patch works. I'm curious, though, where the empty line
> between the --- and your diffstat comes from.

The message you are responding to is *not* an output from format-patch but was written in my MUA.

The way I work when I show "this should work" patch is to:
 (1) Think, edit in my working tree, compile, eyeball "git diff HEAD",
     think again, and test;
 (2) Hit "Reply All" to the message I am going to give "this should
     work" response to, and start composing the response;
 (3) Run "git diff --stat -p HEAD" to have its output appended at
     the end of the message I started to compose in the previous
     step;
 (4) Write everything that should come before the output I appended
     in the previous step, i.e. "-- >8 --", in-body "Subject:", log,
     sign-off, and three-dash lines;
 (5) Send it out; and
 (6) Run "git reset --hard" and move on.

The blank line was added in step (4), not step (3), which does not even have any commit log message, as the patch does not come from any existing commit. Later I may pick it up and apply to a topic branch just like I do for patches from other people and that is the point when such a patch becomes a commit for the first time.

← back to recent threads