[PATCH] git-commit -f <file>: take message from file, and remove file upon completion.

[PATCH] git-commit -f <file>: take message from file, and remove file upon completion.

[Gerrit Pape]

Change (undocumented) git commit -f option from a synonym for git-commit -F
to "Take the commit message from the given file, and remove the file upon
successful commit".

git-revert and git-cherry-pick should make use of this to properly cleanup
the automatically created ./.msg file.

Signed-off-by: Gerrit Pape <pape@smarden.org>
---
 Documentation/git-commit.txt |    7 ++++---
 git-commit.sh                |   25 +++++++++++++++++++++++--
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 53a7bb0..da68c81 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -9,7 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git-commit' [-a | --interactive] [-s] [-v]
-	   [(-c | -C) <commit> | -F <file> | -m <msg> | --amend]
+	   [(-c | -C) <commit> | (-F | -f) <file> | -m <msg> | --amend]
 	   [--no-verify] [-e] [--author <author>]
 	   [--] [[-i | -o ]<file>...]
 
@@ -63,9 +63,10 @@ OPTIONS
 	invoked; with '-c' the user can further edit the commit
 	message.
 
--F <file>::
+-F or -f <file>::
 	Take the commit message from the given file.  Use '-' to
-	read the message from the standard input.
+	read the message from the standard input.  With '-f', the
+	file is removed upon successful commit; with '-F' not.
 
 --author <author>::
 	Override the author name used in the commit.  Use
diff --git a/git-commit.sh b/git-commit.sh
index f28fc24..e4518c4 100755
--- a/git-commit.sh
+++ b/git-commit.sh
@@ -74,6 +74,7 @@ also=
 interactive=
 only=
 logfile=
+logfile_rm=
 use_commit=
 amend=
 edit_flag=
@@ -90,15 +91,31 @@ untracked_files=
 while case "$#" in 0) break;; esac
 do
 	case "$1" in
-	-F|--F|-f|--f|--fi|--fil|--file)
+	-f)
 		case "$#" in 1) usage ;; esac
 		shift
 		no_edit=t
 		log_given=t$log_given
 		logfile="$1"
+		test "$logfile" = '-' || logfile_rm=1
 		shift
 		;;
-	-F*|-f*)
+	-F|--F|--f|--fi|--fil|--file)
+		case "$#" in 1) usage ;; esac
+		shift
+		no_edit=t
+		log_given=t$log_given
+		logfile="$1"
+		shift
+		;;
+	-f*)
+		no_edit=t
+		log_given=t$log_given
+		logfile=`expr "z$1" : 'z-[Ff]\(.*\)'`
+		test "$logfile" = '-' || logfile_rm=1
+		shift
+		;;
+	-F*)
 		no_edit=t
 		log_given=t$log_given
 		logfile=`expr "z$1" : 'z-[Ff]\(.*\)'`
@@ -653,6 +670,10 @@ then
 		       --summary --root HEAD --`
 		echo "Created${initial_commit:+ initial} commit $commit"
 	fi
+	if test -n "$logfile_rm"
+	then
+		rm -f "$logfile"
+	fi
 fi
 
 exit "$ret"
-- 
1.5.1.2

[PATCH] Have git-revert, git-cherry-pick cleanup ./.msg upon successful completion.

[Gerrit Pape]

git-revert and git-cherry-pick left behind the commit message file ./.msg,
have them use the -f option to git-commit to properly cleanup the
automatically created file.

Signed-off-by: Gerrit Pape <pape@smarden.org>
---
 builtin-revert.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin-revert.c b/builtin-revert.c
index 4ba0ee6..9acdf47 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -376,10 +376,10 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 
 	if (!no_commit) {
 		if (edit)
-			return execl_git_cmd("commit", "-n", "-F", ".msg",
+			return execl_git_cmd("commit", "-n", "-f", ".msg",
 				"-e", NULL);
 		else
-			return execl_git_cmd("commit", "-n", "-F", ".msg",
+			return execl_git_cmd("commit", "-n", "-f", ".msg",
 				NULL);
 	}
 	if (reencoded_message)
-- 
1.5.1.2

Re: [PATCH] Have git-revert, git-cherry-pick cleanup ./.msg upon successful completion.

[Alex Riesen]

On 5/4/07, Gerrit Pape <pape@smarden.org> wrote:
> git-revert and git-cherry-pick left behind the commit message file ./.msg,
> have them use the -f option to git-commit to properly cleanup the
> automatically created file.

Could we also have the files in $GIT_DIR instead of in working directory?

Re: [PATCH] Have git-revert, git-cherry-pick cleanup ./.msg upon successful completion.

[Shawn O. Pearce]

Gerrit Pape <pape@smarden.org> wrote:
> git-revert and git-cherry-pick left behind the commit message file ./.msg,
> have them use the -f option to git-commit to properly cleanup the
> automatically created file.

I'm actually sort of against changing the behavior of git-commit
-f to mean "delete the file".  We never did that before.  Users
might get surprised when their file goes away!

What about this change instead?  We make cherry-pick/revert
use the same temporary file as merge, which is under .git/
(something Alex mentioned he wanted).  I think the use of ".msg"
in cherry-pick/revert has always just been a bug, and not a feature,
so I'm really not against changing things around like this.


diff --git a/builtin-revert.c b/builtin-revert.c
index 4ba0ee6..67c13a3 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -237,6 +237,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	int i;
 	char *oneline, *reencoded_message = NULL;
 	const char *message, *encoding;
+	const char *defmsg = git_path("MERGE_MSG");
 
 	git_config(git_default_config);
 	me = action == REVERT ? "revert" : "cherry-pick";
@@ -280,7 +281,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	 * reverse of it if we are revert.
 	 */
 
-	msg_fd = hold_lock_file_for_update(&msg_file, ".msg", 1);
+	msg_fd = hold_lock_file_for_update(&msg_file, defmsg, 1);
 
 	encoding = get_encoding(message);
 	if (!encoding)
@@ -330,7 +331,6 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 				sha1_to_hex(head), "HEAD",
 				sha1_to_hex(next->object.sha1), oneline) ||
 			write_tree(head, 0, NULL)) {
-		const char *target = git_path("MERGE_MSG");
 		add_to_msg("\nConflicts:\n\n");
 		read_cache();
 		for (i = 0; i < active_nr;) {
@@ -345,10 +345,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 			}
 		}
 		if (close(msg_fd) || commit_lock_file(&msg_file) < 0)
-			die ("Error wrapping up .msg");
-		unlink(target);
-		if (rename(".msg", target))
-			die ("Could not move .msg to %s", target);
+			die ("Error wrapping up %s", defmsg);
 		fprintf(stderr, "Automatic %s failed.  "
 			"After resolving the conflicts,\n"
 			"mark the corrected paths with 'git-add <paths>'\n"
@@ -362,7 +359,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 		exit(1);
 	}
 	if (close(msg_fd) || commit_lock_file(&msg_file) < 0)
-		die ("Error wrapping up .msg");
+		die ("Error wrapping up %s", defmsg);
 	fprintf(stderr, "Finished one %s.\n", me);
 
 	/*
@@ -376,11 +373,9 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 
 	if (!no_commit) {
 		if (edit)
-			return execl_git_cmd("commit", "-n", "-F", ".msg",
-				"-e", NULL);
+			return execl_git_cmd("commit", "-n", NULL);
 		else
-			return execl_git_cmd("commit", "-n", "-F", ".msg",
-				NULL);
+			return execl_git_cmd("commit", "-n", "-F", defmsg, NULL);
 	}
 	if (reencoded_message)
 		free(reencoded_message);

-- 
Shawn.

Re: [PATCH] Have git-revert, git-cherry-pick cleanup ./.msg upon successful completion.

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> What about this change instead?  We make cherry-pick/revert
> use the same temporary file as merge, which is under .git/
> (something Alex mentioned he wanted).  I think the use of ".msg"
> in cherry-pick/revert has always just been a bug, and not a feature,
> so I'm really not against changing things around like this.

While I would not say this is not an improvement, this makes
MERGE_MSG even less about merges and pushes us away from a
sensible "git whatnow".

Re: [PATCH] Have git-revert, git-cherry-pick cleanup ./.msg upon successful completion.

[Gerrit Pape]

On Fri, May 04, 2007 at 11:46:15PM -0400, Shawn O. Pearce wrote:
> Gerrit Pape <pape@smarden.org> wrote:
> > git-revert and git-cherry-pick left behind the commit message file ./.msg,
> > have them use the -f option to git-commit to properly cleanup the
> > automatically created file.
> 
> I'm actually sort of against changing the behavior of git-commit
> -f to mean "delete the file".  We never did that before.  Users
> might get surprised when their file goes away!

-f never was documented AFAICS, only -F, and even though the file is
gone, the commit message is available through git show then.  I
personally would like to have such an option to git commit regardless of
revert and cherry-pick.

I concur with Alex that the teporary file should be in the git directory
and not working directory.

Regards, Gerrit.

[PATCH] Have git-revert, git-cherry-pick use $GIT_DIR/COMMIT_MSG instead of ./.msg.

[Gerrit Pape]

On Fri, May 04, 2007 at 05:28:21PM +0200, Alex Riesen wrote:
> On 5/4/07, Gerrit Pape <pape@smarden.org> wrote:
> >git-revert and git-cherry-pick left behind the commit message file ./.msg,
> >have them use the -f option to git-commit to properly cleanup the
> >automatically created file.
>
> Could we also have the files in $GIT_DIR instead of in working directory?

Yes, .git/COMMIT_MSG is already used by git-commit and git-svn, and can be
used here too.

Signed-off-by: Gerrit Pape <pape@smarden.org>
---
 builtin-revert.c |   24 +++++++++++++-----------
 1 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/builtin-revert.c b/builtin-revert.c
index 9acdf47..5aff283 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -126,14 +126,15 @@ char *get_encoding(const char *message)
 	return NULL;
 }
 
-struct lock_file msg_file;
+static const char* msg_file;
+struct lock_file msg_file_lock;
 static int msg_fd;
 
 static void add_to_msg(const char *string)
 {
 	int len = strlen(string);
 	if (write_in_full(msg_fd, string, len) < 0)
-		die ("Could not write to .msg");
+		die ("Could not write to %s", msg_file);
 }
 
 static void add_message_to_msg(const char *message)
@@ -280,7 +281,8 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	 * reverse of it if we are revert.
 	 */
 
-	msg_fd = hold_lock_file_for_update(&msg_file, ".msg", 1);
+	msg_file = git_path("COMMIT_MSG");
+	msg_fd = hold_lock_file_for_update(&msg_file_lock, msg_file, 1);
 
 	encoding = get_encoding(message);
 	if (!encoding)
@@ -344,11 +346,11 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 					i++;
 			}
 		}
-		if (close(msg_fd) || commit_lock_file(&msg_file) < 0)
-			die ("Error wrapping up .msg");
+		if (close(msg_fd) || commit_lock_file(&msg_file_lock) < 0)
+			die ("Error wrapping up %s", msg_file);
 		unlink(target);
-		if (rename(".msg", target))
-			die ("Could not move .msg to %s", target);
+		if (rename(msg_file, target))
+			die ("Could not move %s to %s", msg_file, target);
 		fprintf(stderr, "Automatic %s failed.  "
 			"After resolving the conflicts,\n"
 			"mark the corrected paths with 'git-add <paths>'\n"
@@ -361,8 +363,8 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 		}
 		exit(1);
 	}
-	if (close(msg_fd) || commit_lock_file(&msg_file) < 0)
-		die ("Error wrapping up .msg");
+	if (close(msg_fd) || commit_lock_file(&msg_file_lock) < 0)
+		die ("Error wrapping up %s", msg_file);
 	fprintf(stderr, "Finished one %s.\n", me);
 
 	/*
@@ -376,10 +378,10 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 
 	if (!no_commit) {
 		if (edit)
-			return execl_git_cmd("commit", "-n", "-f", ".msg",
+			return execl_git_cmd("commit", "-n", "-f", msg_file,
 				"-e", NULL);
 		else
-			return execl_git_cmd("commit", "-n", "-f", ".msg",
+			return execl_git_cmd("commit", "-n", "-f", msg_file,
 				NULL);
 	}
 	if (reencoded_message)
-- 
1.5.1.2

[PATCH] git-commit: fix usage to show (-F|-f) <logfile>.

[Gerrit Pape]

Signed-off-by: Gerrit Pape <pape@smarden.org>
---
 git-commit.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-commit.sh b/git-commit.sh
index e4518c4..bbd76c6 100755
--- a/git-commit.sh
+++ b/git-commit.sh
@@ -3,7 +3,7 @@
 # Copyright (c) 2005 Linus Torvalds
 # Copyright (c) 2006 Junio C Hamano
 
-USAGE='[-a | --interactive] [-s] [-v] [--no-verify] [-m <message> | -F <logfile> | (-C|-c) <commit> | --amend] [-u] [-e] [--author <author>] [[-i | -o] <path>...]'
+USAGE='[-a | --interactive] [-s] [-v] [--no-verify] [-m <message> | (-F|-f) <logfile> | (-C|-c) <commit> | --amend] [-u] [-e] [--author <author>] [[-i | -o] <path>...]'
 SUBDIRECTORY_OK=Yes
 . git-sh-setup
 require_work_tree
-- 
1.5.1.2

Re: [PATCH] Have git-revert, git-cherry-pick cleanup ./.msg upon successful completion.

[Shawn O. Pearce]

Junio C Hamano <junkio@cox.net> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
> 
> > What about this change instead?  We make cherry-pick/revert
> > use the same temporary file as merge, which is under .git/
> > (something Alex mentioned he wanted).  I think the use of ".msg"
> > in cherry-pick/revert has always just been a bug, and not a feature,
> > so I'm really not against changing things around like this.
> 
> While I would not say this is not an improvement, this makes
> MERGE_MSG even less about merges and pushes us away from a
> sensible "git whatnow".
 
I think that ship has already sailed.  Look at builtin-revert.c
on:

    333         const char *target = git_path("MERGE_MSG");

We're already using MERGE_MSG to prep the message for a conflicted
cherry-pick or revert that the user needs to resolve by hand.  I
think we do the same thing in git-rebase, don't we?

Gerrit's patch to try and use COMMIT_MSG feels wrong to me, as
git-commit overwrites that file with what it gets from its "input".

I agree my patch steps us further from a "git whatnow", but we're
already in deep with MERGE_MSG.  We might as well keep that existing
convention that it can be used to prep the commit message for the
next git-commit invocation, and record other data somehow for the
"git whatnow" case.

-- 
Shawn.

Re: [PATCH] Have git-revert, git-cherry-pick cleanup ./.msg upon successful completion.

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> Gerrit's patch to try and use COMMIT_MSG feels wrong to me, as
> git-commit overwrites that file with what it gets from its "input".

Yes.

> I agree my patch steps us further from a "git whatnow", but we're
> already in deep with MERGE_MSG.  We might as well keep that existing
> convention that it can be used to prep the commit message for the
> next git-commit invocation, and record other data somehow for the
> "git whatnow" case.

Fair enough.