Re: [RFC PATCH 2/8] commit: move code to update HEAD to libgit

[Junio C Hamano <gitster@pobox.com>]

Phillip Wood <phillip.wood@talktalk.net> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---

This seems to do a lot more than just moving code, most notably, it
uses setenv() to affect what happens in any subprocesses we may
spawn, and it is unclear if it was verified that this patch is free
of unwanted consequences due to that change (and any others I may
have missed while reading this patch, if any).

I suspect that it would be sufficient to make update_head() helper
function take the reflog action message as another parameter
instead to fix the above, but there may be other reasons why you
chose to do it this way---I cannot read it in your empty log
message, though.

I will not give line-by-line style nitpick but in general we do not
leave a SP between function name and the open parenthesis that
starts its argument list.  New code in this patch seems to use
mixture of styles.

> diff --git a/builtin/commit.c b/builtin/commit.c
> index 0b8c1ef6f57cfed328d12255e6834adb4bda4137..497778ba2c02afdd4a337969a27ca781e8389040 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1578,13 +1578,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  	struct strbuf sb = STRBUF_INIT;
>  	struct strbuf author_ident = STRBUF_INIT;
>  	const char *index_file, *reflog_msg;
> -	char *nl;
>  	struct object_id oid;
>  	struct commit_list *parents = NULL;
>  	struct stat statbuf;
>  	struct commit *current_head = NULL;
>  	struct commit_extra_header *extra = NULL;
> -	struct ref_transaction *transaction;
>  	struct strbuf err = STRBUF_INIT;
>  
>  	if (argc == 2 && !strcmp(argv[1], "-h"))
> @@ -1625,10 +1623,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  	reflog_msg = getenv("GIT_REFLOG_ACTION");
>  	if (!current_head) {
>  		if (!reflog_msg)
> -			reflog_msg = "commit (initial)";
> +			setenv ("GIT_REFLOG_ACTION", "commit (initial)", 1);
>  	} else if (amend) {
>  		if (!reflog_msg)
> -			reflog_msg = "commit (amend)";
> +			setenv("GIT_REFLOG_ACTION", "commit (amend)", 1);
>  		parents = copy_commit_list(current_head->parents);
>  	} else if (whence == FROM_MERGE) {
>  		struct strbuf m = STRBUF_INIT;
> @@ -1637,7 +1635,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  		struct commit_list **pptr = &parents;
>  
>  		if (!reflog_msg)
> -			reflog_msg = "commit (merge)";
> +			setenv("GIT_REFLOG_ACTION", "commit (merge)", 1);
>  		pptr = commit_list_append(current_head, pptr);
>  		fp = xfopen(git_path_merge_head(), "r");
>  		while (strbuf_getline_lf(&m, fp) != EOF) {
> @@ -1660,9 +1658,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  			parents = reduce_heads(parents);
>  	} else {
>  		if (!reflog_msg)
> -			reflog_msg = (whence == FROM_CHERRY_PICK)
> -					? "commit (cherry-pick)"
> -					: "commit";
> +			setenv("GIT_REFLOG_ACTION", (whence == FROM_CHERRY_PICK)
> +						? "commit (cherry-pick)"
> +						: "commit", 1);
>  		commit_list_insert(current_head, &parents);
>  	}
>  
> @@ -1707,25 +1705,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  	strbuf_release(&author_ident);
>  	free_commit_extra_headers(extra);
>  
> -	nl = strchr(sb.buf, '\n');
> -	if (nl)
> -		strbuf_setlen(&sb, nl + 1 - sb.buf);
> -	else
> -		strbuf_addch(&sb, '\n');
> -	strbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));
> -	strbuf_insert(&sb, strlen(reflog_msg), ": ", 2);
> -
> -	transaction = ref_transaction_begin(&err);
> -	if (!transaction ||
> -	    ref_transaction_update(transaction, "HEAD", oid.hash,
> -				   current_head
> -				   ? current_head->object.oid.hash : null_sha1,
> -				   0, sb.buf, &err) ||
> -	    ref_transaction_commit(transaction, &err)) {
> +	if (update_head (current_head, &oid, &sb, &err)) {
>  		rollback_index_files();
>  		die("%s", err.buf);
>  	}
> -	ref_transaction_free(transaction);
>  
>  	unlink(git_path_cherry_pick_head());
>  	unlink(git_path_revert_head());
> diff --git a/sequencer.c b/sequencer.c
> index 319208afb3de36c97b6c62d4ecf6e641245e7a54..917ad4a16216b30adb2c2c9650217926d8db8ba7 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1,10 +1,10 @@
>  #include "cache.h"
>  #include "config.h"
>  #include "lockfile.h"
> -#include "sequencer.h"
>  #include "dir.h"
>  #include "object.h"
>  #include "commit.h"
> +#include "sequencer.h"
>  #include "tag.h"
>  #include "run-command.h"
>  #include "exec_cmd.h"
> @@ -750,6 +750,43 @@ int template_untouched(const struct strbuf *sb, const char *template_file,
>  	return rest_is_empty(sb, start - sb->buf);
>  }
>  
> +int update_head(const struct commit *old_head, const struct object_id *new_head,
> +		const struct strbuf *msg, struct strbuf *err)
> +{
> +	struct ref_transaction *transaction;
> +	struct strbuf sb = STRBUF_INIT;
> +	const char *nl, *reflog_msg;
> +	int ret = 0;
> +
> +	reflog_msg = getenv("GIT_REFLOG_ACTION");
> +	if (!reflog_msg)
> +		reflog_msg="";
> +
> +	nl = strchr(msg->buf, '\n');
> +	if (nl) {
> +		strbuf_add(&sb, msg->buf, nl + 1 - msg->buf);
> +	} else {
> +		strbuf_addbuf(&sb, msg);
> +		strbuf_addch(&sb, '\n');
> +	}
> +	strbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));
> +	strbuf_insert(&sb, strlen(reflog_msg), ": ", 2);
> +
> +	transaction = ref_transaction_begin(err);
> +	if (!transaction ||
> +	    ref_transaction_update(transaction, "HEAD", new_head->hash,
> +				   old_head
> +				   ? old_head->object.oid.hash : null_sha1,
> +				   0, sb.buf, err) ||
> +	    ref_transaction_commit(transaction, err)) {
> +		ret = -1;
> +	}
> +	ref_transaction_free(transaction);
> +	strbuf_release(&sb);
> +
> +	return ret;
> +}
> +
>  static int is_original_commit_empty(struct commit *commit)
>  {
>  	const struct object_id *ptree_oid;
> diff --git a/sequencer.h b/sequencer.h
> index dd071cfcd82d165bd23726814b74cbf3384e1a17..87edf40e5274d59f48d5af57678100ea220d2c8a 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -60,4 +60,6 @@ enum cleanup_mode {
>  int message_is_empty(const struct strbuf *sb, enum cleanup_mode cleanup_mode);
>  int template_untouched(const struct strbuf *sb, const char *template_file,
>  		       enum cleanup_mode cleanup_mode);
> +int update_head(const struct commit *old_head, const struct object_id *new_head,
> +		const struct strbuf *msg, struct strbuf *err);
>  #endif