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

From: Jacob Keller <jacob.keller@gmail.com>
Date: Aug 20, 2016, 06:16 UTC
Message-ID: <CA+P7+xoazoY49cXMghVQmCd=X-aEj6xCTqQqr69zciFZAqmw_Q@mail.gmail.com>
In-Reply-To: <CAGZ79kZqkHO58kUvP772jfTUgyYXxYuDkgGB1sOTYYQ6nLCP4A@mail.gmail.com>

On Fri, Aug 19, 2016 at 5:02 PM, Stefan Beller <sbeller@google.com> wrote:

Show 12 quoted lines

> 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.

Show 8 quoted lines

>
>> +               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?

Show 5 quoted lines

> 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

Show 11 quoted lines

>
> 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