All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] use strbuf_addstr() instead of strbuf_addf() with "%s", part 2
@ 2016-09-27 19:08 René Scharfe
  2016-09-27 19:11 ` [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, " René Scharfe
  0 siblings, 1 reply; 6+ messages in thread
From: René Scharfe @ 2016-09-27 19:08 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano

Replace uses of strbuf_addf() for adding strings with more lightweight
strbuf_addstr() calls.  This is shorter and makes the intent clearer.

bc57b9c0cc5a123365a922fa1831177e3fd607ed already converted three cases,
this patch covers two more.

A semantic patch for Coccinelle is included for easier checking for
new cases that might be introduced in the future.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/submodule--helper.c     | 2 +-
 contrib/coccinelle/strbuf.cocci | 6 ++++++
 submodule.c                     | 2 +-
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index e3fdc0a..444ec06 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -753,7 +753,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 		if (suc->recursive_prefix)
 			strbuf_addf(&sb, "%s/%s", suc->recursive_prefix, ce->name);
 		else
-			strbuf_addf(&sb, "%s", ce->name);
+			strbuf_addstr(&sb, ce->name);
 		strbuf_addf(out, _("Skipping unmerged submodule %s"), sb.buf);
 		strbuf_addch(out, '\n');
 		goto cleanup;
diff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci
index 7932d48..4b7553f 100644
--- a/contrib/coccinelle/strbuf.cocci
+++ b/contrib/coccinelle/strbuf.cocci
@@ -3,3 +3,9 @@ expression E1, E2;
 @@
 - strbuf_addf(E1, E2);
 + strbuf_addstr(E1, E2);
+
+@@
+expression E1, E2;
+@@
+- strbuf_addf(E1, "%s", E2);
++ strbuf_addstr(E1, E2);
diff --git a/submodule.c b/submodule.c
index 0ef2ff4..dcc5ce3 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(&sb, '.');
-	strbuf_addf(&sb, "%s", find_unique_abbrev(two->hash, DEFAULT_ABBREV));
+	strbuf_addstr(&sb, find_unique_abbrev(two->hash, DEFAULT_ABBREV));
 	if (message)
 		strbuf_addf(&sb, " %s%s\n", message, reset);
 	else
-- 
2.10.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, part 2
  2016-09-27 19:08 [PATCH 1/2] use strbuf_addstr() instead of strbuf_addf() with "%s", part 2 René Scharfe
@ 2016-09-27 19:11 ` René Scharfe
  2016-09-27 20:28   ` Junio C Hamano
  2016-10-07  0:46   ` Jeff King
  0 siblings, 2 replies; 6+ messages in thread
From: René Scharfe @ 2016-09-27 19:11 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano

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.

1eb47f167d65d1d305b9c196a1bb40eb96117cb1 already converted six cases,
this patch covers three more.

A semantic patch for Coccinelle is included for easier checking for
new cases that might be introduced in the future.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 contrib/coccinelle/strbuf.cocci | 6 ++++++
 diff.c                          | 2 +-
 submodule.c                     | 2 +-
 wt-status.c                     | 3 +--
 4 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci
index 4b7553f..1e24298 100644
--- a/contrib/coccinelle/strbuf.cocci
+++ b/contrib/coccinelle/strbuf.cocci
@@ -9,3 +9,9 @@ expression E1, E2;
 @@
 - strbuf_addf(E1, "%s", E2);
 + strbuf_addstr(E1, E2);
+
+@@
+expression E1, E2, E3;
+@@
+- strbuf_addstr(E1, find_unique_abbrev(E2, E3));
++ strbuf_add_unique_abbrev(E1, E2, E3);
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);
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(&sb, '.');
-	strbuf_addstr(&sb, find_unique_abbrev(two->hash, DEFAULT_ABBREV));
+	strbuf_add_unique_abbrev(&sb->hash, two, DEFAULT_ABBREV);
 	if (message)
 		strbuf_addf(&sb, " %s%s\n", message, reset);
 	else
diff --git a/wt-status.c b/wt-status.c
index 9628c1d..99d1b0a 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1383,8 +1383,7 @@ static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1,
 	if (!strcmp(cb->buf.buf, "HEAD")) {
 		/* HEAD is relative. Resolve it to the right reflog entry. */
 		strbuf_reset(&cb->buf);
-		strbuf_addstr(&cb->buf,
-			      find_unique_abbrev(nsha1, DEFAULT_ABBREV));
+		strbuf_add_unique_abbrev(&cb->buf, nsha1, DEFAULT_ABBREV);
 	}
 	return 1;
 }
-- 
2.10.0



^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, part 2
  2016-09-27 19:11 ` [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, " René Scharfe
@ 2016-09-27 20:28   ` Junio C Hamano
  2016-09-27 20:59     ` René Scharfe
  2016-10-07  0:46   ` Jeff King
  1 sibling, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2016-09-27 20:28 UTC (permalink / raw)
  To: René Scharfe; +Cc: Git List

René Scharfe <l.s.r@web.de> 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(&sb, '.');
> -	strbuf_addstr(&sb, find_unique_abbrev(two->hash, DEFAULT_ABBREV));
> +	strbuf_add_unique_abbrev(&sb->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);


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, part 2
  2016-09-27 20:28   ` Junio C Hamano
@ 2016-09-27 20:59     ` René Scharfe
  0 siblings, 0 replies; 6+ messages in thread
From: René Scharfe @ 2016-09-27 20:59 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git List

Am 27.09.2016 um 22:28 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> 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(&sb, '.');
>> -	strbuf_addstr(&sb, find_unique_abbrev(two->hash, DEFAULT_ABBREV));
>> +	strbuf_add_unique_abbrev(&sb->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(&sb, '.');
-	strbuf_add_unique_abbrev(&sb->hash, two, DEFAULT_ABBREV);
+	strbuf_add_unique_abbrev(&sb, two->hash, DEFAULT_ABBREV);
 	if (message)
 		strbuf_addf(&sb, " %s%s\n", message, reset);
 	else
-- 
2.10.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, part 2
  2016-09-27 19:11 ` [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, " René Scharfe
  2016-09-27 20:28   ` Junio C Hamano
@ 2016-10-07  0:46   ` Jeff King
  2016-10-07 20:45     ` René Scharfe
  1 sibling, 1 reply; 6+ messages in thread
From: Jeff King @ 2016-10-07  0:46 UTC (permalink / raw)
  To: René Scharfe; +Cc: Git List, Junio C Hamano

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(&sb, '.');
> -	strbuf_addstr(&sb, find_unique_abbrev(two->hash, DEFAULT_ABBREV));
> +	strbuf_add_unique_abbrev(&sb->hash, two, DEFAULT_ABBREV);

This one is a similar situation, I think.

-Peff

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, part 2
  2016-10-07  0:46   ` Jeff King
@ 2016-10-07 20:45     ` René Scharfe
  0 siblings, 0 replies; 6+ messages in thread
From: René Scharfe @ 2016-10-07 20:45 UTC (permalink / raw)
  To: Jeff King; +Cc: Git List, Junio C Hamano

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(&sb, '.');
>> -	strbuf_addstr(&sb, find_unique_abbrev(two->hash, DEFAULT_ABBREV));
>> +	strbuf_add_unique_abbrev(&sb->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é

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-10-07 20:45 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-27 19:08 [PATCH 1/2] use strbuf_addstr() instead of strbuf_addf() with "%s", part 2 René Scharfe
2016-09-27 19:11 ` [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, " René Scharfe
2016-09-27 20:28   ` Junio C Hamano
2016-09-27 20:59     ` René Scharfe
2016-10-07  0:46   ` Jeff King
2016-10-07 20:45     ` René Scharfe

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.