[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