Re: [PATCH v2 1/8] for-each-ref: add --stdin option
- From
Jeff King <peff@peff.net>
- Date
- Mar 15, 2023, 17:17 UTC
- Message-ID
- <ZBH9xxJDTT3jzm5u@coredump.intra.peff.net>
- In-Reply-To
- <230315.86h6umxh7c.gmgdl@evledraar.gmail.com>
On Wed, Mar 15, 2023 at 02:37:39PM +0100, Ævar Arnfjörð Bjarmason wrote:
Show 31 quoted lines
> - CALLOC_ARRAY(filter.name_patterns, alloc);
> -
> - while (strbuf_getline(&line, stdin) != EOF) {
> - ALLOC_GROW(filter.name_patterns, nr + 1, alloc);
> - filter.name_patterns[nr++] = strbuf_detach(&line, NULL);
> - }
> -
> - /* Add a terminating NULL string. */
> - ALLOC_GROW(filter.name_patterns, nr + 1, alloc);
> - filter.name_patterns[nr + 1] = NULL;
> + while (strbuf_getline(&line, stdin) != EOF)
> + strvec_push(&stdin_pat, line.buf);
> + filter.name_patterns = stdin_pat.v;
> } else {
> filter.name_patterns = argv;
> }
> @@ -123,10 +117,6 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
> free_commit_list(filter.with_commit);
> free_commit_list(filter.no_commit);
> ref_sorting_release(sorting);
> - if (from_stdin) {
> - for (size_t i = 0; filter.name_patterns[i]; i++)
> - free(filter.name_patterns[i]);
> - free(filter.name_patterns);
> - }
> + strvec_clear(&stdin_pat);
> return 0;
> }
>
> It *is* an extra copy though, as your implementation re-uses the strbuf
> we already allocated.At first I thought you meant "extra allocation" here. But you really do mean an extra copy of the bytes.
The number of allocations is the same either way. In the original, we detach the strbuf in each iteration of the loop as it becomes the final entry in the array, but then have to allocate a new strbuf for the next iteration. With a strvec, we can reuse the same strbuf over and over, but make a new allocation when we add it to the strvec.
So yes, we end up with an extra memcpy() of the bytes. But the flip side is that the final allocations we store in the strvec are correctly sized, without the extra slop that the strbuf added while reading.
> But presumably that's trivial in this case, and if we care I think we > should resurrect something like [1] instead, i.e. we could just teach > the strvec API to have a strvec_push_nodup(). But I doubt that in this > case it'll matter.
Yeah, I'd agree it is not important either way in this case. But I wanted to think it through above, just because it's not clear to me that even in a tight loop, the "allocate buffer and then attach to the strvec" approach would be the better tradeoff.
I guess it would make sense to wait for a case where it _does_ matter and then we could experiment with the two approaches. ;)
-Peff