Some Commit Messages Scare git-rev-list

Some Commit Messages Scare git-rev-list

[Darrin Thompson]

This scripts exhibits some odd behavior. Apparently git-rev-list
mishandles commit messages which do not end in a newline. This as best I
can tell this is a problem introduced since 1.1.5.

Here is a script to reproduce the problem:

rm -rf git-test
mkdir git-test
cd git-test
git-init-db
echo hello > hello
git-add hello

# send scary message to git-commit -F -
echo -n "test commit" | git-commit -F - -a
echo world > world
git-add world
git-update-index --add world
treeid=$(git-write-tree)

# send scary message directly to git-commit-tree
commitid=$(echo -n "another-test" | git-commit-tree $treeid -p HEAD)
git-update-ref HEAD $commitid

# see the wreckage
git-rev-list --pretty HEAD

Running gitk will also show the problem.

--
Darrin

Re: Some Commit Messages Scare git-rev-list

[Linus Torvalds]

On Wed, 12 Apr 2006, Darrin Thompson wrote:
>
> This scripts exhibits some odd behavior. Apparently git-rev-list
> mishandles commit messages which do not end in a newline. This as best I
> can tell this is a problem introduced since 1.1.5.

Fixed like so..

However, your script shows another problem: the "#" added at the end of 
the line for a 

	echo -n "duh" | git-commit -F - -a

seems to be because we append the "git status" output to it, and then we 
drop the lines that start with a '#', but due to the "-n", the first # 
ends up being at the end of the line. 

I suspect that when we get the commit message like that, we should _not_ 
do any of the commit message editing at all.

That's a separate issue, though, and not fixed by this patch.

		Linus

---
diff --git a/commit.c b/commit.c
index d534c9b..c7bb8db 100644
--- a/commit.c
+++ b/commit.c
@@ -400,11 +400,11 @@ static int get_one_line(const char *msg,
 
 	while (len--) {
 		char c = *msg++;
+		if (!c)
+			break;
 		ret++;
 		if (c == '\n')
 			break;
-		if (!c)
-			return 0;
 	}
 	return ret;
 }

Re: Some Commit Messages Scare git-rev-list

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> Fixed like so..

Thanks.

> However, your script shows another problem: the "#" added at the end of 
> the line for a 
>
> 	echo -n "duh" | git-commit -F - -a
>
> seems to be because we append the "git status" output to it, and then we 
> drop the lines that start with a '#', but due to the "-n", the first # 
> ends up being at the end of the line. 
>
> I suspect that when we get the commit message like that, we should _not_ 
> do any of the commit message editing at all.

True.

-- >8 --
[PATCH] git-commit: do not muck with commit message when no_edit is set.

Spotted by Linus and Darrin Thompson.  When we took a commit
message from -F <file> with an incomplete line, we appended "git
status" output, which ended up attaching a lone "#" at the end.

We still need the "do we have anything to commit?" check by
running "status" (which has to know what to do in different
cases with -i/-o/-a), but there is no point appending its output
to the proposed commit message given by the user.

Signed-off-by: Junio C Hamano <junkio@cox.net>

---

 git-commit.sh |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

475443c8489d9167b944367d8ec8bfef77bee0a5
diff --git a/git-commit.sh b/git-commit.sh
index 1e7c09e..bd3dc71 100755
--- a/git-commit.sh
+++ b/git-commit.sh
@@ -537,7 +537,7 @@ t)
 	;;
 esac
 
-if [ -f "$GIT_DIR/MERGE_HEAD" ]; then
+if test -f "$GIT_DIR/MERGE_HEAD" && test -z "$no_edit"; then
 	echo "#"
 	echo "# It looks like you may be committing a MERGE."
 	echo "# If this is not correct, please remove the file"
@@ -605,16 +605,23 @@ else
 	current=
 fi
 
-{
-	test -z "$only_include_assumed" || echo "$only_include_assumed"
-	run_status
-} >>"$GIT_DIR"/COMMIT_EDITMSG
+if test -z "$no_edit"
+then
+	{
+		test -z "$only_include_assumed" || echo "$only_include_assumed"
+		run_status
+	} >>"$GIT_DIR"/COMMIT_EDITMSG
+else
+	# we need to check if there is anything to commit
+	run_status >/dev/null 
+fi
 if [ "$?" != "0" -a ! -f "$GIT_DIR/MERGE_HEAD" -a -z "$amend" ]
 then
 	rm -f "$GIT_DIR/COMMIT_EDITMSG"
 	run_status
 	exit 1
 fi
+
 case "$no_edit" in
 '')
 	case "${VISUAL:-$EDITOR},$TERM" in
-- 
1.3.0.rc3.g72c1

Re: Some Commit Messages Scare git-rev-list

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> We still need the "do we have anything to commit?" check by
> running "status" (which has to know what to do in different
> cases with -i/-o/-a), but there is no point appending its output
> to the proposed commit message given by the user.

BTW, this does not quite work as expected if you did something
like this:

	echo -n 'incomplete line' | git commit -a -s -F -

because we would want to append the Signed-off-by: line.  I am
almost tempted to say "then do not do it", but it might make
sense to do this as well.

-- >8 --
[PATCH] stripspace: make sure not to leave an incomplete line.

When dealing with a commit log message for human consumption, it
never makes sense to keep a log that ends with an incomplete
line, so make it a part of the clean-up process done with
git-stripspace.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
diff --git a/stripspace.c b/stripspace.c
index 96cd0a8..dee1ef0 100644
--- a/stripspace.c
+++ b/stripspace.c
@@ -6,9 +6,9 @@ #include <ctype.h>
  * Remove empty lines from the beginning and end.
  *
  * Turn multiple consecutive empty lines into just one
- * empty line.
+ * empty line.  Return true if it is an incomplete line.
  */
-static void cleanup(char *line)
+static int cleanup(char *line)
 {
 	int len = strlen(line);
 
@@ -21,16 +21,19 @@ static void cleanup(char *line)
 			len--;
 			line[len] = 0;
 		} while (len > 1);
+		return 0;
 	}
+	return 1;
 }
 
 int main(int argc, char **argv)
 {
 	int empties = -1;
+	int incomplete = 0;
 	char line[1024];
 
 	while (fgets(line, sizeof(line), stdin)) {
-		cleanup(line);
+		incomplete = cleanup(line);
 
 		/* Not just an empty line? */
 		if (line[0] != '\n') {
@@ -44,5 +47,7 @@ int main(int argc, char **argv)
 			continue;
 		empties++;
 	}
+	if (incomplete)
+		putchar('\n');
 	return 0;
 }

Re: Some Commit Messages Scare git-rev-list

[Linus Torvalds]

On Wed, 12 Apr 2006, Junio C Hamano wrote:
>
> [PATCH] stripspace: make sure not to leave an incomplete line.

Ack.

		Linus