Re: [PATCH v1 0/3] fixes for commented out code in tests (was "Re: [PATCH] *: fix typos which duplicate a word")

From: Elijah Newren <newren@gmail.com>
Date: Jan 12, 2023, 01:54 UTC
Message-ID: <CABPp-BFxK7SGs3wsOfozSw_Uvr-ynr+x8ciPV2Rmfx6Nr4si6g@mail.gmail.com>
In-Reply-To: <20230111233242.16870-1-rybak.a.v@gmail.com>

On Wed, Jan 11, 2023 at 4:05 PM Andrei Rybak <rybak.a.v@gmail.com> wrote:

Show 17 quoted lines

>
> [ I apologize for some people getting this twice -- I messed up when
>   invoking `git send-email` ]
>
> On 2023-01-06T19:25, Eric Sunshine wrote:
> > Not related to your patch at all, but I notice in this test that the
> > call to test_when_finished() is commented out:
> >
> >     # test_when_finished "stop_daemon_delete_repo test_insensitive" &&
> >
> > which makes me wonder if it was commented out while the test was being
> > debugged but then forgotten, and that the script is now potentially
> > leaking a running daemon if something in the test fails after the
> > daemon was started, or if the daemon does not shut down on its own as
> > it's supposed to do. [cc:+Jeff Hostetler]
>
> Here's a patch series that fixes some of the commented out test code.

Patch 2 is obviously correct. Patches 1 & 3 make sense to me, but it would be nice to have someone familiar with fsmonitor look at #3.

As to your notes about other related testcases...

> I skipped changing the following:

[...]

Show 6 quoted lines

> 2. In t6426-merge-skip-unneeded-updates.sh, second part of the test '2c: Modify
>    b & add c VS rename b->c' is commented out with an explicit "# FIXME:
>    rename/add conflicts are horribly broken right now;" above the commented out
>    part.
>    [ cc Elijah Newren, author of c04ba51739 (t6046: testcases checking whether
>    updates can be skipped in a merge, 2018-04-19) ]

[...]

You missed the cc...but I looked up this email since you did cc me for patch 2/3.

Yeah, the commented out code was never tested, because the only thing I could have tested at the time was incorrect results. So I just took a guess at what the improved testing would look like and apparently made 3 small errors in doing so. I have fixed it locally and can submit a patch.