From: Eric Sunshine <sunshine@sunshineco.com>
To: Duy Nguyen <pclouds@gmail.com>
Cc: Git List <git@vger.kernel.org>
Subject: Re: [PATCH 04/36] t/helper: merge test-lazy-init-name-hash into test-tool
Date: Sun, 18 Mar 2018 04:47:20 -0400 [thread overview]
Message-ID: <CAPig+cRGpD1_U5PgO=-1wrsiyWqY4GmE3tWNdgCnuAh7PUjvHg@mail.gmail.com> (raw)
In-Reply-To: <CACsJy8CeXZJashmh+sLykRR4Mm_EwnASMVyB-4ztEcfQXU7MXw@mail.gmail.com>
On Sun, Mar 18, 2018 at 4:25 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sun, Mar 18, 2018 at 3:11 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sat, Mar 17, 2018 at 3:53 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>>> -extern int test_lazy_init_name_hash(struct index_state *istate, int try_threaded);
>>> +extern int lazy_init_name_hash_for_testing(struct index_state *istate, int try_threaded);
>>
>> I get why you renamed this since the "main" function in the test
>> program wants to be called 'test_lazy_init_name_hash'...
>>
>>> diff --git a/t/helper/test-lazy-init-name-hash.c b/t/helper/test-lazy-init-name-hash.c
>>> @@ -9,6 +10,9 @@ static int perf;
>>> +static int (*init_name_hash)(struct index_state *istate, int try_threaded) =
>>> + lazy_init_name_hash_for_testing;
>>> +
>>> @@ -33,9 +37,9 @@ static void dump_run(void)
>>> if (single) {
>>> - test_lazy_init_name_hash(&the_index, 0);
>>> + init_name_hash(&the_index, 0);
>>
>> ... but I'm having trouble understanding why this indirection through
>> 'init_name_hash' is used rather than just calling
>> lazy_init_name_hash_for_testing() directly. Am I missing something
>> obvious or is 'init_name_hash' just an unneeded artifact of an earlier
>> iteration before the rename in cache.{c,h}?
>
> Nope. It just feels too long to me and because we're already in the
> test I don't feel the need to repeat _for_testing everywhere. If it's
> confusing, I'll remove init_name_hash.
Without an explanatory in-code comment, I'd guess that someone coming
across this in the future will also stumble over it just like I did in
the review.
What if, instead, you rename test_lazy_init_name_hash() to
lazy_init_name_hash_test(), shifting 'test' from the front to the back
of the name? That way, the name remains the same length at the call
sites in the test helper, and you don't have to introduce the
confusing, otherwise unneeded 'init_name_hash'.
next prev parent reply other threads:[~2018-03-18 8:47 UTC|newest]
Thread overview: 110+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-17 7:53 [PATCH 00/36] Combine t/helper binaries into a single one Nguyễn Thái Ngọc Duy
2018-03-17 7:53 ` [PATCH 01/36] t/helper: add an empty test-tool program Nguyễn Thái Ngọc Duy
2018-03-17 7:53 ` [PATCH 02/36] t/helper: merge test-chmtime into test-tool Nguyễn Thái Ngọc Duy
2018-03-17 7:53 ` [PATCH 03/36] t/helper: merge test-sha1 " Nguyễn Thái Ngọc Duy
2018-03-17 7:53 ` [PATCH 04/36] t/helper: merge test-lazy-init-name-hash " Nguyễn Thái Ngọc Duy
2018-03-18 2:11 ` Eric Sunshine
2018-03-18 8:25 ` Duy Nguyen
2018-03-18 8:47 ` Eric Sunshine [this message]
2018-03-19 9:40 ` Jeff Hostetler
2018-03-19 15:51 ` Duy Nguyen
2018-03-17 7:53 ` [PATCH 05/36] t/helper: merge test-config " Nguyễn Thái Ngọc Duy
2018-03-17 7:53 ` [PATCH 06/36] t/helper: merge test-ctype " Nguyễn Thái Ngọc Duy
2018-03-17 7:53 ` [PATCH 07/36] t/helper: merge test-date " Nguyễn Thái Ngọc Duy
2018-03-17 7:53 ` [PATCH 08/36] t/helper: merge (unused) test-delta " Nguyễn Thái Ngọc Duy
2018-03-17 7:53 ` [PATCH 09/36] t/helper: merge test-drop-caches " Nguyễn Thái Ngọc Duy
2018-03-17 7:53 ` [PATCH 10/36] t/helper: merge test-dump-cache-tree " Nguyễn Thái Ngọc Duy
2018-03-17 7:53 ` [PATCH 11/36] t/helper: merge test-dump-split-index " Nguyễn Thái Ngọc Duy
2018-03-17 7:53 ` [PATCH 12/36] t/helper: merge test-example-decorate " Nguyễn Thái Ngọc Duy
2018-03-17 7:53 ` [PATCH 13/36] t/helper: merge test-genrandom " Nguyễn Thái Ngọc Duy
2018-03-17 7:53 ` [PATCH 14/36] t/helper: merge test-hashmap " Nguyễn Thái Ngọc Duy
2018-03-17 7:54 ` [PATCH 15/36] t/helper: merge test-index-version " Nguyễn Thái Ngọc Duy
2018-03-17 7:54 ` [PATCH 16/36] t/helper: merge (unused) test-match-trees " Nguyễn Thái Ngọc Duy
2018-03-17 7:54 ` [PATCH 17/36] t/helper: merge (unused) test-mergesort " Nguyễn Thái Ngọc Duy
2018-03-17 7:54 ` [PATCH 18/36] t/helper: merge test-mktemp " Nguyễn Thái Ngọc Duy
2018-03-17 7:54 ` [PATCH 19/36] t/helper: merge test-online-cpus " Nguyễn Thái Ngọc Duy
2018-03-17 7:54 ` [PATCH 20/36] t/helper: merge test-path-utils " Nguyễn Thái Ngọc Duy
2018-03-17 7:54 ` [PATCH 21/36] t/helper: merge test-prio-queue " Nguyễn Thái Ngọc Duy
2018-03-17 7:54 ` [PATCH 22/36] t/helepr: merge test-read-cache " Nguyễn Thái Ngọc Duy
2018-03-18 2:21 ` Eric Sunshine
2018-03-17 7:54 ` [PATCH 23/36] t/helper: merge test-ref-store " Nguyễn Thái Ngọc Duy
2018-03-17 7:54 ` [PATCH 24/36] t/helper: merge test-regex " Nguyễn Thái Ngọc Duy
2018-03-17 7:54 ` [PATCH 25/36] t/helper: merge test-revision-walking " Nguyễn Thái Ngọc Duy
2018-03-17 7:54 ` [PATCH 26/36] t/helper: merge test-run-command " Nguyễn Thái Ngọc Duy
2018-03-17 7:54 ` [PATCH 27/36] t/helper: merge test-scrap-cache-tree " Nguyễn Thái Ngọc Duy
2018-03-17 7:54 ` [PATCH 28/36] t/helper: merge test-sha1-array " Nguyễn Thái Ngọc Duy
2018-03-17 7:54 ` [PATCH 29/36] t/helper: merge test-sigchain " Nguyễn Thái Ngọc Duy
2018-03-17 7:54 ` [PATCH 30/36] t/helper: merge test-strcmp-offset " Nguyễn Thái Ngọc Duy
2018-03-17 7:54 ` [PATCH 31/36] t/helper: merge test-string-list " Nguyễn Thái Ngọc Duy
2018-03-17 7:54 ` [PATCH 32/36] t/helper: merge test-submodule-config " Nguyễn Thái Ngọc Duy
2018-03-17 7:54 ` [PATCH 33/36] t/helper: merge test-subprocess " Nguyễn Thái Ngọc Duy
2018-03-17 7:54 ` [PATCH 34/36] t/helper: merge test-urlmatch-.. " Nguyễn Thái Ngọc Duy
2018-03-18 2:32 ` Eric Sunshine
2018-03-17 7:54 ` [PATCH 35/36] t/helper: merge test-wildmatch " Nguyễn Thái Ngọc Duy
2018-03-17 7:54 ` [PATCH 36/36] t/helper: merge test-write-cache " Nguyễn Thái Ngọc Duy
2018-03-24 7:42 ` [PATCH v2 00/36] Combine t/helper binaries into a single one Nguyễn Thái Ngọc Duy
2018-03-24 7:42 ` [PATCH v4 1/7] t7700: have closing quote of a test at the beginning of line Nguyễn Thái Ngọc Duy
2018-03-24 7:42 ` [PATCH v2 01/36] t/helper: add an empty test-tool program Nguyễn Thái Ngọc Duy
2018-03-24 7:42 ` [PATCH v4 2/7] repack: add --keep-pack option Nguyễn Thái Ngọc Duy
2018-03-24 7:42 ` [PATCH v2 02/36] t/helper: merge test-chmtime into test-tool Nguyễn Thái Ngọc Duy
2018-03-24 7:42 ` [PATCH v4 3/7] gc: add --keep-largest-pack option Nguyễn Thái Ngọc Duy
2018-03-24 9:36 ` Ævar Arnfjörð Bjarmason
2018-03-24 12:13 ` Duy Nguyen
2018-03-26 2:05 ` Junio C Hamano
2018-03-24 7:42 ` [PATCH v2 03/36] t/helper: merge test-sha1 into test-tool Nguyễn Thái Ngọc Duy
2018-03-24 7:42 ` [PATCH v4 4/7] gc: add gc.bigPackThreshold config Nguyễn Thái Ngọc Duy
2018-03-24 7:42 ` [PATCH v2 04/36] t/helper: merge test-lazy-init-name-hash into test-tool Nguyễn Thái Ngọc Duy
2018-03-24 7:42 ` [PATCH v4 5/7] gc: handle a corner case in gc.bigPackThreshold Nguyễn Thái Ngọc Duy
2018-03-24 7:42 ` [PATCH v2 05/36] t/helper: merge test-config into test-tool Nguyễn Thái Ngọc Duy
2018-03-24 7:42 ` [PATCH v4 6/7] gc --auto: exclude base pack if not enough mem to "repack -ad" Nguyễn Thái Ngọc Duy
2018-03-24 7:42 ` [PATCH v2 06/36] t/helper: merge test-ctype into test-tool Nguyễn Thái Ngọc Duy
2018-03-24 7:42 ` [PATCH v4 7/7] pack-objects: show some progress when counting kept objects Nguyễn Thái Ngọc Duy
2018-03-24 7:42 ` [PATCH v2 07/36] t/helper: merge test-date into test-tool Nguyễn Thái Ngọc Duy
2018-03-24 7:44 ` [PATCH v2 00/36] Combine t/helper binaries into a single one Nguyễn Thái Ngọc Duy
2018-03-24 7:44 ` [PATCH v2 01/36] t/helper: add an empty test-tool program Nguyễn Thái Ngọc Duy
2018-03-26 15:27 ` Johannes Schindelin
2018-03-26 17:09 ` Duy Nguyen
2018-03-26 22:14 ` Johannes Schindelin
2018-03-26 23:07 ` SZEDER Gábor
2018-03-27 13:57 ` Johannes Schindelin
2018-03-24 7:44 ` [PATCH v2 02/36] t/helper: merge test-chmtime into test-tool Nguyễn Thái Ngọc Duy
2018-03-24 7:44 ` [PATCH v2 03/36] t/helper: merge test-sha1 " Nguyễn Thái Ngọc Duy
2018-03-24 7:44 ` [PATCH v2 04/36] t/helper: merge test-lazy-init-name-hash " Nguyễn Thái Ngọc Duy
2018-03-24 7:44 ` [PATCH v2 05/36] t/helper: merge test-config " Nguyễn Thái Ngọc Duy
2018-03-24 7:44 ` [PATCH v2 06/36] t/helper: merge test-ctype " Nguyễn Thái Ngọc Duy
2018-03-24 7:44 ` [PATCH v2 07/36] t/helper: merge test-date " Nguyễn Thái Ngọc Duy
2018-03-24 7:44 ` [PATCH v2 08/36] t/helper: merge (unused) test-delta " Nguyễn Thái Ngọc Duy
2018-03-24 7:44 ` [PATCH v2 09/36] t/helper: merge test-drop-caches " Nguyễn Thái Ngọc Duy
2018-03-24 7:44 ` [PATCH v2 10/36] t/helper: merge test-dump-cache-tree " Nguyễn Thái Ngọc Duy
2018-03-24 7:44 ` [PATCH v2 11/36] t/helper: merge test-dump-split-index " Nguyễn Thái Ngọc Duy
2018-03-24 7:44 ` [PATCH v2 12/36] t/helper: merge test-example-decorate " Nguyễn Thái Ngọc Duy
2018-03-24 7:44 ` [PATCH v2 13/36] t/helper: merge test-genrandom " Nguyễn Thái Ngọc Duy
2018-03-24 7:44 ` [PATCH v2 14/36] t/helper: merge test-hashmap " Nguyễn Thái Ngọc Duy
2018-03-24 7:44 ` [PATCH v2 15/36] t/helper: merge test-index-version " Nguyễn Thái Ngọc Duy
2018-03-24 7:44 ` [PATCH v2 16/36] t/helper: merge (unused) test-match-trees " Nguyễn Thái Ngọc Duy
2018-03-24 7:44 ` [PATCH v2 17/36] t/helper: merge (unused) test-mergesort " Nguyễn Thái Ngọc Duy
2018-03-24 7:44 ` [PATCH v2 18/36] t/helper: merge test-mktemp " Nguyễn Thái Ngọc Duy
2018-03-24 7:44 ` [PATCH v2 19/36] t/helper: merge test-online-cpus " Nguyễn Thái Ngọc Duy
2018-03-24 7:44 ` [PATCH v2 20/36] t/helper: merge test-path-utils " Nguyễn Thái Ngọc Duy
2018-03-24 7:44 ` [PATCH v2 21/36] t/helper: merge test-prio-queue " Nguyễn Thái Ngọc Duy
2018-03-24 7:44 ` [PATCH v2 22/36] t/helper: merge test-read-cache " Nguyễn Thái Ngọc Duy
2018-03-24 7:44 ` [PATCH v2 23/36] t/helper: merge test-ref-store " Nguyễn Thái Ngọc Duy
2018-03-24 7:44 ` [PATCH v2 24/36] t/helper: merge test-regex " Nguyễn Thái Ngọc Duy
2018-03-24 7:44 ` [PATCH v2 25/36] t/helper: merge test-revision-walking " Nguyễn Thái Ngọc Duy
2018-03-24 7:44 ` [PATCH v2 26/36] t/helper: merge test-run-command " Nguyễn Thái Ngọc Duy
2018-03-24 7:44 ` [PATCH v2 27/36] t/helper: merge test-scrap-cache-tree " Nguyễn Thái Ngọc Duy
2018-03-24 7:44 ` [PATCH v2 28/36] t/helper: merge test-sha1-array " Nguyễn Thái Ngọc Duy
2018-03-24 7:44 ` [PATCH v2 29/36] t/helper: merge test-sigchain " Nguyễn Thái Ngọc Duy
2018-03-24 7:44 ` [PATCH v2 30/36] t/helper: merge test-strcmp-offset " Nguyễn Thái Ngọc Duy
2018-03-24 7:45 ` [PATCH v2 31/36] t/helper: merge test-string-list " Nguyễn Thái Ngọc Duy
2018-03-24 7:45 ` [PATCH v2 32/36] t/helper: merge test-submodule-config " Nguyễn Thái Ngọc Duy
2018-03-24 7:45 ` [PATCH v2 33/36] t/helper: merge test-subprocess " Nguyễn Thái Ngọc Duy
2018-03-24 7:45 ` [PATCH v2 34/36] t/helper: merge test-urlmatch-normalization " Nguyễn Thái Ngọc Duy
2018-03-24 7:45 ` [PATCH v2 35/36] t/helper: merge test-wildmatch " Nguyễn Thái Ngọc Duy
2018-03-24 7:45 ` [PATCH v2 36/36] t/helper: merge test-write-cache " Nguyễn Thái Ngọc Duy
2018-03-24 12:50 ` [PATCH v2 00/36] Combine t/helper binaries into a single one Ævar Arnfjörð Bjarmason
2018-03-24 12:57 ` Duy Nguyen
2018-03-26 2:16 ` Junio C Hamano
2018-03-27 14:00 ` Johannes Schindelin
2018-03-27 15:42 ` Junio C Hamano
2018-03-27 16:12 ` Johannes Schindelin
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='CAPig+cRGpD1_U5PgO=-1wrsiyWqY4GmE3tWNdgCnuAh7PUjvHg@mail.gmail.com' \
--to=sunshine@sunshineco.com \
--cc=git@vger.kernel.org \
--cc=pclouds@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).