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

From: Rudy Rigot <rudy.rigot@gmail.com>
Date: Nov 10, 2022, 17:01 UTC
Message-ID: <CANaDLWK9ZhtqdpJJCNvOJ24x0jtUzZjZE5WKdzBPnePA4eGqTg@mail.gmail.com>
In-Reply-To: <CAPig+cTGG-y6myEYOVeF8W9QBdCjhqeghsepi-2R9V-v7=YwZA@mail.gmail.com>

> 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.

I have no issues with anything else from your review, and I'm planning to integrate it all. More specifically:

> 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.

> Does this need to be inline? Is this a hot piece of code, or is this
> merely a premature optimization?

I'll admit my limits, I'm not familiar enough to know. If you feel the inline is unnecessary here, I'm glad to trust you on it, I'll remove it.

Thanks a lot for your in-depth review, I am planning to integrate all the other feedback, and bring another iteration forward, possibly today.