Subject: Re: [PATCH v20 41/48] refs.c: pass the ref log message to _create/delete/update instead of _commit

From: Michael Haggerty <[email-masked]>
Date: Tue, 08 Jul 2014 16:39:47 +0200

On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
> Change the reference transactions so that we pass the reflog message
> through to the create/delete/update function instead of the commit message.
> This allows for individual messages for each change in a multi ref
> transaction.
> 
> Reviewed-by: Jonathan Nieder <[email-masked]>
> Signed-off-by: Ronnie Sahlberg <[email-masked]>
> ---
> [...]
> diff --git a/refs.h b/refs.h
> index 3b321c2..f24b2c1 100644
> --- a/refs.h
> +++ b/refs.h

Would you please document the msg parameter in the block comment that
precedes these three declarations?  Especially important is the fact
that the functions make internal copies of msg, so the caller retains
ownership of its copy.  You might also mention what happens if msg is
NULL (which, as far as I can see, is that a reflog entry is created
anyway (except in the case of a delete) but that the entry doesn't
contain an explanation).

> @@ -297,7 +297,7 @@ int ref_transaction_update(struct ref_transaction *transaction,
>  			   const char *refname,
>  			   const unsigned char *new_sha1,
>  			   const unsigned char *old_sha1,
> -			   int flags, int have_old,
> +			   int flags, int have_old, const char *msg,
>  			   struct strbuf *err);
>  
>  /*
> @@ -312,7 +312,7 @@ int ref_transaction_update(struct ref_transaction *transaction,
>  int ref_transaction_create(struct ref_transaction *transaction,
>  			   const char *refname,
>  			   const unsigned char *new_sha1,
> -			   int flags,
> +			   int flags, const char *msg,
>  			   struct strbuf *err);

It is noteworthy that ref_transaction_delete() accepts a msg parameter,
even though we currently delete a reference's entire reflog when the
reference is deleted.  I prefer to think of this as a shortcoming of the
current reference backend, from which future backends hopefully will not
suffer.  So I like this design choice.

However, I think it is worth noting this dichotomy in the commit message
and perhaps also in the function documentation.

>  /*
> @@ -326,7 +326,7 @@ int ref_transaction_create(struct ref_transaction *transaction,
>  int ref_transaction_delete(struct ref_transaction *transaction,
>  			   const char *refname,
>  			   const unsigned char *old_sha1,
> -			   int flags, int have_old,
> +			   int flags, int have_old, const char *msg,
>  			   struct strbuf *err);
>  
>  /*
> @@ -335,7 +335,7 @@ int ref_transaction_delete(struct ref_transaction *transaction,
>   * problem.
>   */
>  int ref_transaction_commit(struct ref_transaction *transaction,
> -			   const char *msg, struct strbuf *err);
> +			   struct strbuf *err);
>  
>  /*
>   * Free an existing transaction and all associated data.
> [...]

Michael

-- 
Michael Haggerty
[email-masked]
http://softwareswirl.blogspot.com/