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

From: Stefan Beller <sbeller@google.com>
Date: Aug 22, 2016, 17:43 UTC
Message-ID: <CAGZ79kaZS==if2OVz+tusYQGSmhtJ0MgR66_LAd=Fv2yKhw90A@mail.gmail.com>
In-Reply-To: <CA+P7+xoazoY49cXMghVQmCd=X-aEj6xCTqQqr69zciFZAqmw_Q@mail.gmail.com>

On Fri, Aug 19, 2016 at 11:16 PM, Jacob Keller <jacob.keller@gmail.com> wrote:

Show 25 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?
>
>> 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

I agree, null_sha1 is the way to go.

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

I thought a die(...) would be better instead. Looking at the callers of do_submodule_path (which is wrapped via strbuf_git_path_submodule and git_pathdup_submodule), we end up in

./refs/files-backend.c:
    get_packed_ref_cache
    resolve_gitlink_ref_recursive
    read_loose_refs

other submodule related code:
    module_clone
    add_submodule_odb

The refs code doesn't have error handling code at the places where we do the call to submodule path handling, so I think a die(..) is still ok, as these cases would only happen if your super project is hosed. e.g. I think to get into this state you'd roughly do this:

    git submodule update --init
    # make path and name different:
    git mv sub-foo sub-bar

    # (I think) two ways to hose a repository now:

        # a) delete the actual submodule repository
        rm -rf .git/modules/sub-foo

        # or b) rename the submodule name to break the
        # assumption of .git/modules/<name> as the sub repo path.
        git config -f .gitmodules --rename-section submodule.sub-foo
submodule.sub-void

    # now exercise the refs code:
    git status

A wrong path would do no harm in this case; in fact it is better to go with a non existent wrong name as it then falls back to the error handling of the refs code.

However, assume the path would exist. (As that happens when you "swap" two submodules in their path, i.e. when a submodule path becomes another submodules name.

In that case I think "bad things" may happen to the other submodule?

So after typing all this out, I think a call to die(..) is still the way to go.

Thanks, Stefan