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

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

[Matthieu Moy]

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(-)

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

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

[Moritz Neeb]

On 02/28/2016 09:11 PM, Matthieu Moy wrote:
> 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< --
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",

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

[Matthieu Moy]

Moritz Neeb <lists@moritzneeb.de> writes:

> 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"?

> --- 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/

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

[Duy Nguyen]

On Mon, Feb 29, 2016 at 3:11 AM, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
> 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

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

[Matthieu Moy]

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

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

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

[Matthieu Moy]

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(-)

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