Re: [PATCH 2/2] submodule::module_clone(): silence die() message from module_name()

From: Tay Ray Chuan <rctay89@gmail.com>
Date: Oct 11, 2011, 08:44 UTC
Message-ID: <CALUzUxo6YEEpL_MhT=O9sJSUdwcpKBpeM2O8GkbbyxvqmWCFLQ@mail.gmail.com>
In-Reply-To: <4E9348A8.5000500@web.de>

On Tue, Oct 11, 2011 at 3:34 AM, Jens Lehmann <Jens.Lehmann@web.de> wrote:

Show 11 quoted lines

> BTW: this patch applies to next
>
> Am 07.10.2011 11:04, schrieb Tay Ray Chuan:
>> The die() message that may occur in module_name() is not really relevant
>> to the user when called from module_clone(); the latter handles the
>> "failure" (no submodule mapping) anyway.
>
> Makes tons of sense, especially as adding a new submodule currently always
> spews out the "No submodule mapping found in .gitmodules for path 'sub'"
> message right before that mapping is added there. Thanks for noticing that
> and ACK on that change from my side.

Thanks for the review.

Show 9 quoted lines

>> Leave other callers of module_name() unchanged, as the die() message
>> shown is either relevant for user consumption (such as those that exit()
>> when the call fails), or will not occur at all (when called with paths
>> returned by module_list()).
>
> Hmm, while I agree on the first reasoning I'm not sure about the second.
> module_list() asks the index for the submodule paths while module_name()
> gets it's input from .gitmodules, so they can (and sometimes will)
> disagree.

Oh, you're right. I neglected to see how module_list() actually worked.

Show 5 quoted lines

> When cmd_foreach() passes an empty "name" variable to the
> spawned command that might still work (and even make sense), but using the
> empty name in cmd_sync() to access the config is looking like an error to
> me. It might make sense to add an "|| exit" at least to the callsite in
> cmd_sync(). Or am I missing something here?

Cc-ed David, who authored cmd_sync().

David, what do you think of Jens' analysis?

In the meantime, I'll probably reword the second paragraph to say that future work will be needed to analyze non- || exit callsites.

-- 
Cheers,
Ray Chuan