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

Abhijeet Sonar <abhijeet.nkt@gmail.com> writes:

Good thinking. Yes, we may end up having a few commands that are duplicated in these two tests (for setting the stage up, for example), but it would be better to test these two separately.

Without reusing and instead of using two, we do not have to worry about the reusablility of the child_process structure in the first place, which is a huge plus, but in the longer run we should make sure it is safe to reuse child_process and document the safe way to reuse it (run-command.h does document a way to use it once and then clean it up, but the "clean-up" extends only to not leaking resources after we are done---it does not guarantee that it is OK to reuse it).

I think with the updated "we clear cp ourselves if run_command() fails", it should be safe to reuse, but it probably is even safer to do something like this:

	... the first run ...
	if (run_command(&cp))
		child_process_clear(&cp);

	child_process_init(&cp);
	
        ... setup for the second run ...
	strvec_pushv(&cp.args, diff_index_args);
	cp.git_cmd = 1;
	... full set-up without relying on anything done earlier ...

The extra child_process_init() call may serve as an extra documentation that we are reusing the same struct here (we often do "git grep" for use of a specific API function before tree wide code clean-up, and child_process_init() would be a good key to look for).

... goes and looks ...

Oh, I found an interesting one. builtin/fsck.c:cmd_fsck() does this in a loop:

	struct child_process verify = CHILD_PROCESS_INIT;

	... setup ...
	for (... loop ...) {
		child_process_init(&verify);
		... set up various .members of verify struct ...
		strvec_pushl(&verify.args, ... command line ...);
		if (run_command(&verify))
			errors_found |= ...;
	}

This code clearly assumes that it is safe to reuse the child_process structure after you run_command() and let it clean-up if you do another child_process_init(). And I think that is a sensible assumption.

The code in builtin/fsck.c:cmd_fsck() is buggy when run_command() fails, I think. Without doing child_process_clear() there, doesn't it leak the strvec?

------- >8 ------------- >8 ------------- >8 -------
Subject: [PATCH] fsck: clear child_process after failed run_command()

There are two loops that calls run_command() using the same child_process struct near the end of cmd_fsck(). 4d0984be (fsck: do not reuse child_process structs, 2018-11-12) tightened these code paths to reinitialize the structure in order to safely reuse it.

    The run-command API makes no promises about what is left in a struct
    child_process after a command finishes, and it's not safe to simply
    reuse it again for a similar command.

Upon failure, run_command() can return without releasing the resource held by the child_process structure, which is done by calling finish_command() which in turn calls child_process_clear().

Reinitializing the structure without calling child_process_clear() for the next round would leak the .args and .env strvecs.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/fsck.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)