Re: [PATCH v2 0/9] sequencer: dont't fork git commit

From: Phillip Wood <phillip.wood@talktalk.net>
Date: Nov 13, 2017, 11:24 UTC
Message-ID: <f1c0efa2-fb8a-43bc-f229-c22dada95b03@talktalk.net>
In-Reply-To: <xmqqo9oadjn5.fsf@gitster.mtv.corp.google.com>

On 10/11/17 19:21, Junio C Hamano wrote:

Show 16 quoted lines

> Phillip Wood <phillip.wood@talktalk.net> writes:
> 
>> Here's the summary from the previous version
>> These patches teach the sequencer to create commits without forking
>> git commit when the commit message does not need to be edited. This
>> speeds up cherry picking 10 commits by 26% and picking 10 commits with
>> rebase --continue by 44%. The first few patches move bits of
>> builtin/commit.c to sequencer.c. The last two patches actually
>> implement creating commits in sequencer.c.
> 
> Thanks.  The changes since the initial iteration seems quite small
> and I didn't find much objectionable.
> 
> Here are some style fixes I needed to add on top to make the output
> of "diff master HEAD" checkpatch.pl-clean.  I think 3/9 and 9/9 are
> the culprits.

Thanks, I'll update the patches.

Are you happy with the '--signoff' is handled (I didn't modify my changes in the last iteration as you were still thinking about it)?

In the last patch commit_tree_extended() can die in write_loose_object() which means that the current command is not rescheduled as it should be when rebasing. write_loose_object() already an error if there is a problem opening the file, but dies if there is a problem creating the file contents or writing that contents.

Best Wishes

Phillip

Show 55 quoted lines

> diff --git a/sequencer.c b/sequencer.c
> index 1f65e82696..a989588ee5 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -592,7 +592,7 @@ static int read_env_script(struct argv_array *env)
>  	return 0;
>  }
>  
> -static char *get_author(const char* message)
> +static char *get_author(const char *message)
>  {
>  	size_t len;
>  	const char *a;
> @@ -1104,7 +1104,7 @@ static int try_to_commit(struct strbuf *msg, const char *author,
>  	}
>  
>  	if (update_head_with_reflog(current_head, oid,
> -				    getenv("GIT_REFLOG_ACTION"), msg, &err)){
> +				    getenv("GIT_REFLOG_ACTION"), msg, &err)) {
>  		res = error("%s", err.buf);
>  		goto out;
>  	}
> @@ -1121,7 +1121,7 @@ static int try_to_commit(struct strbuf *msg, const char *author,
>  	return res;
>  }
>  
> -static int do_commit(const char *msg_file, const char* author,
> +static int do_commit(const char *msg_file, const char *author,
>  		     struct replay_opts *opts, unsigned int flags)
>  {
>  	int res = 1;
> @@ -1521,7 +1521,7 @@ 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))
> +		if (!is_fixup(command))
>  			author = get_author(msg.message);
>  	}
>  
> diff --git a/sequencer.h b/sequencer.h
> index 27f34be400..e0be354301 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -72,7 +72,7 @@ int template_untouched(const struct strbuf *sb, const char *template_file,
>  		       enum commit_msg_cleanup_mode cleanup_mode);
>  int update_head_with_reflog(const struct commit *old_head,
>  			    const struct object_id *new_head,
> -			    const char* action, const struct strbuf *msg,
> +			    const char *action, const struct strbuf *msg,
>  			    struct strbuf *err);
>  void commit_post_rewrite(const struct commit *current_head,
>  			 const struct object_id *new_head);
> 
>