From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: X-Spam-Status: No, score=-3.9 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI, SPF_HELO_NONE,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.6 Received: from out1.vger.email (out1.vger.email [IPv6:2620:137:e000::1:20]) by dcvr.yhbt.net (Postfix) with ESMTP id A1B5C1F428 for ; Wed, 15 Mar 2023 17:18:13 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232315AbjCORSK (ORCPT ); Wed, 15 Mar 2023 13:18:10 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48924 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232351AbjCORSH (ORCPT ); Wed, 15 Mar 2023 13:18:07 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A8A1284F7C for ; Wed, 15 Mar 2023 10:18:00 -0700 (PDT) Received: (qmail 18424 invoked by uid 109); 15 Mar 2023 17:18:00 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Wed, 15 Mar 2023 17:18:00 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 2293 invoked by uid 111); 15 Mar 2023 17:17:59 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Wed, 15 Mar 2023 13:17:59 -0400 Authentication-Results: peff.net; auth=none Date: Wed, 15 Mar 2023 13:17:59 -0400 From: Jeff King To: =?utf-8?B?w4Z2YXIgQXJuZmrDtnLDsA==?= Bjarmason Cc: Derrick Stolee via GitGitGadget , git@vger.kernel.org, gitster@pobox.com, me@ttaylorr.com, vdye@github.com, Derrick Stolee Subject: Re: [PATCH v2 1/8] for-each-ref: add --stdin option Message-ID: References: <230315.86h6umxh7c.gmgdl@evledraar.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <230315.86h6umxh7c.gmgdl@evledraar.gmail.com> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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