Re: [PATCH 1/3] t7001: reformat to newer style

From: Stefan Beller <sbeller@google.com>
Date: Sep 24, 2018, 18:51 UTC
Message-ID: <CAGZ79kaeWa5PRregdoJp82sSsY1-GF7kk33fxf1kbTPcZWu-bg@mail.gmail.com>
In-Reply-To: <7f77be2b-a084-5b8d-df65-1bb4b9c5da82@gmail.com>

On Mon, Sep 24, 2018 at 6:31 AM Derrick Stolee <stolee@gmail.com> wrote:

Show 8 quoted lines

>
> On 9/21/2018 7:58 PM, Stefan Beller wrote:
> > The old formatting style is a real hindrance of getting people up to speed
> > contributing as they use existing code as an example and follow that style.
> > So let's get rid of the old style and reformat it in our current style.
> I was suspicious of your automated approach catching everything, so I
> looked carefully at this patch. There are still a lot of things
> happening that we would not recommend doing in new tests.

Heh, thanks for calling that out. So we're looking at a full formatter instead of a partial formatter that helps moving in the right direction now. :-/

I would prefer to use automation as much as possible for these tasks to keep it easy to apply it at scale once a file is not touched by master..pu any more.

When applying styles manually, there is sometimes a judgement call, which would like to the inevitable bikeshedding that I'd prefer to avoid.

> > +test_expect_success 'moving the file out of subdirectory' '
> > +     cd path0 && git mv COPYING ../path1/COPYING
> > +'
> Perhaps split this line on the &&?

In real modern testing, this could also be

    git -C path0 mv ...

which would also fix the cd.. below and not needing a subshell there either (using -C again).

Looking at this from a higher level, nowadays I would write tests that have more lines in them, instead of having one git command per test.

Show 5 quoted lines

> > +test_expect_success 'moving to existing tracked target with trailing slash' '
> > +     mkdir path2 &&
> > +     >path2/file && git add path2/file &&
> This line in particular looks a bit strange. What is this doing? At
> least we should split the &&.

Yes.

Show 5 quoted lines

> > +test_expect_success 'do not move directory over existing directory' '
> > +     mkdir path0 && mkdir path0/path2 && test_must_fail git mv path2 path0
> > +'
>
> Split this line.

Okay, I'll go manually over these tests to adapt to new style.

Thanks for looking over the patch! Stefan