From: Johannes Schindelin <johannes.schindelin@gmx.de>
To: git@vger.kernel.org
Cc: "Johannes Schindelin" <johannes.schindelin@gmx.de>,
"Junio C Hamano" <gitster@pobox.com>, "Jeff King" <peff@peff.net>,
"Johannes Sixt" <j6t@kdbg.org>,
"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
"Eric Sunshine" <sunshine@sunshineco.com>
Subject: [PATCH v2 0/4] Finish the conversion from die("BUG: ...") to BUG()
Date: Wed, 2 May 2018 11:38:13 +0200 [thread overview]
Message-ID: <cover.1525253892.git.johannes.schindelin@gmx.de> (raw)
In-Reply-To: <20170513032414.mfrwabt4hovujde2@sigill.intra.peff.net>
The BUG() macro was introduced in this patch series:
https://public-inbox.org/git/20170513032414.mfrwabt4hovujde2@sigill.intra.peff.net
The second patch in that series converted one caller from die("BUG: ")
to use the BUG() macro.
It seems that there was no concrete plan to address the same issue in
the rest of the code base.
This patch series tries to do that.
Note: I would be very surprised if the monster commit 3/4 ("Replace all
die("BUG: ...") calls by BUG() ones") would apply cleanly on `pu` (I
develop this on top of `master`).
For that reason, the commit message contains the precise Unix shell
invocation (GNU sed semantics, not BSD sed ones, because I know that the
Git maintainer as well as the author of the patch introducing BUG() both
use Linux and not macOS or any other platform that would offer a BSD
sed). It should be straight-forward to handle merge
conflicts/non-applying patches by simply re-running that command.
Changes since v2:
- Avoided the entire discussion about the previous 2/6 (now dropped)
that prepared t1406 to handle SIGABRT by side-stepping the issue: the
ref-store test helper will no longer call abort() in BUG() calls but
exit with exit code 99 instead.
- As a consequence, I could fold 3/6 into 4/6 (i.e. *not* special-case
the refs/*.c code but do one wholesale die("BUG: ...") -> BUG()
conversion).
- Further consequence: 1/6, which added handling for ok=sigabrt to
test_must_fail, was dropped.
- I also used the opportunity to explain in the commit message of the last
commit why the localization was dropped from one die(_("BUG: ...")) call.
Johannes Schindelin (4):
test-tool: help verifying BUG() code paths
run-command: use BUG() to report bugs, not die()
Replace all die("BUG: ...") calls by BUG() ones
Convert remaining die*(BUG) messages
apply.c | 4 ++--
archive-tar.c | 2 +-
attr.c | 10 +++++-----
blame.c | 2 +-
builtin/am.c | 20 +++++++++----------
builtin/branch.c | 2 +-
builtin/cat-file.c | 4 ++--
builtin/clone.c | 2 +-
builtin/commit.c | 2 +-
builtin/config.c | 2 +-
builtin/fast-export.c | 2 +-
builtin/fsck.c | 2 +-
builtin/index-pack.c | 4 ++--
builtin/init-db.c | 2 +-
builtin/ls-files.c | 2 +-
builtin/notes.c | 8 ++++----
builtin/pack-objects.c | 4 ++--
builtin/pull.c | 2 +-
builtin/receive-pack.c | 2 +-
builtin/replace.c | 2 +-
builtin/update-index.c | 2 +-
bulk-checkin.c | 2 +-
color.c | 4 ++--
column.c | 2 +-
config.c | 12 +++++------
date.c | 2 +-
diff.c | 12 +++++------
dir-iterator.c | 2 +-
git-compat-util.h | 2 +-
grep.c | 16 +++++++--------
http.c | 8 ++++----
imap-send.c | 2 +-
lockfile.c | 2 +-
mailinfo.c | 2 +-
merge-recursive.c | 12 +++++------
notes-merge.c | 4 ++--
pack-bitmap-write.c | 2 +-
pack-bitmap.c | 6 +++---
pack-objects.c | 2 +-
packfile.c | 6 +++---
pathspec.c | 12 +++++------
pkt-line.c | 2 +-
prio-queue.c | 2 +-
ref-filter.c | 4 ++--
refs.c | 34 ++++++++++++++++----------------
refs/files-backend.c | 20 +++++++++----------
refs/iterator.c | 6 +++---
refs/packed-backend.c | 16 +++++++--------
refs/ref-cache.c | 2 +-
remote.c | 2 +-
revision.c | 4 ++--
run-command.c | 33 ++++++++++++++-----------------
setup.c | 4 ++--
sha1-lookup.c | 2 +-
sha1-name.c | 4 ++--
shallow.c | 6 +++---
sigchain.c | 2 +-
strbuf.c | 4 ++--
submodule.c | 8 ++++----
t/helper/test-example-decorate.c | 16 +++++++--------
t/helper/test-tool.c | 2 ++
tmp-objdir.c | 2 +-
trailer.c | 6 +++---
transport.c | 4 ++--
unpack-trees.c | 2 +-
usage.c | 5 +++++
vcs-svn/fast_export.c | 6 ++++--
worktree.c | 2 +-
wrapper.c | 4 ++--
wt-status.c | 20 +++++++++----------
zlib.c | 4 ++--
71 files changed, 214 insertions(+), 208 deletions(-)
base-commit: 1f1cddd558b54bb0ce19c8ace353fd07b758510d
Published-As: https://github.com/dscho/git/releases/tag/use-bug-macro-v2
Fetch-It-Via: git fetch https://github.com/dscho/git use-bug-macro-v2
Branch-diff vs v1:
1: 97894dd28d7 < -: ------- test_must_fail: support ok=sigabrt
2: 9bbfd73a8e0 < -: ------- t1406: prepare for the refs code to fail with BUG()
3: b44ce003ae6 < -: ------- refs/*: report bugs using the BUG() macro
-: ------- > 1: 6e2bfd3a6eb test-tool: help verifying BUG() code paths
4: 89539a1af3d = 2: 91ddc7ed5ce run-command: use BUG() to report bugs, not die()
5: 42584f2f7c9 ! 3: 2133197fdc9 Replace all die("BUG: ...") calls by BUG() ones
@@ -1199,6 +1199,214 @@
extra_refname = find_descendant_ref(dirname.buf, extras, skip);
if (extra_refname)
+diff --git a/refs/files-backend.c b/refs/files-backend.c
+--- a/refs/files-backend.c
++++ b/refs/files-backend.c
+@@
+ if (refs->store_flags & REF_STORE_MAIN)
+ return;
+
+- die("BUG: operation %s only allowed for main ref store", caller);
++ BUG("operation %s only allowed for main ref store", caller);
+ }
+
+ /*
+@@
+ struct files_ref_store *refs;
+
+ if (ref_store->be != &refs_be_files)
+- die("BUG: ref_store is type \"%s\" not \"files\" in %s",
++ BUG("ref_store is type \"%s\" not \"files\" in %s",
+ ref_store->be->name, caller);
+
+ refs = (struct files_ref_store *)ref_store;
+
+ if ((refs->store_flags & required_flags) != required_flags)
+- die("BUG: operation %s requires abilities 0x%x, but only have 0x%x",
++ BUG("operation %s requires abilities 0x%x, but only have 0x%x",
+ caller, required_flags, refs->store_flags);
+
+ return refs;
+@@
+ strbuf_addf(sb, "%s/logs/%s", refs->gitcommondir, refname);
+ break;
+ default:
+- die("BUG: unknown ref type %d of ref %s",
++ BUG("unknown ref type %d of ref %s",
+ ref_type(refname), refname);
+ }
+ }
+@@
+ strbuf_addf(sb, "%s/%s", refs->gitcommondir, refname);
+ break;
+ default:
+- die("BUG: unknown ref type %d of ref %s",
++ BUG("unknown ref type %d of ref %s",
+ ref_type(refname), refname);
+ }
+ }
+@@
+
+ }
+ if (!ret && sb.len)
+- die("BUG: reverse reflog parser had leftover data");
++ BUG("reverse reflog parser had leftover data");
+
+ fclose(logfp);
+ strbuf_release(&sb);
+@@
+ static int files_reflog_iterator_peel(struct ref_iterator *ref_iterator,
+ struct object_id *peeled)
+ {
+- die("BUG: ref_iterator_peel() called for reflog_iterator");
++ BUG("ref_iterator_peel() called for reflog_iterator");
+ }
+
+ static int files_reflog_iterator_abort(struct ref_iterator *ref_iterator)
+@@
+ assert(err);
+
+ if (transaction->state != REF_TRANSACTION_OPEN)
+- die("BUG: commit called for transaction that is not open");
++ BUG("commit called for transaction that is not open");
+
+ /* Fail if a refname appears more than once in the transaction: */
+ for (i = 0; i < transaction->nr; i++)
+@@
+ */
+ if (refs_for_each_rawref(&refs->base, ref_present,
+ &affected_refnames))
+- die("BUG: initial ref transaction called with existing refs");
++ BUG("initial ref transaction called with existing refs");
+
+ packed_transaction = ref_store_transaction_begin(refs->packed_ref_store, err);
+ if (!packed_transaction) {
+@@
+
+ if ((update->flags & REF_HAVE_OLD) &&
+ !is_null_oid(&update->old_oid))
+- die("BUG: initial ref transaction with old_sha1 set");
++ BUG("initial ref transaction with old_sha1 set");
+ if (refs_verify_refname_available(&refs->base, update->refname,
+ &affected_refnames, NULL,
+ err)) {
+
+diff --git a/refs/iterator.c b/refs/iterator.c
+--- a/refs/iterator.c
++++ b/refs/iterator.c
+@@
+ static int empty_ref_iterator_peel(struct ref_iterator *ref_iterator,
+ struct object_id *peeled)
+ {
+- die("BUG: peel called for empty iterator");
++ BUG("peel called for empty iterator");
+ }
+
+ static int empty_ref_iterator_abort(struct ref_iterator *ref_iterator)
+@@
+ (struct merge_ref_iterator *)ref_iterator;
+
+ if (!iter->current) {
+- die("BUG: peel called before advance for merge iterator");
++ BUG("peel called before advance for merge iterator");
+ }
+ return ref_iterator_peel(*iter->current, peeled);
+ }
+@@
+ * trimming, report it as a bug:
+ */
+ if (strlen(iter->iter0->refname) <= iter->trim)
+- die("BUG: attempt to trim too many characters");
++ BUG("attempt to trim too many characters");
+ iter->base.refname = iter->iter0->refname + iter->trim;
+ } else {
+ iter->base.refname = iter->iter0->refname;
+
+diff --git a/refs/packed-backend.c b/refs/packed-backend.c
+--- a/refs/packed-backend.c
++++ b/refs/packed-backend.c
+@@
+ struct packed_ref_store *refs;
+
+ if (ref_store->be != &refs_be_packed)
+- die("BUG: ref_store is type \"%s\" not \"packed\" in %s",
++ BUG("ref_store is type \"%s\" not \"packed\" in %s",
+ ref_store->be->name, caller);
+
+ refs = (struct packed_ref_store *)ref_store;
+
+ if ((refs->store_flags & required_flags) != required_flags)
+- die("BUG: unallowed operation (%s), requires %x, has %x\n",
++ BUG("unallowed operation (%s), requires %x, has %x\n",
+ caller, required_flags, refs->store_flags);
+
+ return refs;
+@@
+ "packed_refs_unlock");
+
+ if (!is_lock_file_locked(&refs->lock))
+- die("BUG: packed_refs_unlock() called when not locked");
++ BUG("packed_refs_unlock() called when not locked");
+ rollback_lock_file(&refs->lock);
+ }
+
+@@
+ char *packed_refs_path;
+
+ if (!is_lock_file_locked(&refs->lock))
+- die("BUG: write_with_updates() called while unlocked");
++ BUG("write_with_updates() called while unlocked");
+
+ /*
+ * If packed-refs is a symlink, we want to overwrite the
+@@
+ const char *refname, const char *target,
+ const char *logmsg)
+ {
+- die("BUG: packed reference store does not support symrefs");
++ BUG("packed reference store does not support symrefs");
+ }
+
+ static int packed_rename_ref(struct ref_store *ref_store,
+ const char *oldrefname, const char *newrefname,
+ const char *logmsg)
+ {
+- die("BUG: packed reference store does not support renaming references");
++ BUG("packed reference store does not support renaming references");
+ }
+
+ static int packed_copy_ref(struct ref_store *ref_store,
+ const char *oldrefname, const char *newrefname,
+ const char *logmsg)
+ {
+- die("BUG: packed reference store does not support copying references");
++ BUG("packed reference store does not support copying references");
+ }
+
+ static struct ref_iterator *packed_reflog_iterator_begin(struct ref_store *ref_store)
+@@
+ const char *refname, int force_create,
+ struct strbuf *err)
+ {
+- die("BUG: packed reference store does not support reflogs");
++ BUG("packed reference store does not support reflogs");
+ }
+
+ static int packed_delete_reflog(struct ref_store *ref_store,
+
+diff --git a/refs/ref-cache.c b/refs/ref-cache.c
+--- a/refs/ref-cache.c
++++ b/refs/ref-cache.c
+@@
+ dir = &entry->u.subdir;
+ if (entry->flag & REF_INCOMPLETE) {
+ if (!dir->cache->fill_ref_dir)
+- die("BUG: incomplete ref_store without fill_ref_dir function");
++ BUG("incomplete ref_store without fill_ref_dir function");
+
+ dir->cache->fill_ref_dir(dir->cache->ref_store, dir, entry->name);
+ entry->flag &= ~REF_INCOMPLETE;
+
diff --git a/remote.c b/remote.c
--- a/remote.c
+++ b/remote.c
6: 0e85c7a16e3 ! 4: 57b3a3e9fe3 Convert remaining die*(BUG) messages
@@ -4,6 +4,12 @@
These were not caught by the previous commit, as they did not match the
regular expression.
+
+ While at it, remove the localization from one instance: we never want
+ BUG() messages to be translated, as they target Git developers, not the
+ end user (hence it would be quite unhelpful to not only burden the
+ translators, but then even end up with a bug report in a language that
+ no core Git contributor understands).
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
--
2.17.0.windows.1.36.gdf4ca5fb72a
next prev parent reply other threads:[~2018-05-02 9:38 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-05-12 14:19 [Git 2.13.0] BUG: setup_git_env called without repository Josh Hagins
2017-05-12 14:48 ` Johannes Schindelin
2017-05-15 0:22 ` Josh Hagins
2017-05-12 20:34 ` [PATCH] config: complain about --local outside of a git repo Jeff King
2017-05-12 22:31 ` Ævar Arnfjörð Bjarmason
2017-05-13 2:03 ` Jeff King
2017-05-13 3:24 ` [PATCH 0/3] BUG() and "config --local" outside of repo Jeff King
2017-05-13 3:28 ` [PATCH 1/3] usage.c: add BUG() function Jeff King
2017-05-13 3:55 ` Jeff King
2017-05-15 2:28 ` Junio C Hamano
2017-05-13 3:29 ` [PATCH 2/3] setup_git_env: convert die("BUG") to BUG() Jeff King
2017-05-13 3:29 ` [PATCH 3/3] config: complain about --local outside of a git repo Jeff King
2018-05-02 9:38 ` Johannes Schindelin [this message]
2018-05-02 9:38 ` [PATCH v2 1/4] test-tool: help verifying BUG() code paths Johannes Schindelin
2018-05-02 15:18 ` Duy Nguyen
2018-05-05 19:30 ` Johannes Schindelin
2018-05-02 9:38 ` [PATCH v2 2/4] run-command: use BUG() to report bugs, not die() Johannes Schindelin
2018-05-07 9:08 ` Jeff King
2018-05-02 9:38 ` [PATCH v2 3/4] Replace all die("BUG: ...") calls by BUG() ones Johannes Schindelin
2018-05-02 9:38 ` [PATCH v2 4/4] Convert remaining die*(BUG) messages Johannes Schindelin
2018-05-07 9:01 ` [PATCH v2 0/4] Finish the conversion from die("BUG: ...") to BUG() Jeff King
2017-05-13 0:04 ` [PATCH] config: complain about --local outside of a git repo Jonathan Nieder
2017-05-13 2:04 ` Jeff King
2017-05-15 0:31 ` Josh Hagins
2017-05-15 3:18 ` Jeff King
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cover.1525253892.git.johannes.schindelin@gmx.de \
--to=johannes.schindelin@gmx.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=j6t@kdbg.org \
--cc=pclouds@gmail.com \
--cc=peff@peff.net \
--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.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).