On Mon, Sep 24, 2018 at 6:31 AM Derrick Stolee <[email protected]> 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
