threads / patch / 44094

patchuse strbuf_addstr() for adding constant strings to a strbuf, part 2

Subject: [PATCH] use strbuf_addstr() for adding constant strings to a strbuf, part 2

## tl;dr

10 messages between Sep 15, 2016 and Oct 2, 2016. Diffs are folded; open one to read it.

replies: 9people: 4as markdown or json

René Scharfe· Sep 15, 2016, 18:31 UTC · lore

Replace uses of strbuf_addf() for adding strings with more lightweight strbuf_addstr() calls. This makes the intent clearer and avoids potential issues with printf format specifiers.

02962d36845b89145cd69f8bc65e015d78ae3434 already converted six cases, this patch covers eleven 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>
---
Silly question: Is there a natural language that uses percent signs
as letters or e.g. instead of commas? :)
 builtin/fmt-merge-msg.c         | 2 +-
 builtin/merge.c                 | 2 +-
 builtin/submodule--helper.c     | 5 +++--
 contrib/coccinelle/strbuf.cocci | 5 +++++
 merge-recursive.c               | 2 +-
 remote.c                        | 8 ++++----
 wt-status.c                     | 6 +++---
 7 files changed, 18 insertions(+), 12 deletions(-)
 create mode 100644 contrib/coccinelle/strbuf.cocci
Show changes to 7 files +18 −12

builtin/fmt-merge-msg.c, builtin/merge.c, builtin/submodule--helper.c, contrib/coccinelle/strbuf.cocci, merge-recursive.c, remote.c, wt-status.c

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index ac84e99..dc2e9e4 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -395,7 +395,7 @@ static void shortlog(const char *name,
 
 	for (i = 0; i < subjects.nr; i++)
 		if (i >= limit)
-			strbuf_addf(out, "  ...\n");
+			strbuf_addstr(out, "  ...\n");
 		else
 			strbuf_addf(out, "  %s\n", subjects.items[i].string);
 
diff --git a/builtin/merge.c b/builtin/merge.c
index 0ae099f..a8b57c7 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -940,7 +940,7 @@ static void write_merge_state(struct commit_list *remoteheads)
 
 	strbuf_reset(&buf);
 	if (fast_forward == FF_NO)
-		strbuf_addf(&buf, "no-ff");
+		strbuf_addstr(&buf, "no-ff");
 	write_file_buf(git_path_merge_mode(), buf.buf, buf.len);
 }
 
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 9d79f19..ad23155 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -859,8 +859,9 @@ static int update_clone_get_next_task(struct child_process *child,
 		ce = suc->failed_clones[index];
 		if (!prepare_to_clone_next_submodule(ce, child, suc, err)) {
 			suc->current ++;
-			strbuf_addf(err, "BUG: submodule considered for cloning,"
-				    "doesn't need cloning any more?\n");
+			strbuf_addstr(err, "BUG: submodule considered for "
+					   "cloning, doesn't need cloning "
+					   "any more?\n");
 			return 0;
 		}
 		p = xmalloc(sizeof(*p));
diff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci
new file mode 100644
index 0000000..7932d48
--- /dev/null
+++ b/contrib/coccinelle/strbuf.cocci
@@ -0,0 +1,5 @@
+@@
+expression E1, E2;
+@@
+- strbuf_addf(E1, E2);
++ strbuf_addstr(E1, E2);
diff --git a/merge-recursive.c b/merge-recursive.c
index e349126..d2b191b 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -206,7 +206,7 @@ static void output_commit_title(struct merge_options *o, struct commit *commit)
 			find_unique_abbrev(commit->object.oid.hash,
 				DEFAULT_ABBREV));
 		if (parse_commit(commit) != 0)
-			strbuf_addf(&o->obuf, _("(bad commit)\n"));
+			strbuf_addstr(&o->obuf, _("(bad commit)\n"));
 		else {
 			const char *title;
 			const char *msg = get_commit_buffer(commit, NULL);
diff --git a/remote.c b/remote.c
index d29850a..ad6c542 100644
--- a/remote.c
+++ b/remote.c
@@ -2073,7 +2073,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
 			_("Your branch is based on '%s', but the upstream is gone.\n"),
 			base);
 		if (advice_status_hints)
-			strbuf_addf(sb,
+			strbuf_addstr(sb,
 				_("  (use \"git branch --unset-upstream\" to fixup)\n"));
 	} else if (!ours && !theirs) {
 		strbuf_addf(sb,
@@ -2086,7 +2086,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
 			   ours),
 			base, ours);
 		if (advice_status_hints)
-			strbuf_addf(sb,
+			strbuf_addstr(sb,
 				_("  (use \"git push\" to publish your local commits)\n"));
 	} else if (!ours) {
 		strbuf_addf(sb,
@@ -2097,7 +2097,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
 			   theirs),
 			base, theirs);
 		if (advice_status_hints)
-			strbuf_addf(sb,
+			strbuf_addstr(sb,
 				_("  (use \"git pull\" to update your local branch)\n"));
 	} else {
 		strbuf_addf(sb,
@@ -2110,7 +2110,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
 			   ours + theirs),
 			base, ours, theirs);
 		if (advice_status_hints)
-			strbuf_addf(sb,
+			strbuf_addstr(sb,
 				_("  (use \"git pull\" to merge the remote branch into yours)\n"));
 	}
 	free(base);
diff --git a/wt-status.c b/wt-status.c
index 539aac1..f928f0f 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -367,11 +367,11 @@ static void wt_longstatus_print_change_data(struct wt_status *s,
 		if (d->new_submodule_commits || d->dirty_submodule) {
 			strbuf_addstr(&extra, " (");
 			if (d->new_submodule_commits)
-				strbuf_addf(&extra, _("new commits, "));
+				strbuf_addstr(&extra, _("new commits, "));
 			if (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
-				strbuf_addf(&extra, _("modified content, "));
+				strbuf_addstr(&extra, _("modified content, "));
 			if (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
-				strbuf_addf(&extra, _("untracked content, "));
+				strbuf_addstr(&extra, _("untracked content, "));
 			strbuf_setlen(&extra, extra.len - 2);
 			strbuf_addch(&extra, ')');
 		}

-- 
2.10.0
Jeff King· Sep 15, 2016, 18:44 UTC · re: René Scharfe · lore

Re: [PATCH] use strbuf_addstr() for adding constant strings to a strbuf, part 2

On Thu, Sep 15, 2016 at 08:31:00PM +0200, René Scharfe wrote:
Show 6 quoted lines
> Replace uses of strbuf_addf() for adding strings with more lightweight
> strbuf_addstr() calls.  This makes the intent clearer and avoids
> potential issues with printf format specifiers.
> 
> 02962d36845b89145cd69f8bc65e015d78ae3434 already converted six cases,
> this patch covers eleven more.
Great, these all look obviously correct.
> A semantic patch for Coccinelle is included for easier checking for
> new cases that might be introduced in the future.

I think there was some discussion in brian's object_id patches about whether we wanted to carry Coccinelle transformations in the tree, but I don't remember the outcome. I don't have an opinion myself.

> Silly question: Is there a natural language that uses percent signs
> as letters or e.g. instead of commas? :)

I don't know, but if they do, they'd better get used to escaping them. :)

-Peff
Junio C Hamano· Sep 15, 2016, 19:25 UTC · re: Jeff King · lore

Re: [PATCH] use strbuf_addstr() for adding constant strings to a strbuf, part 2

Jeff King <peff@peff.net> writes:
Show 10 quoted lines
> On Thu, Sep 15, 2016 at 08:31:00PM +0200, René Scharfe wrote:
>
>> Replace uses of strbuf_addf() for adding strings with more lightweight
>> strbuf_addstr() calls.  This makes the intent clearer and avoids
>> potential issues with printf format specifiers.
>> 
>> 02962d36845b89145cd69f8bc65e015d78ae3434 already converted six cases,
>> this patch covers eleven more.
>
> Great, these all look obviously correct.
Yes.
Show 5 quoted lines
>> Silly question: Is there a natural language that uses percent signs
>> as letters or e.g. instead of commas? :)
>
> I don't know, but if they do, they'd better get used to escaping them.
> :)

I do not know either, but I am curious where that question comes from. I stared at this patch for a few minutes but couldn't guess.

Jeff King· Sep 15, 2016, 19:38 UTC · re: Junio C Hamano · lore

Re: [PATCH] use strbuf_addstr() for adding constant strings to a strbuf, part 2

On Thu, Sep 15, 2016 at 12:25:43PM -0700, Junio C Hamano wrote:
Show 8 quoted lines
> >> Silly question: Is there a natural language that uses percent signs
> >> as letters or e.g. instead of commas? :)
> >
> > I don't know, but if they do, they'd better get used to escaping them.
> > :)
> 
> I do not know either, but I am curious where that question comes
> from.  I stared at this patch for a few minutes but couldn't guess.

My initial thought is that the next step after picking this low-hanging fruit would be to find cases where the strings do not contain "%", and thus we do not have to care about formatting. But a case like:

  strbuf_addf(&buf, "this does not have any percents!", foo);

is simply broken (albeit in a way that we ignore foo, so it's just ugly code, not a real bug).

So I dunno. I too am curious.
-Peff
René Scharfe· Sep 15, 2016, 19:55 UTC · re: Jeff King · lore

Re: [PATCH] use strbuf_addstr() for adding constant strings to a strbuf, part 2

Am 15.09.2016 um 21:38 schrieb Jeff King:
Show 21 quoted lines
> On Thu, Sep 15, 2016 at 12:25:43PM -0700, Junio C Hamano wrote:
>
>>>> Silly question: Is there a natural language that uses percent signs
>>>> as letters or e.g. instead of commas? :)
>>>
>>> I don't know, but if they do, they'd better get used to escaping them.
>>> :)
>>
>> I do not know either, but I am curious where that question comes
>> from.  I stared at this patch for a few minutes but couldn't guess.
>
> My initial thought is that the next step after picking this low-hanging
> fruit would be to find cases where the strings do not contain "%", and
> thus we do not have to care about formatting. But a case like:
>
>   strbuf_addf(&buf, "this does not have any percents!", foo);
>
> is simply broken (albeit in a way that we ignore foo, so it's just ugly
> code, not a real bug).
>
> So I dunno. I too am curious.
Take this for example:

- strbuf_addf(&o->obuf, _("(bad commit)\n")); + strbuf_addstr(&o->obuf, _("(bad commit)\n"));

If there's a language that uses percent signs instead of parens or as regular letters, then they need to be escaped in the translated string before, but not after the patch. As I wrote: silly.

René
Junio C Hamano· Sep 15, 2016, 20:01 UTC · re: René Scharfe · lore

Re: [PATCH] use strbuf_addstr() for adding constant strings to a strbuf, part 2

René Scharfe <l.s.r@web.de> writes:
Show 8 quoted lines
> Take this for example:
>
> -			strbuf_addf(&o->obuf, _("(bad commit)\n"));
> +			strbuf_addstr(&o->obuf, _("(bad commit)\n"));
>
> If there's a language that uses percent signs instead of parens or as
> regular letters, then they need to be escaped in the translated string
> before, but not after the patch.  As I wrote: silly.

Ahh, OK, so "This use of addf only has format part and nothing else, hence the format part can be taken as-is" which is the Coccinelle rule used to produce this patch is incomplete and always needs manual inspection, in case the format part wanted to give a literal % in the output. E.g. it is a bug to convert this

	strbuf_addf(&buf, _("this is 100%% wrong!"));
to
	strbuf_addstr(&buf, _("this is 100%% wrong!"));

Thanks for clarification. Perhaps the strbuf.cocci rule file can have some comment to warn the person who builds *.patch file to look for % in E2, or something?

René Scharfe· Sep 15, 2016, 21:25 UTC · re: Junio C Hamano · lore

Re: [PATCH] use strbuf_addstr() for adding constant strings to a strbuf, part 2

Am 15.09.2016 um 22:01 schrieb Junio C Hamano:
Show 22 quoted lines
> René Scharfe <l.s.r@web.de> writes:
> 
>> Take this for example:
>>
>> -			strbuf_addf(&o->obuf, _("(bad commit)\n"));
>> +			strbuf_addstr(&o->obuf, _("(bad commit)\n"));
>>
>> If there's a language that uses percent signs instead of parens or as
>> regular letters, then they need to be escaped in the translated string
>> before, but not after the patch.  As I wrote: silly.
> 
> Ahh, OK, so "This use of addf only has format part and nothing else,
> hence the format part can be taken as-is" which is the Coccinelle rule
> used to produce this patch is incomplete and always needs manual
> inspection, in case the format part wanted to give a literal % in
> the output.  E.g. it is a bug to convert this
> 
> 	strbuf_addf(&buf, _("this is 100%% wrong!"));
> 
> to
> 
> 	strbuf_addstr(&buf, _("this is 100%% wrong!"));
Right.  Such strings seem to be quite rare in practice, though. 
> Thanks for clarification.  Perhaps the strbuf.cocci rule file can
> have some comment to warn the person who builds *.patch file to look
> for % in E2, or something?
Something like this?
---
 contrib/coccinelle/strbuf.cocci | 2 ++
 1 file changed, 2 insertions(+)
Show changes to contrib/coccinelle/strbuf.cocci +2 −0
diff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci
index 7932d48..3f535ca 100644
--- a/contrib/coccinelle/strbuf.cocci
+++ b/contrib/coccinelle/strbuf.cocci
@@ -1,3 +1,5 @@
+// Careful, this is not fully equivalent: "%" is no longer treated
+// specially.  Check for "%%", "%m" etc. in the format string (E2).
 @@
 expression E1, E2;
 @@

-- 
2.10.0
Junio C Hamano· Sep 15, 2016, 21:39 UTC · re: René Scharfe · lore

Re: [PATCH] use strbuf_addstr() for adding constant strings to a strbuf, part 2

René Scharfe <l.s.r@web.de> writes:
Show 31 quoted lines
> Am 15.09.2016 um 22:01 schrieb Junio C Hamano:
>> René Scharfe <l.s.r@web.de> writes:
>> 
>>> Take this for example:
>>>
>>> -			strbuf_addf(&o->obuf, _("(bad commit)\n"));
>>> +			strbuf_addstr(&o->obuf, _("(bad commit)\n"));
>>>
>>> If there's a language that uses percent signs instead of parens or as
>>> regular letters, then they need to be escaped in the translated string
>>> before, but not after the patch.  As I wrote: silly.
>> 
>> Ahh, OK, so "This use of addf only has format part and nothing else,
>> hence the format part can be taken as-is" which is the Coccinelle rule
>> used to produce this patch is incomplete and always needs manual
>> inspection, in case the format part wanted to give a literal % in
>> the output.  E.g. it is a bug to convert this
>> 
>> 	strbuf_addf(&buf, _("this is 100%% wrong!"));
>> 
>> to
>> 
>> 	strbuf_addstr(&buf, _("this is 100%% wrong!"));
>
> Right.  Such strings seem to be quite rare in practice, though. 
>
>> Thanks for clarification.  Perhaps the strbuf.cocci rule file can
>> have some comment to warn the person who builds *.patch file to look
>> for % in E2, or something?
>
> Something like this?

Yup, with something like that I would understdood where that puzzling question came from.

Thanks.
Show 15 quoted lines
>
> ---
>  contrib/coccinelle/strbuf.cocci | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci
> index 7932d48..3f535ca 100644
> --- a/contrib/coccinelle/strbuf.cocci
> +++ b/contrib/coccinelle/strbuf.cocci
> @@ -1,3 +1,5 @@
> +// Careful, this is not fully equivalent: "%" is no longer treated
> +// specially.  Check for "%%", "%m" etc. in the format string (E2).
>  @@
>  expression E1, E2;
>  @@
René Scharfe· Oct 2, 2016, 22:58 UTC · re: Junio C Hamano · lore

Re: [PATCH] use strbuf_addstr() for adding constant strings to a strbuf, part 2

Am 15.09.2016 um 23:39 schrieb Junio C Hamano:
Show 36 quoted lines
> René Scharfe <l.s.r@web.de> writes:
> 
>> Am 15.09.2016 um 22:01 schrieb Junio C Hamano:
>>> René Scharfe <l.s.r@web.de> writes:
>>>
>>>> Take this for example:
>>>>
>>>> -			strbuf_addf(&o->obuf, _("(bad commit)\n"));
>>>> +			strbuf_addstr(&o->obuf, _("(bad commit)\n"));
>>>>
>>>> If there's a language that uses percent signs instead of parens or as
>>>> regular letters, then they need to be escaped in the translated string
>>>> before, but not after the patch.  As I wrote: silly.
>>>
>>> Ahh, OK, so "This use of addf only has format part and nothing else,
>>> hence the format part can be taken as-is" which is the Coccinelle rule
>>> used to produce this patch is incomplete and always needs manual
>>> inspection, in case the format part wanted to give a literal % in
>>> the output.  E.g. it is a bug to convert this
>>>
>>> 	strbuf_addf(&buf, _("this is 100%% wrong!"));
>>>
>>> to
>>>
>>> 	strbuf_addstr(&buf, _("this is 100%% wrong!"));
>>
>> Right.  Such strings seem to be quite rare in practice, though. 
>>
>>> Thanks for clarification.  Perhaps the strbuf.cocci rule file can
>>> have some comment to warn the person who builds *.patch file to look
>>> for % in E2, or something?
>>
>> Something like this?
> 
> Yup, with something like that I would understdood where that
> puzzling question came from.
Here's something better than a comment:
-- >8 --
Subject: [PATCH] coccicheck: make transformation for strbuf_addf(sb, "...") more precise

We can replace strbuf_addf() calls that just add a simple string with calls to strbuf_addstr() to make the intent clearer. We need to be careful if that string contains printf format specifications like %%, though, as a simple replacement would change the output.

Add checks to the semantic patch to make sure we only perform the transformation if the second argument is a string constant (possibly translated) that doesn't contain any percent signs.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 contrib/coccinelle/strbuf.cocci | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)
Show changes to contrib/coccinelle/strbuf.cocci +26 −3
diff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci
index 1e24298..63995f2 100644
--- a/contrib/coccinelle/strbuf.cocci
+++ b/contrib/coccinelle/strbuf.cocci
@@ -1,8 +1,31 @@
+@ strbuf_addf_with_format_only @
+expression E;
+constant fmt;
 @@
-expression E1, E2;
+  strbuf_addf(E,
+(
+  fmt
+|
+  _(fmt)
+)
+  );
+
+@ script:python @
+fmt << strbuf_addf_with_format_only.fmt;
 @@
-- strbuf_addf(E1, E2);
-+ strbuf_addstr(E1, E2);
+cocci.include_match("%" not in fmt)
+
+@ extends strbuf_addf_with_format_only @
+@@
+- strbuf_addf
++ strbuf_addstr
+  (E,
+(
+  fmt
+|
+  _(fmt)
+)
+  );
 
 @@
 expression E1, E2;

-- 
2.10.0
brian m. carlson· Sep 15, 2016, 23:47 UTC · re: René Scharfe · lore

Re: [PATCH] use strbuf_addstr() for adding constant strings to a strbuf, part 2

On Thu, Sep 15, 2016 at 08:31:00PM +0200, René Scharfe wrote:
Show 9 quoted lines
> Replace uses of strbuf_addf() for adding strings with more lightweight
> strbuf_addstr() calls.  This makes the intent clearer and avoids
> potential issues with printf format specifiers.
> 
> 02962d36845b89145cd69f8bc65e015d78ae3434 already converted six cases,
> this patch covers eleven more.
> 
> A semantic patch for Coccinelle is included for easier checking for
> new cases that might be introduced in the future.

I think all three of these patches look good. I'm glad to see us getting better use out of Coccinelle.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

← back to recent threads