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

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

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

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.

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

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

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

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.

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

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

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?

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

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

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

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.

Junio C Hamano <gitster@pobox.com> writes:

And nobody seems to be interested in seeing this topic move forward, unfortunately. After reading the patch again, I think this is safe and correct, and if I merge the topic, one of three things can happen.

 (1) the patch does not regress anything unexpectedly, or

 (2) the patch breaks "add -e" completely but the feature is not
     used by anybody and nobody will notice, or

 (3) the patch breaks "add -e" and the its users will start
     complaining too late.

Reverting a merge would not be too involved as the patch is small-ish, so even in case (3) it won't be too much trouble to deal with fallouts. So let me mark the topic for 'next' for now.

It still is not too late for "add -e" users to interject, though.

Thanks.