[GSoC][RFC][PATCH 0/2] STRBUF_INIT_CONST Cover

[GSoC][RFC][PATCH 0/2] STRBUF_INIT_CONST Cover

[Robear Selwans]

This was added according to the issue opened at [https://github.com/gitgitgadget/git/issues/398]

[Copied directly from the issue description]
> This new way to initialize an strbuf would set buf to the specified
> constant string, the appropriate len = strlen(string), and alloc to 0.
 
> That way, we can initialize strbufs with constant values, only
> allocating memory when/if needed.

Things Done:
	- Added a new way to initialize constant `strbuf`
	- Edited functions that try to edit the `strbuf` to check if it was
	initialized as a constant

Robear Selwans (2):
  STRBUF_INIT_CONST: a new way to initialize strbuf
  STRBUF_INIT_CONST: Adapting strbuf_* functions

 strbuf.c | 25 +++++++++++++++++++++++++
 strbuf.h | 15 +++++++++++++++
 2 files changed, 40 insertions(+)

-- 
2.17.1

[GSoC][RFC][PATCH 1/2] STRBUF_INIT_CONST: a new way to initialize strbuf

[Robear Selwans]

A new function `STRBUF_INIT_CONST(const_str)` was added to allow for a
quick initialization of strbuf.

Details:
Using `STRBUF_INIT_CONST(str)` creates a new struct of type `strbuf` and
initializes its `buf`, `len` and `alloc` as `str`, `strlen(str)` and
`0`, respectively.

Use Case:
This is meant to be used to initialize strbufs with constant values and
thus, only allocating memory when needed.

Usage Example:
```
strbuf env_var = STRBUF_INIT_CONST("dev");
```

This was added according to the issue opened at [https://github.com/gitgitgadget/git/issues/398]

Signed-off-by: Robear Selwans <robear.selwans@outlook.com>
---
 strbuf.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/strbuf.h b/strbuf.h
index bfa66569a4..1a1753424c 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -71,6 +71,7 @@ struct strbuf {
 
 extern char strbuf_slopbuf[];
 #define STRBUF_INIT  { .alloc = 0, .len = 0, .buf = strbuf_slopbuf }
+#define STRBUF_INIT_CONST(const_str)  { .alloc = 0, .len = strlen(const_str), .buf = const_str }
 
 /*
  * Predeclare this here, since cache.h includes this file before it defines the
-- 
2.17.1

[GSoC][RFC][PATCH 2/2] STRBUF_INIT_CONST: Adapting strbuf_* functions

[Robear Selwans]

In a previous commit, a new function `STRBUF_INIT_CONST(const_str)`,
which would allow for the quick initialization of constant `strbuf`s,
was introduced.

In this commit, I check through the strbuf_* functions to edit the ones
that would try to edit the passed `strbuf` so that they are guaranteed to
behave in a predictable way when met with a constant.

Added Functions:
    `strbuf_make_var`

Updated Functions:
    `strbuf_grow`
    `strbuf_setlen`
    `strbuf_trim`
    `strbuf_trim_trailing_dir_sep`
    `strbuf_trim_trailing_newline`
    `strbuf_tolower`
    `strbuf_splice`

Functions where comments were added to clarify the expected behavior:
    `strbuf_getcwd`

Signed-off-by: Robear Selwans <robear.selwans@outlook.com>
---
 strbuf.c | 25 +++++++++++++++++++++++++
 strbuf.h | 14 ++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/strbuf.c b/strbuf.c
index f19da55b07..48af039b6e 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -89,6 +89,8 @@ void strbuf_attach(struct strbuf *sb, void *buf, size_t len, size_t alloc)
 
 void strbuf_grow(struct strbuf *sb, size_t extra)
 {
+	if (sb->len > sb->alloc)
+		strbuf_make_var(sb);
 	int new_buf = !sb->alloc;
 	if (unsigned_add_overflows(extra, 1) ||
 	    unsigned_add_overflows(sb->len, extra + 1))
@@ -100,8 +102,18 @@ void strbuf_grow(struct strbuf *sb, size_t extra)
 		sb->buf[0] = '\0';
 }
 
+void strbuf_make_var(struct strbuf *sb)
+{
+	char* str_cpy;
+	ALLOC_ARRAY(str_cpy, sb->len + 1);
+	memcpy(str_cpy, sb->buf, sb->len);
+	strbuf_attach(sb, str_cpy, sb->len, sb->len + 1);
+}
+
 void strbuf_trim(struct strbuf *sb)
 {
+	if (sb->len > sb->alloc)
+		strbuf_make_var(sb);
 	strbuf_rtrim(sb);
 	strbuf_ltrim(sb);
 }
@@ -115,6 +127,8 @@ void strbuf_rtrim(struct strbuf *sb)
 
 void strbuf_trim_trailing_dir_sep(struct strbuf *sb)
 {
+	if (sb->len > sb->alloc)
+		strbuf_make_var(sb);
 	while (sb->len > 0 && is_dir_sep((unsigned char)sb->buf[sb->len - 1]))
 		sb->len--;
 	sb->buf[sb->len] = '\0';
@@ -122,6 +136,9 @@ void strbuf_trim_trailing_dir_sep(struct strbuf *sb)
 
 void strbuf_trim_trailing_newline(struct strbuf *sb)
 {
+	if (sb->buf[sb->len - 1] == '\n')
+		if (sb->len > sb->alloc)
+			strbuf_make_var(sb);
 	if (sb->len > 0 && sb->buf[sb->len - 1] == '\n') {
 		if (--sb->len > 0 && sb->buf[sb->len - 1] == '\r')
 			--sb->len;
@@ -158,6 +175,8 @@ int strbuf_reencode(struct strbuf *sb, const char *from, const char *to)
 
 void strbuf_tolower(struct strbuf *sb)
 {
+	if (sb->len > sb->alloc)
+		strbuf_make_var(sb);
 	char *p = sb->buf, *end = sb->buf + sb->len;
 	for (; p < end; p++)
 		*p = tolower(*p);
@@ -234,6 +253,8 @@ void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,
 		die("`pos' is too far after the end of the buffer");
 	if (pos + len > sb->len)
 		die("`pos + len' is too far after the end of the buffer");
+	if (sb->len > sb->alloc)
+		strbuf_make_var(sb);
 
 	if (dlen >= len)
 		strbuf_grow(sb, dlen - len);
@@ -572,6 +593,10 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
 
 int strbuf_getcwd(struct strbuf *sb)
 {
+	/*
+	 * If sb was a constant, then an error would lead to 
+	 * strbuf_release() being called on the sb.
+	 */
 	size_t oldalloc = sb->alloc;
 	size_t guessed_len = 128;
 
diff --git a/strbuf.h b/strbuf.h
index 1a1753424c..a33bc4d90e 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -128,6 +128,10 @@ static inline void strbuf_swap(struct strbuf *a, struct strbuf *b)
 	SWAP(*a, *b);
 }
 
+/**
+ * Constant string buffer is turned into a variable one.
+ */
+void strbuf_make_var(struct strbuf *sb);
 
 /**
  * Functions related to the size of the buffer
@@ -160,6 +164,16 @@ void strbuf_grow(struct strbuf *sb, size_t amount);
  */
 static inline void strbuf_setlen(struct strbuf *sb, size_t len)
 {
+	if (sb->len > sb->alloc)
+	{
+		if (!len)
+		{
+			sb->buf = strbuf_slopbuf;
+			return;
+		}
+		else
+			strbuf_make_var(sb);
+	}
 	if (len > (sb->alloc ? sb->alloc - 1 : 0))
 		die("BUG: strbuf_setlen() beyond buffer");
 	sb->len = len;
-- 
2.17.1

Re: [GSoC][RFC][PATCH 1/2] STRBUF_INIT_CONST: a new way to initialize strbuf

[Jeff King]

On Tue, Feb 18, 2020 at 04:18:04AM +0000, Robear Selwans wrote:

> A new function `STRBUF_INIT_CONST(const_str)` was added to allow for a
> quick initialization of strbuf.
> 
> Details:
> Using `STRBUF_INIT_CONST(str)` creates a new struct of type `strbuf` and
> initializes its `buf`, `len` and `alloc` as `str`, `strlen(str)` and
> `0`, respectively.
> 
> Use Case:
> This is meant to be used to initialize strbufs with constant values and
> thus, only allocating memory when needed.
> 
> Usage Example:
> ```
> strbuf env_var = STRBUF_INIT_CONST("dev");
> ```

This seems a bit dangerous to me, as we're initializing a non-const
pointer with a string literal. In fact, I'm a little surprised that the
compiler doesn't complain, but I think it's mostly due to historical
C-isms (the type of string literals is array-of-char). Using gcc's
-Wwrite-strings does complain, but there are several other cases already
in Git (looking at a few, I think there are some opportunities for
cleanup).

Your second patch catches cases where the strbuf functions want to write
to the buffer. But we've always been pretty open about the fact that
strbuf.buf is a writeable C-style string. So something like this:

  struct strbuf x = STRBUF_INIT_CONST("foo");
  size_t i;

  for (i = 0; i < x.len; i++)
	x.buf[i] = toupper(x.buf[i]);

would generate no compile-time warnings, but would invoke undefined
behavior (on my system it segfaults when run, but it could have even
more confusing outcomes). Even though this is called out specifically in
the strbuf docs:

   However, it is totally safe to modify anything in the string pointed
   by the `buf` member, between the indices `0` and `len-1` (inclusive).

Of course it would be easy to fix that by adding a strbuf_make_var()
call. But my concern is cases where the const-ness and the use of the
strbuf are far apart. The point of a strbuf is that you can just use it
without worrying, but now it's carrying this extra hidden state.

If we want to pursue this direction, I think we'd do better to give each
strbuf a matching array. Something like:

  #define STRBUF_INIT_FROM(buf) { .alloc = 0, .buf = buf, .len = ARRAY_SIZE(buf)-1 }
  ...
  char foo_buf[] = "this is the constant value";
  struct strbuf foo = STRBUF_INIT_FROM(foo_buf);

That gives you a true writeable buffer with the const data in it. _And_
it opens up the option of strbufs using stack buffers with an empty
initial value for efficiency (i.e., avoiding the heap at all for short
common cases, but being able to grow when needed). One trouble is that
you can't do it all in a single variable. You'd need something like:

  #define DECLARE_STACK_STRBUF(name, contents) \
	char name##_buf[] = (contents);
	struct strbuf name = STRBUF_INIT_FROM(name##_buf)
  ...
  DECLARE_STACK_STRBUF(foo, "this is the constant value");

But that gets weirdly un-C-like (your macro expands to multiple
statements, which is usually a macro pitfall; but we can't use the usual
"do { } while(0)" trick here, because the variables would go out of
scope at the end of the fake block.

So I think there are interesting directions here, but there's a lot of
stuff to figure out.

I notice you put GSoC in your subject line. If you're looking at this as
a microproject, IMHO this is _way_ more complicated and subtle than a
microproject should be. The goal there is to give something so easy that
you get to focus on getting your patches in and interacting with the
community. The scope I'd expect is more along the lines of compiling
with -Wwrite-strings and cleaning up some of the locations that
complain.

-Peff

Re: [GSoC][RFC][PATCH 1/2] STRBUF_INIT_CONST: a new way to initialize strbuf

[Robear Selwans]

On Tue, Feb 18, 2020 at 8:21 AM Jeff King <peff@peff.net> wrote:
> Your second patch catches cases where the strbuf functions want to write
> to the buffer. But we've always been pretty open about the fact that
> strbuf.buf is a writeable C-style string. So something like this:
> ...
> would generate no compile-time warnings, but would invoke undefined
> behavior (on my system it segfaults when run, but it could have even
> more confusing outcomes).
Oh right, I didn't think about that. Ignorant of me to expect everyone to just
call the functions and not edit the buf directly.

> If we want to pursue this direction, I think we'd do better to give each
> strbuf a matching array. Something like:
> ...
> So I think there are interesting directions here, but there's a lot of
> stuff to figure out.
I think that got me a bit fired up now.

> I notice you put GSoC in your subject line. If you're looking at this as
> a microproject, IMHO this is _way_ more complicated and subtle than a
> microproject should be. The goal there is to give something so easy that
> you get to focus on getting your patches in and interacting with the
> community. The scope I'd expect is more along the lines of compiling
> with -Wwrite-strings and cleaning up some of the locations that
> complain.
I'm actually planning to keep on contributing to git, so I kind of
didn't want to
do something trivial. Despite the fact that I'm planning to apply to
git for GSoC,
I'm mostly putting the [GSoC] so that reviewers would go easy on me :D. That
said, I might actually do the -Wwrite-strings clean-up after this one
is finished.

Thanks for the help, I guess I'll start editing it ASAP, then.

- mo7sener

Re: [GSoC][RFC][PATCH 1/2] STRBUF_INIT_CONST: a new way to initialize strbuf

[Jeff King]

On Tue, Feb 18, 2020 at 04:19:38PM +0200, Robear Selwans wrote:

> > I notice you put GSoC in your subject line. If you're looking at this as
> > a microproject, IMHO this is _way_ more complicated and subtle than a
> > microproject should be. The goal there is to give something so easy that
> > you get to focus on getting your patches in and interacting with the
> > community. The scope I'd expect is more along the lines of compiling
> > with -Wwrite-strings and cleaning up some of the locations that
> > complain.
> I'm actually planning to keep on contributing to git, so I kind of
> didn't want to
> do something trivial. Despite the fact that I'm planning to apply to
> git for GSoC,
> I'm mostly putting the [GSoC] so that reviewers would go easy on me :D. That
> said, I might actually do the -Wwrite-strings clean-up after this one
> is finished.

OK. If you want to go further, I certainly won't stop you. :)

-Peff

Re: [GSoC][RFC][PATCH 1/2] STRBUF_INIT_CONST: a new way to initialize strbuf

[Johannes Sixt]

Am 18.02.20 um 05:18 schrieb Robear Selwans:
> A new function `STRBUF_INIT_CONST(const_str)` was added to allow for a
> quick initialization of strbuf.
> 
> Details:
> Using `STRBUF_INIT_CONST(str)` creates a new struct of type `strbuf` and
> initializes its `buf`, `len` and `alloc` as `str`, `strlen(str)` and
> `0`, respectively.
> 
> Use Case:
> This is meant to be used to initialize strbufs with constant values and
> thus, only allocating memory when needed.
> 
> Usage Example:
> ```
> strbuf env_var = STRBUF_INIT_CONST("dev");
> ```
> 
> This was added according to the issue opened at [https://github.com/gitgitgadget/git/issues/398]

I am not a friend of this change at all. Why do so many functions and
strbuf instances have to pay a price (check for immutable string) for a
feature that they are not using?

As the macro is just intended for convenience, I suggest to implement it
using strbuf_addstr() under the hood. That is much less code churn, and
the price is paid only by the strbufs that actually use the feature.

-- Hannes

Re: [GSoC][RFC][PATCH 1/2] STRBUF_INIT_CONST: a new way to initialize strbuf

[René Scharfe]

Am 19.02.20 um 09:13 schrieb Johannes Sixt:
> Am 18.02.20 um 05:18 schrieb Robear Selwans:
>> A new function `STRBUF_INIT_CONST(const_str)` was added to allow for a
>> quick initialization of strbuf.
>>
>> Details:
>> Using `STRBUF_INIT_CONST(str)` creates a new struct of type `strbuf` and
>> initializes its `buf`, `len` and `alloc` as `str`, `strlen(str)` and
>> `0`, respectively.
>>
>> Use Case:
>> This is meant to be used to initialize strbufs with constant values and
>> thus, only allocating memory when needed.
>>
>> Usage Example:
>> ```
>> strbuf env_var = STRBUF_INIT_CONST("dev");
>> ```
>>
>> This was added according to the issue opened at [https://github.com/gitgitgadget/git/issues/398]
>
> I am not a friend of this change at all. Why do so many functions and
> strbuf instances have to pay a price (check for immutable string) for a
> feature that they are not using?
>
> As the macro is just intended for convenience, I suggest to implement it
> using strbuf_addstr() under the hood. That is much less code churn, and
> the price is paid only by the strbufs that actually use the feature.

I was also wondering what the benefits of this change might be.  Saving
one line and thus increasing convenience slightly doesn't justify all
this added complexity.  Saving an allocation in the following sequence
might be worthwhile:

	struct strbuf sb = STRBUF_INIT;
	strbuf_addstr(&sb, "foo");
	/* Use sb without modifying it. */
	strbuf_release(&sb); /* or leak it */

I found two examples of this pattern in the code, one in range-diff.c in
the function show_range_diff(), and below is a patch for getting rid of
the second one.  Are there other reasons why we'd want that feature?
Could you perhaps include a patch that makes use of it in this series,
to highlight its benefits?

-- >8 --
Subject: [PATCH] commit-graph: use progress title directly

merge_commit_graphs() copies the (translated) progress message into a
strbuf and passes the copy to start_delayed_progress() at each loop
iteration.  The latter function takes a string pointer, so let's avoid
the detour and hand the string to it directly.  That's shorter, simpler
and slightly more efficient.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 commit-graph.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index 656dd647d5..f013a84e29 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1657,19 +1657,15 @@ static void merge_commit_graphs(struct write_commit_graph_context *ctx)
 {
 	struct commit_graph *g = ctx->r->objects->commit_graph;
 	uint32_t current_graph_number = ctx->num_commit_graphs_before;
-	struct strbuf progress_title = STRBUF_INIT;

 	while (g && current_graph_number >= ctx->num_commit_graphs_after) {
 		current_graph_number--;

-		if (ctx->report_progress) {
-			strbuf_addstr(&progress_title, _("Merging commit-graph"));
-			ctx->progress = start_delayed_progress(progress_title.buf, 0);
-		}
+		if (ctx->report_progress)
+			ctx->progress = start_delayed_progress(_("Merging commit-graph"), 0);

 		merge_commit_graph(ctx, g);
 		stop_progress(&ctx->progress);
-		strbuf_release(&progress_title);

 		g = g->base_graph;
 	}
--
2.25.1

Re: [GSoC][RFC][PATCH 1/2] STRBUF_INIT_CONST: a new way to initialize strbuf

[Robear Selwans]

On Thu, Feb 20, 2020 at 8:49 PM René Scharfe <l.s.r@web.de> wrote:
>
> Could you perhaps include a patch that makes use of it in this series,
> to highlight its benefits?

Well to begin with, I'm actually doing this in response to this issue
[https://github.com/gitgitgadget/git/issues/398].
The issue was created because of the following mail thread, though.
[https://public-inbox.org/git/20180601200146.114919-1-sbeller@google.com/]
To be honest, I'm not entirely sure about how making these changes
would help, as my experience is still quite limited. But from what
I've read, I think the main
use-case would be using const `strbuf`s to avoid memory leaks when
dealing with config strings.

Re: [GSoC][RFC][PATCH 1/2] STRBUF_INIT_CONST: a new way to initialize strbuf

[Derrick Stolee]

On 2/20/2020 1:49 PM, René Scharfe wrote:
> Am 19.02.20 um 09:13 schrieb Johannes Sixt:
>> As the macro is just intended for convenience, I suggest to implement it
>> using strbuf_addstr() under the hood. That is much less code churn, and
>> the price is paid only by the strbufs that actually use the feature.
> 
> I was also wondering what the benefits of this change might be.  Saving
> one line and thus increasing convenience slightly doesn't justify all
> this added complexity.  Saving an allocation in the following sequence
> might be worthwhile:
> 
> 	struct strbuf sb = STRBUF_INIT;
> 	strbuf_addstr(&sb, "foo");
> 	/* Use sb without modifying it. */
> 	strbuf_release(&sb); /* or leak it */
> 
> I found two examples of this pattern in the code, one in range-diff.c in
> the function show_range_diff(), and below is a patch for getting rid of
> the second one.  Are there other reasons why we'd want that feature?
> Could you perhaps include a patch that makes use of it in this series,
> to highlight its benefits?
> 
> -- >8 --
> Subject: [PATCH] commit-graph: use progress title directly
> 
> merge_commit_graphs() copies the (translated) progress message into a
> strbuf and passes the copy to start_delayed_progress() at each loop
> iteration.  The latter function takes a string pointer, so let's avoid
> the detour and hand the string to it directly.  That's shorter, simpler
> and slightly more efficient.
> 
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  commit-graph.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/commit-graph.c b/commit-graph.c
> index 656dd647d5..f013a84e29 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -1657,19 +1657,15 @@ static void merge_commit_graphs(struct write_commit_graph_context *ctx)
>  {
>  	struct commit_graph *g = ctx->r->objects->commit_graph;
>  	uint32_t current_graph_number = ctx->num_commit_graphs_before;
> -	struct strbuf progress_title = STRBUF_INIT;
> 
>  	while (g && current_graph_number >= ctx->num_commit_graphs_after) {
>  		current_graph_number--;
> 
> -		if (ctx->report_progress) {
> -			strbuf_addstr(&progress_title, _("Merging commit-graph"));
> -			ctx->progress = start_delayed_progress(progress_title.buf, 0);
> -		}
> +		if (ctx->report_progress)
> +			ctx->progress = start_delayed_progress(_("Merging commit-graph"), 0);
> 
>  		merge_commit_graph(ctx, g);
>  		stop_progress(&ctx->progress);
> -		strbuf_release(&progress_title);
> 
>  		g = g->base_graph;
>  	}
> --

Not only is this a good change, it no longer leaks memory.
Thanks!

-Stolee

Re: [GSoC][RFC][PATCH 1/2] STRBUF_INIT_CONST: a new way to initialize strbuf

[René Scharfe]

Am 27.02.20 um 07:50 schrieb Derrick Stolee:
> On 2/20/2020 1:49 PM, René Scharfe wrote:
>> Subject: [PATCH] commit-graph: use progress title directly
>>
>> merge_commit_graphs() copies the (translated) progress message into a
>> strbuf and passes the copy to start_delayed_progress() at each loop
>> iteration.  The latter function takes a string pointer, so let's avoid
>> the detour and hand the string to it directly.  That's shorter, simpler
>> and slightly more efficient.
>>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>>  commit-graph.c | 8 ++------
>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/commit-graph.c b/commit-graph.c
>> index 656dd647d5..f013a84e29 100644
>> --- a/commit-graph.c
>> +++ b/commit-graph.c
>> @@ -1657,19 +1657,15 @@ static void merge_commit_graphs(struct write_commit_graph_context *ctx)
>>  {
>>  	struct commit_graph *g = ctx->r->objects->commit_graph;
>>  	uint32_t current_graph_number = ctx->num_commit_graphs_before;
>> -	struct strbuf progress_title = STRBUF_INIT;
>>
>>  	while (g && current_graph_number >= ctx->num_commit_graphs_after) {
>>  		current_graph_number--;
>>
>> -		if (ctx->report_progress) {
>> -			strbuf_addstr(&progress_title, _("Merging commit-graph"));
>> -			ctx->progress = start_delayed_progress(progress_title.buf, 0);
>> -		}
>> +		if (ctx->report_progress)
>> +			ctx->progress = start_delayed_progress(_("Merging commit-graph"), 0);
>>
>>  		merge_commit_graph(ctx, g);
>>  		stop_progress(&ctx->progress);
>> -		strbuf_release(&progress_title);
>>
>>  		g = g->base_graph;
>>  	}
>> --
>
> Not only is this a good change, it no longer leaks memory.
> Thanks!

strbuf_release() frees the allocated memory, so I don't think the code
was leaking before.  (It would have with strbuf_reset()).

René

Re: [GSoC][RFC][PATCH 2/2] STRBUF_INIT_CONST: Adapting strbuf_* functions

[Junio C Hamano]

Robear Selwans <rwagih.rw@gmail.com> writes:

> On Wed, Feb 19, 2020 at 5:13 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Yes, but the case that matters to _your_ use is sb->alloc == 0.  You
>> do not want to let a broken strbuf (presumably broken by changes
>> other than your own) to pass, when you can detect it.  And for that,
>> paying attention to sb->len _might_ make sense, but then the check
>> won't be
>>
>>         if (sb->alloc < sb->len)
>>                 make it mutable;
>>
>> you'd rather be writing something like
>>
>>         if (!sb->alloc)
>>                 make it mutable;
>>         else if (sb->alloc < sb->len)
>>                 BUG("somebody fed a corrupt strbuf to me");
>
> Ooh so what you meant, is that corrupt `strbuf`s need to be
> anticipated even if they
> don't make much sense. Smart.

I don't know if that is smart, but the point is that sb->alloc is
the only thing you need to care about if you want to see if the
strbuf is borrowing from a const string, and it does not make much
sense not to catch a corruption, _if_ you are to check the value of
sb->len as well.

Re: [GSoC][RFC][PATCH 2/2] STRBUF_INIT_CONST: Adapting strbuf_* functions

[Robear Selwans]

On Wed, Feb 19, 2020 at 5:13 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Yes, but the case that matters to _your_ use is sb->alloc == 0.  You
> do not want to let a broken strbuf (presumably broken by changes
> other than your own) to pass, when you can detect it.  And for that,
> paying attention to sb->len _might_ make sense, but then the check
> won't be
>
>         if (sb->alloc < sb->len)
>                 make it mutable;
>
> you'd rather be writing something like
>
>         if (!sb->alloc)
>                 make it mutable;
>         else if (sb->alloc < sb->len)
>                 BUG("somebody fed a corrupt strbuf to me");

Ooh so what you meant, is that corrupt `strbuf`s need to be
anticipated even if they
don't make much sense. Smart.

Re: [GSoC][RFC][PATCH 2/2] STRBUF_INIT_CONST: Adapting strbuf_* functions

[Junio C Hamano]

Robear Selwans <rwagih.rw@gmail.com> writes:

>> Also, isn't "if (sb->alloc < sb->len)" too loose a check for the new
>> feature?  AFAICS in 1/2, a strbuf that is still borrowing a const
>> string always has sb->alloc==0.  Other instances of strbuf that
>> happens to satisify the above condition, e.g. (sb->len == 5 &&
>> sb->alloc == 1), is an error.  If we are to check the condition
>> about sb->len, shouldn't we diagnose such a case as an error, no?
> AFAIK after reading the documentation for `strbuf`, there is no other
> case where `sb->len > sb->alloc` as `alloc` needs to always be more
> than `len`. I'd like to be corrected if mistaken, though.

Yes, but the case that matters to _your_ use is sb->alloc == 0.  You
do not want to let a broken strbuf (presumably broken by changes
other than your own) to pass, when you can detect it.  And for that,
paying attention to sb->len _might_ make sense, but then the check
won't be 

	if (sb->alloc < sb->len)
		make it mutable;

you'd rather be writing something like

	if (!sb->alloc)
		make it mutable;
	else if (sb->alloc < sb->len)
		BUG("somebody fed a corrupt strbuf to me");

If the primary purpose of make_mutable() is *not* about catching
random strbuf corruption, then the whole "else if" part is not
needed, and the check should become a equation only about sb->alloc,
not a comparison between alloc and len (which would trigger for *both*
your const-initialized strbuf *and* a corrupt one you did not anticipate
to see).

Re: [GSoC][RFC][PATCH 2/2] STRBUF_INIT_CONST: Adapting strbuf_* functions

[Jeff King]

On Wed, Feb 19, 2020 at 03:43:26AM +0200, Robear Selwans wrote:

> On Tue, Feb 18, 2020 at 10:46 PM Junio C Hamano <gitster@pobox.com> wrote:
> > Abhishek Kumar <abhishekkumar8222@gmail.com> writes:
> > >> STRBUF_INIT_CONST: a new way to initialize strbuf
> > > Use imperative mood and be more specific in the commit title -
> > > `strbuf: Teach strbuf to initialize immutable strings`
> > s/T/t/;
> I don't get what you mean by that.

We don't usually capitalize the sentence fragment in a subject line
(hence "replace T with t", but said in sed jargon).

> > Also, isn't "if (sb->alloc < sb->len)" too loose a check for the new
> > feature?  AFAICS in 1/2, a strbuf that is still borrowing a const
> > string always has sb->alloc==0.  Other instances of strbuf that
> > happens to satisify the above condition, e.g. (sb->len == 5 &&
> > sb->alloc == 1), is an error.  If we are to check the condition
> > about sb->len, shouldn't we diagnose such a case as an error, no?
> AFAIK after reading the documentation for `strbuf`, there is no other
> case where `sb->len > sb->alloc` as `alloc` needs to always be more
> than `len`. I'd like to be corrected if mistaken, though.

Yeah, I don't see how it could be for an allocated buffer, since that
would imply writing past the allocated size.

> > As Peff, I am a bit hesitant about leaving a strbuf that hasn't been
> > made mutable around, though.
> Yeah, I started to get that when I read Peff's reply. I think I'll go with
> the approach that Peff suggested, where the constant string is
> copied to a stack array and so is made mutable to a degree. Since
> this is a different way, I guess I'll make that from scratch instead of
> editing the existing one.

Yeah, I think it's sufficiently different to start from scratch. Do note
that I think you may need to stuff an extra bit into the struct somehow.
For a const string, you can set alloc to "0" to denote the situation.
But if we're going to be able to attach another buffer that can be
written to (but may not yet be full), you'd need to be able to set alloc
to the true size of that buffer. And then we need some way to note that
it's not a real heap buffer.

There are probably clever tricks you could play with the low bits of the
"alloc" field to avoid making the struct any bigger. But given the way
we use strbufs, I suspect it may not bring a measurable performance
improvement. And I do have dreams of eventually adding more bits. E.g.,
there are a few cases where it would be convenient to extend the "strbuf
that points to a stack buffer but grows" into "strbuf that points to a
stack buffer but truncates" (e.g., in our error handling routines which
want to avoid calling malloc).

That's definitely a few steps out from where we are now, of course, but
we may get there eventually.

-Peff

Re: [GSoC][RFC][PATCH 2/2] STRBUF_INIT_CONST: Adapting strbuf_* functions

[Robear Selwans]

On Tue, Feb 18, 2020 at 10:46 PM Junio C Hamano <gitster@pobox.com> wrote:
> Abhishek Kumar <abhishekkumar8222@gmail.com> writes:
> >> STRBUF_INIT_CONST: a new way to initialize strbuf
> > Use imperative mood and be more specific in the commit title -
> > `strbuf: Teach strbuf to initialize immutable strings`
> s/T/t/;
I don't get what you mean by that.

> Also, isn't "if (sb->alloc < sb->len)" too loose a check for the new
> feature?  AFAICS in 1/2, a strbuf that is still borrowing a const
> string always has sb->alloc==0.  Other instances of strbuf that
> happens to satisify the above condition, e.g. (sb->len == 5 &&
> sb->alloc == 1), is an error.  If we are to check the condition
> about sb->len, shouldn't we diagnose such a case as an error, no?
AFAIK after reading the documentation for `strbuf`, there is no other
case where `sb->len > sb->alloc` as `alloc` needs to always be more
than `len`. I'd like to be corrected if mistaken, though.

> As Peff, I am a bit hesitant about leaving a strbuf that hasn't been
> made mutable around, though.
Yeah, I started to get that when I read Peff's reply. I think I'll go with
the approach that Peff suggested, where the constant string is
copied to a stack array and so is made mutable to a degree. Since
this is a different way, I guess I'll make that from scratch instead of
editing the existing one.

Re: [GSoC][RFC][PATCH 2/2] STRBUF_INIT_CONST: Adapting strbuf_* functions

[Junio C Hamano]

Abhishek Kumar <abhishekkumar8222@gmail.com> writes:

> I would also prefer the term "immutable" over "const" since const
> already has implications in C programming.

As long as the implication the established word conveys is what the
patch wants to do, it is *better* not to invent another phrase and
instead use the well-known term, no?

>> STRBUF_INIT_CONST: a new way to initialize strbuf
>
> Use imperative mood and be more specific in the commit title -
> `strbuf: Teach strbuf to initialize immutable strings`

s/T/t/;

> I feel this is self-explanatory when you go through the diff.

True.

>  void strbuf_grow(struct strbuf *sb, size_t extra)
>  {
> +    if (sb->len > sb->alloc)
> +        strbuf_make_var(sb);
>      int new_buf = !sb->alloc;

This introduces decl-after-stmt error.

Also, isn't "if (sb->alloc < sb->len)" too loose a check for the new
feature?  AFAICS in 1/2, a strbuf that is still borrowing a const
string always has sb->alloc==0.  Other instances of strbuf that
happens to satisify the above condition, e.g. (sb->len == 5 &&
sb->alloc == 1), is an error.  If we are to check the condition
about sb->len, shouldn't we diagnose such a case as an error, no?

> +void strbuf_make_var(struct strbuf *sb)
> +{
> +    char* str_cpy;

Isn't make_var() an implementation detail that should not leak
to the strbuf API users?  IOW, does it have to be extern?

In our codebase (eh, rather, in C as opposed to C++), the asterisk
sticks to the identifier, not to the type.

>  void strbuf_trim_trailing_newline(struct strbuf *sb)
>  {
> +    if (sb->buf[sb->len - 1] == '\n')
>> +        if (sb->len > sb->alloc)
>> +            strbuf_make_var(sb);
>
> Enclose this explicitly in braces.

Yup.  Also the repetition we see is a sign that something is wrong.
Perhaps adding a small inline helper 

	static inline void strbuf_premutate(sb) 
	{
		if (!sb->alloc) {
			... body of strbuf_make_var() comes here ...
		}
	}

and getting rid of strbuf_make_var() would help?

As Peff, I am a bit hesitant about leaving a strbuf that hasn't been
made mutable around, though.

Re: [GSoC][RFC][PATCH 2/2] STRBUF_INIT_CONST: Adapting strbuf_* functions

[Robear Selwans]

All what you said makes a lot of sense. Thanks for the help.

Re: [GSoC][RFC][PATCH 2/2] STRBUF_INIT_CONST: Adapting strbuf_* functions

[Abhishek Kumar]

Greetings Robear,

I don't have the expertise to talk about the larger picture here, but
I have the following suggestions if you do go with it.

I don't agree with this patchset being two different patches - The
changes in the second patch eliminate possibly buggy behavior of using
the new function.

I would also prefer the term "immutable" over "const" since const
already has implications in C programming.

> STRBUF_INIT_CONST: a new way to initialize strbuf

Use imperative mood and be more specific in the commit title -
`strbuf: Teach strbuf to initialize immutable strings`

> In a previous commit, a new function `STRBUF_INIT_CONST(const_str)`,
> which would allow for the quick initialization of constant `strbuf`s,
> was introduced.

Redundant if you merge both patches.

In this commit, I check through the strbuf_* functions to edit the ones
that would try to edit the passed `strbuf` so that they are guaranteed to
behave in a predictable way when met with a constant.

> Added Functions:
>   `strbuf_make_var`
>
> Updated Functions:
>   `strbuf_grow`
>   `strbuf_setlen`
>   `strbuf_trim`
>    `strbuf_trim_trailing_dir_sep`
>    `strbuf_trim_trailing_newline`
>    `strbuf_tolower`
>    `strbuf_splice`
>
> Functions where comments were added to clarify the expected behavior:
>    `strbuf_getcwd`

I feel this is self-explanatory when you go through the diff.

Signed-off-by: Robear Selwans <robear.selwans@outlook.com>
---
 strbuf.c | 25 +++++++++++++++++++++++++
 strbuf.h | 14 ++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/strbuf.c b/strbuf.c
index f19da55b07..48af039b6e 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -89,6 +89,8 @@ void strbuf_attach(struct strbuf *sb, void *buf,
size_t len, size_t alloc)

 void strbuf_grow(struct strbuf *sb, size_t extra)
 {
+    if (sb->len > sb->alloc)
+        strbuf_make_var(sb);
     int new_buf = !sb->alloc;
     if (unsigned_add_overflows(extra, 1) ||
         unsigned_add_overflows(sb->len, extra + 1))
@@ -100,8 +102,18 @@ void strbuf_grow(struct strbuf *sb, size_t extra)
         sb->buf[0] = '\0';
 }

+void strbuf_make_var(struct strbuf *sb)
+{
+    char* str_cpy;
+    ALLOC_ARRAY(str_cpy, sb->len + 1);
+    memcpy(str_cpy, sb->buf, sb->len);
+    strbuf_attach(sb, str_cpy, sb->len, sb->len + 1);
+}
+
 void strbuf_trim(struct strbuf *sb)
 {
+    if (sb->len > sb->alloc)
+        strbuf_make_var(sb);
     strbuf_rtrim(sb);
     strbuf_ltrim(sb);
 }
@@ -115,6 +127,8 @@ void strbuf_rtrim(struct strbuf *sb)

 void strbuf_trim_trailing_dir_sep(struct strbuf *sb)
 {
+    if (sb->len > sb->alloc)
+        strbuf_make_var(sb);
     while (sb->len > 0 && is_dir_sep((unsigned char)sb->buf[sb->len - 1]))
         sb->len--;
     sb->buf[sb->len] = '\0';
@@ -122,6 +136,9 @@ void strbuf_trim_trailing_dir_sep(struct strbuf *sb)

 void strbuf_trim_trailing_newline(struct strbuf *sb)
 {
+    if (sb->buf[sb->len - 1] == '\n')
> +        if (sb->len > sb->alloc)
> +            strbuf_make_var(sb);

Enclose this explicitly in braces.

     if (sb->len > 0 && sb->buf[sb->len - 1] == '\n') {
         if (--sb->len > 0 && sb->buf[sb->len - 1] == '\r')
             --sb->len;
@@ -158,6 +175,8 @@ int strbuf_reencode(struct strbuf *sb, const char
*from, const char *to)

 void strbuf_tolower(struct strbuf *sb)
 {
+    if (sb->len > sb->alloc)
+        strbuf_make_var(sb);
     char *p = sb->buf, *end = sb->buf + sb->len;
     for (; p < end; p++)
         *p = tolower(*p);
@@ -234,6 +253,8 @@ void strbuf_splice(struct strbuf *sb, size_t pos,
size_t len,
         die("`pos' is too far after the end of the buffer");
     if (pos + len > sb->len)
         die("`pos + len' is too far after the end of the buffer");
+    if (sb->len > sb->alloc)
+        strbuf_make_var(sb);

     if (dlen >= len)
         strbuf_grow(sb, dlen - len);
@@ -572,6 +593,10 @@ int strbuf_readlink(struct strbuf *sb, const char
*path, size_t hint)

 int strbuf_getcwd(struct strbuf *sb)
 {
+    /*
+     * If sb was a constant, then an error would lead to
+     * strbuf_release() being called on the sb.
+     */
     size_t oldalloc = sb->alloc;
     size_t guessed_len = 128;

diff --git a/strbuf.h b/strbuf.h
index 1a1753424c..a33bc4d90e 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -128,6 +128,10 @@ static inline void strbuf_swap(struct strbuf *a,
struct strbuf *b)
     SWAP(*a, *b);
 }

> +/**
> + * Constant string buffer is turned into a variable one.
> + */
> +void strbuf_make_var(struct strbuf *sb);

Use imperative mood - Convert an immutable string buffer to a variable
string buffer.

 /**
  * Functions related to the size of the buffer
@@ -160,6 +164,16 @@ void strbuf_grow(struct strbuf *sb, size_t amount);
  */
 static inline void strbuf_setlen(struct strbuf *sb, size_t len)
 {
> +    if (sb->len > sb->alloc)
> +    {
> +        if (!len)
> +        {
> +            sb->buf = strbuf_slopbuf;
> +            return;
> +        }
> +        else
> +            strbuf_make_var(sb);
> +    }

Use braces on the same line as the condition.

     if (len > (sb->alloc ? sb->alloc - 1 : 0))
         die("BUG: strbuf_setlen() beyond buffer");
     sb->len = len;
--
2.17.1