Re: [PATCH] git branch -D: give a better error message when lockfile creation fails

From: Jeff King <peff@peff.net>
Date: Sep 26, 2009, 03:31 UTC
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:

Show 16 quoted lines

> 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