Re: [PATCH v9 0/8] submodule show inline diff

[Jacob Keller <jacob.keller@gmail.com>]

On Fri, Aug 19, 2016 at 5:02 PM, Stefan Beller <sbeller@google.com> wrote:
> On Fri, Aug 19, 2016 at 4:34 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
>> -               strbuf_git_path(buf, "%s/%s", "modules", path);
>> +               /*
>> +                * Lookup the submodule name from the config. If that fails
>> +                * fall back to assuming the path is the name.
>> +                */
>> +               submodule_config = submodule_from_path(null_sha1, path);
>
> In case you need to reroll: I'd got with just "sub" as the name for
> the config object
> (that seems to be used all the time and is shorter)
>

Sure.

>
>> +               if (submodule_config)
>> +                       strbuf_git_path(buf, "%s/%s", "modules",
>> +                                       submodule_config->name);
>> +               else
>
> I do not think we want to assume the path as the name for the fallback, though.
>

I couldn't think of anything better to do.... There is no error return
flow, so just... leave it as is maybe?

> If `submodule_config == NULL` then
> a) the path/name doesn't exist in the given version.
>     (If null_sha1 is given, HEAD + working tree is assumed, whereas
>     you could also check for a specific commit of the superproject
>     with another sha1)

I can't check for a specific version here because there is no
mechanism to pass in the value we'd check it in... Maybe I need a
separate function for that to check the specific sha1 instead of using
nullsha1? But.. no I don't think that makes sense, we use the current
working tree to get the submodule and then lookup old references in
it... but if we checked an old tree it might be in the wrong path
which does us no good because the name no longer matches? Hmmm

>
> b) or the submodule config cache was not initialized
>    (missing call to gitmodules_config())
>
> c) There is no c) [at least I never came across another reason for a
> NULL return here]
>
> Using the path as the fallback is errorprone (e.g. to b. in the future
> and then you get the wrong submodule repository which is shaded by
> assuming the path and it is hard to debug later or write forward looking
> tests for that now)

Sure, but if it doesn't exist we just fail.. so what should I put in
the string? just leave it empty? The function doesn't have an error
return at the moment.

>
> Thanks,
> Stefan