[PATCH] replace: parse revision argument for -d

[PATCH] replace: parse revision argument for -d

[Michael J Gruber]

'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(-)

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

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

[Christian Couder]

Hi,

On Fri, Oct 26, 2012 at 3:33 PM, Michael J Gruber
<git@drmicha.warpmail.net> 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.
>
> 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.

> 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.

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

[Jeff King]

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

-Peff

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

[Michael J Gruber]

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?

Michael

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

[Jeff King]

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.

-Peff

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

[Michael J Gruber]

Jeff King venit, vidit, dixit 29.10.2012 10:04:
> 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

[PATCHv2] replace: parse revision argument for -d

[Michael J Gruber]

'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(-)

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

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

[Jeff King]

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.

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

Other than that, it looks good to me.

-Peff

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

[Michael J Gruber]

Jeff King venit, vidit, dixit 09.11.2012 17:48:
> 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...]

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

[PATCHv3] replace: parse revision argument for -d

[Michael J Gruber]

'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
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

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

[Jeff King]

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?

-Peff

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

[Michael J Gruber]

Jeff King venit, vidit, dixit 12.11.2012 21:42:
> 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

[PATCHv4] replace: parse revision argument for -d

[Michael J Gruber]

'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(-)

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

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

[Jeff King]

On Tue, Nov 13, 2012 at 11:34:11AM +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:
>     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

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

[Michael J Gruber]

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:

...

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
Notes:
    v3 safeguards the hex buffer against reuse

 builtin/replace.c  | 16 ++++++++++------
...

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

[Jeff King]

On Tue, Nov 13, 2012 at 11:30:19AM +0100, Michael J Gruber wrote:

> 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

Re: Notes in format-patch

[Junio C Hamano]

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.

Re: Notes in format-patch

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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(+)

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

Re: Notes in format-patch

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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.

Re: Notes in format-patch

[Junio C Hamano]

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

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

Re: Notes in format-patch

[Michael J Gruber]

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. 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

Re: Notes in format-patch

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> 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.