Re: [PATCH v2] t7001-mv.sh: modernizing test script using functions

From: "Martin Ågren" <martin.agren@gmail.com>
To: Debra Obondo via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Taylor Blau <me@ttaylorr.com>,
	Debra Obondo <debraobondo@gmail.com>
Subject: Re: [PATCH v2] t7001-mv.sh: modernizing test script using functions
Date: Thu, 3 Nov 2022 20:37:21 +0100	[thread overview]
Message-ID: <CAN0heSohT_zDYequyv9-cCALrbpR0yQnSoPA3+YGMhgKxE5K3g@mail.gmail.com> (raw)
In-Reply-To: <pull.1372.v2.git.git.1667500788132.gitgitgadget@gmail.com>

On Thu, 3 Nov 2022 at 19:39, Debra Obondo via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Debra Obondo <debraobondo@gmail.com>
>
> Test script to verify the presence/absence of files, paths, directories,
> symlinks and other features in 'git mv' command are using the command
> format:
>
> 'test (-e|f|d|h|...)'
>
> Replace them with helper functions of format:
>
> 'test_path_is_*'

Ok.

> Changes since v1
>
> Replacing idiomatic helper functions
>
> '! test_path_is_*'
>
> with
>
> 'test_path_is_missing'
>

The above "Changes since v1" and what I've quoted here should probably
be dropped. We prefer our patches to look as if they've appeared out of
the blue in perfect shape. :-)

> This uses values of 'test_path_bar' in place of '! test_path_foo' to
> bring in the helpful factor of indicating the failure of tests after the
> mv command has been used, that is, it echoes if the feature/test_path
> exists.

This paragraph is excellent. It describes why you've done the patch this
way. This looks much better than version 1 of the patch!

> Signed-off-by: Debra Obondo <debraobondo@gmail.com>

After removing the lines about changes since v1, this patch is

Reviewed-by: Martin Ågren <martin.agren@gmail.com>

> ---
>     [PATCH v2] [OUTREACHY] t7001-mv.sh : Use test_path_is_* functions in
>     test script
>
>     Changes since v1:
>
>     Replacing idiomatic helper functions
>
>     '! test_path_is_*'
>
>     with
>
>     'test_path_is_missing'

This place after the "---" line is an excellent place for including such
"here's what has changed since v1" comments. Good. They will not appear
in the final commit message.

Thanks for contributing!

Martin