Re: [PATCH 3/3] t: drop debug `cat` calls
- From
- Martin Ågren <martin.agren@gmail.com>
- Date
- Feb 24, 2020, 20:24 UTC
- Message-ID
- <CAN0heSo8RSyvTqTtDLcnBsvMJ-DLL_B-sKRZDNUZBhZ1yX=bmQ@mail.gmail.com>
- In-Reply-To
- <xmqqlforpyqe.fsf@gitster-ct.c.googlers.com>
On Mon, 24 Feb 2020 at 20:33, Junio C Hamano <gitster@pobox.com> wrote:
Show 23 quoted lines
> > 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...)
Show 9 quoted lines
> 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