Re: [PATCHv2 2/3] submodule-config: rename commit_sha1 to commit_or_tree

[Stefan Beller <sbeller@google.com>]

On Mon, Nov 21, 2016 at 4:16 PM, Brandon Williams <bmwill@google.com> wrote:
> On 11/21, Stefan Beller wrote:
>> On Mon, Nov 21, 2016 at 4:11 PM, Brandon Williams <bmwill@google.com> wrote:
>> > On 11/21, Stefan Beller wrote:
>> >>
>> >>       switch (lookup_type) {
>> >> @@ -448,7 +448,8 @@ static const struct submodule *config_from(struct submodule_cache *cache,
>> >>
>> >>       /* fill the submodule config into the cache */
>> >>       parameter.cache = cache;
>> >> -     parameter.commit_sha1 = commit_sha1;
>> >> +     // todo: get the actual tree here:
>> >
>> > s/todo/TODO
>> >
>> > Makes it more clear that this is a TODO
>> >
>>
>> The // is more annoying here. I'll review these changes closely
>> before sending out v3.
>
> Well I prefer // to /* */ but that's not the style we use :)

background: Initially I assume we would need to do work here as that part
is exposed to the user in error messages, such that we maybe want to
go the reverse direction and not state a tree but instead the commit containing
it. Since then I decided that it is not worth to optimize for some
hypothetical future
and hence I did not go with the internal todo note I put there. Then I
forgot about
it and it just showed up in the patch here.

Having looked through the patch again, the rest looks clean to me.

Stefan

>
> --
> Brandon Williams