From: Jonathan Nieder <jrnieder@gmail.com>
To: Stefan Beller <sbeller@google.com>
Cc: pclouds@gmail.com, gitster@pobox.com, git@vger.kernel.org,
sunshine@sunshineco.com, mhagger@alum.mit.edu,
ronniesahlberg@gmail.com
Subject: Re: [PATCHv10 02/10] receive-pack.c: die instead of error in assure_connectivity_checked
Date: Mon, 5 Jan 2015 12:17:29 -0800 [thread overview]
Message-ID: <20150105201728.GK29365@google.com> (raw)
In-Reply-To: <1420482355-24995-2-git-send-email-sbeller@google.com>
Stefan Beller wrote:
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1055,15 +1055,15 @@ static void assure_connectivity_checked(struct command *commands,
>
> for (cmd = commands; cmd; cmd = cmd->next) {
> if (should_process_cmd(cmd) && si->shallow_ref[cmd->index]) {
> - error("BUG: connectivity check has not been run on ref %s",
> - cmd->ref_name);
> + die("BUG: connectivity check has not been run on ref %s",
> + cmd->ref_name);
This could stay as error() so that the caller gets to see the list of
refs that didn't experience a connectivity check. Or if that list
isn't important, this error/die call could be dropped and the
'checked_connectivity = 0' setting would be enough.
> checked_connectivity = 0;
> }
> }
> if (!checked_connectivity)
> - error("BUG: run 'git fsck' for safety.\n"
> - "If there are errors, try to remove "
> - "the reported refs above");
> + die("BUG: run 'git fsck' for safety.\n"
> + "If there are errors, try to remove "
> + "the reported refs above");
I find this error message misleading and confusing. It makes it seem
like this is an expected error that we are trying to help the user
recover from. Instead, something impossible has happened. "Try to
remove the reported refs" is actively harmful advice --- that would be
a way for the user to work around a serious bug instead of figuring
out what went wrong and getting git fixed.
Jonathan
next prev parent reply other threads:[~2015-01-05 20:17 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-12-30 23:41 [PATCH 0/9] atomic pushes Stefan Beller
2014-12-30 23:41 ` [PATCHv9 1/9] receive-pack.c: shorten the execute_commands loop over all commands Stefan Beller
2015-01-03 2:20 ` Jonathan Nieder
2015-01-03 9:53 ` Duy Nguyen
2015-01-05 18:02 ` Stefan Beller
2015-01-05 18:25 ` [PATCHv10 01/10] " Stefan Beller
2015-01-05 18:25 ` [PATCHv10 02/10] receive-pack.c: die instead of error in assure_connectivity_checked Stefan Beller
2015-01-05 20:17 ` Jonathan Nieder [this message]
2015-01-05 21:15 ` Stefan Beller
2015-01-05 21:25 ` Jonathan Nieder
2015-01-06 19:40 ` [PATCHv11 02/11] receive-pack.c: die instead of error in case of possible future bug Stefan Beller
2015-01-06 19:46 ` Jonathan Nieder
2015-01-05 20:22 ` [PATCHv10 01/10] receive-pack.c: shorten the execute_commands loop over all commands Jonathan Nieder
2015-01-05 21:07 ` Stefan Beller
2015-01-05 21:18 ` Jonathan Nieder
2015-01-06 19:34 ` [PATCHv11 01/11] " Stefan Beller
2014-12-30 23:41 ` [PATCHv9 2/9] receive-pack.c: move iterating over all commands outside execute_commands Stefan Beller
2014-12-30 23:41 ` [PATCHv9 3/9] receive-pack.c: move transaction handling in a central place Stefan Beller
2014-12-30 23:41 ` [PATCHv9 4/9] receive-pack.c: add execute_commands_atomic function Stefan Beller
2014-12-30 23:41 ` [PATCHv9 5/9] receive-pack.c: negotiate atomic push support Stefan Beller
2014-12-30 23:41 ` [PATCHv9 6/9] send-pack: rename ref_update_to_be_sent to check_to_send_update Stefan Beller
2014-12-30 23:41 ` [PATCHv9 7/9] send-pack.c: add --atomic command line argument Stefan Beller
2014-12-30 23:41 ` [PATCHv9 8/9] push.c: add an --atomic argument Stefan Beller
2014-12-30 23:41 ` [PATCHv9 9/9] t5543-atomic-push.sh: add basic tests for atomic pushes Stefan Beller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150105201728.GK29365@google.com \
--to=jrnieder@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=mhagger@alum.mit.edu \
--cc=pclouds@gmail.com \
--cc=ronniesahlberg@gmail.com \
--cc=sbeller@google.com \
--cc=sunshine@sunshineco.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).