Re: [PATCH v2 3/3] t6421: fix test to work when repo dir contains d0

From: Kyle Lippincott <spectral@google.com>
Date: Aug 3, 2024, 00:03 UTC
Message-ID: <CAO_smVh-gQWy7xUJgFjd6gUWCTV5jTYJ9E9E3rvaQg0EY-2BdQ@mail.gmail.com>
In-Reply-To: <xmqqr0b6a9hp.fsf@gitster.g>

On Fri, Aug 2, 2024 at 2:41 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 30 quoted lines

>
> "Kyle Lippincott via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Kyle Lippincott <spectral@google.com>
> >
> > The `grep` statement in this test looks for `d0.*<string>`, attempting
> > to filter to only show lines that had tabular output where the 2nd
> > column had `d0` and the final column had a substring of
> > [`git -c `]`fetch.negotiationAlgorithm`. These lines also have
> > `child_start` in the 4th column, but this isn't part of the condition.
> >
> > A subsequent line will have `d1` in the 2nd column, `start` in the 4th
> > column, and `/path/to/git/git -c fetch.negotiationAlgorihm` in the final
> > column. If `/path/to/git/git` contains the substring `d0`, then this
> > line is included by `grep` as well as the desired line, leading to an
> > effective doubling of the number of lines, and test failures.
> >
> > Tighten the grep expression to require `d0` to be surrounded by spaces,
> > and to have the `child_start` label.
>
> OK.
>
> I think I actually misinterpreted what you meant with these changes.
> It is not what the patterns are picking.  It is some _other_ trace
> entry we do not necessarily care about, like label:do_write_index
> that has the path to the .git/index.lock file, that can accidentally
> contain d0, that can be picked up with a pattern that is too loose.
> So it really didn't have to clarify what it is looking for, as it
> would not help seeing what false positives the patterns are designed
> to avoid matching.  Sorry about that.

I would have included examples, but they're quite long (>>>80 chars), so seemed very out of place in both commit description and in the codebase. With line wrapping, it wasn't very readable either. At the risk of this also getting line-wrapped into unreadability:

test_line_count: line count for fetches != 1
23:59:48.794453 run-command.c:733            | d0 | main
      | child_start  |     |  0.027328 |           |              |
..........[ch1] class:? argv:[git -c fetch.negotiationAlgorithm=noop
fetch origin --no-tags --no-write-fetch-head --recurse-submodules=no
--filter=blob:none --stdin]
23:59:48.798901 common-main.c:58             | d1 | main
      | start        |     |  0.000852 |           |              |
/usr/local/google/home/spectral/src/oss/d0/git/git -c
fetch.negotiationAlgorithm=noop fetch origin --no-tags
--no-write-fetch-head --recurse-submodules=no --filter=blob:none
--stdin

where each line in the `fetches` file starts with `23:59:48` here. It's 9 columns, separated by `|` characters, and the line we don't want is the second one; the regex `d0.*fetch.negotiationAlgorithm` includes it because of the `d0` in the path.

I considered using `awk -F"|" "\$2~/d0/ && \$9~/fetch\\.negotiationAlgorithm/{ print }" trace.output >fetches`, but it was longer, possibly less clear, and less specific (since it didn't include the $4~/child_start/ condition)

>
> Will queue.
>
>