# [PATCH 0/3] builtin-commit fixes

13 messages from 2007-11-08 to 2007-11-09. Participants: Johannes Schindelin, Johannes Sixt, Kristian Høgsberg, Junio C Hamano.
Thread: https://gitlist.dev/t/10731

## Johannes Schindelin, 2007-11-08 12:14

Subject: [PATCH 0/3] builtin-commit fixes
Message-ID: <Pine.LNX.4.64.0711081213580.4362@racer.site>
URL: https://gitlist.dev/e/Pine.LNX.4.64.0711081213580.4362%40racer.site

```
Hi,

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

Ciao,
Dscho

```

## Johannes Schindelin, 2007-11-08 12:15

Subject: [PATCH 1/3] builtin-commit: fix reflog message generation
Message-ID: <Pine.LNX.4.64.0711081215140.4362@racer.site>
URL: https://gitlist.dev/e/Pine.LNX.4.64.0711081215140.4362%40racer.site
In-Reply-To: <Pine.LNX.4.64.0711081213580.4362@racer.site>

```

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

```

## Johannes Schindelin, 2007-11-08 12:15

Subject: [PATCH 2/3] launch_editor(): read the file, even when EDITOR=:
Message-ID: <Pine.LNX.4.64.0711081215360.4362@racer.site>
URL: https://gitlist.dev/e/Pine.LNX.4.64.0711081215360.4362%40racer.site
In-Reply-To: <Pine.LNX.4.64.0711081213580.4362@racer.site>

```

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

```

## Johannes Schindelin, 2007-11-08 12:16

Subject: [PATCH 3/3] builtin-commit: fix author date with --amend --author=<author>
Message-ID: <Pine.LNX.4.64.0711081215540.4362@racer.site>
URL: https://gitlist.dev/e/Pine.LNX.4.64.0711081215540.4362%40racer.site
In-Reply-To: <Pine.LNX.4.64.0711081213580.4362@racer.site>

```

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

```

## Johannes Sixt, 2007-11-08 12:33

Subject: Re: [PATCH 2/3] launch_editor(): read the file, even when EDITOR=:
Message-ID: <47330222.7000206@viscovery.net>
URL: https://gitlist.dev/e/47330222.7000206%40viscovery.net
In-Reply-To: <Pine.LNX.4.64.0711081215360.4362@racer.site>

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

```

## Johannes Schindelin, 2007-11-08 14:06

Subject: [PATCH REPLACING 2/3] launch_editor(): read the file, even when EDITOR=:
Message-ID: <Pine.LNX.4.64.0711081406280.4362@racer.site>
URL: https://gitlist.dev/e/Pine.LNX.4.64.0711081406280.4362%40racer.site
In-Reply-To: <47330222.7000206@viscovery.net>

```

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

```

## Johannes Schindelin, 2007-11-08 14:14

Subject: [PATCH 4/3] t3700: avoid racy git situation
Message-ID: <Pine.LNX.4.64.0711081414160.4362@racer.site>
URL: https://gitlist.dev/e/Pine.LNX.4.64.0711081414160.4362%40racer.site
In-Reply-To: <Pine.LNX.4.64.0711081213580.4362@racer.site>

```

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

```

## Johannes Sixt, 2007-11-08 14:34

Subject: Re: [PATCH 4/3] t3700: avoid racy git situation
Message-ID: <47331E65.9010209@viscovery.net>
URL: https://gitlist.dev/e/47331E65.9010209%40viscovery.net
In-Reply-To: <Pine.LNX.4.64.0711081414160.4362@racer.site>

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

```

## Johannes Schindelin, 2007-11-08 15:16

Subject: Re: [PATCH 4/3] t3700: avoid racy git situation
Message-ID: <Pine.LNX.4.64.0711081511440.4362@racer.site>
URL: https://gitlist.dev/e/Pine.LNX.4.64.0711081511440.4362%40racer.site
In-Reply-To: <47331E65.9010209@viscovery.net>

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

```

## Kristian Høgsberg, 2007-11-08 15:27

Subject: Re: [PATCH 4/3] t3700: avoid racy git situation
Message-ID: <1194535631.27305.3.camel@hinata.boston.redhat.com>
URL: https://gitlist.dev/e/1194535631.27305.3.camel%40hinata.boston.redhat.com
In-Reply-To: <Pine.LNX.4.64.0711081511440.4362@racer.site>

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

```

## Johannes Sixt, 2007-11-08 15:45

Subject: Re: [PATCH 4/3] t3700: avoid racy git situation
Message-ID: <47332F37.5090402@viscovery.net>
URL: https://gitlist.dev/e/47332F37.5090402%40viscovery.net
In-Reply-To: <Pine.LNX.4.64.0711081511440.4362@racer.site>

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

```

## Junio C Hamano, 2007-11-08 20:42

Subject: Re: [PATCH 4/3] t3700: avoid racy git situation
Message-ID: <7vbqa431vj.fsf@gitster.siamese.dyndns.org>
URL: https://gitlist.dev/e/7vbqa431vj.fsf%40gitster.siamese.dyndns.org
In-Reply-To: <Pine.LNX.4.64.0711081511440.4362@racer.site>

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

```

## Johannes Schindelin, 2007-11-09 03:00

Subject: Re: [PATCH 4/3] t3700: avoid racy git situation
Message-ID: <Pine.LNX.4.64.0711090258530.4362@racer.site>
URL: https://gitlist.dev/e/Pine.LNX.4.64.0711090258530.4362%40racer.site
In-Reply-To: <7vbqa431vj.fsf@gitster.siamese.dyndns.org>

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

```