threads / rfc / 41561

RFC patchlockfile: improve error message when lockfile exists

Subject: [RFC/PATCH] lockfile: improve error message when lockfile exists

## tl;dr

6 messages between Feb 28, 2016 and Mar 1, 2016. Diffs are folded; open one to read it.

replies: 5people: 4as markdown or json

Matthieu Moy· Feb 28, 2016, 20:11 UTC · lore

A common mistake leading a user to see this message is to launch "git commit", let the editor open (and forget about it), and try again to commit.

The previous message was going too quickly to "a git process crashed" and to the advice "remove the file manually".

This patch modifies the message in two ways: first, it considers that "another process is running" is the norm, not the exception, and it explicitly hints the user to look at text editors.

The message is 2 lines longer, but this is not a problem since experienced users do not see the message often.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
This patch was written after seen one more time a student panicked
because he had a "git commit" backgrounded.

I'm not 100% satisfied with the new message. I don't think it harms to have a 5-lines message here but it's probably possible to be a bit more concise.

I'm the author of the previous version of the message, and I remember writting it after debugging a segfaut in Git which led me to see the message multiple times because of a crash. I guess that gave the wrong biais to the message ;-).

 lockfile.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Show changes to lockfile.c +5 −3

diff --git a/lockfile.c b/lockfile.c
index 80d056d..a7d6175 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -150,9 +150,11 @@ void unable_to_lock_message(const char *path, int err, struct strbuf *buf)
 {
 	if (err == EEXIST) {
 		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.",
+		    "Another git process seems to be running in this repository.\n"
+		    "Please terminate it (e.g. quit any text editor that git may be\n"
+		    "waiting for) and try again. If no other git process is running,\n"
+		    "then a process may have crashed in this repository earlier:\n"
+		    "remove the file manually to continue.",
 			    absolute_path(path), strerror(err));
 	} else
 		strbuf_addf(buf, "Unable to create '%s.lock': %s",

-- 
2.7.2.334.g35ed2ae.dirty

Moritz Neeb· Feb 28, 2016, 22:58 UTC · re: Matthieu Moy · lore

Re: [RFC/PATCH] lockfile: improve error message when lockfile exists

On 02/28/2016 09:11 PM, Matthieu Moy wrote:

Show 18 quoted lines

> A common mistake leading a user to see this message is to launch "git
> commit", let the editor open (and forget about it), and try again to
> commit.
> 
> The previous message was going too quickly to "a git process crashed"
> and to the advice "remove the file manually".
> 
> This patch modifies the message in two ways: first, it considers that
> "another process is running" is the norm, not the exception, and it
> explicitly hints the user to look at text editors.
> 
> The message is 2 lines longer, but this is not a problem since
> experienced users do not see the message often.
> 
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
> This patch was written after seen one more time a student panicked
> because he had a "git commit" backgrounded.

I think git commit is almost the only case where this would happen. If the goal is have it shorter, then the editor example could be boiled down to something like "e.g. a commit". But then it is less clear on what the necessary action would be.

> 
> I'm not 100% satisfied with the new message. I don't think it harms to
> have a 5-lines message here but it's probably possible to be a bit
> more concise.

I adapted it a bit, it has about the same length but I tried to take out some repeating:

-- 8< --

Show changes to lockfile.c +5 −3

diff --git a/lockfile.c b/lockfile.c
index 80d056d..ffb4c8d 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -150,9 +150,11 @@ void unable_to_lock_message(const char *path, int err, struct strbuf *buf)
 {
     if (err == EEXIST) {
         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.",
+            "Another git process seems to be running in this repository,\n"
+            "e.g. an editor opened by git. Please make sure all processes\n"
+            "are terminated then try again. If it still fails, a git process\n"
+            "may have crashed in this repository earlier:\n"
+            "remove the file manually to continue.",
                 absolute_path(path), strerror(err));
     } else
         strbuf_addf(buf, "Unable to create '%s.lock': %s",

Matthieu Moy· Feb 29, 2016, 07:25 UTC · re: Moritz Neeb · lore

Re: [RFC/PATCH] lockfile: improve error message when lockfile exists

Moritz Neeb <lists@moritzneeb.de> writes:

Show 6 quoted lines

> On 02/28/2016 09:11 PM, Matthieu Moy wrote:
>
>> This patch was written after seen one more time a student panicked
>> because he had a "git commit" backgrounded.
>
> I think git commit is almost the only case where this would happen.

Right. It's not the only case when Git launches an editor, but probably the only one when it does so keeping a lock.

> If the goal is have it shorter, then the editor example could be boiled down to
> something like "e.g. a commit". But then it is less clear on what the
> necessary action would be.

Maybe just ""e.g. an editor opened by git" -> ""e.g. an editor opened by git commit"?

Show 14 quoted lines

> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -150,9 +150,11 @@ void unable_to_lock_message(const char *path, int err, struct strbuf *buf)
>  {
>      if (err == EEXIST) {
>          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.",
> +            "Another git process seems to be running in this repository,\n"
> +            "e.g. an editor opened by git. Please make sure all processes\n"
> +            "are terminated then try again. If it still fails, a git process\n"
> +            "may have crashed in this repository earlier:\n"
> +            "remove the file manually to continue.",

I like your version better than mine indeed.

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

Duy Nguyen· Feb 29, 2016, 00:35 UTC · re: Matthieu Moy · lore

Re: [RFC/PATCH] lockfile: improve error message when lockfile exists

On Mon, Feb 29, 2016 at 3:11 AM, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:

Show 17 quoted lines

> diff --git a/lockfile.c b/lockfile.c
> index 80d056d..a7d6175 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -150,9 +150,11 @@ void unable_to_lock_message(const char *path, int err, struct strbuf *buf)
>  {
>         if (err == EEXIST) {
>                 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.",
> +                   "Another git process seems to be running in this repository.\n"
> +                   "Please terminate it (e.g. quit any text editor that git may be\n"
> +                   "waiting for) and try again. If no other git process is running,\n"
> +                   "then a process may have crashed in this repository earlier:\n"
> +                   "remove the file manually to continue.",
>                             absolute_path(path), strerror(err));

i18n police checking in :) This message looks very much translatable. Could you check if that's true while you're touching this code, and if so _() it?

>         } else
>                 strbuf_addf(buf, "Unable to create '%s.lock': %s",

also this one, I think. -- Duy

Matthieu Moy· Mar 1, 2016, 17:04 UTC · re: Matthieu Moy · lore

[PATCH v2 1/2] lockfile: mark strings for translation

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 lockfile.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Show changes to lockfile.c +3 −3

diff --git a/lockfile.c b/lockfile.c
index 80d056d..62583d1 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -149,13 +149,13 @@ static int lock_file_timeout(struct lock_file *lk, const char *path,
 void unable_to_lock_message(const char *path, int err, struct strbuf *buf)
 {
 	if (err == EEXIST) {
-		strbuf_addf(buf, "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.",
+		    "process is running and remove the file manually to continue."),
 			    absolute_path(path), strerror(err));
 	} else
-		strbuf_addf(buf, "Unable to create '%s.lock': %s",
+		strbuf_addf(buf, _("Unable to create '%s.lock': %s"),
 			    absolute_path(path), strerror(err));
 }

-- 
2.7.2.334.g35ed2ae.dirty

Matthieu Moy· Mar 1, 2016, 17:04 UTC · re: Matthieu Moy · lore

[PATCH v2 2/2] lockfile: improve error message when lockfile exists

A common mistake leading a user to see this message is to launch "git commit", let the editor open (and forget about it), and try again to commit.

The previous message was going too quickly to "a git process crashed" and to the advice "remove the file manually".

This patch modifies the message in two ways: first, it considers that "another process is running" is the norm, not the exception, and it explicitly hints the user to look at text editors.

The message is 2 lines longer, but this is not a problem since experienced users do not see the message often.

Helped-by: Moritz Neeb <lists@moritzneeb.de>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 lockfile.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Show changes to lockfile.c +5 −3

diff --git a/lockfile.c b/lockfile.c
index 62583d1..9268cdf 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -150,9 +150,11 @@ void unable_to_lock_message(const char *path, int err, struct strbuf *buf)
 {
 	if (err == EEXIST) {
 		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."),
+		    "Another git process seems to be running in this repository, e.g.\n"
+		    "an editor opened by 'git commit'. Please make sure all processes\n"
+		    "are terminated then try again. If it still fails, a git process\n"
+		    "may have crashed in this repository earlier:\n"
+		    "remove the file manually to continue."),
 			    absolute_path(path), strerror(err));
 	} else
 		strbuf_addf(buf, _("Unable to create '%s.lock': %s"),

-- 
2.7.2.334.g35ed2ae.dirty