git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "René Scharfe" <l.s.r@web.de>
To: Johannes Sixt <j6t@kdbg.org>, Robear Selwans <rwagih.rw@gmail.com>
Cc: git@vger.kernel.org,
	"brian m. carlson" <sandals@crustytoothpaste.net>,
	Jeff King <peff@peff.net>, Junio C Hamano <gitster@pobox.com>,
	Derrick Stolee <stolee@gmail.com>
Subject: Re: [GSoC][RFC][PATCH 1/2] STRBUF_INIT_CONST: a new way to initialize strbuf
Date: Thu, 20 Feb 2020 19:49:18 +0100	[thread overview]
Message-ID: <113d4221-d7bd-cf0c-ebfc-1fc08442c303@web.de> (raw)
In-Reply-To: <467c035f-c7cd-01e1-e64c-2c915610de01@kdbg.org>

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

  reply	other threads:[~2020-02-20 18:49 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-18  4:18 [GSoC][RFC][PATCH 0/2] STRBUF_INIT_CONST Cover Robear Selwans
2020-02-18  4:18 ` [GSoC][RFC][PATCH 1/2] STRBUF_INIT_CONST: a new way to initialize strbuf Robear Selwans
2020-02-18  6:21   ` Jeff King
2020-02-18 14:19     ` Robear Selwans
2020-02-18 20:33       ` Jeff King
2020-02-19  8:13   ` Johannes Sixt
2020-02-20 18:49     ` René Scharfe [this message]
2020-02-21  5:21       ` Robear Selwans
2020-02-27  6:50       ` Derrick Stolee
2020-02-27 15:55         ` René Scharfe
2020-02-18  4:18 ` [GSoC][RFC][PATCH 2/2] STRBUF_INIT_CONST: Adapting strbuf_* functions Robear Selwans

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=113d4221-d7bd-cf0c-ebfc-1fc08442c303@web.de \
    --to=l.s.r@web.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=j6t@kdbg.org \
    --cc=peff@peff.net \
    --cc=rwagih.rw@gmail.com \
    --cc=sandals@crustytoothpaste.net \
    --cc=stolee@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).