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

[Junio C Hamano <gitster@pobox.com>]

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

> On 26/06/24 17:00, Karthik Nayak wrote:
>> Not worth a reroll, but you don't have to create file.new twice.
>
> Actually, now that I think of it, those two were better off being
> separate tests.  It might so happen the first call to describe
> refreshes the index, due to which the second call with the --broken
> option does not bug-out in the way it would if the command was run by
> itself. Having them separate would give them enough isolation so that
> previous command does not interfere with the later.

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.

>>> Range-diff against v4:
>>> 1:  1da5fa48d9 ! 1:  52f590b70f describe: refresh the index when 'broken' flag is used
>>>      @@ builtin/describe.c: int cmd_describe(int argc, const char **argv, const char *pr
>>>       +			cp.git_cmd = 1;
>>>       +			cp.no_stdin = 1;
>>>       +			cp.no_stdout = 1;
>>>      -+			run_command(&cp);
>>>      -+			strvec_clear(&cp.args);
>>>      ++			if (run_command(&cp))
>>>      ++				child_process_clear(&cp);
>>>       +
>>>        			strvec_pushv(&cp.args, diff_index_args);
>>>        			cp.git_cmd = 1;
>>> --
>>> 2.45.2.606.g9005149a4a.dirty
>> Other than this, this looks good to me.
> I am not sure if I follow this one.  Am I expected to not share the
> struct child_process between the two sub-process calls?

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(-)

diff --git c/builtin/fsck.c w/builtin/fsck.c
index d13a226c2e..398b492184 100644
--- c/builtin/fsck.c
+++ w/builtin/fsck.c
@@ -1078,8 +1078,10 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
 				strvec_push(&commit_graph_verify.args, "--progress");
 			else
 				strvec_push(&commit_graph_verify.args, "--no-progress");
-			if (run_command(&commit_graph_verify))
+			if (run_command(&commit_graph_verify)) {
+				child_process_clear(&commit_graph_verify);
 				errors_found |= ERROR_COMMIT_GRAPH;
+			}
 		}
 	}
 
@@ -1096,8 +1098,10 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
 				strvec_push(&midx_verify.args, "--progress");
 			else
 				strvec_push(&midx_verify.args, "--no-progress");
-			if (run_command(&midx_verify))
+			if (run_command(&midx_verify)) {
+				child_process_clear(&midx_verify);
 				errors_found |= ERROR_MULTI_PACK_INDEX;
+			}
 		}
 	}