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

Gatla Vishweshwar Reddy <[email protected]> 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 is done by spawning a child
> process running "git apply --recount --cached <file>", which is an
> unnecessary subprocess since the apply machinery is available as a
> native C API.
>
> Replace the run_command() call with a direct call to apply_all_patches()
> using an initialized apply_state with the cached and recount options set
> appropriately. This avoids the overhead of forking a subprocess, keeps
> the operation within the same process, and makes the intent of the code
> clearer to the reader.
>
> Remove the now-unused includes of "run-command.h" and "strvec.h" since
> no other code in this file requires them after this change.
>
> Signed-off-by: Gatla Vishweshwar Reddy <[email protected]>
> ---
>
> ---
> Changes in v4:
> - Pass NULL instead of prefix to init_apply_state() since the file
>   path from repo_git_path() is a git-internal path that should not
>   be prefixed. This is safe regardless of whether repo->gitdir is
>   absolute or relative, as prefix_filename(NULL, arg) returns the
>   path unchanged (abspath.c line 269).
> - Add a test in t3702-add-edit.sh verifying that "git add -e" works
>   correctly when run from a subdirectory.
> - Tested with t3702-add-edit.sh: all 4 tests pass.

Now the way "apply" API is used in this new code path should be
pretty much parallel to existing "git apply" and "git am" code
paths, we should be fine.  I do not use "git add -e", but those who
do who may care more more deeply about keeping this feature working
than I do may want to lend an extra pair of eyes on this patch.

Thanks.

> diff --git a/builtin/add.c b/builtin/add.c
> index c859f66519..20a86a1611 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -13,7 +13,6 @@
>  #include "dir.h"
>  #include "gettext.h"
>  #include "pathspec.h"
> -#include "run-command.h"
>  #include "object-file.h"
>  #include "odb.h"
>  #include "odb/transaction.h"
> @@ -23,9 +22,9 @@
>  #include "diff.h"
>  #include "read-cache.h"
>  #include "revision.h"
> -#include "strvec.h"
>  #include "submodule.h"
>  #include "add-interactive.h"
> +#include "apply.h"
>
>  static const char * const builtin_add_usage[] = {
>  	N_("git add [<options>] [--] <pathspec>..."),
> @@ -187,7 +186,8 @@ 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 apply_state state;
> +	const char *apply_argv[2];
>  	struct rev_info rev;
>  	int out;
>  	struct stat st;
> @@ -217,11 +217,16 @@ 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))
> +	apply_argv[0] = file;
> +	apply_argv[1] = NULL;
> +	if (init_apply_state(&state, repo, NULL))
> +		die(_("could not initialize apply state"));
> +	state.cached = 1;
> +	if (check_apply_state(&state, 0))
> +		die(_("could not check apply state"));
> +	if (apply_all_patches(&state, 1, apply_argv, APPLY_OPT_RECOUNT))
>  		die(_("could not apply '%s'"), file);
> +	clear_apply_state(&state);
>
>  	unlink(file);
>  	free(file);
> diff --git a/t/t3702-add-edit.sh b/t/t3702-add-edit.sh
> index 8bacacbac6..f628564005 100755
> --- a/t/t3702-add-edit.sh
> +++ b/t/t3702-add-edit.sh
> @@ -124,5 +124,15 @@ test_expect_success 'add -e notices editor failure' '
>  	test_must_fail env GIT_EDITOR=false git add -e &&
>  	test_expect_code 1 git diff --exit-code
>  '
> +test_expect_success 'add -e works from a subdirectory' '
> +	git reset --hard &&
> +	echo change >>file &&
> +	mkdir -p subdir &&
> +	(
> +		cd subdir &&
> +		GIT_EDITOR=cat git add -e ../file
> +	) &&
> +	git diff --cached | grep -q "^+change"
> +'
>
>  test_done