Re: [PATCH v2 1/8] for-each-ref: add --stdin option

[Jeff King <peff@peff.net>]

On Wed, Mar 15, 2023 at 02:37:39PM +0100, Ævar Arnfjörð Bjarmason wrote:

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