Re: [PATCH v1 2/8] Add a function to update HEAD after creating a commit

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

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

> @@ -1735,25 +1733,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);

The old code treated sb (which has the log message we gave to
commit_tree_extended() to create the commit) as expendable at this
point and (1) truncated it to the title line, and (2) prepended the
reflog action prefix, so that it can pass it to the ref transaction
code to use it as the reflog message.

Which was quite ugly X-<.

> -	transaction = ref_transaction_begin(&err);
> -	if (!transaction ||
> -	    ref_transaction_update(transaction, "HEAD", &oid,
> -				   current_head
> -				   ? &current_head->object.oid : &null_oid,
> -				   0, sb.buf, &err) ||
> -	    ref_transaction_commit(transaction, &err)) {
> +	if (update_head(current_head, &oid, reflog_msg, &sb, &err)) {
>  		rollback_index_files();
>  		die("%s", err.buf);
>  	}

> @@ -751,6 +751,42 @@ 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 char *action, const struct strbuf *msg,
> +		struct strbuf *err)
> +{
> +	struct ref_transaction *transaction;
> +	struct strbuf sb = STRBUF_INIT;

It no longer is necessary to call this variable "sb"; the original
had a single instance of strbuf that was reused for different
purposes and could not give it a more specific name, but we can
afford to call this one reflog_message or something.

> +	const char *nl;
> +	int ret = 0;
> +
> +	if (action) {
> +		strbuf_addstr(&sb, action);
> +		strbuf_addstr(&sb, ": ");
> +	}
> +
> +	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');
> +	}

The updated code is a lot more natural and straight-forward.  I
quite like it.

I however do not think update_head() is such a good name for a
helper function in the global scope.  builtin/clone.c has a static
one that has quite different semantics with the same name (I am not
saying that builtin/clone.c will in the future start including the
sequencer.h header file; I am pointing out that update_head() is not
a good global name that will be understood by everybody).

> diff --git a/sequencer.h b/sequencer.h
> index 65a4b0c25185d7ad5115035abb766d1b95df9a62..1db06caea35bed556dfaabca1c6be8a80857ed5e 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -68,4 +68,7 @@ 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 char* action, const struct strbuf *msg,
> +		struct strbuf *err);
>  #endif