Re: [PATCH] builtin/add.c: replace run_command() with direct apply_all_patches() call

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 10, 2026, 06:41 UTC
Message-ID: <xmqqmrvzfitd.fsf@gitster.g>
In-Reply-To: <20260709192619.46791-1-gatlavishweshwarreddy26@gmail.com>

Gatla Vishweshwar Reddy <gatlavishweshwarreddy26@gmail.com> writes:

> When the user runs "git add -e", the diff of the working tree changes
> is written to a temporary file, opened in an editor, and then applied
> back to the index. The application step was done by spawning a child

"was" -> "is"; in the first part of the log message that gives an observation, we describe the status quo in the present tense.

Show 28 quoted lines

> process running "git apply --recount --cached <file>", which is an
> unnecessary subprocess since the apply machinery is available as a
> native C API.
> @@ -187,7 +186,6 @@ static int edit_patch(struct repository *repo,
>  		      const char *prefix)
>  {
>  	char *file = repo_git_path(repo, "ADD_EDIT.patch");
> -	struct child_process child = CHILD_PROCESS_INIT;
>  	struct rev_info rev;
>  	int out;
>  	struct stat st;
> @@ -217,11 +215,15 @@ static int edit_patch(struct repository *repo,
>  	if (!st.st_size)
>  		die(_("empty patch. aborted"));
>  
> -	child.git_cmd = 1;
> -	strvec_pushl(&child.args, "apply", "--recount", "--cached", file,
> -		     NULL);
> -	if (run_command(&child))
> +	struct apply_state state;
> +	const char *apply_argv[] = { file, NULL };
> +
> +	if (init_apply_state(&state, repo, prefix))
> +		die(_("could not initialize apply state"));
> +	state.cached = 1;
> +	if (apply_all_patches(&state, 1, apply_argv, APPLY_OPT_RECOUNT))
>  		die(_("could not apply '%s'"), file);
> +	clear_apply_state(&state);

Compared to existing callers of the apply_all_patches() API function, this implementation curiously lacks a prior call to check_apply_state().

Has this been tested, and do we have sufficient test coverage for it?

Calling check_apply_state() should flip state->check_index on, given that state.cached is set to 1 above. If I remember correctly, having this bit enabled is required for apply_patch() to toggle the .update_index member, which in turn allows apply_all_patches() to update the index with the patch results. Please double-check this logic, since it has been a while since I looked at these specific code paths.

If my assumption holds, this patch might inadvertently stop writing the result to the index, even though the original intent of replacing 'apply --cached' was clearly to update it.

Thanks.

>  
>  	unlink(file);
>  	free(file);