git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v1 8/8] sequencer: try to commit without forking 'git commit'

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Nov 7, 2017, 01:36 UTC
Message-ID
<alpine.DEB.2.21.1.1711070203230.6482@virtualbox>
In-Reply-To
<20171106112709.2121-9-phillip.wood@talktalk.net>
Hi Phillip,
On Mon, 6 Nov 2017, Phillip Wood wrote:
Show 9 quoted lines
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> If the commit message does not need to be edited then create the
> commit without forking 'git commit'. Taking the best time of ten runs
> with a warm cache this reduces the time taken to cherry-pick 10
> commits by 27% (from 282ms to 204ms), and the time taken by 'git
> rebase --continue' to pick 10 commits by 45% (from 386ms to 212ms) on
> my computer running linux. Some of greater saving for rebase is
> because it longer wastes time creating the commit summary just to

I usually leave the grammar reviews to people who prefer to review grammar over code, but in this case I think the "no" in "no longer" is rather crucial.

> throw it away.

Those are impressive improvements, and I am certain that they will be even more noticable on Windows, where creating processes is a lot more expensive than on Linux (which is the reason why you will find a lot more multi-threaded processes on Windows...).

Show 13 quoted lines
> The code to create the commit is based on builtin/commit.c. It is
> slightly simplified as it doesn't have to deal with merges and
> modified so try and return an error rather than dying so that the
> sequencer exits cleanly, as it would when forking 'git commit'.
> 
> Even when not forking 'git commit' the commit message is written to a
> file and CHERRY_PICK_HEAD is created unnecessarily. This could be
> eliminated in future. I hacked up a version that does not write these
> files and just passed an strbuf (with the wrong message for fixup and
> squash commands) to do_commit() but I couldn't measure any significant
> time difference when running cherry-pick or rebase. I think
> eliminating the writes properly for rebase would require a bit of
> effort as the code would need to be restructured.
True. And it totally makes sense to go for the big bucks.
Show 19 quoted lines
> diff --git a/sequencer.c b/sequencer.c
> index b8cf679751449591d6f97102904e060ebee9d7a1..0636d027e9e1cdebaab4802e5becd89e8398a425 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -592,6 +592,18 @@ static int read_env_script(struct argv_array *env)
>  	return 0;
>  }
>  
> +static char *get_author(const char* message)
> +{
> +	size_t len;
> +	const char *a;
> +
> +	a = find_commit_header(message, "author", &len);
> +	if (a)
> +		return xmemdupz(a, len);
> +
> +	return NULL;
> +}

I was surprised that there is no helper for that yet, so I looked, but it seems that the three existing callers of `find_commit_header(..., "author", ...)` want to split the ident line right away, and do not need the duplicated buffer.

In short: the code added here is necessary.
Show 30 quoted lines
> @@ -984,6 +996,151 @@ int print_commit_summary(const char *prefix, const struct object_id *oid,
>  	return ret;
>  }
>  
> +static int parse_head(struct commit **head)
> +{
> +	struct commit *current_head;
> +	struct object_id oid;
> +
> +	if (get_oid("HEAD", &oid)) {
> +		current_head = NULL;
> +	} else {
> +		current_head = lookup_commit_reference(&oid);
> +		if (!current_head)
> +			return error(_("could not parse HEAD"));
> +		if (oidcmp(&oid, &current_head->object.oid)) {
> +			warning(_("HEAD %s is not a commit!"),
> +				oid_to_hex(&oid));
> +		}
> +		if (parse_commit(current_head))
> +			return error(_("could not parse HEAD commit"));
> +	}
> +	*head = current_head;
> +
> +	return 0;
> +}
> +
> +static int try_to_commit(struct strbuf *msg, const char *author,
> +			 struct replay_opts *opts, unsigned int flags,
> +			 struct object_id *oid)

Since this is a file-local function, i.e. not in any way tied to a process exit status, it should probably return -1 in the case of errors, as Git does elsewhere, too.

Show 17 quoted lines
> +{
> +	struct object_id tree;
> +	struct commit *current_head;
> +	struct commit_list *parents = NULL;
> +	struct commit_extra_header *extra = NULL;
> +	struct strbuf err = STRBUF_INIT;
> +	struct strbuf amend_msg = STRBUF_INIT;
> +	char *amend_author = NULL;
> +	const char *gpg_sign;
> +	enum cleanup_mode cleanup;
> +	int res = 0;
> +
> +	if (parse_head(&current_head))
> +		return -1;
> +
> +	if (flags & AMEND_MSG) {
> +		const char *exclude_gpgsig[2] = { "gpgsig", NULL };

Git's current source code seems to prefer to infer the array length; The `2` is unnecessary here.

Show 8 quoted lines
> +		const char *out_enc = get_commit_output_encoding();
> +		const char *message = logmsg_reencode(current_head, NULL,
> +						      out_enc);
> +
> +		if (!msg) {
> +			const char *body = NULL;
> +
> +			find_commit_subject(message, &body);

Maybe `orig_message` would be better here; I expected `body` to refer to the part of the commit message *after* the subject, but reading the code of `find_commit_subject()`, I find that it stores the beginning of the commit message.

Dunno.
> +			msg = &amend_msg;
> +			strbuf_addstr(msg, body);
> +		}
> +		author = amend_author = get_author (message);
Please lose the space after the function name.
Show 20 quoted lines
> +		unuse_commit_buffer(current_head, message);
> +		if (!author) {
> +			res = error(_("unable to parse commit author"));
> +			goto out;
> +		}
> +		parents = copy_commit_list(current_head->parents);
> +		extra = read_commit_extra_headers(current_head, exclude_gpgsig);
> +	} else if (current_head) {
> +		commit_list_insert(current_head, &parents);
> +	}
> +
> +	cleanup = (flags & CLEANUP_MSG) ? CLEANUP_ALL : default_msg_cleanup;
> +	if (cleanup != CLEANUP_NONE)
> +		strbuf_stripspace(msg, cleanup == CLEANUP_ALL);
> +	if (!opts->allow_empty_message && message_is_empty(msg, cleanup)) {
> +		res = 1;
> +		goto out;
> +	}
> +
> +	gpg_sign = (opts->gpg_sign) ? opts->gpg_sign : default_gpg_sign;

Others will probably complain about those extra parentheses. I am not offended by them, though.

Show 8 quoted lines
> +	if (write_cache_as_tree(tree.hash, 0, NULL)) {
> +		res = error(_("git write-tree failed to write a tree"));
> +		goto out;
> +	}
> +
> +	if (!(flags & ALLOW_EMPTY) && !oidcmp(current_head ?
> +					      &current_head->tree->object.oid :
> +					      &empty_tree_oid, &tree)) {
I'll leave it to Junio to comment on the formatting here.
Show 25 quoted lines
> +		res = 1;
> +		goto out;
> +	}
> +
> +	if (commit_tree_extended(msg->buf, msg->len, tree.hash, parents,
> +				 oid->hash, author, gpg_sign, extra)) {
> +		res = error(_("failed to write commit object"));
> +		goto out;
> +	}
> +
> +	if (update_head(current_head, oid, getenv("GIT_REFLOG_ACTION"), msg,
> +			&err)){
> +		res = error("%s", err.buf);
> +		goto out;
> +	}
> +
> +	if (flags & AMEND_MSG)
> +		commit_post_rewrite(current_head, oid);
> +
> +out:
> +	free_commit_extra_headers(extra);
> +	strbuf_release(&err);
> +	strbuf_release(&amend_msg);
> +	if (amend_author)
> +		free(amend_author);

Git's source code uses the fact that `free(NULL);` is essentially a no-op (and certainly allowed) to avoid conditionals in such cases.

That would make the `if (amend_author)` unnecessary.
Show 8 quoted lines
> +
> +	return res;
> +}
> +
> +static int do_commit(const char *msg_file, const char* author,
> +		     struct replay_opts *opts, unsigned int flags)
> +{
> +	int res = 1;
Same as above, the error code should most likely be -1 instead.
> +	if (~flags & EDIT_MSG && ~flags & VERIFY_MSG) {

I *think* it is more common to write `!(flags & EDIT_MSG)` in Git's source code.

Show 12 quoted lines
> +		struct object_id oid;
> +		struct strbuf sb = STRBUF_INIT;
> +
> +		if (msg_file && strbuf_read_file(&sb, msg_file, 2048) < 0)
> +			return error_errno(_("unable to read commit message "
> +					     "from '%s'"),
> +					   msg_file);
> +
> +		res = try_to_commit(msg_file ? &sb : NULL, author, opts, flags,
> +				    &oid);
> +		strbuf_release(&sb);
> +		if (res == 0) {
Usually, Git's source code uses `if (!res)` in such cases.
Show 7 quoted lines
> +			unlink(git_path_cherry_pick_head());
> +			unlink(git_path_merge_msg());
> +			if (!is_rebase_i(opts))
> +				res = print_commit_summary(NULL, &oid,
> +						SUMMARY_SHOW_AUTHOR_DATE);
> +			return res;
> +		}

I wonder whether we should move the `return res;` one line lower, to avoid falling through to call `run_git_commit()` if `try_to_commit()` failed...

> +	}
> +	if (res == 1)
> +		return run_git_commit(msg_file, opts, flags);

Maybe this code could be simplified even further by moving this conditional to the beginning of the function, as:

	if ((flags & (EDIT_MSG | VERIFY_MSG)))
		return run_git_commit(msg_file, opts, flags);

But maybe I misunderstood and you really wanted to fall back on `run_git_commit()` if `try_to_commit()` failed?

Show 36 quoted lines
> +
> +	return res;
> +}
> +
>  static int is_original_commit_empty(struct commit *commit)
>  {
>  	const struct object_id *ptree_oid;
> @@ -1235,6 +1392,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
>  	struct object_id head;
>  	struct commit *base, *next, *parent;
>  	const char *base_label, *next_label;
> +	char *author = NULL;
>  	struct commit_message msg = { NULL, NULL, NULL, NULL };
>  	struct strbuf msgbuf = STRBUF_INIT;
>  	int res, unborn = 0, allow;
> @@ -1350,6 +1508,8 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
>  			strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));
>  			strbuf_addstr(&msgbuf, ")\n");
>  		}
> +		if (!is_fixup (command))
> +			author = get_author(msg.message);
>  	}
>  
>  	if (command == TODO_REWORD)
> @@ -1435,9 +1595,13 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
>  		goto leave;
>  	} else if (allow)
>  		flags |= ALLOW_EMPTY;
> -	if (!opts->no_commit)
> +	if (!opts->no_commit) {
>  fast_forward_edit:
> -		res = run_git_commit(msg_file, opts, flags);
> +		if (author || command == TODO_REVERT || (flags & AMEND_MSG))
> +			res = do_commit(msg_file, author, opts, flags);
> +		else
> +			res = error(_("unable to parse commit author"));
Would this be a bug here? Or do we expect `get_author()` to possibly fail?
Show 10 quoted lines
> +	}
>  
>  	if (!res && final_fixup) {
>  		unlink(rebase_path_fixup_msg());
> @@ -1446,6 +1610,8 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
>  
>  leave:
>  	free_message(commit, &msg);
> +	if (author)
> +		free(author);
As above, please write an unconditional `free(author);` here.

All in all, this patch series was a nice an pleasant read. I am impressed by the performance wins.

Thank you very much, Dscho

Previous: Phillip WoodNext: Phillip Wood
Message 32 of 120 in “sequencer: dont't fork git commit”
  1. 0/8 sequencer: dont't fork git commitPhillip Wood, Sep 25, 2017
  2. 1/8 commit: move empty message checks to libgitPhillip Wood, Sep 25, 2017
  3. 4/8 commit: move post-rewrite code to libgitPhillip Wood, Sep 25, 2017
  4. 2/8 commit: move code to update HEAD to libgitPhillip Wood, Sep 25, 2017
  5. Junio C HamanoOct 7, 2017
  6. Phillip WoodOct 24, 2017
  7. Junio C HamanoOct 24, 2017
  8. 3/8 sequencer: refactor update_head()Phillip Wood, Sep 25, 2017
  9. 5/8 commit: move print_commit_summary() to libgitPhillip Wood, Sep 25, 2017
  10. 6/8 sequencer: simplify adding Signed-off-by: trailerPhillip Wood, Sep 25, 2017
  11. 7/8 sequencer: load commit related configPhillip Wood, Sep 25, 2017
  12. 8/8 sequencer: try to commit without forking 'git commit'Phillip Wood, Sep 25, 2017
  13. 0/8 sequencer: dont't fork git commitPhillip Wood, Nov 6, 2017
  14. 3/8 commit: move post-rewrite code to libgitPhillip Wood, Nov 6, 2017
  15. Junio C HamanoNov 7, 2017
  16. Phillip WoodNov 7, 2017
  17. 5/8 sequencer: don't die in print_commit_summary()Phillip Wood, Nov 6, 2017
  18. Junio C HamanoNov 7, 2017
  19. Johannes SchindelinNov 7, 2017
  20. Junio C HamanoNov 7, 2017
  21. Phillip WoodNov 10, 2017
  22. Junio C HamanoNov 10, 2017
  23. Phillip WoodNov 13, 2017
  24. 6/8 sequencer: simplify adding Signed-off-by: trailerPhillip Wood, Nov 6, 2017
  25. Johannes SchindelinNov 7, 2017
  26. Junio C HamanoNov 7, 2017
  27. Phillip WoodNov 7, 2017
  28. 7/8 sequencer: load commit related configPhillip Wood, Nov 6, 2017
  29. Johannes SchindelinNov 7, 2017
  30. Phillip WoodNov 7, 2017
  31. 8/8 sequencer: try to commit without forking 'git commit'Phillip Wood, Nov 6, 2017
  32. Johannes SchindelinNov 7, 2017
  33. Phillip WoodNov 7, 2017
  34. Johannes SchindelinNov 7, 2017
  35. 4/8 commit: move print_commit_summary() to libgitPhillip Wood, Nov 6, 2017
  36. Junio C HamanoNov 7, 2017
  37. Phillip WoodNov 7, 2017
  38. Junio C HamanoNov 8, 2017
  39. 2/8 Add a function to update HEAD after creating a commitPhillip Wood, Nov 6, 2017
  40. Junio C HamanoNov 7, 2017
  41. Johannes SchindelinNov 7, 2017
  42. Phillip WoodNov 7, 2017
  43. 1/8 commit: move empty message checks to libgitPhillip Wood, Nov 6, 2017
  44. Johannes SchindelinNov 7, 2017
  45. Phillip WoodNov 7, 2017
  46. 0/9 sequencer: dont't fork git commitPhillip Wood, Nov 10, 2017
  47. 1/9 t3404: check intermediate squash messagesPhillip Wood, Nov 10, 2017
  48. 2/9 commit: move empty message checks to libgitPhillip Wood, Nov 10, 2017
  49. Ramsay JonesNov 10, 2017
  50. Phillip WoodNov 13, 2017
  51. 6/9 sequencer: don't die in print_commit_summary()Phillip Wood, Nov 10, 2017
  52. 3/9 Add a function to update HEAD after creating a commitPhillip Wood, Nov 10, 2017
  53. Junio C HamanoNov 10, 2017
  54. Phillip WoodNov 13, 2017
  55. 4/9 commit: move post-rewrite code to libgitPhillip Wood, Nov 10, 2017
  56. 9/9 sequencer: try to commit without forking 'git commit'Phillip Wood, Nov 10, 2017
  57. 5/9 commit: move print_commit_summary() to libgitPhillip Wood, Nov 10, 2017
  58. 7/9 sequencer: simplify adding Signed-off-by: trailerPhillip Wood, Nov 10, 2017
  59. 8/9 sequencer: load commit related configPhillip Wood, Nov 10, 2017
  60. Junio C HamanoNov 10, 2017
  61. Phillip WoodNov 13, 2017
  62. Junio C HamanoNov 14, 2017
  63. 0/8 sequencer: don't fork git commitPhillip Wood, Nov 17, 2017
  64. 1/8 t3404: check intermediate squash messagesPhillip Wood, Nov 17, 2017
  65. 2/8 commit: move empty message checks to libgitPhillip Wood, Nov 17, 2017
  66. 3/8 Add a function to update HEAD after creating a commitPhillip Wood, Nov 17, 2017
  67. 4/8 commit: move post-rewrite code to libgitPhillip Wood, Nov 17, 2017
  68. 6/8 sequencer: simplify adding Signed-off-by: trailerPhillip Wood, Nov 17, 2017
  69. 7/8 sequencer: load commit related configPhillip Wood, Nov 17, 2017
  70. 5/8 commit: move print_commit_summary() to libgitPhillip Wood, Nov 17, 2017
  71. 8/8 sequencer: try to commit without forking 'git commit'Phillip Wood, Nov 17, 2017
  72. Junio C HamanoNov 18, 2017
  73. Junio C HamanoNov 18, 2017
  74. Phillip WoodNov 18, 2017
  75. Phillip WoodNov 18, 2017
  76. 0/9 sequencer: don't fork git commitPhillip Wood, Nov 24, 2017
  77. 1/9 t3404: check intermediate squash messagesPhillip Wood, Nov 24, 2017
  78. 6/9 sequencer: simplify adding Signed-off-by: trailerPhillip Wood, Nov 24, 2017
  79. 2/9 commit: move empty message checks to libgitPhillip Wood, Nov 24, 2017
  80. 4/9 commit: move post-rewrite code to libgitPhillip Wood, Nov 24, 2017
  81. 3/9 Add a function to update HEAD after creating a commitPhillip Wood, Nov 24, 2017
  82. 5/9 commit: move print_commit_summary() to libgitPhillip Wood, Nov 24, 2017
  83. 7/9 sequencer: load commit related configPhillip Wood, Nov 24, 2017
  84. Junio C HamanoNov 24, 2017
  85. Phillip WoodNov 24, 2017
  86. Junio C HamanoDec 4, 2017
  87. Phillip WoodDec 5, 2017
  88. Phillip WoodDec 5, 2017
  89. Phillip WoodDec 9, 2017
  90. 8/9 sequencer: try to commit without forking 'git commit'Phillip Wood, Nov 24, 2017
  91. 9/9 t3512/t3513: remove KNOWN_FAILURE_CHERRY_PICK_SEES_EMPTY_COMMIT=1Phillip Wood, Nov 24, 2017
  92. Stefan BellerDec 4, 2017
  93. Phillip WoodDec 5, 2017
  94. 0/9 sequencer: don't fork git commitPhillip Wood, Dec 11, 2017
  95. 1/9 t3404: check intermediate squash messagesPhillip Wood, Dec 11, 2017
  96. 4/9 commit: move post-rewrite code to libgitPhillip Wood, Dec 11, 2017
  97. 3/9 Add a function to update HEAD after creating a commitPhillip Wood, Dec 11, 2017
  98. 5/9 commit: move print_commit_summary() to libgitPhillip Wood, Dec 11, 2017
  99. 2/9 commit: move empty message checks to libgitPhillip Wood, Dec 11, 2017
  100. 6/9 sequencer: simplify adding Signed-off-by: trailerPhillip Wood, Dec 11, 2017
  101. 9/9 t3512/t3513: remove KNOWN_FAILURE_CHERRY_PICK_SEES_EMPTY_COMMIT=1Phillip Wood, Dec 11, 2017
  102. 8/9 sequencer: try to commit without forking 'git commit'Phillip Wood, Dec 11, 2017
  103. Jonathan NiederJan 10, 2018
  104. Johannes SchindelinJan 10, 2018
  105. Phillip WoodJan 11, 2018
  106. Johannes SchindelinJan 11, 2018
  107. 7/9 sequencer: load commit related configPhillip Wood, Dec 11, 2017
  108. Phillip WoodDec 11, 2017
  109. Junio C HamanoDec 11, 2017
  110. Phillip WoodDec 12, 2017
  111. sequencer: improve config handlingPhillip Wood, Dec 13, 2017
  112. Error in `git': free(): invalid pointer (was Re: [PATCH] sequencer: improve config handling)Kaartic Sivaraam, Dec 20, 2017
  113. Johannes SchindelinDec 21, 2017
  114. Kaartic SivaraamDec 21, 2017
  115. Johannes SchindelinDec 22, 2017
  116. Kaartic SivaraamDec 25, 2017
  117. phillip.wood@talktalk.netDec 21, 2017
  118. Kaartic SivaraamDec 21, 2017
  119. phillip.wood@talktalk.netDec 22, 2017
  120. Kaartic SivaraamDec 21, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.