Re: [RFC PATCH v2 12/12] submodule: remove the .gitmodules file when it is empty

From: Antonio Ospite <ao2@ao2.it>
Date: Aug 3, 2018, 08:57 UTC
Message-ID: <20180803105717.7a023507f96651a4769c1fd7@ao2.it>
In-Reply-To: <CAGZ79kaDK9nuUm_hqAZKMHydiaBDhDkM_y1UFwpRHoGjCQoq4w@mail.gmail.com>

On Thu, 2 Aug 2018 14:15:54 -0700 Stefan Beller <sbeller@google.com> wrote:

Show 8 quoted lines

> On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite <ao2@ao2.it> wrote:
> >
> > In particular this makes it possible to really clean things up when
> > removing the last submodule with "git rm".
> 
> This sentence is a continuation of the subject line, and I had to reread
> it to follow along.
>

OK, I'll fix that.

Show 9 quoted lines

> >
> > The rationale is that if git creates .gitmodules when adding the first
> > submodule it should also remove it when removing the last submodule.
> 
> I agree with this sentiment. It seems slightly odd to me to have this tied
> in the same patch series that changes .gitmodules reading behavior
> as I could think of this feature as orthogonal to what this series achieved
> up to patch 10.
>

I will send this as a separate series, I briefly mentioned this possibility in the cover letter.

Show 22 quoted lines

> > -       test_cmp expect actual &&
> > +       test_cmp expect.both_deleted actual &&
> 
> This seems to be the re-occuring pattern in t3600, and given that
> we have
> 
>   cat >expect <<EOF
>   M  .gitmodules
>   D  submod
>   EOF
>   cat >expect.both_deleted<<EOF
>   D  .gitmodules
>   D  submod
>   EOF
> 
> with no other writing of expect in the range, this seems to be correct.
> Maybe worth testing that we do not delete a .gitmodules file if we have
> more than one submodule? (But I would expect this to be covered implicitly
> somewhere in the test suite. If so that would be worth mentioning in the
> commit message instead of writing a test -- just looking quickly we
> do have " git rm --cached submodule2" in t7406 which might be sufficient?)
>

I think I will remove the new test in t7400 as the changes to t3600 should cover that case already.

I will check about the case of more than one submodule and I'll add a comment to the commit message.

[...]

> Thanks for this series!
> Overall it was a pleasant read, though I had some comments.
>

Thanks,
   Antonio

-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?