[PATCH] Use a temporary index for interactive git-commit

[PATCH] Use a temporary index for interactive git-commit

[Conrad Irwin]

Hitherto even an aborted git commit -p or git commit --interactive has
added the selected changes to the index.

Signed-off-by: Conrad Irwin <conrad.irwin@gmail.com>
---

Following up to my email and patch of a few days ago to add support for
git commit -p (http://marc.info/?l=git&m=129338900419850&w=2).

I'm not sure that adding parameters to functions willy-nilly is the best
way of doing this. Can anyone suggest a cleaner mechanism?

Conrad

 Documentation/git-commit.txt |    2 +-
 builtin/add.c                |   18 +++++++++++++-----
 builtin/checkout.c           |    2 +-
 builtin/commit.c             |   27 +++++++++++++++++++--------
 builtin/reset.c              |    2 +-
 commit.h                     |    4 ++--
 6 files changed, 37 insertions(+), 18 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 6e7ab5a..81156f0 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -43,7 +43,7 @@ The content to be added can be specified in several ways:
 5. by using the --interactive or --patch switches with the 'commit' command
    to decide one by one which files or hunks should be part of the commit,
    before finalizing the operation.  Currently, this is done by invoking
-   'git add --interactive' or 'git add --patch'.
+   'git add --interactive' or 'git add --patch' on a temporary index.
 
 The `--dry-run` option can be used to obtain a
 summary of what is included by any of the above for the next
diff --git a/builtin/add.c b/builtin/add.c
index 3d074b3..6941fd6 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -214,10 +214,17 @@ static const char **validate_pathspec(int argc, const char **argv, const char *p
 }
 
 int run_add_interactive(const char *revision, const char *patch_mode,
-			const char **pathspec)
+			const char **pathspec, const char *index_file)
 {
 	int status, ac, pc = 0;
 	const char **args;
+	char index[PATH_MAX];
+	const char *env[2] = { NULL };
+
+	if (index_file && *index_file) {
+		env[0] =  index;
+		snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
+	}
 
 	if (pathspec)
 		while (pathspec[pc])
@@ -237,12 +244,13 @@ int run_add_interactive(const char *revision, const char *patch_mode,
 	}
 	args[ac] = NULL;
 
-	status = run_command_v_opt(args, RUN_GIT_CMD);
+	status = run_command_v_opt_cd_env(args, RUN_GIT_CMD, NULL, env);
 	free(args);
 	return status;
 }
 
-int interactive_add(int argc, const char **argv, const char *prefix, int patch)
+int interactive_add(int argc, const char **argv, const char *prefix, int patch,
+			const char *index_file)
 {
 	const char **pathspec = NULL;
 
@@ -254,7 +262,7 @@ int interactive_add(int argc, const char **argv, const char *prefix, int patch)
 
 	return run_add_interactive(NULL,
 				   patch ? "--patch" : NULL,
-				   pathspec);
+				   pathspec, index_file);
 }
 
 static int edit_patch(int argc, const char **argv, const char *prefix)
@@ -378,7 +386,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	if (patch_interactive)
 		add_interactive = 1;
 	if (add_interactive)
-		exit(interactive_add(argc - 1, argv + 1, prefix, patch_interactive));
+		exit(interactive_add(argc - 1, argv + 1, prefix, patch_interactive, NULL));
 
 	if (edit_interactive)
 		return(edit_patch(argc, argv, prefix));
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 757f9a0..7212e42 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -636,7 +636,7 @@ static int git_checkout_config(const char *var, const char *value, void *cb)
 static int interactive_checkout(const char *revision, const char **pathspec,
 				struct checkout_opts *opts)
 {
-	return run_add_interactive(revision, "--patch=checkout", pathspec);
+	return run_add_interactive(revision, "--patch=checkout", pathspec, NULL);
 }
 
 struct tracking_name_data {
diff --git a/builtin/commit.c b/builtin/commit.c
index f3cdf1d..599d9ff 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -297,14 +297,6 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int
 
 	if (is_status)
 		refresh_flags |= REFRESH_UNMERGED;
-	if (interactive || patch_interactive) {
-		if (interactive_add(argc, argv, prefix, patch_interactive) != 0)
-			die("interactive add failed");
-		if (read_cache_preload(NULL) < 0)
-			die("index file corrupt");
-		commit_style = COMMIT_AS_IS;
-		return get_index_file();
-	}
 
 	if (*argv)
 		pathspec = get_pathspec(prefix, argv);
@@ -312,6 +304,25 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int
 	if (read_cache_preload(pathspec) < 0)
 		die("index file corrupt");
 
+	if (interactive || patch_interactive) {
+		fd = hold_locked_index(&index_lock, 1);
+
+		refresh_cache_or_die(refresh_flags);
+
+		if (write_cache(fd, active_cache, active_nr) ||
+		    close_lock_file(&index_lock))
+			die("unable to write new_index file");
+
+		if (interactive_add(argc, argv, prefix, patch_interactive, index_lock.filename) != 0)
+			die("interactive add failed");
+
+		discard_cache();
+		read_cache_from(index_lock.filename);
+
+		commit_style = COMMIT_NORMAL;
+		return index_lock.filename;
+	}
+
 	/*
 	 * Non partial, non as-is commit.
 	 *
diff --git a/builtin/reset.c b/builtin/reset.c
index 5de2bce..67d604e 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -184,7 +184,7 @@ static int interactive_reset(const char *revision, const char **argv,
 	if (*argv)
 		pathspec = get_pathspec(prefix, argv);
 
-	return run_add_interactive(revision, "--patch=reset", pathspec);
+	return run_add_interactive(revision, "--patch=reset", pathspec, NULL);
 }
 
 static int read_from_tree(const char *prefix, const char **argv,
diff --git a/commit.h b/commit.h
index 951c22e..1a47bd4 100644
--- a/commit.h
+++ b/commit.h
@@ -160,9 +160,9 @@ extern struct commit_list *get_shallow_commits(struct object_array *heads,
 int is_descendant_of(struct commit *, struct commit_list *);
 int in_merge_bases(struct commit *, struct commit **, int);
 
-extern int interactive_add(int argc, const char **argv, const char *prefix, int patch);
+extern int interactive_add(int argc, const char **argv, const char *prefix, int patch, const char *index_file);
 extern int run_add_interactive(const char *revision, const char *patch_mode,
-			       const char **pathspec);
+			       const char **pathspec, const char *index_file);
 
 static inline int single_parent(struct commit *commit)
 {
-- 
1.7.3.4.629.g17fdc.dirty

Re: [PATCH] Use a temporary index for interactive git-commit

[Jonathan Nieder]

Conrad Irwin wrote:

> I'm not sure that adding parameters to functions willy-nilly is the best
> way of doing this. Can anyone suggest a cleaner mechanism?

Would a simple

	setenv(INDEX_ENVIRONMENT, index_file);

in the caller make sense?

Re: [PATCH] Use a temporary index for interactive git-commit

[Jeff King]

On Thu, Dec 30, 2010 at 12:47:18AM +0000, Conrad Irwin wrote:

> Hitherto even an aborted git commit -p or git commit --interactive has
> added the selected changes to the index.

Hmm. I see how it could be confusing if you do ^C in "git commit -p" and
it actually commits what you had staged. But if I am reading the patch
right here:

> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -378,7 +386,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  	if (patch_interactive)
>  		add_interactive = 1;
>  	if (add_interactive)
> -		exit(interactive_add(argc - 1, argv + 1, prefix, patch_interactive));
> +		exit(interactive_add(argc - 1, argv + 1, prefix, patch_interactive, NULL));
>  

this behavior will not apply to "git add -p". So doesn't that introduce
a new confusing inconsistency, that ^C from "git commit -p" abandons
changes entirely, but from "git add -p" will silently stage changes?

-Peff

Re: [PATCH] Use a temporary index for interactive git-commit

[Conrad Irwin]

On 30 December 2010 04:33, Jeff King <peff@peff.net> wrote:
>
> this behavior will not apply to "git add -p". So doesn't that introduce
> a new confusing inconsistency, that ^C from "git commit -p" abandons
> changes entirely, but from "git add -p" will silently stage changes?
>

That is an interesting observation. The intention of the commit was
not to make the
interactive add atomic, just isolated from the real index (much like
git commit /path/to/file.c
or git commit -a). This means that if you leave the commit message
empty (even after a
successful git-add--interactive) you have not staged any new files.

I'd agree that it would also make sense for git-add--interactive to be
atomic, so that when you
abort it forgets everything you've told it to do (and that this patch
highlights that very nicely).
At the moment git-add--interactive is atomic at a file level, i.e. it
remembers each decision for
git add --interactive, and for git add --patch, it saves your
decisions only once you've got to
the end of a file (not that you'd notice).

While I could fix git add -p in the same manner as git commit -p, by
using a temporary isolated
index, it would be better to change git-add--interactive directly, and
thus git checkout -p and
git reset -p too.

Conrad