{"thread":{"id":"26160","subject":"[PATCH] Use a temporary index for interactive git-commit","startedAt":"2010-12-30T00:47:18Z","lastAt":"2010-12-30T12:41:18Z","messageCount":4,"participants":["Conrad Irwin","Jonathan Nieder","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"158742","messageId":"1293670038-8606-1-git-send-email-conrad.irwin@gmail.com","threadId":"26160","inReplyTo":null,"subject":"[PATCH] Use a temporary index for interactive git-commit","fromName":"Conrad Irwin","fromEmail":"conrad.irwin@gmail.com","sentAt":"2010-12-30T00:47:18Z","receivedAt":"2010-12-30T00:47:18Z","isPatch":true,"sender":{"key":"conrad.irwin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/94272?v=4"},"body":"Hitherto even an aborted git commit -p or git commit --interactive has\nadded the selected changes to the index.\n\nSigned-off-by: Conrad Irwin <conrad.irwin@gmail.com>\n---\n\nFollowing up to my email and patch of a few days ago to add support for\ngit commit -p (http://marc.info/?l=git&m=129338900419850&w=2).\n\nI'm not sure that adding parameters to functions willy-nilly is the best\nway of doing this. Can anyone suggest a cleaner mechanism?\n\nConrad\n\n Documentation/git-commit.txt |    2 +-\n builtin/add.c                |   18 +++++++++++++-----\n builtin/checkout.c           |    2 +-\n builtin/commit.c             |   27 +++++++++++++++++++--------\n builtin/reset.c              |    2 +-\n commit.h                     |    4 ++--\n 6 files changed, 37 insertions(+), 18 deletions(-)\n\ndiff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt\nindex 6e7ab5a..81156f0 100644\n--- a/Documentation/git-commit.txt\n+++ b/Documentation/git-commit.txt\n@@ -43,7 +43,7 @@ The content to be added can be specified in several ways:\n 5. by using the --interactive or --patch switches with the 'commit' command\n    to decide one by one which files or hunks should be part of the commit,\n    before finalizing the operation.  Currently, this is done by invoking\n-   'git add --interactive' or 'git add --patch'.\n+   'git add --interactive' or 'git add --patch' on a temporary index.\n \n The `--dry-run` option can be used to obtain a\n summary of what is included by any of the above for the next\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 3d074b3..6941fd6 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -214,10 +214,17 @@ static const char **validate_pathspec(int argc, const char **argv, const char *p\n }\n \n int run_add_interactive(const char *revision, const char *patch_mode,\n-\t\t\tconst char **pathspec)\n+\t\t\tconst char **pathspec, const char *index_file)\n {\n \tint status, ac, pc = 0;\n \tconst char **args;\n+\tchar index[PATH_MAX];\n+\tconst char *env[2] = { NULL };\n+\n+\tif (index_file && *index_file) {\n+\t\tenv[0] =  index;\n+\t\tsnprintf(index, sizeof(index), \"GIT_INDEX_FILE=%s\", index_file);\n+\t}\n \n \tif (pathspec)\n \t\twhile (pathspec[pc])\n@@ -237,12 +244,13 @@ int run_add_interactive(const char *revision, const char *patch_mode,\n \t}\n \targs[ac] = NULL;\n \n-\tstatus = run_command_v_opt(args, RUN_GIT_CMD);\n+\tstatus = run_command_v_opt_cd_env(args, RUN_GIT_CMD, NULL, env);\n \tfree(args);\n \treturn status;\n }\n \n-int interactive_add(int argc, const char **argv, const char *prefix, int patch)\n+int interactive_add(int argc, const char **argv, const char *prefix, int patch,\n+\t\t\tconst char *index_file)\n {\n \tconst char **pathspec = NULL;\n \n@@ -254,7 +262,7 @@ int interactive_add(int argc, const char **argv, const char *prefix, int patch)\n \n \treturn run_add_interactive(NULL,\n \t\t\t\t   patch ? \"--patch\" : NULL,\n-\t\t\t\t   pathspec);\n+\t\t\t\t   pathspec, index_file);\n }\n \n static int edit_patch(int argc, const char **argv, const char *prefix)\n@@ -378,7 +386,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \tif (patch_interactive)\n \t\tadd_interactive = 1;\n \tif (add_interactive)\n-\t\texit(interactive_add(argc - 1, argv + 1, prefix, patch_interactive));\n+\t\texit(interactive_add(argc - 1, argv + 1, prefix, patch_interactive, NULL));\n \n \tif (edit_interactive)\n \t\treturn(edit_patch(argc, argv, prefix));\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 757f9a0..7212e42 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -636,7 +636,7 @@ static int git_checkout_config(const char *var, const char *value, void *cb)\n static int interactive_checkout(const char *revision, const char **pathspec,\n \t\t\t\tstruct checkout_opts *opts)\n {\n-\treturn run_add_interactive(revision, \"--patch=checkout\", pathspec);\n+\treturn run_add_interactive(revision, \"--patch=checkout\", pathspec, NULL);\n }\n \n struct tracking_name_data {\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex f3cdf1d..599d9ff 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -297,14 +297,6 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int\n \n \tif (is_status)\n \t\trefresh_flags |= REFRESH_UNMERGED;\n-\tif (interactive || patch_interactive) {\n-\t\tif (interactive_add(argc, argv, prefix, patch_interactive) != 0)\n-\t\t\tdie(\"interactive add failed\");\n-\t\tif (read_cache_preload(NULL) < 0)\n-\t\t\tdie(\"index file corrupt\");\n-\t\tcommit_style = COMMIT_AS_IS;\n-\t\treturn get_index_file();\n-\t}\n \n \tif (*argv)\n \t\tpathspec = get_pathspec(prefix, argv);\n@@ -312,6 +304,25 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int\n \tif (read_cache_preload(pathspec) < 0)\n \t\tdie(\"index file corrupt\");\n \n+\tif (interactive || patch_interactive) {\n+\t\tfd = hold_locked_index(&index_lock, 1);\n+\n+\t\trefresh_cache_or_die(refresh_flags);\n+\n+\t\tif (write_cache(fd, active_cache, active_nr) ||\n+\t\t    close_lock_file(&index_lock))\n+\t\t\tdie(\"unable to write new_index file\");\n+\n+\t\tif (interactive_add(argc, argv, prefix, patch_interactive, index_lock.filename) != 0)\n+\t\t\tdie(\"interactive add failed\");\n+\n+\t\tdiscard_cache();\n+\t\tread_cache_from(index_lock.filename);\n+\n+\t\tcommit_style = COMMIT_NORMAL;\n+\t\treturn index_lock.filename;\n+\t}\n+\n \t/*\n \t * Non partial, non as-is commit.\n \t *\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 5de2bce..67d604e 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -184,7 +184,7 @@ static int interactive_reset(const char *revision, const char **argv,\n \tif (*argv)\n \t\tpathspec = get_pathspec(prefix, argv);\n \n-\treturn run_add_interactive(revision, \"--patch=reset\", pathspec);\n+\treturn run_add_interactive(revision, \"--patch=reset\", pathspec, NULL);\n }\n \n static int read_from_tree(const char *prefix, const char **argv,\ndiff --git a/commit.h b/commit.h\nindex 951c22e..1a47bd4 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -160,9 +160,9 @@ extern struct commit_list *get_shallow_commits(struct object_array *heads,\n int is_descendant_of(struct commit *, struct commit_list *);\n int in_merge_bases(struct commit *, struct commit **, int);\n \n-extern int interactive_add(int argc, const char **argv, const char *prefix, int patch);\n+extern int interactive_add(int argc, const char **argv, const char *prefix, int patch, const char *index_file);\n extern int run_add_interactive(const char *revision, const char *patch_mode,\n-\t\t\t       const char **pathspec);\n+\t\t\t       const char **pathspec, const char *index_file);\n \n static inline int single_parent(struct commit *commit)\n {\n-- \n1.7.3.4.629.g17fdc.dirty\n"},{"id":"158744","messageId":"20101230025126.GA1868@burratino","threadId":"26160","inReplyTo":"1293670038-8606-1-git-send-email-conrad.irwin@gmail.com","subject":"Re: [PATCH] Use a temporary index for interactive git-commit","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-30T02:51:26Z","receivedAt":"2010-12-30T02:51:26Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Conrad Irwin wrote:\n\n> I'm not sure that adding parameters to functions willy-nilly is the best\n> way of doing this. Can anyone suggest a cleaner mechanism?\n\nWould a simple\n\n\tsetenv(INDEX_ENVIRONMENT, index_file);\n\nin the caller make sense?\n"},{"id":"158747","messageId":"20101230043355.GA24555@sigill.intra.peff.net","threadId":"26160","inReplyTo":"1293670038-8606-1-git-send-email-conrad.irwin@gmail.com","subject":"Re: [PATCH] Use a temporary index for interactive git-commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-12-30T04:33:55Z","receivedAt":"2010-12-30T04:33:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 30, 2010 at 12:47:18AM +0000, Conrad Irwin wrote:\n\n> Hitherto even an aborted git commit -p or git commit --interactive has\n> added the selected changes to the index.\n\nHmm. I see how it could be confusing if you do ^C in \"git commit -p\" and\nit actually commits what you had staged. But if I am reading the patch\nright here:\n\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -378,7 +386,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n>  \tif (patch_interactive)\n>  \t\tadd_interactive = 1;\n>  \tif (add_interactive)\n> -\t\texit(interactive_add(argc - 1, argv + 1, prefix, patch_interactive));\n> +\t\texit(interactive_add(argc - 1, argv + 1, prefix, patch_interactive, NULL));\n>  \n\nthis behavior will not apply to \"git add -p\". So doesn't that introduce\na new confusing inconsistency, that ^C from \"git commit -p\" abandons\nchanges entirely, but from \"git add -p\" will silently stage changes?\n\n-Peff\n"},{"id":"158755","messageId":"AANLkTina562i6KzYR4AQbTNExJbCBA3bKHwA_7W45qAG@mail.gmail.com","threadId":"26160","inReplyTo":"20101230043355.GA24555@sigill.intra.peff.net","subject":"Re: [PATCH] Use a temporary index for interactive git-commit","fromName":"Conrad Irwin","fromEmail":"conrad.irwin@gmail.com","sentAt":"2010-12-30T12:41:18Z","receivedAt":"2010-12-30T12:41:18Z","isPatch":true,"sender":{"key":"conrad.irwin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/94272?v=4"},"body":"On 30 December 2010 04:33, Jeff King <peff@peff.net> wrote:\n>\n> this behavior will not apply to \"git add -p\". So doesn't that introduce\n> a new confusing inconsistency, that ^C from \"git commit -p\" abandons\n> changes entirely, but from \"git add -p\" will silently stage changes?\n>\n\nThat is an interesting observation. The intention of the commit was\nnot to make the\ninteractive add atomic, just isolated from the real index (much like\ngit commit /path/to/file.c\nor git commit -a). This means that if you leave the commit message\nempty (even after a\nsuccessful git-add--interactive) you have not staged any new files.\n\nI'd agree that it would also make sense for git-add--interactive to be\natomic, so that when you\nabort it forgets everything you've told it to do (and that this patch\nhighlights that very nicely).\nAt the moment git-add--interactive is atomic at a file level, i.e. it\nremembers each decision for\ngit add --interactive, and for git add --patch, it saves your\ndecisions only once you've got to\nthe end of a file (not that you'd notice).\n\nWhile I could fix git add -p in the same manner as git commit -p, by\nusing a temporary isolated\nindex, it would be better to change git-add--interactive directly, and\nthus git checkout -p and\ngit reset -p too.\n\nConrad\n"}]}