threads / patch / 26160

patchUse a temporary index for interactive git-commit

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

## tl;dr

4 messages between Dec 30, 2010 and Dec 30, 2010. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Conrad Irwin· Dec 30, 2010, 00:47 UTC · lore

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

Show changes to 6 files +37 −18

Documentation/git-commit.txt, builtin/add.c, builtin/checkout.c, builtin/commit.c, builtin/reset.c, commit.h

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

Jonathan Nieder· Dec 30, 2010, 02:51 UTC · re: Conrad Irwin · lore

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

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?

Jeff King· Dec 30, 2010, 04:33 UTC · re: Conrad Irwin · lore

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

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:

Show 9 quoted lines

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

Conrad Irwin· Dec 30, 2010, 12:41 UTC · re: Jeff King · lore

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

On 30 December 2010 04:33, Jeff King <peff@peff.net> wrote:

Show 5 quoted lines

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