Re: [PATCH 3/3] t: drop debug `cat` calls

["Martin Ågren" <martin.agren@gmail.com>]

On Mon, 24 Feb 2020 at 20:33, Junio C Hamano <gitster@pobox.com> wrote:
>
> Martin Ågren <martin.agren@gmail.com> writes:
>
> >       git fsck 2>out &&
> > -     cat out &&
> >       ! grep "commit $new" out
> >  '
>
> This one on the other hand *DOES* rely on 'out' being created; we do
> not want to take the failing 'grep' as a sign of success if it is
> because 'out' is missing.
>
> >       test_must_fail git update-index --nonsense 2>msg &&
> > -     cat msg &&
> >       test -s msg
> >  '
>
> This one does not.  "test -s msg" on non-existent msg will fail, so
> this is closer to category 2/3.
>
> So, I am OK to have two patches that catch two classes, but the
> division between 2/3 and 3/3 in this series does not look the right
> one.

Heh. You don't want to know how long I waffled on whether to split the
one hunk out to 2/3 and make the rest 3/3 vs. having a slightly larger
2/2.

For the first patch 1/3, if we lose the cat entirely, we risk bugs in
*git* being hidden. For the hunk in patch 2/3, I first thought it was in
the same category, before I realized that kwdelfile.c disappearing would
be a bug in *p4* as opposed to *git p4*. Since we're not in the business
of testing/verifying other people's software, we can afford to drop that
call entirely. At one point, I had this in the commit message, but in
the end I figured one reason for the removal was enough and just kept
the "we'll soon grep" argument.

I realize now that the line between 2/3 and 3/3 is blurry.

FWIW, for patch 3/3 my reasoning was that for the similar concern about
the file not existing, we'd depend on the shell messing up the
redirection quite badly and not creating a file at all, yet continuing
with the && cascade. Which seemed like a pretty crazy bug. And again,
that shouldn't be our worry. (I see now that there's a case in 3/3 where
a buggy test_cmp could delete "actual" and we'd fail to notice after
this commit. That probably also sorts under pathological bugs...)

> I am also OK to have a single patch with updated log message, saying
> "removal of 'cat <file>' may miss a failure mode that <file> did not
> get created, which would have been caught as a test failure in the
> original, but the <file>s used by cats removed in this patch are
> either impossible to be missing (because a preceding step in the
> test created it, or the &&-cascade would have failed if it failed to
> create the file), or followed by another step in the test that would
> fail if the file is missing (e.g. running grep on the file), so it is
> safe to drop these cats", or something like that.

Let me re-roll with 1/2 (=1/3) and 2/2 (=1/3+2/3).

Thanks for a review.

Martin