From: Stefan Beller <sbeller@google.com>
Date: Fri, 18 Nov 2016 23:33:22 GMT
Subject: Re: [PATCH 13/16] submodule: teach unpack_trees() to update submodules
Message-ID: <CAGZ79kb-TKgQm1_p3NyJS7mso2FFWMNHa7F8vwzocjSJfyjXJQ@mail.gmail.com>
In-Reply-To: <f54d446aa7734cb4aec4b51c7b81a2b6@exmbdft7.ad.twosigma.com>

On Tue, Nov 15, 2016 at 4:22 PM, David Turner <David.Turner@twosigma.com> wrote:
>>       msgs[ERROR_NOT_UPTODATE_DIR] =
>>               _("Updating the following directories would lose untracked
>> files in it:\n%s");
>> +     msgs[ERROR_NOT_UPTODATE_SUBMODULE] =
>> +             _("Updating the following submodules would lose modifications
>> in
>> +it:\n%s");
>
> s/it/them/

done, also fixed the existing ERROR_NOT_UPTODATE_DIR.

>> +             if (!S_ISGITLINK(ce->ce_mode)) {
>
> I generally prefer to avoid if (!x) { A } else { B } -- I would rather just see if (x) { B } else { A }.

done.

>> +                             if (submodule_is_interesting(old->name, null_sha1)
>> +                                 && ok_to_remove_submodule(old->name))
>> +                                     return 0;
>> +                     }
>
> Do we need a return 1 in here somewhere?  Because otherwise, we fall through and return 0 later.

Otherwise we would fall through and run

    if (errno == ENOENT)
        return 0;
    return o->gently ? -1 :
        add_rejected_path(o, error_type, ce->name);

which produces different results than 0?