Re: [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, part 2

[René Scharfe]

Am 07.10.2016 um 02:46 schrieb Jeff King:

On Tue, Sep 27, 2016 at 09:11:58PM +0200, René Scharfe wrote:


Call strbuf_add_unique_abbrev() to add abbreviated hashes to strbufs
instead of taking detours through find_unique_abbrev() and its static
buffer.  This is shorter and a bit more efficient.
[...]
diff --git a/diff.c b/diff.c
index a178ed3..be11e4e 100644
--- a/diff.c
+++ b/diff.c
@@ -3109,7 +3109,7 @@ static void fill_metainfo(struct strbuf *msg,
}
strbuf_addf(msg, "%s%sindex %s..", line_prefix, set,
find_unique_abbrev(one->oid.hash, abbrev));
-   strbuf_addstr(msg, find_unique_abbrev(two->oid.hash, abbrev));
+   strbuf_add_unique_abbrev(msg, two->oid.hash, abbrev);
if (one->mode == two->mode)
strbuf_addf(msg, " %06o", one->mode);
strbuf_addf(msg, "%s\n", reset);


This one is an interesting case, and maybe a good example of why blind
coccinelle usage can have some pitfalls. :)


Thank you for paying attention. :)  In general I agree that the 
surrounding code of such changes should be checked; the issue at hand 
could be part of a bigger problem.



We get rid of the strbuf_addstr(), but notice that we leave untouched
the find_unique_abbrev() call immediately above. There was a symmetry to
the two that has been lost.

Probably either:

  strbuf_addf(msg, "%s%sindex %s..%s", line_prefix, set
find_unique_abbrev(one->oid.hash, abbrev),
find_unique_abbrev(two->oid.hash, abbrev));

or:

  strbuf_addf(msg, "%s%sindex ", line_prefix, set);
  strbuf_add_unique_abbrev(msg, one->oid.hash, abbrev);
  strbuf_addstr(msg, "..");
  strbuf_add_unique_abbrev(msg, two->oid.hash, abbrev);

would be a more appropriate refactoring. The problem is in the original
patch (which also lacks symmetry; either this predates the multi-buffer
find_unique_abbrev, or the original author didn't know about it), but I
think your refactor makes it slightly worse.


I still think the automatically generated patch is a net win, but we 
shouldn't stop there.



I noticed because I have another series which touches these lines, and
it wants to symmetrically swap out find_unique_abbrev for something
else. :) I don't think it's a big enough deal to switch now (and I've
already rebased my series which will touch these lines), but I wanted to
mention it as a thing to watch out for as we do more of these kinds of
automated transformations.


OK, then I'll wait for that series to land.


--- a/submodule.c
+++ b/submodule.c
@@ -396,7 +396,7 @@ static void show_submodule_header(FILE *f, const char *path,
find_unique_abbrev(one->hash, DEFAULT_ABBREV));
if (!fast_backward && !fast_forward)
strbuf_addch(, '.');
-   strbuf_addstr(, find_unique_abbrev(two->hash, DEFAULT_ABBREV));
+   strbuf_add_unique_abbrev(>hash, two, DEFAULT_ABBREV);


This one is a similar situation, I think.


Yes, and there are some more.  Will take a look.

I don't know how to crack printf-style formats using semantic patches. 
It's easy for fixed formats (silly example):


- strbuf_addf(sb, "%s%s", a, b);
+ strbuf_addf(sb, "%s", a);
+ strbuf_addf(sb, "%s", b);

But how to do that for arbitrary formats?  Probably not worth it..

René

Re: [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, part 2

[Jeff King]

On Tue, Sep 27, 2016 at 09:11:58PM +0200, René Scharfe wrote:

> Call strbuf_add_unique_abbrev() to add abbreviated hashes to strbufs
> instead of taking detours through find_unique_abbrev() and its static
> buffer.  This is shorter and a bit more efficient.
> [...]
> diff --git a/diff.c b/diff.c
> index a178ed3..be11e4e 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3109,7 +3109,7 @@ static void fill_metainfo(struct strbuf *msg,
>   }
>   strbuf_addf(msg, "%s%sindex %s..", line_prefix, set,
>   find_unique_abbrev(one->oid.hash, abbrev));
> - strbuf_addstr(msg, find_unique_abbrev(two->oid.hash, abbrev));
> + strbuf_add_unique_abbrev(msg, two->oid.hash, abbrev);
>   if (one->mode == two->mode)
>   strbuf_addf(msg, " %06o", one->mode);
>   strbuf_addf(msg, "%s\n", reset);

This one is an interesting case, and maybe a good example of why blind
coccinelle usage can have some pitfalls. :)

We get rid of the strbuf_addstr(), but notice that we leave untouched
the find_unique_abbrev() call immediately above. There was a symmetry to
the two that has been lost.

Probably either:

  strbuf_addf(msg, "%s%sindex %s..%s", line_prefix, set
find_unique_abbrev(one->oid.hash, abbrev),
find_unique_abbrev(two->oid.hash, abbrev));

or:

  strbuf_addf(msg, "%s%sindex ", line_prefix, set);
  strbuf_add_unique_abbrev(msg, one->oid.hash, abbrev);
  strbuf_addstr(msg, "..");
  strbuf_add_unique_abbrev(msg, two->oid.hash, abbrev);

would be a more appropriate refactoring. The problem is in the original
patch (which also lacks symmetry; either this predates the multi-buffer
find_unique_abbrev, or the original author didn't know about it), but I
think your refactor makes it slightly worse.

I noticed because I have another series which touches these lines, and
it wants to symmetrically swap out find_unique_abbrev for something
else. :) I don't think it's a big enough deal to switch now (and I've
already rebased my series which will touch these lines), but I wanted to
mention it as a thing to watch out for as we do more of these kinds of
automated transformations.

> --- a/submodule.c
> +++ b/submodule.c
> @@ -396,7 +396,7 @@ static void show_submodule_header(FILE *f, const char 
> *path,
>   find_unique_abbrev(one->hash, DEFAULT_ABBREV));
>   if (!fast_backward && !fast_forward)
>   strbuf_addch(, '.');
> - strbuf_addstr(, find_unique_abbrev(two->hash, DEFAULT_ABBREV));
> + strbuf_add_unique_abbrev(>hash, two, DEFAULT_ABBREV);

This one is a similar situation, I think.

-Peff

Re: [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, part 2

[René Scharfe]

Am 27.09.2016 um 22:28 schrieb Junio C Hamano:
> René Scharfe  writes:
>> diff --git a/submodule.c b/submodule.c
>> index dcc5ce3..8cf40ea 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -396,7 +396,7 @@ static void show_submodule_header(FILE *f, const char 
>> *path,
>>  find_unique_abbrev(one->hash, DEFAULT_ABBREV));
>>  if (!fast_backward && !fast_forward)
>>  strbuf_addch(, '.');
>> -strbuf_addstr(, find_unique_abbrev(two->hash, DEFAULT_ABBREV));
>> +strbuf_add_unique_abbrev(>hash, two, DEFAULT_ABBREV);
> 
> I wonder how could this change come out of this definition:
> 
> @@
> expression E1, E2, E3;
> @@
> - strbuf_addstr(E1, find_unique_abbrev(E2, E3));
> + strbuf_add_unique_abbrev(E1, E2, E3);

Impossible.  I added "->hash" manually during a rebase (merging
a0d12c44, wrongly).  Good catch, thanks!

Seeing proof of skipping compile-testing I wonder what else I do
forget in my daily life. :-|  I'll better go to sleep now..

Fixup patch, generated by reverting the diff, re-adding the
semantic patch and using coccicheck; compiles and survives make
test:
---
 submodule.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/submodule.c b/submodule.c
index 8cf40ea..bb06b60 100644
--- a/submodule.c
+++ b/submodule.c
@@ -396,7 +396,7 @@ static void show_submodule_header(FILE *f, const char *path,
find_unique_abbrev(one->hash, DEFAULT_ABBREV));
if (!fast_backward && !fast_forward)
strbuf_addch(, '.');
-   strbuf_add_unique_abbrev(>hash, two, DEFAULT_ABBREV);
+   strbuf_add_unique_abbrev(, two->hash, DEFAULT_ABBREV);
if (message)
strbuf_addf(, " %s%s\n", message, reset);
else
-- 
2.10.0

Re: [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, part 2

[Junio C Hamano]

René Scharfe  writes:

> diff --git a/diff.c b/diff.c
> index a178ed3..be11e4e 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3109,7 +3109,7 @@ static void fill_metainfo(struct strbuf *msg,
>   }
>   strbuf_addf(msg, "%s%sindex %s..", line_prefix, set,
>   find_unique_abbrev(one->oid.hash, abbrev));
> - strbuf_addstr(msg, find_unique_abbrev(two->oid.hash, abbrev));
> + strbuf_add_unique_abbrev(msg, two->oid.hash, abbrev);

OK.

> diff --git a/submodule.c b/submodule.c
> index dcc5ce3..8cf40ea 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -396,7 +396,7 @@ static void show_submodule_header(FILE *f, const char 
> *path,
>   find_unique_abbrev(one->hash, DEFAULT_ABBREV));
>   if (!fast_backward && !fast_forward)
>   strbuf_addch(, '.');
> - strbuf_addstr(, find_unique_abbrev(two->hash, DEFAULT_ABBREV));
> + strbuf_add_unique_abbrev(>hash, two, DEFAULT_ABBREV);

I wonder how could this change come out of this definition:

@@
expression E1, E2, E3;
@@
- strbuf_addstr(E1, find_unique_abbrev(E2, E3));
+ strbuf_add_unique_abbrev(E1, E2, E3);