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

[Stefan Beller <sbeller@google.com>]

On Mon, Sep 24, 2018 at 6:31 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> 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.

> > +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.

> > +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