Re: [PATCH] Make error message after failing commit_lock_file() less confusing

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Nov 30, 2015 at 6:40 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> The error message after a failing commit_lock_file() call sometimes
> looks like this, causing confusion:
>
>   $ git remote add remote git@server.com/repo.git
>   error: could not commit config file .git/config
>   # Huh?!
>   # I didn't want to commit anything, especially not my config file!
>
> While in the narrow context of the lockfile module using the verb
> 'commit' in the error message makes perfect sense, in the broader
> context of git the word 'commit' already has a very specific meaning,
> hence the confusion.
>
> Reword these error messages to say "could not write" instead of "could
> not commit".
>
> While at it, include strerror in the error messages after writing the
> config file or the credential store fails to provide some information
> about the cause of the failure, and update the style of the error
> message after writing the reflog fails to match surrounding error
> messages (i.e. no '' around the pathname and no () around the error
> description).
>
> Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
> ---
> diff --git a/config.c b/config.c
> @@ -64,7 +64,8 @@ static void rewrite_credential_file(const char *fn, struct credential *c,
>                 print_line(extra);
>         parse_credential_file(fn, c, NULL, print_line);
>         if (commit_lock_file(&credential_lock) < 0)
> -               die_errno("unable to commit credential store");
> +               die_errno("unable to write credential store: %s",
> +                         strerror(errno));

Hmm, this is already calling die_errno(), so adding another strerror()
to the mix seems superfluous.

>  }
>
>  static void store_credential_file(const char *fn, struct credential *c)
> diff --git a/fast-import.c b/fast-import.c
> @@ -1824,7 +1824,7 @@ static void dump_marks(void)
>
>         dump_marks_helper(f, 0, marks);
>         if (commit_lock_file(&mark_lock)) {
> -               failure |= error("Unable to commit marks file %s: %s",
> +               failure |= error("Unable to write file %s: %s",
>                         export_marks_file, strerror(errno));

Since you're already doing some normalization of the error messages
with this patch, do you want to drop capitalization on this one?

>                 return;
>         }