Re: [PATCH v2 2/2] commit.c: check for lock error and return early

2014-04-17 Thread Junio C Hamano
Michael Haggerty  writes:

> On 04/16/2014 08:56 PM, Ronnie Sahlberg wrote:
>> Move the check for the lock failure to happen immediately after
>> lock_any_ref_for_update().
>> Previously the lock and the check-if-lock-failed was separated by a handful
>> of string manipulation statements.
>
> Please flow sentences together into paragraphs for easier reading,
> rather than having an extremely ragged right-hand margin.
>
> The rest looks good.

Thanks, both.  I tentatively queued with the suggested log message
tweaks, and I think result reads better.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v2 2/2] commit.c: check for lock error and return early

2014-04-16 Thread Michael Haggerty
On 04/16/2014 08:56 PM, Ronnie Sahlberg wrote:
> Move the check for the lock failure to happen immediately after
> lock_any_ref_for_update().
> Previously the lock and the check-if-lock-failed was separated by a handful
> of string manipulation statements.

Please flow sentences together into paragraphs for easier reading,
rather than having an extremely ragged right-hand margin.

The rest looks good.

Michael

>
> Moving the check to occur immediately after the failed lock makes the
> code slightly easier to read and makes it follow the pattern of
>  try-to-take-a-lock()
>  if (check-if-lock-failed){
> error
>  }
> ---
>  builtin/commit.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/builtin/commit.c b/builtin/commit.c
> index d9550c5..c6320f1 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1672,6 +1672,10 @@ int cmd_commit(int argc, const char **argv, const char 
> *prefix)
>  ? NULL
>  : current_head->object.sha1,
>  0, NULL);
> + if (!ref_lock) {
> + rollback_index_files();
> + die(_("cannot lock HEAD ref"));
> + }
>  
>   nl = strchr(sb.buf, '\n');
>   if (nl)
> @@ -1681,10 +1685,6 @@ int cmd_commit(int argc, const char **argv, const char 
> *prefix)
>   strbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));
>   strbuf_insert(&sb, strlen(reflog_msg), ": ", 2);
>  
> - if (!ref_lock) {
> - rollback_index_files();
> - die(_("cannot lock HEAD ref"));
> - }
>   if (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {
>   rollback_index_files();
>   die(_("cannot update HEAD ref"));
> 


-- 
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html