On 1/5/2021 12:55 PM, Eric Sunshine wrote:
> On Tue, Jan 5, 2021 at 9:44 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@xxxxxxxxx> wrote:
>> 'git for-each-repo --config=X' should return success without calling any
>> subcommands when the config key 'X' has no value. The current
>> implementation instead segfaults.
>> [...]
>> Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
>> ---
>> diff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c
>> @@ -51,6 +51,9 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix)
>> values = repo_config_get_value_multi(the_repository,
>> config_key);
>> + if (!values)
>> + return result;
>
> Probably not worth a re-roll, but the above has higher cognitive load than:
>
> if (!value)
> return 0;
>
> which indicates clearly that the command succeeded, whereas `return
> result` makes the reader scan all the code above the conditional to
> figure out what values `result` could possibly hold.
True. Looking at this again, it might be better to just update the
loop to be
for (i = 0; values && i < values->nr; i++) {
which would run the loop zero times when there are zero results.
>> diff --git a/t/t0068-for-each-repo.sh b/t/t0068-for-each-repo.sh
>> @@ -27,4 +27,8 @@ test_expect_success 'run based on configured value' '
>> +test_expect_success 'do nothing on empty config' '
>> + git for-each-repo --config=bogus.config -- these args would fail
>> +'
>
> The `these args would fail` arguments add mystery to the test,
> prompting the reader to wonder what exactly is going on: "Fail how?",
> "Is it supposed to fail?". It might be less confusing to use something
> more direct like `nonexistent` or `does not exist`.
I guess the point is that if we actually did try running a subcommand
on a repo (for instance, accidentally running it on the current repo)
then the command would fail when running "git these args would fail".
To me, "nonexistent" or "does not exist" doesn't adequately describe
this (hypothetical) situation.
Perhaps "fail-subcommand" might be better?
-Stolee
