Johannes SchindelinHi,
The following three commits are on top of kh/commit. With these 3 patches, the test suite passes.
Ciao, Dscho
Johannes SchindelinJohannes Schindelin[PATCH 1/3] builtin-commit: fix reflog message generation
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(-)
Show changes to builtin-commit.c +8 −7
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[PATCH 2/3] launch_editor(): read the file, even when EDITOR=:
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(-)
Show changes to builtin-tag.c +10 −11
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 SixtRe: [PATCH 2/3] launch_editor(): read the file, even when EDITOR=:
Johannes Schindelin schrieb:
Show 8 quoted lines
> + 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[PATCH REPLACING 2/3] launch_editor(): read the file, even when EDITOR=:
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(-)
Show changes to builtin-tag.c +5 −12
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[PATCH 3/3] builtin-commit: fix author date with --amend --author=<author>
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(-)
Show changes to builtin-commit.c +16 −1
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 Schindelin[PATCH 4/3] t3700: avoid racy git situation
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(-)
Show changes to t/t3700-add.sh +6 −1
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 SixtRe: [PATCH 4/3] t3700: avoid racy git situation
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 SchindelinRe: [PATCH 4/3] t3700: avoid racy git situation
Hi,
On Thu, 8 Nov 2007, Johannes Sixt wrote:
Show 7 quoted lines
> 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øgsbergRe: [PATCH 4/3] t3700: avoid racy git situation
On Thu, 2007-11-08 at 15:16 +0000, Johannes Schindelin wrote:
Show 31 quoted lines
> 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 SixtRe: [PATCH 4/3] t3700: avoid racy git situation
Johannes Schindelin schrieb:
Show 5 quoted lines
> 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 HamanoRe: [PATCH 4/3] t3700: avoid racy git situation
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 11 quoted lines
> 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 SchindelinRe: [PATCH 4/3] t3700: avoid racy git situation
Hi,
On Thu, 8 Nov 2007, Junio C Hamano wrote:
Show 19 quoted lines
> 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