Re: [PATCH v4] status: long status advice adapted to recent capabilities

[Rudy Rigot <rudy.rigot@gmail.com>]

Oh, thanks, all of this helps a ton. I know exactly what to do about
those two things now.


On Thu, Nov 10, 2022 at 11:30 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Thu, Nov 10, 2022 at 12:02 PM Rudy Rigot <rudy.rigot@gmail.com> wrote:
> > > the <<- operator allows you to indent the here-doc body
> > > (with TABs, not spaces), so you can align the body with the rest of
> > > the code
> >
> > Unfortunately, that's how I had done it first, but since some of those
> > lines are blank, the test code had lines just made of "<tab><tab>" and
> > nothing else, which made the check-whitespace check fail. I considered
> > replacing empty line with something on the fly with sed (like just an
> > "x" character for instance), but this felt hacky and brittle (in the
> > unlikely case where an actual "x" would find itself genuinely lost in
> > the middle of that output, the test would mistakenly pass). I went
> > with the solution I'm presenting here because the readability
> > downsides of missing that indentation felt less bad. Definitely
> > willing to be convinced though.
>
> Okay, I see what you're getting at. Fortunately, there is a simple
> solution as long as those lines are truly blank as emitted by `git
> status`: just leave the blank lines completely blank in the here-doc
> body (don't bother inserting a TAB on the blank line). This should
> product the exact output you want:
>
>     cat >../expected <<-\EOF &&
>     On branch test
>
>     No commits yet
>     ...
>     EOF
>
> Although it should not be needed here, the `sed` approach is generally
> fine, and we use it often enough in tests, though usually with a more
> uncommon letter such as "Q". See, for instance, the q_to_nul(),
> q_to_tab(), etc. functions in t/test-lib-functions.sh.
>
> > > I presume the reason you're escaping the "trash" directory is because
> > > you don't want these untracked "actual" and "expected" files to
> > > pollute the `git status` output you're testing?
> >
> > You are presuming right! The test was being flappy in CI runs before I
> > changed this, which I found used as a solution in other
> > git-status-related tests currently in the codebase. I'm not familiar
> > with the trash directory approach, but I'll figure it out.
>
> Each test script is run in a temporary "trash" directory which gets
> thrown away when the script finishes. We want tests to constrain
> themselves to the trash directory so they don't inadvertently destroy
> a user's files outside the directory.
>
> I see what you mean about some existing status-related tests using
> files such as "../actual" and "../expect". It's not at all obvious in
> a lot of those cases but they are safe[*] because those tests have
> already cd'd into a subdirectory of the "trash" directory, thus
> "../actual" is referring to the "trash" directory itself, hence the
> tests do constrain themselves to "trash".
>
> Anyhow, I suspect that crafting a custom .gitignore file in the test
> setup should satisfy this particular case and allow "actual" and
> "expected" to reside in the "trash" directory itself without mucking
> up `git status` output.
>
> [*] Unfortunately, some of those scripts are poorly structured because
> they `cd` around between tests, which can leave CWD in an unexpected
> state if some test fails and subsequent tests expect CWD to be
> somewhere other than where it was left by the failed test. These days,
> we only allow tests to `cd` within a subshell so that CWD is restored
> automatically whether the test itself succeeds or fails. So, this is
> safe:
>
>     test_expect_success 'title' '
>         do something &&
>         mkdir foo &&
>         (
>             cd foo &&
>             do something else >../actual &&
>         )
>         grep foo actual
>     '