On Tue, Oct 11, 2011 at 3:34 AM, Jens Lehmann <Jens.Lehmann@xxxxxx> wrote: > 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. >> 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. > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html