[PATCH v3 0/3] Git commit --patch (again)

[PATCH v3 0/3] Git commit --patch (again)

[conrad.irwin]

From: Conrad Irwin <conrad.irwin@gmail.com>

Hi all,

I've rebased my support for git commit -p onto the current master
branch. I've posted it to the list twice before [1][2].

The purpose of the branch is to add the -p/--patch flag, used by other
commands such as git add and git checkout, to git commit. This makes it
much easier, and more convenient, to construct well-formed patches after
a session of writing code.

This branch also includes a fix for git-commit --interactive, so that if
the committer aborts after selecting hunks, the index is unchanged (just
as specifying filenames in the arguments doesn't permanently add them to
the index).

Feedback (of any variety) would be much appreciated,

Conrad

[1] http://thread.gmane.org/gmane.comp.version-control.git/164193
[2] http://thread.gmane.org/gmane.comp.version-control.git/165951

Conrad Irwin (3):
  Use a temporary index for git commit --interactive
  Allow git commit --interactive with paths
  Add support for -p/--patch to git-commit

 Documentation/git-commit.txt |   24 ++++++++++++++-------
 builtin/add.c                |    6 ++--
 builtin/commit.c             |   46 +++++++++++++++++++++++++++++++-----------
 commit.h                     |    2 +-
 4 files changed, 54 insertions(+), 24 deletions(-)

-- 
1.7.5.188.g4817

[PATCH v3 1/3] Use a temporary index for git commit --interactive

[conrad.irwin]

From: Conrad Irwin <conrad.irwin@gmail.com>

Change the behaviour of git commit --interactive so that when you abort
the commit (by leaving the commit message empty) the index remains
unchanged.

Hitherto an aborted commit --interactive has added the selected hunks to
the index regardless of whether the commit succeeded or not.

Signed-off-by: Conrad Irwin <conrad.irwin@gmail.com>
---
 Documentation/git-commit.txt |    3 ++-
 builtin/commit.c             |   36 ++++++++++++++++++++++++++++--------
 2 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index d0534b8..ed50271 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -41,7 +41,8 @@ The content to be added can be specified in several ways:
 
 5. by using the --interactive switch with the 'commit' command to decide one
    by one which files should be part of the commit, before finalizing the
-   operation.  Currently, this is done by invoking 'git add --interactive'.
+   operation.  Currently, this is done by invoking 'git add --interactive'
+   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/commit.c b/builtin/commit.c
index 67757e9..636aea6 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -336,18 +336,11 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int
 	int fd;
 	struct string_list partial;
 	const char **pathspec = NULL;
+	char *old_index_env = NULL;
 	int refresh_flags = REFRESH_QUIET;
 
 	if (is_status)
 		refresh_flags |= REFRESH_UNMERGED;
-	if (interactive) {
-		if (interactive_add(argc, argv, prefix) != 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);
@@ -355,6 +348,33 @@ 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) {
+		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 create temporary index"));
+
+		old_index_env = getenv(INDEX_ENVIRONMENT);
+		setenv(INDEX_ENVIRONMENT, index_lock.filename, 1);
+
+		if (interactive_add(argc, argv, prefix) != 0)
+			die(_("interactive add failed"));
+
+		if (old_index_env && *old_index_env)
+			setenv(INDEX_ENVIRONMENT, old_index_env, 1);
+		else
+			unsetenv(INDEX_ENVIRONMENT);
+
+		discard_cache();
+		read_cache_from(index_lock.filename);
+
+		commit_style = COMMIT_NORMAL;
+		return index_lock.filename;
+	}
+
 	/*
 	 * Non partial, non as-is commit.
 	 *
-- 
1.7.5.188.g4817

[PATCH v3 2/3] Allow git commit --interactive with paths

[conrad.irwin]

From: Conrad Irwin <conrad.irwin@gmail.com>

Make git commit --interactive feel more like git add --interactive by
allowing the user to restrict the list of files they have to deal with.

Signed-off-by: Conrad Irwin <conrad.irwin@gmail.com>
---
 builtin/commit.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 636aea6..7707af8 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1084,8 +1084,6 @@ static int parse_and_validate_options(int argc, const char *argv[],
 
 	if (all && argc > 0)
 		die(_("Paths with -a does not make sense."));
-	else if (interactive && argc > 0)
-		die(_("Paths with --interactive does not make sense."));
 
 	if (null_termination && status_format == STATUS_FORMAT_LONG)
 		status_format = STATUS_FORMAT_PORCELAIN;
-- 
1.7.5.188.g4817

[PATCH v3 3/3] Add support for -p/--patch to git-commit

[conrad.irwin]

From: Conrad Irwin <conrad.irwin@gmail.com>

The --interactive flag is already shared by git add and git commit,
share the -p and --patch flags too.

Signed-off-by: Conrad Irwin <conrad.irwin@gmail.com>
---
 Documentation/git-commit.txt |   25 ++++++++++++++++---------
 builtin/add.c                |    6 +++---
 builtin/commit.c             |   10 +++++++---
 commit.h                     |    2 +-
 4 files changed, 27 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index ed50271..66918c7 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -8,11 +8,12 @@ git-commit - Record changes to the repository
 SYNOPSIS
 --------
 [verse]
-'git commit' [-a | --interactive] [-s] [-v] [-u<mode>] [--amend] [--dry-run]
-	   [(-c | -C | --fixup | --squash) <commit>] [-F <file> | -m <msg>]
-	   [--reset-author] [--allow-empty] [--allow-empty-message] [--no-verify]
-	   [-e] [--author=<author>] [--date=<date>] [--cleanup=<mode>]
-	   [--status | --no-status] [-i | -o] [--] [<file>...]
+'git commit' [-a | --interactive | --patch] [-s] [-v] [-u<mode>] [--amend]
+	   [--dry-run] [(-c | -C | --fixup | --squash) <commit>]
+	   [-F <file> | -m <msg>] [--reset-author] [--allow-empty]
+	   [--allow-empty-message] [--no-verify] [-e] [--author=<author>]
+	   [--date=<date>] [--cleanup=<mode>] [--status | --no-status]
+	   [-i | -o] [--] [<file>...]
 
 DESCRIPTION
 -----------
@@ -39,10 +40,10 @@ The content to be added can be specified in several ways:
    that have been removed from the working tree, and then perform the
    actual commit;
 
-5. by using the --interactive switch with the 'commit' command to decide one
-   by one which files should be part of the commit, before finalizing the
-   operation.  Currently, this is done by invoking 'git add --interactive'
-   on a temporary index.
+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' 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
@@ -60,6 +61,12 @@ OPTIONS
 	been modified and deleted, but new files you have not
 	told git about are not affected.
 
+-p::
+--patch::
+	Use the interactive patch selection interface to chose
+	which changes to commit. See linkgit:git-add[1] for
+	details.
+
 -C <commit>::
 --reuse-message=<commit>::
 	Take an existing commit object, and reuse the log message
diff --git a/builtin/add.c b/builtin/add.c
index d39a6ab..f02524b 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -241,7 +241,7 @@ int run_add_interactive(const char *revision, const char *patch_mode,
 	return status;
 }
 
-int interactive_add(int argc, const char **argv, const char *prefix)
+int interactive_add(int argc, const char **argv, const char *prefix, int patch)
 {
 	const char **pathspec = NULL;
 
@@ -252,7 +252,7 @@ int interactive_add(int argc, const char **argv, const char *prefix)
 	}
 
 	return run_add_interactive(NULL,
-				   patch_interactive ? "--patch" : NULL,
+				   patch ? "--patch" : NULL,
 				   pathspec);
 }
 
@@ -377,7 +377,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));
+		exit(interactive_add(argc - 1, argv + 1, prefix, patch_interactive));
 
 	if (edit_interactive)
 		return(edit_patch(argc, argv, prefix));
diff --git a/builtin/commit.c b/builtin/commit.c
index 7707af8..008c1ec 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -83,7 +83,7 @@ static const char *template_file;
 static const char *author_message, *author_message_buffer;
 static char *edit_message, *use_message;
 static char *fixup_message, *squash_message;
-static int all, edit_flag, also, interactive, only, amend, signoff;
+static int all, edit_flag, also, interactive, patch_interactive, only, amend, signoff;
 static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
 static int no_post_rewrite, allow_empty_message;
 static char *untracked_files_arg, *force_date, *ignore_submodule_arg;
@@ -152,6 +152,7 @@ static struct option builtin_commit_options[] = {
 	OPT_BOOLEAN('a', "all", &all, "commit all changed files"),
 	OPT_BOOLEAN('i', "include", &also, "add specified files to index for commit"),
 	OPT_BOOLEAN(0, "interactive", &interactive, "interactively add files"),
+	OPT_BOOLEAN('p', "patch", &patch_interactive, "interactively add changes"),
 	OPT_BOOLEAN('o', "only", &only, "commit only specified files"),
 	OPT_BOOLEAN('n', "no-verify", &no_verify, "bypass pre-commit hook"),
 	OPT_BOOLEAN(0, "dry-run", &dry_run, "show what would be committed"),
@@ -360,7 +361,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int
 		old_index_env = getenv(INDEX_ENVIRONMENT);
 		setenv(INDEX_ENVIRONMENT, index_lock.filename, 1);
 
-		if (interactive_add(argc, argv, prefix) != 0)
+		if (interactive_add(argc, argv, prefix, patch_interactive) != 0)
 			die(_("interactive add failed"));
 
 		if (old_index_env && *old_index_env)
@@ -1061,8 +1062,11 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		author_message_buffer = read_commit_message(author_message);
 	}
 
+	if (patch_interactive)
+		interactive = 1;
+
 	if (!!also + !!only + !!all + !!interactive > 1)
-		die(_("Only one of --include/--only/--all/--interactive can be used."));
+		die(_("Only one of --include/--only/--all/--interactive/--patch can be used."));
 	if (argc == 0 && (also || (only && !amend)))
 		die(_("No paths with --include/--only does not make sense."));
 	if (argc == 0 && only && amend)
diff --git a/commit.h b/commit.h
index b3c3bb7..43940e2 100644
--- a/commit.h
+++ b/commit.h
@@ -161,7 +161,7 @@ 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);
+extern int interactive_add(int argc, const char **argv, const char *prefix, int patch);
 extern int run_add_interactive(const char *revision, const char *patch_mode,
 			       const char **pathspec);
 
-- 
1.7.5.188.g4817

Re: [PATCH v3 3/3] Add support for -p/--patch to git-commit

[Valentin Haenel]

Being a big fan of the --patch options for various commands, I like this
one.

Since it uses the add--interactive.perl stuff, it would also be affacted
by 'interactive.singlekey'. So it might be worth mentioning that in
config.txt for sake of completeness. I currently have some patches that
modify config.txt in this regard, cooking in next, see: 46b522c

V-

Re: [PATCH v3 3/3] Add support for -p/--patch to git-commit

[Conrad Irwin]

On Sat, May 7, 2011 at 3:46 AM, Valentin Haenel <valentin@fsfe.org> wrote:
> Since it uses the add--interactive.perl stuff, it would also be affacted
> by 'interactive.singlekey'. So it might be worth mentioning that in
> config.txt for sake of completeness. I currently have some patches that
> modify config.txt in this regard, cooking in next, see: 46b522c
>

Ok, I've re-rolled the 3rd patch to use documentation in a similar
manner to yours for consistency. (It actually links to the
documentation for git-add as opposed to hinting that more information
may be found there).

I've also created a fourth patch that applies to the merge of this
patch set with vh/config-interactive-singlekey-doc to add git-commit
to the list of commands affected by interactive.singlekey.

Thanks,
Conrad

[PATCH v4 3/3] Add support for -p/--patch to git-commit

[Conrad Irwin]

The --interactive flag is already shared by git add and git commit,
share the -p and --patch flags too.

Signed-off-by: Conrad Irwin <conrad.irwin@gmail.com>
---
 Documentation/git-commit.txt |   25 ++++++++++++++++---------
 builtin/add.c                |    6 +++---
 builtin/commit.c             |   10 +++++++---
 commit.h                     |    2 +-
 4 files changed, 27 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index ed50271..7951cb7 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -8,11 +8,12 @@ git-commit - Record changes to the repository
 SYNOPSIS
 --------
 [verse]
-'git commit' [-a | --interactive] [-s] [-v] [-u<mode>] [--amend] [--dry-run]
-	   [(-c | -C | --fixup | --squash) <commit>] [-F <file> | -m <msg>]
-	   [--reset-author] [--allow-empty] [--allow-empty-message] [--no-verify]
-	   [-e] [--author=<author>] [--date=<date>] [--cleanup=<mode>]
-	   [--status | --no-status] [-i | -o] [--] [<file>...]
+'git commit' [-a | --interactive | --patch] [-s] [-v] [-u<mode>] [--amend]
+	   [--dry-run] [(-c | -C | --fixup | --squash) <commit>]
+	   [-F <file> | -m <msg>] [--reset-author] [--allow-empty]
+	   [--allow-empty-message] [--no-verify] [-e] [--author=<author>]
+	   [--date=<date>] [--cleanup=<mode>] [--status | --no-status]
+	   [-i | -o] [--] [<file>...]
 
 DESCRIPTION
 -----------
@@ -39,10 +40,10 @@ The content to be added can be specified in several ways:
    that have been removed from the working tree, and then perform the
    actual commit;
 
-5. by using the --interactive switch with the 'commit' command to decide one
-   by one which files should be part of the commit, before finalizing the
-   operation.  Currently, this is done by invoking 'git add --interactive'
-   on a temporary index.
+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. See the ``Interactive Mode`` section of
+   linkgit:git-add[1] to learn how to operate these modes.
 
 The `--dry-run` option can be used to obtain a
 summary of what is included by any of the above for the next
@@ -60,6 +61,12 @@ OPTIONS
 	been modified and deleted, but new files you have not
 	told git about are not affected.
 
+-p::
+--patch::
+	Use the interactive patch selection interface to chose
+	which changes to commit. See linkgit:git-add[1] for
+	details.
+
 -C <commit>::
 --reuse-message=<commit>::
 	Take an existing commit object, and reuse the log message
diff --git a/builtin/add.c b/builtin/add.c
index d39a6ab..f02524b 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -241,7 +241,7 @@ int run_add_interactive(const char *revision, const char *patch_mode,
 	return status;
 }
 
-int interactive_add(int argc, const char **argv, const char *prefix)
+int interactive_add(int argc, const char **argv, const char *prefix, int patch)
 {
 	const char **pathspec = NULL;
 
@@ -252,7 +252,7 @@ int interactive_add(int argc, const char **argv, const char *prefix)
 	}
 
 	return run_add_interactive(NULL,
-				   patch_interactive ? "--patch" : NULL,
+				   patch ? "--patch" : NULL,
 				   pathspec);
 }
 
@@ -377,7 +377,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));
+		exit(interactive_add(argc - 1, argv + 1, prefix, patch_interactive));
 
 	if (edit_interactive)
 		return(edit_patch(argc, argv, prefix));
diff --git a/builtin/commit.c b/builtin/commit.c
index 7707af8..008c1ec 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -83,7 +83,7 @@ static const char *template_file;
 static const char *author_message, *author_message_buffer;
 static char *edit_message, *use_message;
 static char *fixup_message, *squash_message;
-static int all, edit_flag, also, interactive, only, amend, signoff;
+static int all, edit_flag, also, interactive, patch_interactive, only, amend, signoff;
 static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
 static int no_post_rewrite, allow_empty_message;
 static char *untracked_files_arg, *force_date, *ignore_submodule_arg;
@@ -152,6 +152,7 @@ static struct option builtin_commit_options[] = {
 	OPT_BOOLEAN('a', "all", &all, "commit all changed files"),
 	OPT_BOOLEAN('i', "include", &also, "add specified files to index for commit"),
 	OPT_BOOLEAN(0, "interactive", &interactive, "interactively add files"),
+	OPT_BOOLEAN('p', "patch", &patch_interactive, "interactively add changes"),
 	OPT_BOOLEAN('o', "only", &only, "commit only specified files"),
 	OPT_BOOLEAN('n', "no-verify", &no_verify, "bypass pre-commit hook"),
 	OPT_BOOLEAN(0, "dry-run", &dry_run, "show what would be committed"),
@@ -360,7 +361,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int
 		old_index_env = getenv(INDEX_ENVIRONMENT);
 		setenv(INDEX_ENVIRONMENT, index_lock.filename, 1);
 
-		if (interactive_add(argc, argv, prefix) != 0)
+		if (interactive_add(argc, argv, prefix, patch_interactive) != 0)
 			die(_("interactive add failed"));
 
 		if (old_index_env && *old_index_env)
@@ -1061,8 +1062,11 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		author_message_buffer = read_commit_message(author_message);
 	}
 
+	if (patch_interactive)
+		interactive = 1;
+
 	if (!!also + !!only + !!all + !!interactive > 1)
-		die(_("Only one of --include/--only/--all/--interactive can be used."));
+		die(_("Only one of --include/--only/--all/--interactive/--patch can be used."));
 	if (argc == 0 && (also || (only && !amend)))
 		die(_("No paths with --include/--only does not make sense."));
 	if (argc == 0 && only && amend)
diff --git a/commit.h b/commit.h
index b3c3bb7..43940e2 100644
--- a/commit.h
+++ b/commit.h
@@ -161,7 +161,7 @@ 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);
+extern int interactive_add(int argc, const char **argv, const char *prefix, int patch);
 extern int run_add_interactive(const char *revision, const char *patch_mode,
 			       const char **pathspec);
 
-- 
1.7.5.188.g4817

Re: [PATCH v4 3/3] Add support for -p/--patch to git-commit

[Junio C Hamano]

Conrad Irwin <conrad.irwin@gmail.com> writes:

> diff --git a/builtin/add.c b/builtin/add.c
> index d39a6ab..f02524b 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -241,7 +241,7 @@ int run_add_interactive(const char *revision, const char *patch_mode,
>  	return status;
>  }
>  
> -int interactive_add(int argc, const char **argv, const char *prefix)
> +int interactive_add(int argc, const char **argv, const char *prefix, int patch)
>  {
>  	const char **pathspec = NULL;
>  
> @@ -252,7 +252,7 @@ int interactive_add(int argc, const char **argv, const char *prefix)
>  	}
>  
>  	return run_add_interactive(NULL,
> -				   patch_interactive ? "--patch" : NULL,
> +				   patch ? "--patch" : NULL,
>  				   pathspec);
>  }

This removes one reason for patch_interactive to be a file level global
variable. Perhaps we should plan to move builtin_add_options[] and the
file level global variables in builtin/add.c to cmd_add() and turn them
into on-stack variables.

It obviously is a totally separate topic, though.

[PATCH] Add commit to list of config.singlekey commands

[Conrad Irwin]

Signed-off-by: Conrad Irwin <conrad.irwin@gmail.com>
---
 Documentation/config.txt |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 285c7f7..1a060ec 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1310,9 +1310,10 @@ interactive.singlekey::
 	In interactive commands, allow the user to provide one-letter
 	input with a single key (i.e., without hitting enter).
 	Currently this is used by the `\--patch` mode of
-	linkgit:git-add[1], linkgit:git-reset[1], linkgit:git-stash[1] and
-	linkgit:git-checkout[1]. Note that this setting is silently
-	ignored if portable keystroke input is not available.
+	linkgit:git-add[1], linkgit:git-checkout[1], linkgit:git-commit[1],
+	linkgit:git-reset[1], and linkgit:git-stash[1]. Note that this
+	setting is silently ignored if portable keystroke input
+	is not available.
 
 log.date::
 	Set the default date-time mode for the 'log' command.
-- 
1.7.5.188.g4817

Re: [PATCH v3 0/3] Git commit --patch (again)

[Sverre Rabbelier]

Heya,

On Sat, May 7, 2011 at 07:59,  <conrad.irwin@gmail.com> wrote:
> I've rebased my support for git commit -p onto the current master
> branch. I've posted it to the list twice before [1][2].

I thought this got merged and have been disappointed a few times when
'git commit -p' didn't work. So I'm in favor :)

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH v3 0/3] Git commit --patch (again)

[Jeff King]

On Fri, May 06, 2011 at 10:59:58PM -0700, conrad.irwin@gmail.com wrote:

> I've rebased my support for git commit -p onto the current master
> branch. I've posted it to the list twice before [1][2].

Thanks for reposting. I had been meaning to look at this again, but
hadn't gotten around to it. So thanks for being persistent. :)

>   Use a temporary index for git commit --interactive

I think the intent of this one is good. In reviewing your initial
series, I had wondered about consistency with respect to the atomicity
of "git commit -p" versus "git add -p" (i.e., what state is the index
left in when you abort). But reading through the discussion again, I
think we should worry more about consistency between "git commit -i" and
"git commit --interactive". That is, both should produce no changes to
the index when the commit is aborted. So I think your patch is a step in
the right direction.

That still leaves an inconsistency in "git add -p" versus "git commit
-p" (e.g., if you abort "git add -p" with "^C"). But if we care, the
right solution is probably to make "git add -p" atomic. That can be a
separate topic, though, and I'm not sure anyone really cares enough to
work on it.

I have one final question. If I do abort a commit, is there any way to
recover the state that was in the temporary index? That is, if I abort
"git commit -i" by using an empty commit message, it is easy enough to
use shell history to repeat the command (possibly with a different set
of files). But if I spend some time selecting (and possibly editing)
hunks, and then decide to abort the commit, is there any way to recover
the intermediate index state?

>From my reading of the code, it looks like "no". We will rollback the
lockfile which contains the new index when aborting the commit.

I'm not sure if it is worth caring about. If you are really interested
in index state, you are probably better off using "git add -p" and "git
commit" separately. And even if we kept the index file around, it
requires a fairly savvy plumbing user to be able to pick changes out of
it.

>   Allow git commit --interactive with paths

Hmm. Test t7501.8 explicitly tests that this isn't allowed. But the test
is poorly written, and falsely returns success even with your patch.

The original test should have looked like this:

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 7f7f7c7..8090b3c 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -45,7 +45,8 @@ test_expect_success \
 test_expect_success PERL \
 	"using paths with --interactive" \
 	"echo bong-o-bong >file &&
-	! (echo 7 | git commit -m foo --interactive file)"
+	! ({ echo 2; echo 1; echo; echo 7; } |
+	git commit -m foo --interactive file)"
 
 test_expect_success \
 	"using invalid commit with -C" \

which does properly fail with your change. Your commit should tweak that
test (speaking of which, it would be nice for patch 1 to have a test,
too).

Other than that, the code in all 3 looks fine to me.

-Peff

Re: [PATCH v3 0/3] Git commit --patch (again)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I'm not sure if it is worth caring about. If you are really interested
> in index state, you are probably better off using "git add -p" and "git
> commit" separately.

I agree with this. I do not foresee myself using "commit -p" ever for this
exact reason. I however didn't see anything wrong in the series and I do
not see any reason to reject it, either. It's just another long rope other
people can use to tangle their neck with ;-).

> Hmm. Test t7501.8 explicitly tests that this isn't allowed. But the test
> is poorly written, and falsely returns success even with your patch.

Thanks. Let me see if I can simply amend what I queued already ;-)

Re: [PATCH v3 0/3] Git commit --patch (again)

[Jeff King]

On Mon, May 09, 2011 at 09:53:05AM -0700, Junio C Hamano wrote:

> I agree with this. I do not foresee myself using "commit -p" ever for this
> exact reason. I however didn't see anything wrong in the series and I do
> not see any reason to reject it, either. It's just another long rope other
> people can use to tangle their neck with ;-).

Heh. Maybe your title should be "Git Hangman". :)

> > Hmm. Test t7501.8 explicitly tests that this isn't allowed. But the test
> > is poorly written, and falsely returns success even with your patch.
> 
> Thanks. Let me see if I can simply amend what I queued already ;-)

It's unfortunately not quite as simple as having that test succeed, as
it changes state that breaks later tests. I didn't investigate deeply,
though.

-Peff

Re: [PATCH v3 0/3] Git commit --patch (again)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It's unfortunately not quite as simple as having that test succeed, as
> it changes state that breaks later tests. I didn't investigate deeply,
> though.

Yeah, that test that hardcodes the exact commit sequence is disgusting.
In the meantime...

-- >8 --
From: Jeff King <peff@peff.net>
Subject: [PATCH] t7501.8: feed a meaningful command

The command expects "git commit --interactive <path>" to fail because you
cannot (yet) limit "commit --interactive" with a pathspec, but even if the
command allowed to take <path>, the test would have failed as saying just
7:quit would leave the index the same as the current commit, leading to an
attempt to create an empty commit that would fail without --allow-empty.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t7501-commit.sh |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index a76c474..3d2b14d 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -41,10 +41,12 @@ test_expect_success \
 	"echo King of the bongo >file &&
 	test_must_fail git commit -m foo -a file"
 
-test_expect_success PERL \
-	"using paths with --interactive" \
-	"echo bong-o-bong >file &&
-	! (echo 7 | git commit -m foo --interactive file)"
+test_expect_success PERL 'cannot use paths with --interactive' '
+	echo bong-o-bong >file &&
+	# 2: update, 1:st path, that is all, 7: quit
+	( echo 2; echo 1; echo; echo 7 ) |
+	test_must_fail git commit -m foo --interactive file
+'
 
 test_expect_success \
 	"using invalid commit with -C" \
-- 
1.7.5.1.290.g1b565

Re: [PATCH v3 0/3] Git commit --patch (again)

[Junio C Hamano]

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

> Jeff King <peff@peff.net> writes:
>
>> It's unfortunately not quite as simple as having that test succeed, as
>> it changes state that breaks later tests. I didn't investigate deeply,
>> though.
>
> Yeah, that test that hardcodes the exact commit sequence is disgusting.
> In the meantime...
>
> -- >8 --
> From: Jeff King <peff@peff.net>
> Subject: [PATCH] t7501.8: feed a meaningful command

And then on top of that, Conrad's "allow commit --interactive <path>"
would come, with this squashed in.  The last "reset HEAD^" is nasty but I
don't have enough energy to fix 12ace0b (Add test case for basic commit
functionality., 2007-07-31) today.

 t/t7501-commit.sh |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 3d2b14d..c2fd116 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -41,11 +41,12 @@ test_expect_success \
 	"echo King of the bongo >file &&
 	test_must_fail git commit -m foo -a file"
 
-test_expect_success PERL 'cannot use paths with --interactive' '
+test_expect_success PERL 'can use paths with --interactive' '
 	echo bong-o-bong >file &&
 	# 2: update, 1:st path, that is all, 7: quit
 	( echo 2; echo 1; echo; echo 7 ) |
-	test_must_fail git commit -m foo --interactive file
+	git commit -m foo --interactive file &&
+	git reset --hard HEAD^
 '
 
 test_expect_success \

Re: [PATCH v3 0/3] Git commit --patch (again)

[Jeff King]

On Mon, May 09, 2011 at 04:56:20PM -0700, Junio C Hamano wrote:

> > Yeah, that test that hardcodes the exact commit sequence is disgusting.
> > In the meantime...
> >
> > -- >8 --
> > From: Jeff King <peff@peff.net>
> > Subject: [PATCH] t7501.8: feed a meaningful command
> 
> And then on top of that, Conrad's "allow commit --interactive <path>"
> would come, with this squashed in.  The last "reset HEAD^" is nasty but I
> don't have enough energy to fix 12ace0b (Add test case for basic commit
> functionality., 2007-07-31) today.
> 
>  t/t7501-commit.sh |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
> index 3d2b14d..c2fd116 100755
> --- a/t/t7501-commit.sh
> +++ b/t/t7501-commit.sh
> @@ -41,11 +41,12 @@ test_expect_success \
>  	"echo King of the bongo >file &&
>  	test_must_fail git commit -m foo -a file"
>  
> -test_expect_success PERL 'cannot use paths with --interactive' '
> +test_expect_success PERL 'can use paths with --interactive' '
>  	echo bong-o-bong >file &&
>  	# 2: update, 1:st path, that is all, 7: quit
>  	( echo 2; echo 1; echo; echo 7 ) |
> -	test_must_fail git commit -m foo --interactive file
> +	git commit -m foo --interactive file &&
> +	git reset --hard HEAD^
>  '

Yeah, that reset is a hack. Looking through the tests that come after,
I don't think the extra commit is silently hurting any of them, so it's
really just the stupid hard-coded rev-list one.

I looked at rewriting it into something like "git log --format=%s", but
I couldn't come up with a commit message that reasonably argued "this
tests the same thing as the original". Because I haven't really figured
out what the original is supposed to be testing. That we made commits?
That we made a particular sequence of commits with those messages? With
particular trees?

I feel like each test should be responsible for figuring out whether it
committed or not (and AFAICT, they do). So I would be tempted to just
delete the rev-list test.

-Peff

Re: [PATCH v3 0/3] Git commit --patch (again)

[Jeff King]

On Mon, May 09, 2011 at 04:53:00PM -0700, Junio C Hamano wrote:

> -- >8 --
> From: Jeff King <peff@peff.net>
> Subject: [PATCH] t7501.8: feed a meaningful command
> 
> The command expects "git commit --interactive <path>" to fail because you
> cannot (yet) limit "commit --interactive" with a pathspec, but even if the
> command allowed to take <path>, the test would have failed as saying just
> 7:quit would leave the index the same as the current commit, leading to an
> attempt to create an empty commit that would fail without --allow-empty.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Thanks. The commit message and SBO-forgery are fine by me. :)

-Peff

Re: [PATCH v3 0/3] Git commit --patch (again)

[Conrad Irwin]

On Mon, May 9, 2011 at 7:44 AM, Jeff King <peff@peff.net> wrote:
>
> That still leaves an inconsistency in "git add -p" versus "git commit
> -p" (e.g., if you abort "git add -p" with "^C"). But if we care, the
> right solution is probably to make "git add -p" atomic. That can be a
> separate topic, though, and I'm not sure anyone really cares enough to
> work on it.

I have pondered this problem too. "git add -p" is a particularly unintuitive,
it inherits the update-the-index-after-every-complete-file semantics of
"git add --interactive", but without ever making the file boundaries clear
to the user. Depending on how much this bugs me, I might try to fix it
in the future, but I wonder whether any users of "git add --interactive"
are relying on the file based granularity.

>
> I have one final question. If I do abort a commit, is there any way to
> recover the state that was in the temporary index? That is, if I abort
> "git commit -i" by using an empty commit message, it is easy enough to
> use shell history to repeat the command (possibly with a different set
> of files). But if I spend some time selecting (and possibly editing)
> hunks, and then decide to abort the commit, is there any way to recover
> the intermediate index state?

Not really. You could use your knowledge of git-commit to assume that the
tree object with the most recent ctime is probably useful, but that only
works some of the time. The occasions on which I have wanted to abort
the process half-way, I've just created a temporary commit with what I
have so far. A "git reset --soft" at that point makes the whole process
long-hand for "git add -p".

>
>>   Allow git commit --interactive with paths
>
> Hmm. Test t7501.8 explicitly tests that this isn't allowed. But the test
> is poorly written, and falsely returns success even with your patch.
>

Well spotted. Thank you Junio for fixing that test.

> which does properly fail with your change. Your commit should tweak that
> test (speaking of which, it would be nice for patch 1 to have a test,
> too).

I'll try to get my head around the tests :).

Conrad

[PATCH] Test atomic git-commit --interactive

[Conrad Irwin]

Signed-off-by: Conrad Irwin <conrad.irwin@gmail.com>
---
 t/t7501-commit.sh |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index a76c474..2a4e453 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -130,6 +130,15 @@ test_expect_success PERL \
 	"interactive add" \
 	"echo 7 | git commit --interactive | grep 'What now'"
 
+test_expect_success PERL \
+	"--interactive doesn't change index if editor aborts" \
+	"EDITOR=false echo zoo >file && \
+	test_must_fail git diff --exit-code > diff1 && \
+	(echo u ; echo '*' ; echo q) |\
+	test_must_fail git commit --interactive && \
+	git diff > diff2 && \
+	git diff --no-index diff1 diff2"
+
 test_expect_success \
 	"showing committed revisions" \
 	"git rev-list HEAD >current"
-- 
1.7.5.188.g4817

Re: [PATCH] Test atomic git-commit --interactive

[Jeff King]

On Tue, May 10, 2011 at 12:12:31PM -0700, Conrad Irwin wrote:

> +test_expect_success PERL \
> +	"--interactive doesn't change index if editor aborts" \
> +	"EDITOR=false echo zoo >file && \

This EDITOR bit does nothing, since it sets the value only for the
"echo" command. The test happens to do what you want, though, since we
set EDITOR to ":" in test-lib.sh, and that is also a fine value for your
test (it's probably a better test, anyway; it simulates the user
aborting with an empty message instead of the editor crashing).

Even though the implicit EDITOR works, I think it makes sense to be
explicit that it is something we are relying on. So we need to put it in
front of the "git commit" invocation, like:

  EDITOR=: test_must_fail git commit --interactive

Unfortunately, some shells do behave reasonably when setting single-shot
environment variables with functions, so we are stuck with:

  (EDITOR=: && export EDITOR &&
   test_must_fail git commit --interactive)

> +	test_must_fail git diff --exit-code > diff1 && \

You can drop the backslash-continuation on lines that end with "&&" or
"|", which makes things a bit more readable.

You do still need ones for:

  test_expect_success "description" \
                      "actual test"

The usual style for our tests is:

  test_expect_success 'description' '
          actual test
  '

but this particular script does not follow that, so it's probably more
sensible to follow the surrounding style as you did.

> +	(echo u ; echo '*' ; echo q) |\
> +	test_must_fail git commit --interactive && \
> +	git diff > diff2 && \

Minor style nit: we usually write ">file" without the space.

> +	git diff --no-index diff1 diff2"

When comparing results with a simple diff, we generally use "test_cmp",
so that unrelated tests do not rely on "git diff --no-index".

So the result should look like:

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 7f7f7c7..ccb0b95 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -131,6 +131,16 @@ test_expect_success PERL \
 	"interactive add" \
 	"echo 7 | git commit --interactive | grep 'What now'"
 
+test_expect_success PERL \
+	"--interactive doesn't change index if editor aborts" \
+	"echo zoo >file &&
+	test_must_fail git diff --exit-code >diff1 &&
+	(echo u ; echo '*' ; echo q) |
+	(EDITOR=: && export EDITOR &&
+	 test_must_fail git commit --interactive) &&
+	git diff >diff2 &&
+	test_cmp diff1 diff2"
+
 test_expect_success \
 	"showing committed revisions" \
 	"git rev-list HEAD >current"

-Peff

Re: [PATCH] Test atomic git-commit --interactive

[Conrad Irwin]

On Tue, May 10, 2011 at 12:43 PM, Jeff King <peff@peff.net> wrote:
> On Tue, May 10, 2011 at 12:12:31PM -0700, Conrad Irwin wrote:
>
>> +test_expect_success PERL \
>> +     "--interactive doesn't change index if editor aborts" \
>> +     "EDITOR=false echo zoo >file && \
>
> This EDITOR bit does nothing, since it sets the value only for the
> "echo" command. The test happens to do what you want, though, since we
> set EDITOR to ":" in test-lib.sh, and that is also a fine value for your
> test (it's probably a better test, anyway; it simulates the user
> aborting with an empty message instead of the editor crashing).

>  (EDITOR=: && export EDITOR &&
>   test_must_fail git commit --interactive)

That makes sense, I was unaware of the : command, thank you.

> You can drop the backslash-continuation on lines that end with "&&" or
> "|", which makes things a bit more readable.

Good to know.

> So the result should look like:

Awesome, thanks.

Conrad