[PATCH 0/3] builtin-commit fixes

[PATCH 0/3] builtin-commit fixes

[Johannes Schindelin]

Hi,

The following three commits are on top of kh/commit. With these 3 patches, 
the test suite passes.

Ciao,
Dscho

[PATCH 1/3] builtin-commit: fix reflog message generation

[Johannes Schindelin]

Instead of strdup()ing, we can just reuse the buffer in which the
commit message is stored, and which is supposed to hold the reflog
message anyway.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin-commit.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index f108e90..bba9b82 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -488,7 +488,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	int header_len, parent_count = 0;
 	struct strbuf sb;
 	const char *index_file, *reflog_msg;
-	char *nl, *header_line;
+	char *nl;
 	unsigned char commit_sha1[20];
 	struct ref_lock *ref_lock;
 
@@ -585,12 +585,13 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 					   0);
 
 	nl = strchr(sb.buf + header_len, '\n');
-	header_line = xstrndup(sb.buf + header_len,
-			       nl - (sb.buf + header_len));
-	strbuf_release(&sb);
-	strbuf_addf(&sb, "%s: %s\n", reflog_msg, header_line);
-	strbuf_addch(&sb, '\0');
-	free(header_line);
+	if (nl)
+		strbuf_setlen(&sb, nl + 1 - sb.buf);
+	else
+		strbuf_addch(&sb, '\n');
+	strbuf_remove(&sb, 0, header_len);
+	strbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));
+	strbuf_insert(&sb, strlen(reflog_msg), ": ", 2);
 
 	if (!ref_lock)
 		die("cannot lock HEAD ref");
-- 
1.5.3.5.1634.g0fa78

[PATCH 2/3] launch_editor(): read the file, even when EDITOR=:

[Johannes Schindelin]

Earlier we just returned in case EDITOR=: but the message stored
in the file was not read back.  Fix this.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin-tag.c |   21 ++++++++++-----------
 1 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/builtin-tag.c b/builtin-tag.c
index c3b76da..f5e0f8a 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -20,7 +20,6 @@ static char signingkey[1000];
 void launch_editor(const char *path, struct strbuf *buffer)
 {
 	const char *editor, *terminal;
-	struct child_process child;
 	const char *args[3];
 
 	editor = getenv("GIT_EDITOR");
@@ -42,17 +41,17 @@ void launch_editor(const char *path, struct strbuf *buffer)
 	if (!editor)
 		editor = "vi";
 
-	if (!strcmp(editor, ":"))
-		return;
+	if (strcmp(editor, ":")) {
+		struct child_process child;
+		memset(&child, 0, sizeof(child));
+		child.argv = args;
+		args[0] = editor;
+		args[1] = path;
+		args[2] = NULL;
 
-	memset(&child, 0, sizeof(child));
-	child.argv = args;
-	args[0] = editor;
-	args[1] = path;
-	args[2] = NULL;
-
-	if (run_command(&child))
-		die("There was a problem with the editor %s.", editor);
+		if (run_command(&child))
+			die("There was a problem with the editor %s.", editor);
+	}
 
 	if (strbuf_read_file(buffer, path, 0) < 0)
 		die("could not read message file '%s': %s",
-- 
1.5.3.5.1634.g0fa78

[PATCH 3/3] builtin-commit: fix author date with --amend --author=<author>

[Johannes Schindelin]

When amending a commit with a new author, the author date is taken
from the original commit.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin-commit.c |   17 ++++++++++++++++-
 1 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index bba9b82..03f0b9d 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -278,7 +278,6 @@ static void determine_author_info(struct strbuf *sb)
 			die("malformed --author parameter\n");
 		name = xstrndup(force_author, eoname - force_author);
 		email = xstrndup(eoname + 2, eomail - eoname - 2);
-		/* REVIEW: drops author date from amended commit on --amend --author=<author> */
 		strbuf_addf(sb, "author %s\n",
 			    fmt_ident(name, email,
 				      getenv("GIT_AUTHOR_DATE"), 1));
@@ -525,6 +524,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	} else if (amend) {
 		struct commit_list *c;
 		struct commit *commit;
+		const char *author;
 
 		reflog_msg = "commit (amend)";
 		commit = lookup_commit(head_sha1);
@@ -534,6 +534,21 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		for (c = commit->parents; c; c = c->next)
 			strbuf_addf(&sb, "parent %s\n",
 				      sha1_to_hex(c->item->object.sha1));
+
+		/* determine author date */
+		author = strstr(commit->buffer, "\nauthor");
+		if (author && !memmem(commit->buffer,
+					author + 1 - commit->buffer,
+					"\n\n", 2)) {
+			const char *email_end = strchr(author + 1, '>');
+			const char *line_end = strchr(author + 1, '\n');
+			if (email_end && line_end && email_end < line_end) {
+				char *date = xstrndup(email_end + 1,
+					line_end - email_end - 1);
+				setenv("GIT_AUTHOR_DATE", date, 1);
+				free(date);
+			}
+		}
 	} else if (in_merge) {
 		struct strbuf m;
 		FILE *fp;
-- 
1.5.3.5.1634.g0fa78

Re: [PATCH 2/3] launch_editor(): read the file, even when EDITOR=:

[Johannes Sixt]

Johannes Schindelin schrieb:
> +		struct child_process child;
> +		memset(&child, 0, sizeof(child));
> +		child.argv = args;
> +		args[0] = editor;
> +		args[1] = path;
> +		args[2] = NULL;
>  
> +		if (run_command(&child))

You can shorten this to

		const char *args[] = { editor, path, NULL };
		if (run_command_v_opt(args, 0))

(and possibly remove 'args' that is in the surrounding block).

-- Hannes

[PATCH REPLACING 2/3] launch_editor(): read the file, even when EDITOR=:

[Johannes Schindelin]

Earlier we just returned in case EDITOR=: but the message stored
in the file was not read back.  Fix this, at the same time
simplifying the code as suggested by Johannes Sixt.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin-tag.c |   17 +++++------------
 1 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/builtin-tag.c b/builtin-tag.c
index c3b76da..86d8121 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -20,8 +20,6 @@ static char signingkey[1000];
 void launch_editor(const char *path, struct strbuf *buffer)
 {
 	const char *editor, *terminal;
-	struct child_process child;
-	const char *args[3];
 
 	editor = getenv("GIT_EDITOR");
 	if (!editor && editor_program)
@@ -42,17 +40,12 @@ void launch_editor(const char *path, struct strbuf *buffer)
 	if (!editor)
 		editor = "vi";
 
-	if (!strcmp(editor, ":"))
-		return;
+	if (strcmp(editor, ":")) {
+		const char *args[] = { editor, path, NULL };
 
-	memset(&child, 0, sizeof(child));
-	child.argv = args;
-	args[0] = editor;
-	args[1] = path;
-	args[2] = NULL;
-
-	if (run_command(&child))
-		die("There was a problem with the editor %s.", editor);
+		if (run_command_v_opt(args, 0))
+			die("There was a problem with the editor %s.", editor);
+	}
 
 	if (strbuf_read_file(buffer, path, 0) < 0)
 		die("could not read message file '%s': %s",
-- 
1.5.3.5.1634.g0fa78

[PATCH 4/3] t3700: avoid racy git situation

[Johannes Schindelin]

Wow, the builtin commit is fast.  It sometimes triggers a racy
situation in the test case for "git add --refresh -- foo".

So when that test fails, simply sleep one second and try again.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3700-add.sh |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index a328bf5..f4950a3 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -152,7 +152,12 @@ test_expect_success 'git add --refresh' '
 	*) echo fail; (exit 1);;
 	esac &&
 	git add --refresh -- foo &&
-	test -z "`git diff-index HEAD -- foo`"
+	test -z "`git diff-index HEAD -- foo`" || {
+		echo Sleeping one second to avoid racy situation &&
+		sleep 1 &&
+		git add --refresh -- foo &&
+		test -z "`git diff-index HEAD -- foo`"
+	}
 '
 
 test_done
-- 
1.5.3.5.1634.g0fa78

Re: [PATCH 4/3] t3700: avoid racy git situation

[Johannes Sixt]

Johannes Schindelin schrieb:
> Wow, the builtin commit is fast.  It sometimes triggers a racy
> situation in the test case for "git add --refresh -- foo".
> 
> So when that test fails, simply sleep one second and try again.

[/me looks at the calender - no, it's not April Fool's day]

Wouldn't it be better to fix git-commit (or git-add)? I would like to help, 
but you already seem to have done the analysis, so...

-- Hannes

Re: [PATCH 4/3] t3700: avoid racy git situation

[Johannes Schindelin]

Hi,

On Thu, 8 Nov 2007, Johannes Sixt wrote:

> Johannes Schindelin schrieb:
> > Wow, the builtin commit is fast.  It sometimes triggers a racy
> > situation in the test case for "git add --refresh -- foo".
> > 
> > So when that test fails, simply sleep one second and try again.
> 
> [/me looks at the calender - no, it's not April Fool's day]

No. The builtin commit is really that fast.

> Wouldn't it be better to fix git-commit (or git-add)? I would like to 
> help, but you already seem to have done the analysis, so...

The problem is that the index has the same timestamp as the file "foo".

Therefore, git cannot tell if "foo" is up-to-date in the index, since it 
could have been modified (and indeed is) just a fraction of a second later 
than the index was last updated.

And since diff-index, as called from the test script, does not generate a 
diff, but really only determines if the index information suggests that 
the files are up-to-date, there is not really much you can do.

This is our good old friend, the racy git problem.

NOTE: other scms do not have this problem, mostly because they are too 
slow to trigger it.

Ciao,
Dscho

Re: [PATCH 4/3] t3700: avoid racy git situation

[Kristian Høgsberg]

On Thu, 2007-11-08 at 15:16 +0000, Johannes Schindelin wrote:
> Hi,
> 
> On Thu, 8 Nov 2007, Johannes Sixt wrote:
> 
> > Johannes Schindelin schrieb:
> > > Wow, the builtin commit is fast.  It sometimes triggers a racy
> > > situation in the test case for "git add --refresh -- foo".
> > > 
> > > So when that test fails, simply sleep one second and try again.
> > 
> > [/me looks at the calender - no, it's not April Fool's day]
> 
> No. The builtin commit is really that fast.
> 
> > Wouldn't it be better to fix git-commit (or git-add)? I would like to 
> > help, but you already seem to have done the analysis, so...
> 
> The problem is that the index has the same timestamp as the file "foo".
> 
> Therefore, git cannot tell if "foo" is up-to-date in the index, since it 
> could have been modified (and indeed is) just a fraction of a second later 
> than the index was last updated.
> 
> And since diff-index, as called from the test script, does not generate a 
> diff, but really only determines if the index information suggests that 
> the files are up-to-date, there is not really much you can do.
> 
> This is our good old friend, the racy git problem.
> 
> NOTE: other scms do not have this problem, mostly because they are too 
> slow to trigger it.

Ah, so that's what that was.  Thanks for fixing that, I wasn't sure what
was going on and didn't have the time to dig into it.  I have a patch
that fixes the author date for commit in determine_author_info() instead
and fixes some of the option validation problems mentioned by Björn.  I
have an updated launch_editor patch too, but what you sent out looks
good, so I'll just send the latest builtin-commit.

cheers,
Kristian

Re: [PATCH 4/3] t3700: avoid racy git situation

[Johannes Sixt]

Johannes Schindelin schrieb:
> The problem is that the index has the same timestamp as the file "foo".
> 
> Therefore, git cannot tell if "foo" is up-to-date in the index, since it 
> could have been modified (and indeed is) just a fraction of a second later 
> than the index was last updated.

The test goes like this:

         >foo && git add foo && git commit -a -m "commit all" &&
         test -z "`git diff-index HEAD -- foo`" &&
         git read-tree HEAD &&
         case "`git diff-index HEAD -- foo`" in
         :100644" "*"M   foo") echo ok;;
         *) echo fail; (exit 1);;
         esac &&
         git add --refresh -- foo &&
         test -z "`git diff-index HEAD -- foo`"

I'm pretty sure that this test completed within one second even before 
git-commit was a builtin. (I assume that timestamps don't have sub-second 
resolution.) Why then didn't this racy-git problem occur with the scripted 
git-commit?

-- Hannes

Re: [PATCH 4/3] t3700: avoid racy git situation

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> The problem is that the index has the same timestamp as the file "foo".
>
> Therefore, git cannot tell if "foo" is up-to-date in the index, since it 
> could have been modified (and indeed is) just a fraction of a second later 
> than the index was last updated.
>
> And since diff-index, as called from the test script, does not generate a 
> diff, but really only determines if the index information suggests that 
> the files are up-to-date, there is not really much you can do.
>
> This is our good old friend, the racy git problem.

That sounds very wrong.

What happened to the ce_smudge_racily_clean_entry() call that is
done from write_index()?

> NOTE: other scms do not have this problem, mostly because they are too 
> slow to trigger it.

I recall racy-hg mentioned on their development list twice, a
few months apart, and CVS has the "delay the return from a
commit to the next full second" or something to work around
problems in the timestamp recorded in CVS/Entries.

Re: [PATCH 4/3] t3700: avoid racy git situation

[Johannes Schindelin]

Hi,

On Thu, 8 Nov 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > The problem is that the index has the same timestamp as the file "foo".
> >
> > Therefore, git cannot tell if "foo" is up-to-date in the index, since 
> > it could have been modified (and indeed is) just a fraction of a 
> > second later than the index was last updated.
> >
> > And since diff-index, as called from the test script, does not 
> > generate a diff, but really only determines if the index information 
> > suggests that the files are up-to-date, there is not really much you 
> > can do.
> >
> > This is our good old friend, the racy git problem.
> 
> That sounds very wrong.
> 
> What happened to the ce_smudge_racily_clean_entry() call that is
> done from write_index()?

And sure enough I am wrong.  My patch assumes that it was the second 
diff-index which failed, the one after "git add --refresh".

Alas, in shell "a && b && c && d || e" will execute e if a fails.  So 
after debugging this a bit more carefully, it seems that it is indeed the 
missing update_index --refresh that Kristian mentioned, which leads to 
this error.

And my patch is wrong.

Ciao,
Dscho

[PATCH 0/3] builtin-commit fixes

[Wincent Colaiuta]

A little series to bring built-in commit back in line with the
behaviour as implemented by git-commit.sh and described in the
documentation.

1/3 Allow --no-verify to bypass commit-msg hook

- git-commit.sh used to do this, but builtin-commit wasn't.

2/3 Documentation: fix --no-verify documentation for "git commit"

- The "git commit" man page should mention the commit-msg hook to
bring it in line with what's in the hook notes and in
Documentation/hooks.txt.

3/3 Fix commit-msg hook to allow editing

- Again, git-commit.sh allowed this, but builtin-commit wasn't
doing so.

I have manually tested these changes and they seem to work, but
the fact that the test suite didn't break when the behaviour of
"git commit" changed is indicative of a hole in the suite. I am
not very familiar yet with the test machinery, but I am going
to see if I can whip something up.

Cheers,
Wincent