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@xxxxxx>
---
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
