{"thread":{"id":"40908","subject":"[PATCH] Make error message after failing commit_lock_file() less confusing","startedAt":"2015-11-30T11:40:53Z","lastAt":"2015-12-16T18:20:16Z","messageCount":8,"participants":["SZEDER Gábor","Jeff King","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"273825","messageId":"1448883653-9140-1-git-send-email-szeder@ira.uka.de","threadId":"40908","inReplyTo":null,"subject":"[PATCH] Make error message after failing commit_lock_file() less confusing","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2015-11-30T11:40:53Z","receivedAt":"2015-11-30T11:40:53Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The error message after a failing commit_lock_file() call sometimes\nlooks like this, causing confusion:\n\n $ git remote add remote git@server.com/repo.git\n error: could not commit config file .git/config\n # Huh?!\n # I didn't want to commit anything, especially not my config file!\n\nWhile in the narrow context of the lockfile module using the verb\n'commit' in the error message makes perfect sense, in the broader\ncontext of git the word 'commit' already has a very specific meaning,\nhence the confusion.\n\nReword these error messages to say \"could not write\" instead of \"could\nnot commit\".\n\nWhile at it, include strerror in the error messages after writing the\nconfig file or the credential store fails to provide some information\nabout the cause of the failure, and update the style of the error\nmessage after writing the reflog fails to match surrounding error\nmessages (i.e. no '' around the pathname and no () around the error\ndescription).\n\nSigned-off-by: SZEDER Gábor \n---\n\nThe error message is of course bikeshedable.\nSome callers of commit_lock_file() say something like \"could not move\nupdated whatever file into place: \" on error. While\nit's truer to the meaning of the original error message and conveys\nmore exactly what went wrong (i.e. the actual writing had already\nfinished at that point, it's the move that failed), I think those\ndetails are not necessary and the short and simple \"could not write\"\nis better.\n\n config.c | 6 ++++--\n credential-store.c | 3 ++-\n fast-import.c | 2 +-\n refs.c | 2 +-\n 4 files changed, 8 insertions(+), 5 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 248a21ab94..86a5eb2571 100644\n--- a/config.c\n+++ b/config.c\n@@ -2144,7 +2144,8 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \t}\n \n \tif (commit_lock_file(lock) < 0) {\n-\t\terror(\"could not commit config file %s\", config_filename);\n+\t\terror(\"could not write config file %s: %s\", config_filename,\n+\t\t strerror(errno));\n \t\tret = CONFIG_NO_WRITE;\n \t\tlock = NULL;\n \t\tgoto out_free;\n@@ -2330,7 +2331,8 @@ int git_config_rename_section_in_file(const char *config_filename,\n \tfclose(config_file);\n unlock_and_out:\n \tif (commit_lock_file(lock) < 0)\n-\t\tret = error(\"could not commit config file %s\", config_filename);\n+\t\tret = error(\"could not write config file %s: %s\",\n+\t\t\t config_filename, strerror(errno));\n out:\n \tfree(filename_buf);\n \treturn ret;\ndiff --git a/credential-store.c b/credential-store.c\nindex 00aea3aa30..fc67d16c10 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -64,7 +64,8 @@ static void rewrite_credential_file(const char *fn, struct credential *c,\n \t\tprint_line(extra);\n \tparse_credential_file(fn, c, NULL, print_line);\n \tif (commit_lock_file(&credential_lock) < 0)\n-\t\tdie_errno(\"unable to commit credential store\");\n+\t\tdie_errno(\"unable to write credential store: %s\",\n+\t\t\t strerror(errno));\n }\n \n static void store_credential_file(const char *fn, struct credential *c)\ndiff --git a/fast-import.c b/fast-import.c\nindex e3b421d514..3c65edb5c4 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1824,7 +1824,7 @@ static void dump_marks(void)\n \n \tdump_marks_helper(f, 0, marks);\n \tif (commit_lock_file(&mark_lock)) {\n-\t\tfailure |= error(\"Unable to commit marks file %s: %s\",\n+\t\tfailure |= error(\"Unable to write file %s: %s\",\n \t\t\texport_marks_file, strerror(errno));\n \t\treturn;\n \t}\ndiff --git a/refs.c b/refs.c\nindex 132eff52ca..b1c7b229b7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -4684,7 +4684,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,\n \t\t\t\t\tget_lock_file_path(lock->lk));\n \t\t\trollback_lock_file(&reflog_lock);\n \t\t} else if (commit_lock_file(&reflog_lock)) {\n-\t\t\tstatus |= error(\"unable to commit reflog '%s' (%s)\",\n+\t\t\tstatus |= error(\"unable to write reflog %s: %s\",\n \t\t\t\t\tlog_file, strerror(errno));\n \t\t} else if (update && commit_ref(lock)) {\n \t\t\tstatus |= error(\"couldn't set %s\", lock->ref_name);\n-- \n2.6.3.420.g7bbb372\n"},{"id":"273884","messageId":"20151201231701.GB13245@sigill.intra.peff.net","threadId":"40908","inReplyTo":"1448883653-9140-1-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH] Make error message after failing commit_lock_file() less confusing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-01T23:17:01Z","receivedAt":"2015-12-01T23:17:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 30, 2015 at 12:40:53PM +0100, SZEDER Gábor wrote:\n\n> The error message after a failing commit_lock_file() call sometimes\n> looks like this, causing confusion:\n> \n> $ git remote add remote git@server.com/repo.git\n> error: could not commit config file .git/config\n> # Huh?!\n> # I didn't want to commit anything, especially not my config file!\n\nI like the intent of this patch; I've had the same \"huh\" moment myself.\n\n> The error message is of course bikeshedable.\n\nYou chose \"write\", which I think is OK. It's really a \"rename\", and\nmaybe that matters for some values of errno. I'd guess in practice\nprobably not (the likely reason is going to be something like EPERM).\nAnd I can't think of a concise way to express rename (just saying\n\"rename\" is confusing, too, without indicating that it's from the\ntempfile to the final resting place).\n\nSo perhaps \"write\" is the best we can do.\n\n-Peff\n"},{"id":"273896","messageId":"xmqqy4dd618u.fsf@gitster.mtv.corp.google.com","threadId":"40908","inReplyTo":"20151201231701.GB13245@sigill.intra.peff.net","subject":"Re: [PATCH] Make error message after failing commit_lock_file() less confusing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-02T02:19:29Z","receivedAt":"2015-12-02T02:19:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Nov 30, 2015 at 12:40:53PM +0100, SZEDER Gábor wrote:\n>\n>> The error message after a failing commit_lock_file() call sometimes\n>> looks like this, causing confusion:\n>> \n>> $ git remote add remote git@server.com/repo.git\n>> error: could not commit config file .git/config\n>> # Huh?!\n>> # I didn't want to commit anything, especially not my config file!\n>\n> I like the intent of this patch; I've had the same \"huh\" moment myself.\n>\n>> The error message is of course bikeshedable.\n>\n> You chose \"write\", which I think is OK. It's really a \"rename\", and\n> maybe that matters for some values of errno. I'd guess in practice\n> probably not (the likely reason is going to be something like EPERM).\n> And I can't think of a concise way to express rename (just saying\n> \"rename\" is confusing, too, without indicating that it's from the\n> tempfile to the final resting place).\n>\n> So perhaps \"write\" is the best we can do.\n\nYeah, \"finalize\" came to me but \"write\" is far easier to understand\nby laypeople.\n"},{"id":"273900","messageId":"CAPig+cROdVgf=EL3jKyyPpLiRA_JyHD8P336-YDCWhK8nGSQaQ@mail.gmail.com","threadId":"40908","inReplyTo":"1448883653-9140-1-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH] Make error message after failing commit_lock_file() less confusing","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-02T04:28:09Z","receivedAt":"2015-12-02T04:28:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Nov 30, 2015 at 6:40 AM, SZEDER Gábor wrote:\n> The error message after a failing commit_lock_file() call sometimes\n> looks like this, causing confusion:\n>\n> $ git remote add remote git@server.com/repo.git\n> error: could not commit config file .git/config\n> # Huh?!\n> # I didn't want to commit anything, especially not my config file!\n>\n> While in the narrow context of the lockfile module using the verb\n> 'commit' in the error message makes perfect sense, in the broader\n> context of git the word 'commit' already has a very specific meaning,\n> hence the confusion.\n>\n> Reword these error messages to say \"could not write\" instead of \"could\n> not commit\".\n>\n> While at it, include strerror in the error messages after writing the\n> config file or the credential store fails to provide some information\n> about the cause of the failure, and update the style of the error\n> message after writing the reflog fails to match surrounding error\n> messages (i.e. no '' around the pathname and no () around the error\n> description).\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> diff --git a/config.c b/config.c\n> @@ -64,7 +64,8 @@ static void rewrite_credential_file(const char *fn, struct credential *c,\n> print_line(extra);\n> parse_credential_file(fn, c, NULL, print_line);\n> if (commit_lock_file(&credential_lock) < 0)\n> - die_errno(\"unable to commit credential store\");\n> + die_errno(\"unable to write credential store: %s\",\n> + strerror(errno));\n\nHmm, this is already calling die_errno(), so adding another strerror()\nto the mix seems superfluous.\n\n> }\n>\n> static void store_credential_file(const char *fn, struct credential *c)\n> diff --git a/fast-import.c b/fast-import.c\n> @@ -1824,7 +1824,7 @@ static void dump_marks(void)\n>\n> dump_marks_helper(f, 0, marks);\n> if (commit_lock_file(&mark_lock)) {\n> - failure |= error(\"Unable to commit marks file %s: %s\",\n> + failure |= error(\"Unable to write file %s: %s\",\n> export_marks_file, strerror(errno));\n\nSince you're already doing some normalization of the error messages\nwith this patch, do you want to drop capitalization on this one?\n\n> return;\n> }\n"},{"id":"273969","messageId":"20151203113034.Horde.-yFsRNWlpgbiVEym_u1l1wY@webmail.informatik.kit.edu","threadId":"40908","inReplyTo":"CAPig+cROdVgf=EL3jKyyPpLiRA_JyHD8P336-YDCWhK8nGSQaQ@mail.gmail.com","subject":"Re: [PATCH] Make error message after failing commit_lock_file() less confusing","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2015-12-03T10:30:34Z","receivedAt":"2015-12-03T10:30:34Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nQuoting Eric Sunshine :\n\n>> diff --git a/config.c b/config.c\n>> @@ -64,7 +64,8 @@ static void rewrite_credential_file(const char \n>> *fn, struct credential *c,\n>> print_line(extra);\n>> parse_credential_file(fn, c, NULL, print_line);\n>> if (commit_lock_file(&credential_lock) < 0)\n>> - die_errno(\"unable to commit credential store\");\n>> + die_errno(\"unable to write credential store: %s\",\n>> + strerror(errno));\n>\n> Hmm, this is already calling die_errno(), so adding another strerror()\n> to the mix seems superfluous.\n\nOops, of course.\n\n>> }\n>>\n>> static void store_credential_file(const char *fn, struct credential *c)\n>> diff --git a/fast-import.c b/fast-import.c\n>> @@ -1824,7 +1824,7 @@ static void dump_marks(void)\n>>\n>> dump_marks_helper(f, 0, marks);\n>> if (commit_lock_file(&mark_lock)) {\n>> - failure |= error(\"Unable to commit marks file %s: %s\",\n>> + failure |= error(\"Unable to write file %s: %s\",\n>> export_marks_file, strerror(errno));\n>\n> Since you're already doing some normalization of the error messages\n> with this patch, do you want to drop capitalization on this one?\n\nThe neighboring error messages are capitalized as well, as are many \n(but not all) other error and warning messages in fast-import.c.\n"},{"id":"273970","messageId":"1449138687-23675-1-git-send-email-szeder@ira.uka.de","threadId":"40908","inReplyTo":"20151203113034.Horde.-yFsRNWlpgbiVEym_u1l1wY@webmail.informatik.kit.edu","subject":"[PATCH v2] Make error message after failing commit_lock_file() less confusing","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2015-12-03T10:31:27Z","receivedAt":"2015-12-03T10:31:27Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The error message after a failing commit_lock_file() call sometimes\nlooks like this, causing confusion:\n\n $ git remote add remote git@server.com/repo.git\n error: could not commit config file .git/config\n # Huh?!\n # I didn't want to commit anything, especially not my config file!\n\nWhile in the narrow context of the lockfile module using the verb\n'commit' in the error message makes perfect sense, in the broader\ncontext of git the word 'commit' already has a very specific meaning,\nhence the confusion.\n\nReword these error messages to say \"could not write\" instead of \"could\nnot commit\".\n\nWhile at it, include strerror in the error messages after writing the\nconfig file fails, to provide some information about the cause of the\nfailure, and update the style of the error message after writing the\nreflog fails, to match surrounding error messages (i.e. no '' around\nthe pathname and no () around the error description).\n\nSigned-off-by: SZEDER Gábor \n---\n\nNotes:\n Changes since v1:\n * Don't pass strerror() to die_errno().\n\n config.c | 6 ++++--\n credential-store.c | 2 +-\n fast-import.c | 2 +-\n refs.c | 2 +-\n 4 files changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 248a21ab94..86a5eb2571 100644\n--- a/config.c\n+++ b/config.c\n@@ -2144,7 +2144,8 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \t}\n \n \tif (commit_lock_file(lock) < 0) {\n-\t\terror(\"could not commit config file %s\", config_filename);\n+\t\terror(\"could not write config file %s: %s\", config_filename,\n+\t\t strerror(errno));\n \t\tret = CONFIG_NO_WRITE;\n \t\tlock = NULL;\n \t\tgoto out_free;\n@@ -2330,7 +2331,8 @@ int git_config_rename_section_in_file(const char *config_filename,\n \tfclose(config_file);\n unlock_and_out:\n \tif (commit_lock_file(lock) < 0)\n-\t\tret = error(\"could not commit config file %s\", config_filename);\n+\t\tret = error(\"could not write config file %s: %s\",\n+\t\t\t config_filename, strerror(errno));\n out:\n \tfree(filename_buf);\n \treturn ret;\ndiff --git a/credential-store.c b/credential-store.c\nindex 00aea3aa30..54c4e04737 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -64,7 +64,7 @@ static void rewrite_credential_file(const char *fn, struct credential *c,\n \t\tprint_line(extra);\n \tparse_credential_file(fn, c, NULL, print_line);\n \tif (commit_lock_file(&credential_lock) < 0)\n-\t\tdie_errno(\"unable to commit credential store\");\n+\t\tdie_errno(\"unable to write credential store\");\n }\n \n static void store_credential_file(const char *fn, struct credential *c)\ndiff --git a/fast-import.c b/fast-import.c\nindex e3b421d514..3c65edb5c4 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1824,7 +1824,7 @@ static void dump_marks(void)\n \n \tdump_marks_helper(f, 0, marks);\n \tif (commit_lock_file(&mark_lock)) {\n-\t\tfailure |= error(\"Unable to commit marks file %s: %s\",\n+\t\tfailure |= error(\"Unable to write file %s: %s\",\n \t\t\texport_marks_file, strerror(errno));\n \t\treturn;\n \t}\ndiff --git a/refs.c b/refs.c\nindex 132eff52ca..b1c7b229b7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -4684,7 +4684,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,\n \t\t\t\t\tget_lock_file_path(lock->lk));\n \t\t\trollback_lock_file(&reflog_lock);\n \t\t} else if (commit_lock_file(&reflog_lock)) {\n-\t\t\tstatus |= error(\"unable to commit reflog '%s' (%s)\",\n+\t\t\tstatus |= error(\"unable to write reflog %s: %s\",\n \t\t\t\t\tlog_file, strerror(errno));\n \t\t} else if (update && commit_ref(lock)) {\n \t\t\tstatus |= error(\"couldn't set %s\", lock->ref_name);\n-- \n2.6.3.420.g7bbb372\n"},{"id":"274563","messageId":"1450264975-9760-1-git-send-email-szeder@ira.uka.de","threadId":"40908","inReplyTo":"1449138687-23675-1-git-send-email-szeder@ira.uka.de","subject":"[PATCH] credential-store: don't pass strerror to die_errno()","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2015-12-16T11:22:55Z","receivedAt":"2015-12-16T11:22:55Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Signed-off-by: SZEDER Gábor \n---\n\nv2 fixed this, but it fell on the floor, I suppose because of the\nmaintainer switch. Anyway, I should have noticed it while the patch\nwas still cooking, sorry.\n\n credential-store.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/credential-store.c b/credential-store.c\nindex fc67d16c1088..54c4e0473737 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -64,8 +64,7 @@ static void rewrite_credential_file(const char *fn, struct credential *c,\n \t\tprint_line(extra);\n \tparse_credential_file(fn, c, NULL, print_line);\n \tif (commit_lock_file(&credential_lock) < 0)\n-\t\tdie_errno(\"unable to write credential store: %s\",\n-\t\t\t strerror(errno));\n+\t\tdie_errno(\"unable to write credential store\");\n }\n \n static void store_credential_file(const char *fn, struct credential *c)\n-- \n2.7.0.rc0.37.g77d69b9\n"},{"id":"274595","messageId":"20151216182016.GA31510@sigill.intra.peff.net","threadId":"40908","inReplyTo":"1450264975-9760-1-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH] credential-store: don't pass strerror to die_errno()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-16T18:20:16Z","receivedAt":"2015-12-16T18:20:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 16, 2015 at 12:22:55PM +0100, SZEDER Gábor wrote:\n\n> Signed-off-by: SZEDER Gábor \n> ---\n> \n> v2 fixed this, but it fell on the floor, I suppose because of the\n> maintainer switch. Anyway, I should have noticed it while the patch\n> was still cooking, sorry.\n\nOops. I do remember seeing Eric point out the problem and I think I was\nplanning to fix it up before merging. But I didn't make a note in\nwhats-cooking, so Junio had no idea of my plan.\n\n> credential-store.c | 3 +--\n> 1 file changed, 1 insertion(+), 2 deletions(-)\n\nPatch is obviously correct. Thanks.\n\n-Peff\n"}]}