Abhijeet Sonar <abhijeet.nkt@xxxxxxxxx> 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@xxxxxxxxx>
---
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;
+ }
}
}
