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

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

[Miklos Vajna]

Previously the old error message just told the user that it was not
possible to delete the ref from the packed-refs file. Give instructions
on how to resolve the problem.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 refs.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

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];
-- 
1.6.5.rc1.44.ga1675.dirty

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

[Jeff King]

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

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

[Miklos Vajna]

Previously the old error message just told the user that it was not
possible to delete the ref from the packed-refs file. Give instructions
on how to resolve the problem.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---

On Fri, Sep 25, 2009 at 11:31:43PM -0400, Jeff King <peff@peff.net> wrote:
> afterwards). So probably you would need to first refactor
> unable_to_lock_index_die() to handle just printing the error without
> dying.

Agreed, I forgot builtin-remote. What about this?

I removed the NORETURN macro as otherwise gcc would issue a warning, as
it does not realise that unable_to_lock_index_die() never returns.

 cache.h    |    3 ++-
 lockfile.c |   22 +++++++++++++++++-----
 refs.c     |    4 +++-
 3 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/cache.h b/cache.h
index 1a6412d..ffec86b 100644
--- a/cache.h
+++ b/cache.h
@@ -489,7 +489,8 @@ struct lock_file {
 };
 #define LOCK_DIE_ON_ERROR 1
 #define LOCK_NODEREF 2
-extern NORETURN void unable_to_lock_index_die(const char *path, int err);
+extern int unable_to_lock_index(const char *path, int err, int noreturn);
+extern void unable_to_lock_index_die(const char *path, int err);
 extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);
 extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);
 extern int commit_lock_file(struct lock_file *);
diff --git a/lockfile.c b/lockfile.c
index eb931ed..aa8c444 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -156,17 +156,29 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 }
 
 
-NORETURN void unable_to_lock_index_die(const char *path, int err)
+int unable_to_lock_index(const char *path, int err, int noreturn)
 {
+	struct strbuf buf = STRBUF_INIT;
+	int ret;
+
 	if (err == EEXIST) {
-		die("Unable to create '%s.lock': %s.\n\n"
+		strbuf_addf(&buf, "Unable to create '%s.lock': %s.\n\n"
 		    "If no other git process is currently running, this probably means a\n"
 		    "git process crashed in this repository earlier. Make sure no other git\n"
 		    "process is running and remove the file manually to continue.",
 		    path, strerror(err));
-	} else {
-		die("Unable to create '%s.lock': %s", path, strerror(err));
-	}
+	} else
+		strbuf_addf(&buf, "Unable to create '%s.lock': %s", path, strerror(err));
+	if (noreturn)
+		die(buf.buf);
+	ret = error(buf.buf);
+	strbuf_release(&buf);
+	return ret;
+}
+
+void unable_to_lock_index_die(const char *path, int err)
+{
+	unable_to_lock_index(path, err, 1);
 }
 
 int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)
diff --git a/refs.c b/refs.c
index 24865cf..4eb4fc7 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(git_path("packed-refs"), errno, 0);
 		return error("cannot delete '%s' from packed refs", refname);
+	}
 
 	for (list = packed_ref_list; list; list = list->next) {
 		char line[PATH_MAX + 100];
-- 
1.6.5.rc1.44.ga1675.dirty

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

[Shawn O. Pearce]

Miklos Vajna <vmiklos@frugalware.org> wrote:
> On Fri, Sep 25, 2009 at 11:31:43PM -0400, Jeff King <peff@peff.net> wrote:
> > afterwards). So probably you would need to first refactor
> > unable_to_lock_index_die() to handle just printing the error without
> > dying.
> 
> I removed the NORETURN macro as otherwise gcc would issue a warning, as
> it does not realise that unable_to_lock_index_die() never returns.

Please don't.  If you refactor the error message formatting into
a static function called from the two extern ones, you can still
centralize the formatting but also keep NORETURN on the method that
doesn't return.  The annotation is useful and should not be removed.
 
> -extern NORETURN void unable_to_lock_index_die(const char *path, int err);
> +extern int unable_to_lock_index(const char *path, int err, int noreturn);
> +extern void unable_to_lock_index_die(const char *path, int err);

-- 
Shawn.

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

[Matthieu Moy]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> Miklos Vajna <vmiklos@frugalware.org> wrote:
>> On Fri, Sep 25, 2009 at 11:31:43PM -0400, Jeff King <peff@peff.net> wrote:
>> > afterwards). So probably you would need to first refactor
>> > unable_to_lock_index_die() to handle just printing the error without
>> > dying.
>> 
>> I removed the NORETURN macro as otherwise gcc would issue a warning, as
>> it does not realise that unable_to_lock_index_die() never returns.
>
> Please don't.  If you refactor the error message formatting into
> a static function called from the two extern ones, you can still
> centralize the formatting but also keep NORETURN on the method that
> doesn't return.  The annotation is useful and should not be removed.

I guess the lint-trap would be to add a

  die("unable_to_lock_index() should have died already");

at the end of unable_to_lock_index_die().

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

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

[Miklos Vajna]

Previously the old error message just told the user that it was not
possible to delete the ref from the packed-refs file. Give instructions
on how to resolve the problem.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---

Changes since the previous one:

* unable_to_lock_index() renamed to unable_to_lock()
* NORETURN is back for unable_to_lock_index_die()

 cache.h    |    1 +
 lockfile.c |   23 ++++++++++++++++++-----
 refs.c     |    4 +++-
 3 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/cache.h b/cache.h
index 1a6412d..797cc4c 100644
--- a/cache.h
+++ b/cache.h
@@ -489,6 +489,7 @@ struct lock_file {
 };
 #define LOCK_DIE_ON_ERROR 1
 #define LOCK_NODEREF 2
+extern int unable_to_lock(const char *path, int err, int noreturn);
 extern NORETURN void unable_to_lock_index_die(const char *path, int err);
 extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);
 extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);
diff --git a/lockfile.c b/lockfile.c
index eb931ed..3bacda4 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -156,17 +156,30 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 }
 
 
-NORETURN void unable_to_lock_index_die(const char *path, int err)
+int unable_to_lock(const char *path, int err, int noreturn)
 {
+	struct strbuf buf = STRBUF_INIT;
+	int ret;
+
 	if (err == EEXIST) {
-		die("Unable to create '%s.lock': %s.\n\n"
+		strbuf_addf(&buf, "Unable to create '%s.lock': %s.\n\n"
 		    "If no other git process is currently running, this probably means a\n"
 		    "git process crashed in this repository earlier. Make sure no other git\n"
 		    "process is running and remove the file manually to continue.",
 		    path, strerror(err));
-	} else {
-		die("Unable to create '%s.lock': %s", path, strerror(err));
-	}
+	} else
+		strbuf_addf(&buf, "Unable to create '%s.lock': %s", path, strerror(err));
+	if (noreturn)
+		die(buf.buf);
+	ret = error(buf.buf);
+	strbuf_release(&buf);
+	return ret;
+}
+
+NORETURN void unable_to_lock_index_die(const char *path, int err)
+{
+	unable_to_lock(path, err, 1);
+	die("unable_to_lock() should have died already");
 }
 
 int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)
diff --git a/refs.c b/refs.c
index 24865cf..3d635ae 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(git_path("packed-refs"), errno, 0);
 		return error("cannot delete '%s' from packed refs", refname);
+	}
 
 	for (list = packed_ref_list; list; list = list->next) {
 		char line[PATH_MAX + 100];
-- 
1.6.5.rc1.44.ga1675.dirty

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

[Jeff King]

On Sun, Sep 27, 2009 at 01:15:09AM +0200, Miklos Vajna wrote:

> Changes since the previous one:
> 
> * unable_to_lock_index() renamed to unable_to_lock()
> * NORETURN is back for unable_to_lock_index_die()

Much better, but:

> +	if (noreturn)
> +		die(buf.buf);
> +	ret = error(buf.buf);

These need to be:

  die("%s", buf.buf);

as the resulting message (which contains the filename) may have '%' in
it.

> +NORETURN void unable_to_lock_index_die(const char *path, int err)
> +{
> +	unable_to_lock(path, err, 1);
> +	die("unable_to_lock() should have died already");
>  }

Maybe it is just me, but that extra die() that should never be reached
is terribly ugly. I would do it with two functions, one that dies and
one that doesn't, with a helper to format the message. IOW, this:

-- >8 --
From: Miklos Vajna <vmiklos@frugalware.org>
Subject: [PATCH] git branch -D: give a better error message when lockfile creation fails

Previously the old error message just told the user that it was not
possible to delete the ref from the packed-refs file. Give instructions
on how to resolve the problem.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h    |    1 +
 lockfile.c |   26 ++++++++++++++++++++------
 refs.c     |    4 +++-
 3 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/cache.h b/cache.h
index 1a6412d..a5eeead 100644
--- a/cache.h
+++ b/cache.h
@@ -489,6 +489,7 @@ struct lock_file {
 };
 #define LOCK_DIE_ON_ERROR 1
 #define LOCK_NODEREF 2
+extern int unable_to_lock_error(const char *path, int err);
 extern NORETURN void unable_to_lock_index_die(const char *path, int err);
 extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);
 extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);
diff --git a/lockfile.c b/lockfile.c
index eb931ed..6851fa5 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -155,18 +155,32 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 	return lk->fd;
 }
 
-
-NORETURN void unable_to_lock_index_die(const char *path, int err)
+static char *unable_to_lock_message(const char *path, int err)
 {
+	struct strbuf buf = STRBUF_INIT;
+
 	if (err == EEXIST) {
-		die("Unable to create '%s.lock': %s.\n\n"
+		strbuf_addf(&buf, "Unable to create '%s.lock': %s.\n\n"
 		    "If no other git process is currently running, this probably means a\n"
 		    "git process crashed in this repository earlier. Make sure no other git\n"
 		    "process is running and remove the file manually to continue.",
 		    path, strerror(err));
-	} else {
-		die("Unable to create '%s.lock': %s", path, strerror(err));
-	}
+	} else
+		strbuf_addf(&buf, "Unable to create '%s.lock': %s", path, strerror(err));
+	return strbuf_detach(&buf, NULL);
+}
+
+int unable_to_lock_error(const char *path, int err)
+{
+	char *msg = unable_to_lock_message(path, err);
+	error("%s", msg);
+	free(msg);
+	return -1;
+}
+
+NORETURN void unable_to_lock_index_die(const char *path, int err)
+{
+	die("%s", unable_to_lock_message(path, err));
 }
 
 int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)
diff --git a/refs.c b/refs.c
index 24865cf..808f56b 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_error(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];
-- 
1.6.5.rc2.197.g25cf3

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

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 355 bytes --]

On Sun, Sep 27, 2009 at 04:21:23AM -0400, Jeff King <peff@peff.net> wrote:
> Maybe it is just me, but that extra die() that should never be reached
> is terribly ugly. I would do it with two functions, one that dies and
> one that doesn't, with a helper to format the message. IOW, this:

Okay, that's fine with me - thanks for providing a patch as well.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

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

[Matthieu Moy]

Miklos Vajna <vmiklos@frugalware.org> writes:

> diff --git a/refs.c b/refs.c
> index 24865cf..4eb4fc7 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(git_path("packed-refs"), errno, 0);
>  		return error("cannot delete '%s' from packed refs", refname);
> +	}

If unable_to_lock_index is called for something other that the index,
it should probably be renamed.

Other than that, I like what the patch does (I'm the one who wrote the
message for the index ;-) ).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

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

[Miklos Vajna]

Previously the old error message just told the user that it was not
possible to delete the ref from the packed-refs file. Give instructions
on how to resolve the problem.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 refs.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

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];
-- 
1.6.5.rc1.44.ga1675.dirty