Recovering the index after a git crash?

Recovering the index after a git crash?

[Matthieu Moy]

Hi,

I'm debugging a git assertion failure, which leaves a .git/index.lock
in the way. After running the failed command, git refuses to do
anything in this directory, complaining with:

  fatal: unable to create '.git/index.lock': File exists

What I'm doing then is just "rm .git/index.lock". Is it safe to do it?
I guess I should just make sure there's no git process running, but is
there anything else to check?

I'll write a FAQ entry on the wiki with answers, and that would
probably be a good idea to give indication to the user directly in the
error message too, otherwise, the problem can be blocking for
beginners.

Thanks,

<my life>
I had to support students using SVN for a month in January, and even
when SVN says:

  svn: run 'svn cleanup' to remove locks (type 'svn help cleanup' for details)

it's still hard for many students to understand that they have to run
"svn cleanup" and prefer sending me a mail saying "it doesn't work
anymore" :-(.
</my life>

-- 
Matthieu

[PATCH/RFC] More friendly message when locking the index fails.

[Matthieu Moy]

Just saying that index.lock exists doesn't tell the user _what_ to do
to fix the problem. We should give an indication that it's normally
safe to delete index.lock after making sure git isn't running here.
---
> I'll write a FAQ entry on the wiki with answers, and that would
> probably be a good idea to give indication to the user directly in the
> error message too, otherwise, the problem can be blocking for
> beginners.

Something along the lines of this patch maybe?

 builtin-update-index.c |   11 +++++++++--
 lockfile.c             |    9 ++++++++-
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/builtin-update-index.c b/builtin-update-index.c
index 5604977..ab8ab8f 100644
--- a/builtin-update-index.c
+++ b/builtin-update-index.c
@@ -742,8 +742,15 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		if (newfd < 0) {
 			if (refresh_flags & REFRESH_QUIET)
 				exit(128);
-			die("unable to create '%s.lock': %s",
-			    get_index_file(), strerror(lock_error));
+			if (lock_error == EEXIST) {
+				die("Unable to create '%s.lock': %s\n"
+				    "This probably means a git process crashed in this repository earlier.\n"
+				    "Make sure no other git process is running and remove the file manually.",
+				    get_index_file(), strerror(lock_error));
+			} else {
+				die("Unable to create '%s.lock': %s",
+				    get_index_file(), strerror(lock_error));
+			}
 		}
 		if (write_cache(newfd, active_cache, active_nr) ||
 		    commit_locked_index(lock_file))
diff --git a/lockfile.c b/lockfile.c
index 021c337..bb59f7e 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -159,7 +159,14 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)
 {
 	int fd = lock_file(lk, path, flags);
 	if (fd < 0 && (flags & LOCK_DIE_ON_ERROR))
-		die("unable to create '%s.lock': %s", path, strerror(errno));
+		if (errno == EEXIST) {
+			die("Unable to create '%s.lock': %s\n"
+			    "This probably means a git process crashed in this repository earlier.\n"
+			    "Make sure no other git process is running and remove the file manually.",
+			    path, strerror(errno));
+		} else {
+			die("Unable to create '%s.lock': %s", path, strerror(errno));
+		}
 	return fd;
 }
 
-- 
1.6.1.2.322.g01114.dirty

Re: [PATCH/RFC] More friendly message when locking the index fails.

[Sverre Rabbelier]

On Wed, Feb 4, 2009 at 10:58, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
> +                                   "This probably means a git process crashed in this repository earlier.\n"
> +                                   "Make sure no other git process is running and remove the file manually.",
> +                                   get_index_file(), strerror(lock_error));

How about "If no other git process is currently running, this probably
means...." instead? E.g., sometimes I run 'git commit' in one terminal
window, and then switch to another before committing (to review the
diff for example), which would cause an index.lock file to be present
validly.

-- 
Cheers,

Sverre Rabbelier

Re: Recovering the index after a git crash?

[Mikael Magnusson]

2009/2/4 Matthieu Moy <Matthieu.Moy@imag.fr>:
> Hi,
>
> I'm debugging a git assertion failure, which leaves a .git/index.lock
> in the way. After running the failed command, git refuses to do
> anything in this directory, complaining with:
>
>  fatal: unable to create '.git/index.lock': File exists
>
> What I'm doing then is just "rm .git/index.lock". Is it safe to do it?
> I guess I should just make sure there's no git process running, but is
> there anything else to check?

It should be safe, yes. Often it is even safe to remove .git/index
itself, since you can restore it from the latest commit with 'git
reset'.

-- 
Mikael Magnusson

[PATCH] More friendly message when locking the index fails.

[Matthieu Moy]

Just saying that index.lock exists doesn't tell the user _what_ to do
to fix the problem. We should give an indication that it's normally
safe to delete index.lock after making sure git isn't running here.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
Sverre Rabbelier <srabbelier@gmail.com> writes:

> How about "If no other git process is currently running, this probably
> means...." instead? E.g., sometimes I run 'git commit' in one terminal
> window, and then switch to another before committing (to review the
> diff for example), which would cause an index.lock file to be present
> validly.

Right, here's an updated version.

 builtin-update-index.c |   12 ++++++++++--
 lockfile.c             |   10 +++++++++-
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/builtin-update-index.c b/builtin-update-index.c
index 5604977..23b97db 100644
--- a/builtin-update-index.c
+++ b/builtin-update-index.c
@@ -742,8 +742,16 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		if (newfd < 0) {
 			if (refresh_flags & REFRESH_QUIET)
 				exit(128);
-			die("unable to create '%s.lock': %s",
-			    get_index_file(), strerror(lock_error));
+			if (lock_error == EEXIST) {
+				die("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.",
+				    get_index_file(), strerror(lock_error));
+			} else {
+				die("Unable to create '%s.lock': %s",
+				    get_index_file(), strerror(lock_error));
+			}
 		}
 		if (write_cache(newfd, active_cache, active_nr) ||
 		    commit_locked_index(lock_file))
diff --git a/lockfile.c b/lockfile.c
index 021c337..9bde859 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -159,7 +159,15 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)
 {
 	int fd = lock_file(lk, path, flags);
 	if (fd < 0 && (flags & LOCK_DIE_ON_ERROR))
-		die("unable to create '%s.lock': %s", path, strerror(errno));
+		if (errno == EEXIST) {
+			die("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(errno));
+		} else {
+			die("Unable to create '%s.lock': %s", path, strerror(errno));
+		}
 	return fd;
 }
 
-- 
1.6.1.2.351.g032a4.dirty

[PATCH v3] More friendly message when locking the index fails.

[Matthieu Moy]

Just saying that index.lock exists doesn't tell the user _what_ to do
to fix the problem. We should give an indication that it's normally
safe to delete index.lock after making sure git isn't running here.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
Oops, I hadn't noticed, but the previous version triggered a warning
because of lack of braces on the if. Sorry.

 builtin-update-index.c |   12 ++++++++++--
 lockfile.c             |   13 +++++++++++--
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/builtin-update-index.c b/builtin-update-index.c
index 5604977..23b97db 100644
--- a/builtin-update-index.c
+++ b/builtin-update-index.c
@@ -742,8 +742,16 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		if (newfd < 0) {
 			if (refresh_flags & REFRESH_QUIET)
 				exit(128);
-			die("unable to create '%s.lock': %s",
-			    get_index_file(), strerror(lock_error));
+			if (lock_error == EEXIST) {
+				die("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.",
+				    get_index_file(), strerror(lock_error));
+			} else {
+				die("Unable to create '%s.lock': %s",
+				    get_index_file(), strerror(lock_error));
+			}
 		}
 		if (write_cache(newfd, active_cache, active_nr) ||
 		    commit_locked_index(lock_file))
diff --git a/lockfile.c b/lockfile.c
index 021c337..c812596 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -158,8 +158,17 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)
 {
 	int fd = lock_file(lk, path, flags);
-	if (fd < 0 && (flags & LOCK_DIE_ON_ERROR))
-		die("unable to create '%s.lock': %s", path, strerror(errno));
+	if (fd < 0 && (flags & LOCK_DIE_ON_ERROR)) {
+		if (errno == EEXIST) {
+			die("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(errno));
+		} else {
+			die("Unable to create '%s.lock': %s", path, strerror(errno));
+		}
+	}
 	return fd;
 }
 
-- 
1.6.1.2.351.g80eb2.dirty

Re: [PATCH v3] More friendly message when locking the index fails.

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> Just saying that index.lock exists doesn't tell the user _what_ to do
> to fix the problem. We should give an indication that it's normally
> safe to delete index.lock after making sure git isn't running here.
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
> Oops, I hadn't noticed, but the previous version triggered a warning
> because of lack of braces on the if. Sorry.

Perhaps it is a good idea to introduce

NORETURN void unable_to_lock_index_die(const char *path, int err)
{
	if (err == EEXIST)
	        die(...);
 	else
        	die(...);
}

in lockfile.c and call it from these two places you are touching?

>  builtin-update-index.c |   12 ++++++++++--
>  lockfile.c             |   13 +++++++++++--
>  2 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/builtin-update-index.c b/builtin-update-index.c
> index 5604977..23b97db 100644
> --- a/builtin-update-index.c
> +++ b/builtin-update-index.c
> @@ -742,8 +742,16 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>  		if (newfd < 0) {
>  			if (refresh_flags & REFRESH_QUIET)
>  				exit(128);
> -			die("unable to create '%s.lock': %s",
> -			    get_index_file(), strerror(lock_error));
> +			if (lock_error == EEXIST) {
> +				die("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.",
> +				    get_index_file(), strerror(lock_error));
> +			} else {
> +				die("Unable to create '%s.lock': %s",
> +				    get_index_file(), strerror(lock_error));
> +			}
>  		}
>  		if (write_cache(newfd, active_cache, active_nr) ||
>  		    commit_locked_index(lock_file))
> diff --git a/lockfile.c b/lockfile.c
> index 021c337..c812596 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -158,8 +158,17 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
>  int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)
>  {
>  	int fd = lock_file(lk, path, flags);
> -	if (fd < 0 && (flags & LOCK_DIE_ON_ERROR))
> -		die("unable to create '%s.lock': %s", path, strerror(errno));
> +	if (fd < 0 && (flags & LOCK_DIE_ON_ERROR)) {
> +		if (errno == EEXIST) {
> +			die("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(errno));
> +		} else {
> +			die("Unable to create '%s.lock': %s", path, strerror(errno));
> +		}
> +	}
>  	return fd;
>  }
>  
> -- 
> 1.6.1.2.351.g80eb2.dirty
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] More friendly message when locking the index fails.

[Matthieu Moy]

Just saying that index.lock exists doesn't tell the user _what_ to do
to fix the problem. We should give an indication that it's normally
safe to delete index.lock after making sure git isn't running here.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
> Perhaps it is a good idea to introduce
> 
> NORETURN void unable_to_lock_index_die(const char *path, int err)
> {
> 	if (err == EEXIST)
> 	        die(...);
>  	else
>         	die(...);
> }
> 
> in lockfile.c and call it from these two places you are touching?

Here it is!

 builtin-update-index.c |    3 +--
 cache.h                |    1 +
 lockfile.c             |   16 +++++++++++++++-
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/builtin-update-index.c b/builtin-update-index.c
index 5604977..dd43d5b 100644
--- a/builtin-update-index.c
+++ b/builtin-update-index.c
@@ -742,8 +742,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		if (newfd < 0) {
 			if (refresh_flags & REFRESH_QUIET)
 				exit(128);
-			die("unable to create '%s.lock': %s",
-			    get_index_file(), strerror(lock_error));
+			unable_to_lock_index_die(get_index_file(), lock_error);
 		}
 		if (write_cache(newfd, active_cache, active_nr) ||
 		    commit_locked_index(lock_file))
diff --git a/cache.h b/cache.h
index 37dfb1c..bb5a190 100644
--- a/cache.h
+++ b/cache.h
@@ -484,6 +484,7 @@ 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 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 021c337..1db1a2f 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -155,11 +155,25 @@ 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)
+{
+	if (errno == EEXIST) {
+		die("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));
+	}
+}
+
 int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)
 {
 	int fd = lock_file(lk, path, flags);
 	if (fd < 0 && (flags & LOCK_DIE_ON_ERROR))
-		die("unable to create '%s.lock': %s", path, strerror(errno));
+		unable_to_lock_index_die(path, errno);
 	return fd;
 }
 
-- 
1.6.2.rc1.14.g7f87d

Re: [PATCH] More friendly message when locking the index fails.

[Sverre Rabbelier]

Heya,

On Thu, Feb 19, 2009 at 13:54, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
> Just saying that index.lock exists doesn't tell the user _what_ to do
> to fix the problem. We should give an indication that it's normally
> safe to delete index.lock after making sure git isn't running here.

Surely we need a 'git cleanup' which will attempt to clean up such
mess, fail miserably most of the time and might even corrupt your
repository further! Oh wait, wrong VCS...

-- 
Cheers,

Sverre Rabbelier