[GSoC,v2,2/3] t9811: Remove the -q quiet mode from some instances of grep

Commit Message

Anthony Wang April 7, 2025, 11:18 a.m. UTC

Remove the `-q` quiet mode from some instances of `grep`,
as the lack of `-q` on the `grep` on line 99 implies that its output is
required, when that is not the case. This change ensures consistency and
avoids confusion about whether the output of `grep` is used.

---
 t/t9811-git-p4-label-import.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Eric Sunshine April 7, 2025, 4:26 p.m. UTC | #1

On Mon, Apr 7, 2025 at 7:18 AM Anthony Wang <[e-mail]> wrote:
> Remove the `-q` quiet mode from some instances of `grep`,
> as the lack of `-q` on the `grep` on line 99 implies that its output is
> required, when that is not the case. This change ensures consistency and
> avoids confusion about whether the output of `grep` is used.
> ---

Missing sign-off.

Rather than mentioning line 99 explicitly, it probably would be more
helpful to readers to say instead:

    ... the lack of `-q` on the "TAG_F1" `grep` implies...

> diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
> @@ -97,8 +97,8 @@ test_expect_success 'two labels on the same changelist' '
>                 git tag >output &&
>                 grep TAG_F1 output &&
> -               grep -q TAG_F1_1 output &&
> -               grep -q TAG_F1_2 output &&
> +               grep TAG_F1_1 output &&
> +               grep TAG_F1_2 output &&

For such a simple change, it probably would be more common on this
project to combine patches [2/3] (which drops `-q`) and [3/3] (which
replaces `grep` with `test_grep`), and to simply explain as a
side-note in the commit message of the combined patch why `-q` is
being dropped. However, it's also fairly subjective, and others may
feel differently, so keeping the changes as separate patches is also
valid.

Junio C Hamano April 7, 2025, 10:11 p.m. UTC | #2

Eric Sunshine <[e-mail]> writes:

> For such a simple change, it probably would be more common on this
> project to combine patches [2/3] (which drops `-q`) and [3/3] (which
> replaces `grep` with `test_grep`), and to simply explain as a
> side-note in the commit message of the combined patch why `-q` is
> being dropped.

True, and in this case, I'd say this is better done as a single
patch to go directly to "show-ref --verify" that loses an extra
external command per tag to kill three birds with a single patch.
(1) lose the problem of pipe hiding the status, (2) clearly say
what is expected to exist and not to exist, and (3) futureproof
the test so that new tags with similar names (e.g. earlier steps
may be updated to start creating tag T_TAG_F1_1, and grep would
happily report a partial match with TAG_F1_1) will not make the
verification invalid.

Thanks.

Patch

diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
index 5abac938d0..e69dae55dc 100755
--- a/t/t9811-git-p4-label-import.sh
+++ b/t/t9811-git-p4-label-import.sh
@@ -97,8 +97,8 @@  test_expect_success 'two labels on the same changelist' '
 
 		git tag >output &&
 		grep TAG_F1 output &&
-		grep -q TAG_F1_1 output &&
-		grep -q TAG_F1_2 output &&
+		grep TAG_F1_1 output &&
+		grep TAG_F1_2 output &&
 
 		cd main &&