Re: git submodules implementation question

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 29, 2016, 23:15 UTC
Message-ID: <xmqqbn0b6ua8.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAN5XQfsg_sJbyjfdc=-e85jiCQNUqagwgh6TVOXN+NskZ7KkVw@mail.gmail.com>

Uma Srinivasan <usrinivasan@twitter.com> writes:

Show 23 quoted lines

> On Mon, Aug 29, 2016 at 2:13 PM, Uma Srinivasan <usrinivasan@twitter.com> wrote:
>> Ok that makes sense. Thanks much.
>>
>> Uma
>>
> With respect to my original problem with a corrupted .git directory
> under the submodule directory, I am thinking of adding the following 4
> lines marked with ### to is_submodule_modified() to detect the
> corrupted dir and die quickly instead of forking several child
> processes:
>
>                strbuf_addf(&buf, "%s/.git", path);
>                git_dir = read_gitfile(buf.buf);
>                if (!git_dir) {
>                  ### strbuf_addf(&head_ref, "%s/HEAD",buf.buf);
>                  ### if (strbuf_read_file(&temp_ref, head_ref.buf,0) < 0) {
>                           ### die("Corrupted .git dir in submodule %s", path);
>                  ###}
>                          git_dir = buf.buf;
>               }
>
> This fixes my issue but what do you think? Is this the right way to
> fix it? Is there a better way?

I think we already have a helper function that does a lot better than "does it have a file called HEAD" to ask "is this a git directory?" and its name I think is "is_git_directory" (I know, we are not imaginative when naming our functions).

As to the check makes sense in the context of this function, I am not an expert to judge. I'd expect Jens, Heiko and/or Stefan to know better than I do.