Re: [PATCH v3 1/2] t/lib-read-tree-m-3way: modernize style

From: Christian Couder <christian.couder@gmail.com>
Date: Feb 7, 2022, 11:41 UTC
Message-ID: <CAP8UFD2e6xEQa7045Mup8LAHBs=B9EwwvZcLMsp43OM-dsY9eQ@mail.gmail.com>
In-Reply-To: <20220202064300.3601-1-shaoxuan.yuan02@gmail.com>

On Fri, Feb 4, 2022 at 6:00 AM Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> wrote:

Show 7 quoted lines

>
> Many invocations of the test_expect_success command in this
> file are written in old style where the command, an optional
> prerequisite, and the test title are written on separate
> lines, and the executable script string begins on its own
> line, and these lines are pasted together with backslashes
> as necessary.

It's not very clear here if "these lines" means only the separate lines with the command, an optional prerequisite, and the test title, or if it also means the first (or maybe many) line(s) of the executable script string.

Show 5 quoted lines

> An invocation of the test_expect_success command in modern
> test scripts however writes the prerequisite and the title
> on the same line as the test_expect_success command itself,
> and ends the line with a single quote that begins the
> executable script string.

It could also be 'test_expect_failure' instead of 'test_expect_success'.

> Update the style for uniformity.
>
> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>

Show 9 quoted lines

>  for p in M? Z/M?
>  do
>      echo This is modified $p in the branch A. >$p
> -    test_expect_success \
> -       'change in branch A (modification)' \
> -        "git update-index $p"
> +    test_expect_success 'change in branch A (modification)' '
> +        git update-index $p
> +    '

The above is not just about moving single quotes from one line to another, but it changes some double quotes to single quotes, which means that $p might not be interpreted in the same way. This is not just a style issue and it should be explained in the commit message why it's ok to make this change.

Show 11 quoted lines

>  done
>
>  for p in AN AA Z/AN Z/AA
>  do
>      echo This is added $p in the branch A. >$p
> -    test_expect_success \
> -       'change in branch A (addition)' \
> -       "git update-index --add $p"
> +    test_expect_success 'change in branch A (addition)' '
> +           git update-index --add $p
> +    '

Here also some double quotes are changed into single quotes.

>  done

Show 8 quoted lines

>  to_remove=$(echo ?D Z/?D)
>  rm -f $to_remove
> -test_expect_success \
> -    'change in branch B (removal)' \
> -    "git update-index --remove $to_remove"
> +test_expect_success 'change in branch B (removal)' '
> +    git update-index --remove $to_remove
> +'

Here also.

Show 9 quoted lines

>  for p in ?M Z/?M
>  do
>      echo This is modified $p in the branch B. >$p
> -    test_expect_success \
> -       'change in branch B (modification)' \
> -       "git update-index $p"
> +    test_expect_success 'change in branch B (modification)' '
> +           git update-index $p
> +    '

Here also.

>  done