threads / patch / 28938

patchrevert: prettify fatal messages

Subject: [PATCH] revert: prettify fatal messages

## tl;dr

5 messages between Nov 15, 2011 and Nov 16, 2011. Diffs are folded; open one to read it.

replies: 4people: 4as markdown or json

Ramkumar Ramachandra· Nov 15, 2011, 09:31 UTC · lore

Some of the fatal messages printed by revert and cherry-pick look ugly like the following:

  fatal: Could not open .git/sequencer/todo.: No such file or directory

The culprit here is the die_errno() function that takes a custom error message string as an argument and appends ": <message from errno>" before printing it. So, we can fix the problem by not terminating the string argument to the function with a '.' (period). Fatal messages look nicer now:

  fatal: Could not open .git/sequencer/todo: No such file or directory
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Candidate for 'maint'?
 builtin/revert.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)
Show changes to builtin/revert.c +5 −5
diff --git a/builtin/revert.c b/builtin/revert.c
index 87df70e..e0319c8 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -331,7 +331,7 @@ static void write_message(struct strbuf *msgbuf, const char *filename)
 	int msg_fd = hold_lock_file_for_update(&msg_file, filename,
 					       LOCK_DIE_ON_ERROR);
 	if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
-		die_errno(_("Could not write to %s."), filename);
+		die_errno(_("Could not write to %s"), filename);
 	strbuf_release(msgbuf);
 	if (commit_lock_file(&msg_file) < 0)
 		die(_("Error wrapping up %s"), filename);
@@ -767,7 +767,7 @@ static void read_populate_todo(struct commit_list **todo_list,
 
 	fd = open(todo_file, O_RDONLY);
 	if (fd < 0)
-		die_errno(_("Could not open %s."), todo_file);
+		die_errno(_("Could not open %s"), todo_file);
 	if (strbuf_read(&buf, fd, 0) < 0) {
 		close(fd);
 		strbuf_release(&buf);
@@ -845,7 +845,7 @@ static int create_seq_dir(void)
 	if (file_exists(seq_dir))
 		return error(_("%s already exists."), seq_dir);
 	else if (mkdir(seq_dir, 0777) < 0)
-		die_errno(_("Could not create sequencer directory '%s'."), seq_dir);
+		die_errno(_("Could not create sequencer directory %s"), seq_dir);
 	return 0;
 }
 
@@ -859,7 +859,7 @@ static void save_head(const char *head)
 	fd = hold_lock_file_for_update(&head_lock, head_file, LOCK_DIE_ON_ERROR);
 	strbuf_addf(&buf, "%s\n", head);
 	if (write_in_full(fd, buf.buf, buf.len) < 0)
-		die_errno(_("Could not write to %s."), head_file);
+		die_errno(_("Could not write to %s"), head_file);
 	if (commit_lock_file(&head_lock) < 0)
 		die(_("Error wrapping up %s."), head_file);
 }
@@ -876,7 +876,7 @@ static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
 		die(_("Could not format %s."), todo_file);
 	if (write_in_full(fd, buf.buf, buf.len) < 0) {
 		strbuf_release(&buf);
-		die_errno(_("Could not write to %s."), todo_file);
+		die_errno(_("Could not write to %s"), todo_file);
 	}
 	if (commit_lock_file(&todo_lock) < 0) {
 		strbuf_release(&buf);

-- 
1.7.6.351.gb35ac.dirty
Matthieu Moy· Nov 15, 2011, 10:04 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH] revert: prettify fatal messages

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 12 quoted lines
> Some of the fatal messages printed by revert and cherry-pick look ugly
> like the following:
>
>   fatal: Could not open .git/sequencer/todo.: No such file or directory
>
> The culprit here is the die_errno() function that takes a custom error
> message string as an argument and appends ": <message from errno>"
> before printing it.  So, we can fix the problem by not terminating the
> string argument to the function with a '.' (period).  Fatal messages
> look nicer now:
>
>   fatal: Could not open .git/sequencer/todo: No such file or directory
Sounds (obviously) good.

(I just misread the subject line at first, and understood that this was a revert of an earlier commit, while this is actually a commit touching the "revert" part of Git)

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Ramkumar Ramachandra· Nov 16, 2011, 06:23 UTC · re: Junio C Hamano · lore

Re: [PATCH] revert: prettify fatal messages

Hi,
Matthieu Moy wrote:
> (I just misread the subject line at first, and understood that this was> a revert of an earlier commit, while this is actually a commit touching> the "revert" part of Git)

Reminds me of an earlier note Jonathan made about the git-revert commit message format. To avoid confusion, reverts should always look like the following:

  Revert [commit message]
  This reverts commit [unabbreviated sha1 hex].
Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>>  Candidate for 'maint'?
>
> Too minor to matter when 'master' is already at -rc2.

Ah, I haven't been paying much attention to these release cycles. I suppose we can put this patch in `master` after the release? I wince everytime I see these ugly fatal messages: it happens shockingly often while testing :)

Thanks.
-- Ram
Jonathan Nieder· Nov 16, 2011, 06:42 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH] revert: prettify fatal messages

Ramkumar Ramachandra wrote:
Show 6 quoted lines
> To avoid confusion, reverts should always look
> like the following:
>
>   Revert [commit message]
>
>   This reverts commit [unabbreviated sha1 hex].
Don't forget the description of why it is being reverted. :)

Actually, I think Matthieu's confusion makes a lot of sense. I don't think it matters very much, but it can sometimes be clearer to say "cherry-pick/revert: remove spurious period in error message".

Another benefit to moving the juicy part to sequencer.c, I guess.

← back to recent threads