From: Phil Hord <phil.hord@gmail.com>
Date: Mon, 29 Oct 2012 17:38:28 GMT
Subject: Re: [PATCH v2] git-submodule add: Add -r/--record option.
Message-ID: <CABURp0pFLi+2A+9wi-ZamiRze2u6z+6oyoCsNpWOLq_cq2L1rQ@mail.gmail.com>
In-Reply-To: <20121029114310.GA16046@sigill.intra.peff.net>

On Mon, Oct 29, 2012 at 7:43 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Oct 29, 2012 at 07:29:45AM -0400, W. Trevor King wrote:
>
>> On Mon, Oct 29, 2012 at 06:58:55AM -0400, Jeff King wrote:
>> > Can you send an updated version of the patch that summarizes the
>> > situation in the commit message?
>>
>> Sure.  Should I include Phil's $submodule_<var-name> export, or would
>> you rather have that be a separate series?
>
> I think it probably makes sense as a separate patch in the same series,
> since it is meant to support the same workflows.

I agree.  I did expect to clean it up some, but also to suffer some
review.  Feel free to clean it up as you see fit and submit it with
your series.

> I am not sure it is sufficient as-is, though. It does not seem to ever
> clear variables, only set them, which means that values could leak
> across iterations of the loop,  [...] E.g., when
> the first submodule has submodule.*.foo set but the second one does not,
> you will still end up with $submodule_foo set when you process the
> second one.

Good point.  That should not happen.

> or down to recursive calls.

Frankly, I consider that to be a feature.  However, I can see how it
would be considered inconsistent in many ways, so it's probably best
to squash it.  :-\

Phil