Re: [PATCH v2] describe: refresh the index when 'broken' flag is used

From: Abhijeet Sonar <abhijeet.nkt@gmail.com>
Date: Jun 25, 2024, 13:22 UTC
Message-ID: <68eb8651-c56e-449f-8102-b82506607387@gmail.com>
In-Reply-To: <CAOLa=ZT5asAE8-=jY7iR2hsGGshjQtBvpjcVacQk5YFa0u3Sfg@mail.gmail.com>

Hi Karthik, thanks for taking the time to review my patch.

On 25/06/24 17:57, Karthik Nayak wrote:

> We should also call `strvec_clear(&update_index_cp.args);` to clear up
> used memory.
>
> Nit: we could actually `cp` for both the child processes here.

Will do those, thanks!

> It would be nice to cleanup the repo at the end of the test by adding
> `test_when_finished "rm -rf stat-dirty" &&` here

All the files generated by the tests in this file are under `trash directory.t6120-describe/` which seems to be automatically deleted when all tests succeed. Also, given that rest of the tests in this file don't perform any manual cleanup, I am not sure if doing so really falls under the scope of this patch.

> You want to do everything apart from the repo init in a subshell, this
> ensures we don't carry over the working directory to the next test.

Ah, now I see why other tests wrap all commands except `git init` in parentheses. Thanks!

> Can't this be merged with the previous test?

Having those separate looked nicer to me. I will merge them together.