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

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

[Gatla Vishweshwar Reddy]

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
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 <gatlavishweshwarreddy26@gmail.com>
---
 builtin/add.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index c859f66519..8172c0c935 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,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);
 
 	unlink(file);
 	free(file);
-- 
2.54.0

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

[Junio C Hamano]

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.

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

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

[Gatla Vishweshwar Reddy]

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 <gatlavishweshwarreddy26@gmail.com>
---

Changes in v2:
- Fixed commit message: "was done" -> "is done" (present tense)
- Added check_apply_state() call after setting state.cached = 1,
  which sets state.check_index = 1 required for index updates

In response to review:

- check_apply_state() with cached=1 correctly
  sets check_index=1, ensuring apply_all_patches() updates the index
  as intended. Verified by reading apply.c lines 172-175.

- Tested with t3700-add.sh: all 58 tests pass


 builtin/add.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index c859f66519..a7266020cd 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,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,17 @@ 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 (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);
--
2.54.0

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

[Junio C Hamano]

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

> @@ -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,17 @@ 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 };

These are -Wdeclaration-after-statement violations; we should move
them to the beginning of the function alongside the other variable
declarations.

> +
> +	if (init_apply_state(&state, repo, prefix))
> +		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);

Does it work properly when run in a subdirectory, such as "cd t &&
git add -e")?  The apply_all_patches() function adjusts the path to
patch files by calling prefix_filename() to prepend state->prefix,
which represents our current directory.

This is not a rhetorical question, as I am unsure what "file"
actually holds at this point after calling repo_git_path().  I don't
know if it is ADD_EDIT.patch relative to a specific directory, an
absolute path to the file, or something else entirely.  It would be
highly beneficial to include a test or two verifying the behavour of
'add -e' from within a subdirectory.

Thanks.

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

[Gatla Vishweshwar Reddy]

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 <gatlavishweshwarreddy26@gmail.com>
---

Changes in v3:
- Moved struct apply_state and apply_argv declarations to the top of
  the function to fix -Wdeclaration-after-statement violations

In response to review:
- repo_git_path() returns an absolute path built from gitdir.
  prefix_filename() in apply_all_patches() explicitly skips absolute
  paths (see abspath.c lines 271-272 where is_absolute_path(arg)
  causes the prefix to be skipped). Running "git add -e" from a
  subdirectory is therefore safe.
- A dedicated test for "git add -e" from a subdirectory would be
  valuable. I looked but found no existing "add -e" tests in the test
  suite to use as a reference. I would appreciate guidance on the
  preferred approach, or I can attempt to write one if you can point
  me to a similar test pattern.

 builtin/add.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index c859f66519..1858adf289 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, prefix))
+		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);
--
2.54.0

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

[Junio C Hamano]

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

> In response to review:
> - repo_git_path() returns an absolute path built from gitdir.
>   prefix_filename() in apply_all_patches() explicitly skips absolute
>   paths (see abspath.c lines 271-272 where is_absolute_path(arg)
>   causes the prefix to be skipped). Running "git add -e" from a
>   subdirectory is therefore safe.

I agree that we are safe when it is absolute (no room for prefix to
take part); my question was more about repo_git_path() that derives
its value from repo->gitdir which may or may not be absolute.

Does it always give you absolute, or sometimes it is relative and
sometimes it is absolute?

> - A dedicated test for "git add -e" from a subdirectory would be
>   valuable. I looked but found no existing "add -e" tests in the test
>   suite to use as a reference.

"git grep -e 'add -e' t/" finds t3702.

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

[Gatla Vishweshwar Reddy]

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 <gatlavishweshwarreddy26@gmail.com>
---

---
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.

In response to review:
- You are right that repo->gitdir may not always be absolute
  (setup.c line 1109). Passing NULL as prefix to init_apply_state()
  avoids the issue entirely — prefix_filename(NULL, arg) sets
  pfx_len=0 and returns the path unchanged regardless of whether
  it is absolute or relative.

- t3702-add-edit.sh was found via "git grep -e 'add -e' t/" as
  suggested. A new test using GIT_EDITOR=cat verifies that
  "git add -e" works correctly from a subdirectory.

 builtin/add.c       | 19 ++++++++++++-------
 t/t3702-add-edit.sh | 10 ++++++++++
 2 files changed, 22 insertions(+), 7 deletions(-)

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
--
2.54.0

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

[Junio C Hamano]

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 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 <gatlavishweshwarreddy26@gmail.com>
> ---
>
> ---
> 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