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

[Johannes Schindelin]

Hi Phillip,

On Thu, 11 Jan 2018, Phillip Wood wrote:

> On 10/01/18 22:40, Johannes Schindelin wrote:
> > Hi,
> > 
> > On Wed, 10 Jan 2018, Jonathan Nieder wrote:
> > 
> >> that this causes the prepare-commit-msg hook not to be invoked, which
> >> I think is unintentional.  Should we check for such a hook and take
> >> the slowpath when it is present?
> > 
> > We could also easily recreate the functionality:
> > 
> > if (find_hook("pre-commit")) {
> > struct argv_array hook_env = ARGV_ARRAY_INIT;
> > 
> > argv_array_pushf(_env, "GIT_INDEX_FILE=%s",
> > get_index_file());
> > argv_array_push(_env, "GIT_EDITOR=:");
> > ret = run_hook_le(hook_env.argv, "pre-commit", NULL);
> > argv_array_clear(_env);
> > }
> 
> Thanks Johannes, though it needs to run the 'prepare-commit-msg' hook,
> the current code in master only runs the 'pre-commit' hook when we edit
> the message. I'll send a patch with a test.

Sorry, yes, that's the hook I meant ;-) the quoted text by Jonathan even
mentions it explicitly.

Ciao,
Johannes

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

[Phillip Wood]

On 10/01/18 22:40, Johannes Schindelin wrote:
> Hi,
> 
> On Wed, 10 Jan 2018, Jonathan Nieder wrote:
> 
>> Phillip Wood wrote:
>>
>>> From: Phillip Wood 
>>>
>>> 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 no longer wastes time creating the commit summary just to
>>> throw it away.
>>
>> Neat!  Dmitry Torokhov (cc-ed) noticed[1]

Thanks for reporting and bisecting this Dmitry. When I was preparing
this series I checked to see if it needed to run the 'pre-commit' hook
but missed the 'prepare-commit-msg' hook.

> that this causes the
>> prepare-commit-msg hook not to be invoked, which I think is
>> unintentional.  Should we check for such a hook and take the slowpath
>> when it is present?
> 
> We could also easily recreate the functionality:
> 
>   if (find_hook("pre-commit")) {
>   struct argv_array hook_env = ARGV_ARRAY_INIT;
> 
>   argv_array_pushf(_env, "GIT_INDEX_FILE=%s",
>   get_index_file());
>   argv_array_push(_env, "GIT_EDITOR=:");
>   ret = run_hook_le(hook_env.argv, "pre-commit", NULL);
>   argv_array_clear(_env);
>   }

Thanks Johannes, though it needs to run the 'prepare-commit-msg' hook,
the current code in master only runs the 'pre-commit' hook when we edit
the message. I'll send a patch with a test.

Best Wishes

Phillip

> (This assumes that the in-process try_to_commit() is only called if the
> commit message is not to be edited interactively, which is currently the
> case.)
> 
> Ciao,
> Dscho
> 

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

[Johannes Schindelin]

Hi,

On Wed, 10 Jan 2018, Jonathan Nieder wrote:

> Phillip Wood wrote:
> 
> > From: Phillip Wood 
> >
> > 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 no longer wastes time creating the commit summary just to
> > throw it away.
> 
> Neat!  Dmitry Torokhov (cc-ed) noticed[1] that this causes the
> prepare-commit-msg hook not to be invoked, which I think is
> unintentional.  Should we check for such a hook and take the slowpath
> when it is present?

We could also easily recreate the functionality:

if (find_hook("pre-commit")) {
struct argv_array hook_env = ARGV_ARRAY_INIT;

argv_array_pushf(_env, "GIT_INDEX_FILE=%s",
get_index_file());
argv_array_push(_env, "GIT_EDITOR=:");
ret = run_hook_le(hook_env.argv, "pre-commit", NULL);
argv_array_clear(_env);
}

(This assumes that the in-process try_to_commit() is only called if the
commit message is not to be edited interactively, which is currently the
case.)

Ciao,
Dscho

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

[Jonathan Nieder]

Hi,

Phillip Wood wrote:

> From: Phillip Wood 
>
> 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 no longer wastes time creating the commit summary just to
> throw it away.

Neat!  Dmitry Torokhov (cc-ed) noticed[1] that this causes the
prepare-commit-msg hook not to be invoked, which I think is
unintentional.  Should we check for such a hook and take the slowpath
when it is present?

Thanks,
Jonathan

[1] 
https://public-inbox.org/git/cakdakrquj1hfkeckjur2op+8c1i+zr36o-+aryif4ufas_z...@mail.gmail.com/

[PATCH v5 8/9] sequencer: try to commit without forking 'git commit'

[Phillip Wood]

From: Phillip Wood 

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 no longer wastes time creating the commit summary just to
throw it away.

The code to create the commit is based on builtin/commit.c. It is
simplified as it doesn't have to deal with merges and modified so that
it does not die but returns an error to make sure 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.

Signed-off-by: Phillip Wood 
---

Notes:
changes since v4:
 - changed cleanup and gpg handling to reflect the changes in the last patch

changes since v3:
 - take account of change print_commit_summary() return type after
   dropping the patch that made it return an error instead of dying.

changes since v2:
 - style fixes

changes since v1:
 - added comments to explain return value of try_to_commit()
 - removed unnecessary NULL tests before calling free()
 - style cleanups
 - corrected commit message
 - prefixed cleanup_mode constants to reflect the changes to patch 2
   in this series

 sequencer.c | 176 +++-
 1 file changed, 174 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 
3ce1e5b71474f1cd25b232a319fb7b0e13dc6e14..74770bd00cc3840573057a1868e0a3acb05a71bb
 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -638,6 +638,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", );
+   if (a)
+   return xmemdupz(a, len);
+
+   return NULL;
+}
+
 static const char staged_changes_advice[] =
 N_("you have staged changes in your working tree\n"
 "If these changes are meant to be squashed into the previous commit, run:\n"
@@ -996,6 +1008,158 @@ void print_commit_summary(const char *prefix, const 
struct object_id *oid,
strbuf_release();
 }
 
+static int parse_head(struct commit **head)
+{
+   struct commit *current_head;
+   struct object_id oid;
+
+   if (get_oid("HEAD", )) {
+   current_head = NULL;
+   } else {
+   current_head = lookup_commit_reference();
+   if (!current_head)
+   return error(_("could not parse HEAD"));
+   if (oidcmp(, _head->object.oid)) {
+   warning(_("HEAD %s is not a commit!"),
+   oid_to_hex());
+   }
+   if (parse_commit(current_head))
+   return error(_("could not parse HEAD commit"));
+   }
+   *head = current_head;
+
+   return 0;
+}
+
+/*
+ * Try to commit without forking 'git commit'. In some cases we need
+ * to run 'git commit' to display an error message
+ *
+ * Returns:
+ *  -1 - error unable to commit
+ *   0 - success
+ *   1 - run 'git commit'
+ */
+static int try_to_commit(struct strbuf *msg, const char *author,
+struct replay_opts *opts, unsigned int flags,
+struct object_id *oid)
+{
+   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;
+   enum commit_msg_cleanup_mode cleanup;
+   int res = 0;
+
+   if (parse_head(_head))
+   return -1;
+
+   if (flags & AMEND_MSG) {
+   const char *exclude_gpgsig[] = { "gpgsig", NULL };
+   const char *out_enc = get_commit_output_encoding();
+   const char *message = logmsg_reencode(current_head, NULL,
+ out_enc);
+
+   if (!msg) {
+   const char *orig_message = NULL;
+
+   find_commit_subject(message, _message);
+   msg = _msg;
+