From: Jeff King <peff@peff.net>
Date: Sat, 26 Sep 2009 03:31:43 GMT
Subject: Re: [PATCH] git branch -D: give a better error message when lockfile creation fails
Message-ID: <20090926033143.GA9917@coredump.intra.peff.net>
In-Reply-To: <1253923602-17818-1-git-send-email-vmiklos@frugalware.org>

On Sat, Sep 26, 2009 at 02:06:42AM +0200, Miklos Vajna wrote:

> diff --git a/refs.c b/refs.c
> index 24865cf..221d49c 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -972,8 +972,10 @@ static int repack_without_ref(const char *refname)
>  	if (!found)
>  		return 0;
>  	fd = hold_lock_file_for_update(&packlock, git_path("packed-refs"), 0);
> -	if (fd < 0)
> +	if (fd < 0) {
> +		unable_to_lock_index_die(git_path("packed-refs"), errno);
>  		return error("cannot delete '%s' from packed refs", refname);
> +	}
>  
>  	for (list = packed_ref_list; list; list = list->next) {
>  		char line[PATH_MAX + 100];

I have several concerns about this patch:

  1. We used to return error(), but now we die. Are there any callers
     which care about the difference?

  2. If we did want to die, then the "return error()" just below is
     unreachable, and should be removed.

  3. If we did want to die, should we not just pass LOCK_DIE_ON_ERROR to
     hold_lock_file_for_update?

I suspect (2) and (3) are irrelevant because the answer to (1) is that
yes, some callers do care (e.g., it looks like builtin-remote calls
delete_ref, and notes an error but continues to do useful work
afterwards). So probably you would need to first refactor
unable_to_lock_index_die() to handle just printing the error without
dying.

-Peff