{"thread":{"id":"50877","subject":"[PATCH 0/7] harden unexpected object types checks","startedAt":"2019-04-05T03:37:43Z","lastAt":"2019-04-10T02:13:30Z","messageCount":41,"participants":["Taylor Blau","SZEDER Gábor","Jeff King","Eric Sunshine","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"373172","messageId":"cover.1554435033.git.me@ttaylorr.com","threadId":"50877","inReplyTo":null,"subject":"[PATCH 0/7] harden unexpected object types checks","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-04-05T03:37:39Z","receivedAt":"2019-04-05T03:37:43Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi everybody,\n\nPeff pointed out to me a couple of weeks ago that we could reproducibly\ncrash Git when doing the following:\n\n  $ git rev-list --objects <blob> <broken-tree>\n\nWhere <blob> is a normal blob, and <broken-tree> is a tree which\ncontains an entry that refers to <blob> but gives it a type other than\n'blob'. (This is described in detail in 2/7 and fixed in 3/7.)\n\nWe decided to continue, trying to come up with more tests that exercise\nsimilar object corruption, and the tests\n't6102-rev-list-unexpected-objects.sh' are what we came up with.\n\nThe series goes as follows:\n\n  1. Prepare ourselves by moving a helper in 't' into\n     test-lib-functions.sh so that we can use it in a new location.\n\n  2. Write out a handful of tests that exercises cases similar to the\n     above, and mark the ones with bugs as 'test_expect_failure'.\n\n  3. Fix (most) of them in each subsequent commit.\n\nThe exception we make for step (3) is that don't provide a complete fix,\nonly restore behavior to before the commit at which it regressed.\n\nI'll be brief here, since most of the detail is described at length in\nthe patches themselves. This said, please do ask questions where I\nwasn't clear, or could have been clearer. (This series grew larger than\nI originally expected it to, so perhaps there is detail that I\naccumulated and didn't devote enough time to).\n\nThanks as always in advance for your review.\n\n\nJeff King (3):\n  get_commit_tree(): return NULL for broken tree\n  rev-list: let traversal die when --missing is not in use\n  rev-list: detect broken root trees\n\nTaylor Blau (4):\n  t: move 'hex2oct' into test-lib-functions.sh\n  t: introduce tests for unexpected object types\n  list-objects.c: handle unexpected non-blob entries\n  list-objects.c: handle unexpected non-tree entries\n\n builtin/rev-list.c                     |   4 +-\n commit.c                               |   6 +-\n list-objects.c                         |  13 +++\n t/t1007-hash-object.sh                 |   4 -\n t/t1450-fsck.sh                        |   4 -\n t/t5601-clone.sh                       |   4 -\n t/t6102-rev-list-unexpected-objects.sh | 127 +++++++++++++++++++++++++\n t/test-lib-functions.sh                |   6 ++\n 8 files changed, 152 insertions(+), 16 deletions(-)\n create mode 100755 t/t6102-rev-list-unexpected-objects.sh\n\n--\n2.21.0.203.g358da99528\n"},{"id":"373173","messageId":"f09c374557070b286391f1f69c4288d7d0769851.1554435033.git.me@ttaylorr.com","threadId":"50877","inReplyTo":"cover.1554435033.git.me@ttaylorr.com","subject":"[PATCH 1/7] t: move 'hex2oct' into test-lib-functions.sh","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-04-05T03:37:42Z","receivedAt":"2019-04-05T03:37:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The helper 'hex2oct' is used to convert base-16 encoded data into a\nbase-8 binary form, and is useful for preparing data for commands that\naccept input in a binary format, such as 'git hash-object', via\n'printf'.\n\nThis helper is defined identically in three separate places throughout\n't'. Move the definition to test-lib-function.sh, so that it can be used\nin new test suites, and its definition is not redundant.\n\nThis will likewise make our job easier in the subsequent commit, which\nalso uses 'hex2oct'.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/t1007-hash-object.sh  | 4 ----\n t/t1450-fsck.sh         | 4 ----\n t/t5601-clone.sh        | 4 ----\n t/test-lib-functions.sh | 6 ++++++\n 4 files changed, 6 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh\nindex a37753047e..7099d33508 100755\n--- a/t/t1007-hash-object.sh\n+++ b/t/t1007-hash-object.sh\n@@ -199,10 +199,6 @@ test_expect_success 'too-short tree' '\n \ttest_i18ngrep \"too-short tree object\" err\n '\n \n-hex2oct() {\n-    perl -ne 'printf \"\\\\%03o\", hex for /../g'\n-}\n-\n test_expect_success 'malformed mode in tree' '\n \thex_sha1=$(echo foo | git hash-object --stdin -w) &&\n \tbin_sha1=$(echo $hex_sha1 | hex2oct) &&\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 49f08d5b9c..0f268a3664 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -256,10 +256,6 @@ test_expect_success 'unparseable tree object' '\n \ttest_i18ngrep ! \"fatal: empty filename in tree entry\" out\n '\n \n-hex2oct() {\n-\tperl -ne 'printf \"\\\\%03o\", hex for /../g'\n-}\n-\n test_expect_success 'tree entry with type mismatch' '\n \ttest_when_finished \"remove_object \\$blob\" &&\n \ttest_when_finished \"remove_object \\$tree\" &&\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex d6948cbdab..3f49943010 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -611,10 +611,6 @@ test_expect_success 'GIT_TRACE_PACKFILE produces a usable pack' '\n \tgit -C replay.git index-pack -v --stdin <tmp.pack\n '\n \n-hex2oct () {\n-\tperl -ne 'printf \"\\\\%03o\", hex for /../g'\n-}\n-\n test_expect_success 'clone on case-insensitive fs' '\n \tgit init icasefs &&\n \t(\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 80402a428f..349eabe851 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1202,6 +1202,12 @@ depacketize () {\n \t'\n }\n \n+# Converts base-16 data into base-8. The output is given as a sequence of\n+# escaped octals, suitable for consumption by 'printf'.\n+hex2oct () {\n+\tperl -ne 'printf \"\\\\%03o\", hex for /../g'\n+}\n+\n # Set the hash algorithm in use to $1.  Only useful when testing the testsuite.\n test_set_hash () {\n \ttest_hash_algo=\"$1\"\n-- \n2.21.0.203.g358da99528\n\n"},{"id":"373174","messageId":"ef6b4f380435d9743a56f47f68c18123bf0a0933.1554435033.git.me@ttaylorr.com","threadId":"50877","inReplyTo":"cover.1554435033.git.me@ttaylorr.com","subject":"[PATCH 2/7] t: introduce tests for unexpected object types","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-04-05T03:37:44Z","receivedAt":"2019-04-05T03:37:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Call an object's type \"unexpected\" when the actual type of an object\ndoes not match Git's contextual expectation. For example, a tree entry\nwhose mode differs from the object's actual type, or a commit's parent\nwhich is not another commit, and so on.\n\nThis can manifest itself in various unfortunate ways, including Git\nSIGSEGV-ing under specific conditions. Consider the following example:\nGit traverses a blob (say, via `git rev-list`), and then tries to read\nout a tree-entry which lists that object as something other than a blob.\nIn this case, `lookup_blob()` will return NULL, and the subsequent\ndereference will result in a SIGSEGV.\n\nIntroduce tests that present objects of \"unexpected\" type in the above\nfashion to 'git rev-list'. Mark as failures the combinations that are\nalready broken (i.e., they exhibit the segfault described above). In the\ncases that are not broken (i.e., they have NULL-ness checks or similar),\nmark these as expecting success.\n\nLet A be the object referenced with an unexpected type, and B be the\nobject doing the referencing. Do the following:\n\n  - test 'git rev-list --objects A B'. This causes A to be \"cached\", and\n    presents the above scenario.\n\nLikewise, if we have a tree entry that claims to be a tree (for example)\nbut points to another object type (say, a blob), there are two ways we\nmight find out:\n\n  - when we call lookup_tree(), we might find that we've already seen\n    the object referenced as another type, in which case we'd get NULL\n\n  - we call lookup_tree() successfully, but when we try to read the\n    object, we find out it's something else.\n\nWe should check that we behave sensibly in both cases (especially\nbecause it is easy for a malicious actor to provoke one case or the\nother).\n\nCo-authored-by: Jeff King <peff@peff.net>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t6102-rev-list-unexpected-objects.sh | 123 +++++++++++++++++++++++++\n 1 file changed, 123 insertions(+)\n create mode 100755 t/t6102-rev-list-unexpected-objects.sh\n\ndiff --git a/t/t6102-rev-list-unexpected-objects.sh b/t/t6102-rev-list-unexpected-objects.sh\nnew file mode 100755\nindex 0000000000..472b08528a\n--- /dev/null\n+++ b/t/t6102-rev-list-unexpected-objects.sh\n@@ -0,0 +1,123 @@\n+#!/bin/sh\n+\n+test_description='git rev-list should handle unexpected object types'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup well-formed objects' '\n+\tblob=\"$(printf \"foo\" | git hash-object -w --stdin)\" &&\n+\ttree=\"$(printf \"100644 blob $blob\\tfoo\" | git mktree)\" &&\n+\tcommit=\"$(git commit-tree $tree -m \"first commit\")\"\n+'\n+\n+test_expect_success 'setup unexpected non-blob entry' '\n+\tprintf \"100644 foo\\0$(echo $tree | hex2oct)\" >broken-tree &&\n+\tbroken_tree=\"$(git hash-object -w --literally -t tree broken-tree)\"\n+'\n+\n+test_expect_failure 'traverse unexpected non-blob entry (lone)' '\n+\ttest_must_fail git rev-list --objects $broken_tree\n+'\n+\n+test_expect_failure 'traverse unexpected non-blob entry (seen)' '\n+\ttest_must_fail git rev-list --objects $tree $broken_tree\n+'\n+\n+test_expect_success 'setup unexpected non-tree entry' '\n+\tprintf \"40000 foo\\0$(echo $blob | hex2oct)\" >broken-tree &&\n+\tbroken_tree=\"$(git hash-object -w --literally -t tree broken-tree)\"\n+'\n+\n+test_expect_failure 'traverse unexpected non-tree entry (lone)' '\n+\ttest_must_fail git rev-list --objects $broken_tree\n+'\n+\n+test_expect_failure 'traverse unexpected non-tree entry (seen)' '\n+\ttest_must_fail git rev-list --objects $blob $broken_tree >output 2>&1\n+'\n+\n+test_expect_success 'setup unexpected non-commit parent' '\n+\tgit cat-file commit $commit |\n+\t\tperl -lpe \"/^author/ && print q(parent $blob)\" \\\n+\t\t>broken-commit &&\n+\tbroken_commit=\"$(git hash-object -w --literally -t commit \\\n+\t\tbroken-commit)\"\n+'\n+\n+test_expect_success 'traverse unexpected non-commit parent (lone)' '\n+\ttest_must_fail git rev-list --objects $broken_commit >output 2>&1 &&\n+\ttest_i18ngrep \"not a commit\" output\n+'\n+\n+test_expect_success 'traverse unexpected non-commit parent (seen)' '\n+\ttest_must_fail git rev-list --objects $commit $broken_commit \\\n+\t\t>output 2>&1 &&\n+\ttest_i18ngrep \"not a commit\" output\n+'\n+\n+test_expect_success 'setup unexpected non-tree root' '\n+\tgit cat-file commit $commit |\n+\tsed -e \"s/$tree/$blob/\" >broken-commit &&\n+\tbroken_commit=\"$(git hash-object -w --literally -t commit \\\n+\t\tbroken-commit)\"\n+'\n+\n+test_expect_failure 'traverse unexpected non-tree root (lone)' '\n+\ttest_must_fail git rev-list --objects $broken_commit\n+'\n+\n+test_expect_failure 'traverse unexpected non-tree root (seen)' '\n+\ttest_must_fail git rev-list --objects $blob $broken_commit\n+'\n+\n+test_expect_success 'setup unexpected non-commit tag' '\n+\tgit tag -a -m \"tagged commit\" tag $commit &&\n+\ttest_when_finished \"git tag -d tag\" &&\n+\tgit cat-file -p tag | sed -e \"s/$commit/$blob/\" >broken-tag &&\n+\ttag=$(git hash-object -w --literally -t tag broken-tag)\n+'\n+\n+test_expect_success 'traverse unexpected non-commit tag (lone)' '\n+\ttest_must_fail git rev-list --objects $tag\n+'\n+\n+test_expect_success 'traverse unexpected non-commit tag (seen)' '\n+\ttest_must_fail git rev-list --objects $blob $tag >output 2>&1 &&\n+\ttest_i18ngrep \"not a commit\" output\n+'\n+\n+test_expect_success 'setup unexpected non-tree tag' '\n+\tgit tag -a -m \"tagged tree\" tag $tree &&\n+\ttest_when_finished \"git tag -d tag\" &&\n+\tgit cat-file -p tag |\n+\tsed -e \"s/$tree/$blob/\" >broken-tag &&\n+\ttag=$(git hash-object -w --literally -t tag broken-tag)\n+'\n+\n+test_expect_success 'traverse unexpected non-tree tag (lone)' '\n+\ttest_must_fail git rev-list --objects $tag\n+'\n+\n+test_expect_success 'traverse unexpected non-tree tag (seen)' '\n+\ttest_must_fail git rev-list --objects $blob $tag >output 2>&1 &&\n+\ttest_i18ngrep \"not a tree\" output\n+'\n+\n+test_expect_success 'setup unexpected non-blob tag' '\n+\tgit tag -a -m \"tagged blob\" tag $blob &&\n+\ttest_when_finished \"git tag -d tag\" &&\n+\tgit cat-file -p tag |\n+\tsed -e \"s/$blob/$commit/\" >broken-tag &&\n+\ttag=$(git hash-object -w --literally -t tag broken-tag)\n+'\n+\n+test_expect_failure 'traverse unexpected non-blob tag (lone)' '\n+\ttest_must_fail git rev-list --objects $tag\n+'\n+\n+test_expect_success 'traverse unexpected non-blob tag (seen)' '\n+\ttest_must_fail git rev-list --objects $commit $tag >output 2>&1 &&\n+\ttest_i18ngrep \"not a blob\" output\n+'\n+\n+test_done\n-- \n2.21.0.203.g358da99528\n\n"},{"id":"373175","messageId":"75f20ee26def257aa45dd31cd016ee76f81dcb83.1554435033.git.me@ttaylorr.com","threadId":"50877","inReplyTo":"cover.1554435033.git.me@ttaylorr.com","subject":"[PATCH 3/7] list-objects.c: handle unexpected non-blob entries","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-04-05T03:37:46Z","receivedAt":"2019-04-05T03:37:50Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Fix one of the cases described in the previous commit where a tree-entry\nthat is promised to a blob is in fact a non-blob.\n\nWhen 'lookup_blob()' returns NULL, it is because Git has cached the\nrequested object as a non-blob. In this case, prevent a SIGSEGV by\n'die()'-ing immediately before attempting to dereference the result.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n list-objects.c                         | 5 +++++\n t/t6102-rev-list-unexpected-objects.sh | 5 +++--\n 2 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/list-objects.c b/list-objects.c\nindex dc77361e11..ea04bbdee6 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -133,6 +133,11 @@ static void process_tree_contents(struct traversal_context *ctx,\n \t\t\t\t\tbase, entry.path);\n \t\telse {\n \t\t\tstruct blob *b = lookup_blob(ctx->revs->repo, &entry.oid);\n+\t\t\tif (!b) {\n+\t\t\t\tdie(_(\"entry '%s' in tree %s has blob mode, \"\n+\t\t\t\t      \"but is not a blob\"),\n+\t\t\t\t    entry.path, oid_to_hex(&tree->object.oid));\n+\t\t\t}\n \t\t\tb->object.flags |= NOT_USER_GIVEN;\n \t\t\tprocess_blob(ctx, b, base, entry.path);\n \t\t}\ndiff --git a/t/t6102-rev-list-unexpected-objects.sh b/t/t6102-rev-list-unexpected-objects.sh\nindex 472b08528a..76fe9be30f 100755\n--- a/t/t6102-rev-list-unexpected-objects.sh\n+++ b/t/t6102-rev-list-unexpected-objects.sh\n@@ -19,8 +19,9 @@ test_expect_failure 'traverse unexpected non-blob entry (lone)' '\n \ttest_must_fail git rev-list --objects $broken_tree\n '\n \n-test_expect_failure 'traverse unexpected non-blob entry (seen)' '\n-\ttest_must_fail git rev-list --objects $tree $broken_tree\n+test_expect_success 'traverse unexpected non-blob entry (seen)' '\n+\ttest_must_fail git rev-list --objects $tree $broken_tree >output 2>&1 &&\n+\ttest_i18ngrep \"is not a blob\" output\n '\n \n test_expect_success 'setup unexpected non-tree entry' '\n-- \n2.21.0.203.g358da99528\n\n"},{"id":"373176","messageId":"6d59461830b89fab5eabae1c1b883888bd9d9fca.1554435033.git.me@ttaylorr.com","threadId":"50877","inReplyTo":"cover.1554435033.git.me@ttaylorr.com","subject":"[PATCH 4/7] list-objects.c: handle unexpected non-tree entries","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-04-05T03:37:48Z","receivedAt":"2019-04-05T03:37:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Apply similar treatment as the previous commit for non-tree entries,\ntoo.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n list-objects.c                         | 5 +++++\n t/t6102-rev-list-unexpected-objects.sh | 5 +++--\n 2 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/list-objects.c b/list-objects.c\nindex ea04bbdee6..bb7e61ef4b 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -125,6 +125,11 @@ static void process_tree_contents(struct traversal_context *ctx,\n \n \t\tif (S_ISDIR(entry.mode)) {\n \t\t\tstruct tree *t = lookup_tree(ctx->revs->repo, &entry.oid);\n+\t\t\tif (!t) {\n+\t\t\t\tdie(_(\"entry '%s' in tree %s has tree mode, \"\n+\t\t\t\t      \"but is not a tree\"),\n+\t\t\t\t    entry.path, oid_to_hex(&tree->object.oid));\n+\t\t\t}\n \t\t\tt->object.flags |= NOT_USER_GIVEN;\n \t\t\tprocess_tree(ctx, t, base, entry.path);\n \t\t}\ndiff --git a/t/t6102-rev-list-unexpected-objects.sh b/t/t6102-rev-list-unexpected-objects.sh\nindex 76fe9be30f..30976385a8 100755\n--- a/t/t6102-rev-list-unexpected-objects.sh\n+++ b/t/t6102-rev-list-unexpected-objects.sh\n@@ -33,8 +33,9 @@ test_expect_failure 'traverse unexpected non-tree entry (lone)' '\n \ttest_must_fail git rev-list --objects $broken_tree\n '\n \n-test_expect_failure 'traverse unexpected non-tree entry (seen)' '\n-\ttest_must_fail git rev-list --objects $blob $broken_tree >output 2>&1\n+test_expect_success 'traverse unexpected non-tree entry (seen)' '\n+\ttest_must_fail git rev-list --objects $blob $broken_tree >output 2>&1 &&\n+\ttest_i18ngrep \"is not a tree\" output\n '\n \n test_expect_success 'setup unexpected non-commit parent' '\n-- \n2.21.0.203.g358da99528\n\n"},{"id":"373177","messageId":"a3975f74d4a5153d1275baa4b15e0afb9741541d.1554435033.git.me@ttaylorr.com","threadId":"50877","inReplyTo":"cover.1554435033.git.me@ttaylorr.com","subject":"[PATCH 5/7] get_commit_tree(): return NULL for broken tree","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-04-05T03:37:50Z","receivedAt":"2019-04-05T03:37:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"From: Jeff King <peff@peff.net>\n\nReturn NULL from 'get_commit_tree()' when a commit's root tree is\ncorrupt, doesn't exist, or points to an object which is not a tree.\n\nIn [1], this situation became a BUG(), but it can certainly occur in\ncases which are not a bug in Git, for e.g., if a caller manually crafts\na commit whose tree is corrupt in any of the above ways.\n\nNote that the expect_failure test in t6102 triggers this BUG(), but we\ncan't flip it to expect_success yet. Solving this problem actually\nreveals a second bug.\n\n[1]: 7b8a21dba1 (commit-graph: lazy-load trees for commits, 2018-04-06)\n\nCo-authored-by: Taylor Blau <me@ttaylorr.com>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n commit.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex a5333c7ac6..e2cde566a9 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -345,10 +345,10 @@ struct tree *get_commit_tree(const struct commit *commit)\n \tif (commit->maybe_tree || !commit->object.parsed)\n \t\treturn commit->maybe_tree;\n \n-\tif (commit->graph_pos == COMMIT_NOT_FROM_GRAPH)\n-\t\tBUG(\"commit has NULL tree, but was not loaded from commit-graph\");\n+\tif (commit->graph_pos != COMMIT_NOT_FROM_GRAPH)\n+\t\treturn get_commit_tree_in_graph(the_repository, commit);\n \n-\treturn get_commit_tree_in_graph(the_repository, commit);\n+\treturn NULL;\n }\n \n struct object_id *get_commit_tree_oid(const struct commit *commit)\n-- \n2.21.0.203.g358da99528\n\n"},{"id":"373178","messageId":"a3a80b4b2a988eb65d85a5acd54c584d047073c7.1554435033.git.me@ttaylorr.com","threadId":"50877","inReplyTo":"cover.1554435033.git.me@ttaylorr.com","subject":"[PATCH 6/7] rev-list: let traversal die when --missing is not in use","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-04-05T03:37:54Z","receivedAt":"2019-04-05T03:37:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"From: Jeff King <peff@peff.net>\n\nCommit 7c0fe330d5 (rev-list: handle missing tree objects properly,\n2018-10-05) taught the traversal machinery used by git-rev-list to\nignore missing trees, so that rev-list could handle them itself.\n\nHowever, it does so only by checking via oid_object_info_extended() that\nthe object exists at all. This can miss several classes of errors that\nwere previously detected by rev-list:\n\n  - type mismatches (e.g., we expected a tree but got a blob)\n\n  - failure to read the object data (e.g., due to bitrot on disk)\n\nThis is especially important because we use \"rev-list --objects\" as our\nconnectivity check to admit new objects to the repository, and it will\nnow miss these cases (though the bitrot one is less important here,\nbecause we'd typically have just hashed and stored the object).\n\nThere are a few options to fix this:\n\n 1. we could check these properties in rev-list when we do the existence\n    check. This is probably too expensive in practice (perhaps even for\n    a type check, but definitely for checking the whole content again,\n    which implies loading each object into memory twice).\n\n 2. teach the traversal machinery to differentiate between a missing\n    object, and one that could not be loaded as expected. This probably\n    wouldn't be too hard to detect type mismatches, but detecting bitrot\n    versus a truly missing object would require deep changes to the\n    object-loading code.\n\n 3. have the traversal machinery communicate the failure to the caller,\n    so that it can decide how to proceed without re-evaluting the object\n    itself.\n\nOf those, I think (3) is probably the best path forward. However, this\npatch does none of them. In the name of expediently fixing the\nregression to a normal \"rev-list --objects\" that we use for connectivity\nchecks, this simply restores the pre-7c0fe330d5 behavior of having the\ntraversal die as soon as it fails to load a tree (when --missing is set\nto MA_ERROR, which is the default).\n\nNote that we can't get rid of the object-existence check in\nfinish_object(), because this also handles blobs (which are not\notherwise checked at all by the traversal code).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/rev-list.c                     | 4 +++-\n t/t6102-rev-list-unexpected-objects.sh | 4 ++--\n 2 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 425a5774db..9f31837d30 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -379,7 +379,6 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \trepo_init_revisions(the_repository, &revs, prefix);\n \trevs.abbrev = DEFAULT_ABBREV;\n \trevs.commit_format = CMIT_FMT_UNSPECIFIED;\n-\trevs.do_not_die_on_missing_tree = 1;\n \n \t/*\n \t * Scan the argument list before invoking setup_revisions(), so that we\n@@ -409,6 +408,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n+\tif (arg_missing_action)\n+\t\trevs.do_not_die_on_missing_tree = 1;\n+\n \targc = setup_revisions(argc, argv, &revs, &s_r_opt);\n \n \tmemset(&info, 0, sizeof(info));\ndiff --git a/t/t6102-rev-list-unexpected-objects.sh b/t/t6102-rev-list-unexpected-objects.sh\nindex 30976385a8..c8d4b31f8f 100755\n--- a/t/t6102-rev-list-unexpected-objects.sh\n+++ b/t/t6102-rev-list-unexpected-objects.sh\n@@ -29,7 +29,7 @@ test_expect_success 'setup unexpected non-tree entry' '\n \tbroken_tree=\"$(git hash-object -w --literally -t tree broken-tree)\"\n '\n \n-test_expect_failure 'traverse unexpected non-tree entry (lone)' '\n+test_expect_success 'traverse unexpected non-tree entry (lone)' '\n \ttest_must_fail git rev-list --objects $broken_tree\n '\n \n@@ -64,7 +64,7 @@ test_expect_success 'setup unexpected non-tree root' '\n \t\tbroken-commit)\"\n '\n \n-test_expect_failure 'traverse unexpected non-tree root (lone)' '\n+test_expect_success 'traverse unexpected non-tree root (lone)' '\n \ttest_must_fail git rev-list --objects $broken_commit\n '\n \n-- \n2.21.0.203.g358da99528\n\n"},{"id":"373179","messageId":"358da99528daba2c9d34430fe0d262b6b5d23009.1554435033.git.me@ttaylorr.com","threadId":"50877","inReplyTo":"cover.1554435033.git.me@ttaylorr.com","subject":"[PATCH 7/7] rev-list: detect broken root trees","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-04-05T03:37:55Z","receivedAt":"2019-04-05T03:37:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"From: Jeff King <peff@peff.net>\n\nWhen the traversal machinery sees a commit without a root tree, it\nassumes that the tree was part of a BOUNDARY commit, and quietly ignores\nthe tree. But it could also be caused by a commit whose root tree is\nbroken or missing.\n\nInstead, let's die() when we see a NULL root tree. We can differentiate\nit from the BOUNDARY case by seeing if the commit was actually parsed.\nThis covers that case, plus future-proofs us against any others where we\nmight try to show an unparsed commit.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n list-objects.c                         | 3 +++\n t/t6102-rev-list-unexpected-objects.sh | 6 ++++--\n 2 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/list-objects.c b/list-objects.c\nindex bb7e61ef4b..b5651ddd5b 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -374,6 +374,9 @@ static void do_traverse(struct traversal_context *ctx)\n \t\t\tstruct tree *tree = get_commit_tree(commit);\n \t\t\ttree->object.flags |= NOT_USER_GIVEN;\n \t\t\tadd_pending_tree(ctx->revs, tree);\n+\t\t} else if (commit->object.parsed) {\n+\t\t\tdie(_(\"unable to load root tree for commit %s\"),\n+\t\t\t      oid_to_hex(&commit->object.oid));\n \t\t}\n \t\tctx->show_commit(commit, ctx->show_data);\n \ndiff --git a/t/t6102-rev-list-unexpected-objects.sh b/t/t6102-rev-list-unexpected-objects.sh\nindex c8d4b31f8f..98f5cffbb6 100755\n--- a/t/t6102-rev-list-unexpected-objects.sh\n+++ b/t/t6102-rev-list-unexpected-objects.sh\n@@ -68,8 +68,10 @@ test_expect_success 'traverse unexpected non-tree root (lone)' '\n \ttest_must_fail git rev-list --objects $broken_commit\n '\n \n-test_expect_failure 'traverse unexpected non-tree root (seen)' '\n-\ttest_must_fail git rev-list --objects $blob $broken_commit\n+test_expect_success 'traverse unexpected non-tree root (seen)' '\n+\ttest_must_fail git rev-list --objects $blob $broken_commit \\\n+\t\t>output 2>&1 &&\n+\ttest_i18ngrep \"not a tree\" output\n '\n \n test_expect_success 'setup unexpected non-commit tag' '\n-- \n2.21.0.203.g358da99528\n"},{"id":"373193","messageId":"20190405105033.GT32732@szeder.dev","threadId":"50877","inReplyTo":"ef6b4f380435d9743a56f47f68c18123bf0a0933.1554435033.git.me@ttaylorr.com","subject":"Re: [PATCH 2/7] t: introduce tests for unexpected object types","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-04-05T10:50:33Z","receivedAt":"2019-04-05T10:50:40Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Apr 04, 2019 at 08:37:44PM -0700, Taylor Blau wrote:\n> diff --git a/t/t6102-rev-list-unexpected-objects.sh b/t/t6102-rev-list-unexpected-objects.sh\n> new file mode 100755\n> index 0000000000..472b08528a\n> --- /dev/null\n> +++ b/t/t6102-rev-list-unexpected-objects.sh\n> @@ -0,0 +1,123 @@\n> +#!/bin/sh\n> +\n> +test_description='git rev-list should handle unexpected object types'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup well-formed objects' '\n> +\tblob=\"$(printf \"foo\" | git hash-object -w --stdin)\" &&\n> +\ttree=\"$(printf \"100644 blob $blob\\tfoo\" | git mktree)\" &&\n> +\tcommit=\"$(git commit-tree $tree -m \"first commit\")\"\n> +'\n> +\n> +test_expect_success 'setup unexpected non-blob entry' '\n> +\tprintf \"100644 foo\\0$(echo $tree | hex2oct)\" >broken-tree &&\n> +\tbroken_tree=\"$(git hash-object -w --literally -t tree broken-tree)\"\n> +'\n> +\n> +test_expect_failure 'traverse unexpected non-blob entry (lone)' '\n> +\ttest_must_fail git rev-list --objects $broken_tree\n> +'\n> +\n> +test_expect_failure 'traverse unexpected non-blob entry (seen)' '\n> +\ttest_must_fail git rev-list --objects $tree $broken_tree\n> +'\n> +\n> +test_expect_success 'setup unexpected non-tree entry' '\n> +\tprintf \"40000 foo\\0$(echo $blob | hex2oct)\" >broken-tree &&\n> +\tbroken_tree=\"$(git hash-object -w --literally -t tree broken-tree)\"\n> +'\n> +\n> +test_expect_failure 'traverse unexpected non-tree entry (lone)' '\n> +\ttest_must_fail git rev-list --objects $broken_tree\n> +'\n> +\n> +test_expect_failure 'traverse unexpected non-tree entry (seen)' '\n> +\ttest_must_fail git rev-list --objects $blob $broken_tree >output 2>&1\n\nThis test saves standard output and error, but doesn't look at them.\n\n> +'\n> +\n> +test_expect_success 'setup unexpected non-commit parent' '\n> +\tgit cat-file commit $commit |\n> +\t\tperl -lpe \"/^author/ && print q(parent $blob)\" \\\n> +\t\t>broken-commit &&\n\nDon't run git commands upstream of a pipe, because the pipe hides\ntheir exit code.  This applies to several other tests below as well.\n\nWouldn't a 'sed' one-liner suffice, so we won't have yet another perl\ndependency?\n\n> +\tbroken_commit=\"$(git hash-object -w --literally -t commit \\\n> +\t\tbroken-commit)\"\n> +'\n> +\n> +test_expect_success 'traverse unexpected non-commit parent (lone)' '\n> +\ttest_must_fail git rev-list --objects $broken_commit >output 2>&1 &&\n> +\ttest_i18ngrep \"not a commit\" output\n\nPlease make sure that this \"not a commit\" message goes to the file\ndescriptor it is supposed to, i.e., assuming it's part of an error\nmessage:\n\n  test_must_fail git rev-list .... 2>err &&\n  test_i18ngrep \"...\" err\n\nThis applies to several other tests below and in other patches as\nwell.\n\n> +'\n> +\n> +test_expect_success 'traverse unexpected non-commit parent (seen)' '\n> +\ttest_must_fail git rev-list --objects $commit $broken_commit \\\n> +\t\t>output 2>&1 &&\n> +\ttest_i18ngrep \"not a commit\" output\n> +'\n> +\n> +test_expect_success 'setup unexpected non-tree root' '\n> +\tgit cat-file commit $commit |\n> +\tsed -e \"s/$tree/$blob/\" >broken-commit &&\n> +\tbroken_commit=\"$(git hash-object -w --literally -t commit \\\n> +\t\tbroken-commit)\"\n> +'\n> +\n> +test_expect_failure 'traverse unexpected non-tree root (lone)' '\n> +\ttest_must_fail git rev-list --objects $broken_commit\n> +'\n> +\n> +test_expect_failure 'traverse unexpected non-tree root (seen)' '\n> +\ttest_must_fail git rev-list --objects $blob $broken_commit\n> +'\n> +\n> +test_expect_success 'setup unexpected non-commit tag' '\n> +\tgit tag -a -m \"tagged commit\" tag $commit &&\n> +\ttest_when_finished \"git tag -d tag\" &&\n> +\tgit cat-file -p tag | sed -e \"s/$commit/$blob/\" >broken-tag &&\n> +\ttag=$(git hash-object -w --literally -t tag broken-tag)\n> +'\n> +\n> +test_expect_success 'traverse unexpected non-commit tag (lone)' '\n> +\ttest_must_fail git rev-list --objects $tag\n> +'\n> +\n> +test_expect_success 'traverse unexpected non-commit tag (seen)' '\n> +\ttest_must_fail git rev-list --objects $blob $tag >output 2>&1 &&\n> +\ttest_i18ngrep \"not a commit\" output\n> +'\n> +\n> +test_expect_success 'setup unexpected non-tree tag' '\n> +\tgit tag -a -m \"tagged tree\" tag $tree &&\n> +\ttest_when_finished \"git tag -d tag\" &&\n> +\tgit cat-file -p tag |\n> +\tsed -e \"s/$tree/$blob/\" >broken-tag &&\n> +\ttag=$(git hash-object -w --literally -t tag broken-tag)\n> +'\n> +\n> +test_expect_success 'traverse unexpected non-tree tag (lone)' '\n> +\ttest_must_fail git rev-list --objects $tag\n> +'\n> +\n> +test_expect_success 'traverse unexpected non-tree tag (seen)' '\n> +\ttest_must_fail git rev-list --objects $blob $tag >output 2>&1 &&\n> +\ttest_i18ngrep \"not a tree\" output\n> +'\n> +\n> +test_expect_success 'setup unexpected non-blob tag' '\n> +\tgit tag -a -m \"tagged blob\" tag $blob &&\n> +\ttest_when_finished \"git tag -d tag\" &&\n> +\tgit cat-file -p tag |\n> +\tsed -e \"s/$blob/$commit/\" >broken-tag &&\n> +\ttag=$(git hash-object -w --literally -t tag broken-tag)\n> +'\n> +\n> +test_expect_failure 'traverse unexpected non-blob tag (lone)' '\n> +\ttest_must_fail git rev-list --objects $tag\n> +'\n> +\n> +test_expect_success 'traverse unexpected non-blob tag (seen)' '\n> +\ttest_must_fail git rev-list --objects $commit $tag >output 2>&1 &&\n> +\ttest_i18ngrep \"not a blob\" output\n> +'\n> +\n> +test_done\n> -- \n> 2.21.0.203.g358da99528\n> \n"},{"id":"373243","messageId":"20190405182412.GC2284@sigill.intra.peff.net","threadId":"50877","inReplyTo":"20190405105033.GT32732@szeder.dev","subject":"Re: [PATCH 2/7] t: introduce tests for unexpected object types","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T18:24:12Z","receivedAt":"2019-04-05T18:24:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 05, 2019 at 12:50:33PM +0200, SZEDER Gábor wrote:\n\n> > +test_expect_failure 'traverse unexpected non-tree entry (seen)' '\n> > +\ttest_must_fail git rev-list --objects $blob $broken_tree >output 2>&1\n> \n> This test saves standard output and error, but doesn't look at them.\n\nI think we want to be checking for \"not a tree\" here, which is later\nadded with the fix. But either we should have the test_i18ngrep here\ninitially, or we should add both the redirect and the grep with the fix.\n\n> > +test_expect_success 'setup unexpected non-commit parent' '\n> > +\tgit cat-file commit $commit |\n> > +\t\tperl -lpe \"/^author/ && print q(parent $blob)\" \\\n> > +\t\t>broken-commit &&\n> \n> Don't run git commands upstream of a pipe, because the pipe hides\n> their exit code.  This applies to several other tests below as well.\n\nI disagree with that rule here. We're not testing \"cat-file\" in any\nmeaningful way, but just getting some stock output from a known-good\ncommit.\n\n> Wouldn't a 'sed' one-liner suffice, so we won't have yet another perl\n> dependency?\n\nHeh, this was actually the subject of much discussion before the patches\nhit the list. If you can write such a one-liner that is both readable\nand portable, please share it. I got disgusted with sed and suggested\nthis perl.\n\n-Peff\n"},{"id":"373244","messageId":"20190405183142.GD2284@sigill.intra.peff.net","threadId":"50877","inReplyTo":"ef6b4f380435d9743a56f47f68c18123bf0a0933.1554435033.git.me@ttaylorr.com","subject":"Re: [PATCH 2/7] t: introduce tests for unexpected object types","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T18:31:42Z","receivedAt":"2019-04-05T18:31:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 04, 2019 at 08:37:44PM -0700, Taylor Blau wrote:\n\n> Let A be the object referenced with an unexpected type, and B be the\n> object doing the referencing. Do the following:\n> \n>   - test 'git rev-list --objects A B'. This causes A to be \"cached\", and\n>     presents the above scenario.\n> \n> Likewise, if we have a tree entry that claims to be a tree (for example)\n> but points to another object type (say, a blob), there are two ways we\n> might find out:\n> \n>   - when we call lookup_tree(), we might find that we've already seen\n>     the object referenced as another type, in which case we'd get NULL\n> \n>   - we call lookup_tree() successfully, but when we try to read the\n>     object, we find out it's something else.\n> \n> We should check that we behave sensibly in both cases (especially\n> because it is easy for a malicious actor to provoke one case or the\n> other).\n\nI think our pasting together of multiple commits adding the lone/seen\ncases ended up in some redundancy in the description. In particular, I'm\nnot sure what the first paragraph/bullet quoted above is trying to say,\nas it corresponds to the second bullet in the later list. Maybe collapse\nthem together like:\n\n  We might hit an unexpected type in two different ways (imagine we have\n  a tree entry that claims to be a tree but actually points to a blob):\n\n    - when we call lookup_tree(), we might find that we've already seen\n      the object referenced as a blob, in which case we'd get NULL. We\n      can exercise this with \"git rev-list --objects $blob $tree\", which\n      guarantees that the blob will have been parsed before we look in\n      the tree. These tests are marked as \"seen\" in the test script.\n\n    - we call lookup_tree() successfully, but when we try to read the\n      object, we find out it's something else. We construct our tests\n      such that $blob is not otherwise mentioned in $tree. These tests\n      are marked as \"lone\" in the script.\n\n-Peff\n"},{"id":"373245","messageId":"20190405184111.GE2284@sigill.intra.peff.net","threadId":"50877","inReplyTo":"a3a80b4b2a988eb65d85a5acd54c584d047073c7.1554435033.git.me@ttaylorr.com","subject":"Re: [PATCH 6/7] rev-list: let traversal die when --missing is not in use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T18:41:11Z","receivedAt":"2019-04-05T18:41:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 04, 2019 at 08:37:54PM -0700, Taylor Blau wrote:\n\n>  3. have the traversal machinery communicate the failure to the caller,\n>     so that it can decide how to proceed without re-evaluting the object\n>     itself.\n> \n> Of those, I think (3) is probably the best path forward. However, this\n> patch does none of them. In the name of expediently fixing the\n> regression to a normal \"rev-list --objects\" that we use for connectivity\n> checks, this simply restores the pre-7c0fe330d5 behavior of having the\n> traversal die as soon as it fails to load a tree (when --missing is set\n> to MA_ERROR, which is the default).\n\nI think this is worth doing, as it restores the earlier behavior. But a\nfew general thoughts (which I've shared already with you, but for the\nbenefit of the list):\n\n - actually doing the \"communicate failure to the caller\" would probably\n   not be too bad as a single-bit PARSE_FAILED flag in obj->flags. But\n   it does require the caller understanding which objects the traversal\n   would try to parse (i.e., rev-list would have to understand that it\n   is on its own to check blobs, even if they don't have a PARSE_FAILED\n   flag).\n\n - speaking of blobs, this series does not help rev-list find a\n   mis-typed or bit-rotted blob at all, because it never opens the\n   blobs. Does that mean my expectations for rev-list are simply too\n   high, and that we should be expecting fsck-like checks to catch\n   these? I dunno.\n\n   It would not be too expensive to convert the existing \"do we have the\n   blob\" check in rev-list to \"do we have it, and is its type correct?\".\n   But obviously finding bitrot would be super-expensive. Which leads me\n   to...\n\n - there actually _is_ a --verify-objects option, which would check even\n   blobs for bitrot. It was added long ago in 5a48d24012 (rev-list\n   --verify-object, 2011-09-01) for use with check_connected(). But it\n   was deemed too slow for normal use, and ripped out in d21c463d55\n   (fetch/receive: remove over-pessimistic connectivity check,\n   2012-03-15).\n\nThat last one implies that we're OK relying on the incoming index-pack\nto catch these cases (which is going to do a sha1 over each object).\n\nIt does seem like we should bother to notice failures when it's _free_\nto do so, which is the case with these tree-loading failures. Which is\nbasically what this patch is doing.\n\n-Peff\n"},{"id":"373246","messageId":"20190405184229.GB8796@szeder.dev","threadId":"50877","inReplyTo":"20190405182412.GC2284@sigill.intra.peff.net","subject":"Re: [PATCH 2/7] t: introduce tests for unexpected object types","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-04-05T18:42:29Z","receivedAt":"2019-04-05T18:42:35Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Apr 05, 2019 at 02:24:12PM -0400, Jeff King wrote:\n> On Fri, Apr 05, 2019 at 12:50:33PM +0200, SZEDER Gábor wrote:\n> \n> > > +test_expect_failure 'traverse unexpected non-tree entry (seen)' '\n> > > +\ttest_must_fail git rev-list --objects $blob $broken_tree >output 2>&1\n> > \n> > This test saves standard output and error, but doesn't look at them.\n> \n> I think we want to be checking for \"not a tree\" here, which is later\n> added with the fix. But either we should have the test_i18ngrep here\n> initially, or we should add both the redirect and the grep with the fix.\n> \n> > > +test_expect_success 'setup unexpected non-commit parent' '\n> > > +\tgit cat-file commit $commit |\n> > > +\t\tperl -lpe \"/^author/ && print q(parent $blob)\" \\\n> > > +\t\t>broken-commit &&\n> > \n> > Don't run git commands upstream of a pipe, because the pipe hides\n> > their exit code.  This applies to several other tests below as well.\n> \n> I disagree with that rule here. We're not testing \"cat-file\" in any\n> meaningful way, but just getting some stock output from a known-good\n> commit.\n\nIt makes auditing harder and sets bad example.\n \n> > Wouldn't a 'sed' one-liner suffice, so we won't have yet another perl\n> > dependency?\n> \n> Heh, this was actually the subject of much discussion before the patches\n> hit the list. If you can write such a one-liner that is both readable\n> and portable, please share it. I got disgusted with sed and suggested\n> this perl.\n\nHm, so the trivial s/// with '\\n' in the replacement part is not\nportable, then?  Oh, well.\n\n"},{"id":"373249","messageId":"20190405185241.GG2284@sigill.intra.peff.net","threadId":"50877","inReplyTo":"20190405184229.GB8796@szeder.dev","subject":"Re: [PATCH 2/7] t: introduce tests for unexpected object types","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T18:52:42Z","receivedAt":"2019-04-05T18:52:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 05, 2019 at 08:42:29PM +0200, SZEDER Gábor wrote:\n\n> > > Don't run git commands upstream of a pipe, because the pipe hides\n> > > their exit code.  This applies to several other tests below as well.\n> > \n> > I disagree with that rule here. We're not testing \"cat-file\" in any\n> > meaningful way, but just getting some stock output from a known-good\n> > commit.\n> \n> It makes auditing harder and sets bad example.\n\nI disagree that it's a bad example. Just because a reader might not\nrealize that it is sometimes OK and sometimes not, does not make it bad\nto do so in the OK case. It means the reader needs to understand the\nrule in order to correctly apply it.\n\nI am sympathetic to the auditing issue, though.\n\nI dunno. In this case it is not too bad to do:\n\n  git cat-file commit $commit >good-commit &&\n  perl ... <good-commit >broken-commit\n\nbut I am not sure I am on board with a blanket rule of \"git must never\nbe on the left-hand side of a pipe\".\n\n> > > Wouldn't a 'sed' one-liner suffice, so we won't have yet another perl\n> > > dependency?\n> > \n> > Heh, this was actually the subject of much discussion before the patches\n> > hit the list. If you can write such a one-liner that is both readable\n> > and portable, please share it. I got disgusted with sed and suggested\n> > this perl.\n> \n> Hm, so the trivial s/// with '\\n' in the replacement part is not\n> portable, then?  Oh, well.\n\nI don't think it is, but I could be wrong. POSIX does say that \"\\n\"\nmatches a newline in the pattern space, but nothing about it on the RHS\nof a substitution. I have a vague feeling of running into problems in\nthe past, but I could just be misremembering.\n\nWe also tried matching /^tree/ and using \"a\\\" to append a line, but it\nwas both hard to read and hit portability issues with bsd sed.\n\n-Peff\n"},{"id":"373252","messageId":"CAPig+cQ7f6C-YBE5LT3qiXSyjpqQQEknmE8eHdQJvzCAzkBNPA@mail.gmail.com","threadId":"50877","inReplyTo":"20190405182412.GC2284@sigill.intra.peff.net","subject":"Re: [PATCH 2/7] t: introduce tests for unexpected object types","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-04-05T19:25:43Z","receivedAt":"2019-04-05T19:26:00Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Apr 5, 2019 at 2:24 PM Jeff King <peff@peff.net> wrote:\n> On Fri, Apr 05, 2019 at 12:50:33PM +0200, SZEDER Gábor wrote:\n> > > +   git cat-file commit $commit |\n> > > +           perl -lpe \"/^author/ && print q(parent $blob)\" \\\n> > > +           >broken-commit &&\n>\n> > Wouldn't a 'sed' one-liner suffice, so we won't have yet another perl\n> > dependency?\n>\n> Heh, this was actually the subject of much discussion before the patches\n> hit the list. If you can write such a one-liner that is both readable\n> and portable, please share it. I got disgusted with sed and suggested\n> this perl.\n\nTrivial and portable 'sed' equivalent:\n\ngit cat-file commit $commit | sed \"/^author/ { h; s/.*/parent $blob/; G; }\"\n"},{"id":"373260","messageId":"20190405205345.GB8166@sigill.intra.peff.net","threadId":"50877","inReplyTo":"CAPig+cQ7f6C-YBE5LT3qiXSyjpqQQEknmE8eHdQJvzCAzkBNPA@mail.gmail.com","subject":"Re: [PATCH 2/7] t: introduce tests for unexpected object types","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T20:53:45Z","receivedAt":"2019-04-05T20:53:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 05, 2019 at 03:25:43PM -0400, Eric Sunshine wrote:\n\n> On Fri, Apr 5, 2019 at 2:24 PM Jeff King <peff@peff.net> wrote:\n> > On Fri, Apr 05, 2019 at 12:50:33PM +0200, SZEDER Gábor wrote:\n> > > > +   git cat-file commit $commit |\n> > > > +           perl -lpe \"/^author/ && print q(parent $blob)\" \\\n> > > > +           >broken-commit &&\n> >\n> > > Wouldn't a 'sed' one-liner suffice, so we won't have yet another perl\n> > > dependency?\n> >\n> > Heh, this was actually the subject of much discussion before the patches\n> > hit the list. If you can write such a one-liner that is both readable\n> > and portable, please share it. I got disgusted with sed and suggested\n> > this perl.\n> \n> Trivial and portable 'sed' equivalent:\n> \n> git cat-file commit $commit | sed \"/^author/ { h; s/.*/parent $blob/; G; }\"\n\nI always forget about the hold space. That's pretty readable (though\nbeing sed, it's terse enough that I actually think the perl is more\nreadable; that may be personal taste, though).\n\n-Peff\n"},{"id":"373276","messageId":"20190406052322.GA37216@Taylors-MBP.hsd1.wa.comcast.net","threadId":"50877","inReplyTo":"20190405183142.GD2284@sigill.intra.peff.net","subject":"Re: [PATCH 2/7] t: introduce tests for unexpected object types","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-04-06T05:23:22Z","receivedAt":"2019-04-06T05:27:23Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Peff,\n\nOn Fri, Apr 05, 2019 at 02:31:42PM -0400, Jeff King wrote:\n> On Thu, Apr 04, 2019 at 08:37:44PM -0700, Taylor Blau wrote:\n>\n> > Let A be the object referenced with an unexpected type, and B be the\n> > object doing the referencing. Do the following:\n> >\n> >   - test 'git rev-list --objects A B'. This causes A to be \"cached\", and\n> >     presents the above scenario.\n> >\n> > Likewise, if we have a tree entry that claims to be a tree (for example)\n> > but points to another object type (say, a blob), there are two ways we\n> > might find out:\n> >\n> >   - when we call lookup_tree(), we might find that we've already seen\n> >     the object referenced as another type, in which case we'd get NULL\n> >\n> >   - we call lookup_tree() successfully, but when we try to read the\n> >     object, we find out it's something else.\n> >\n> > We should check that we behave sensibly in both cases (especially\n> > because it is easy for a malicious actor to provoke one case or the\n> > other).\n>\n> I think our pasting together of multiple commits adding the lone/seen\n> cases ended up in some redundancy in the description. In particular, I'm\n> not sure what the first paragraph/bullet quoted above is trying to say,\n> as it corresponds to the second bullet in the later list.\n\nI agree that this is at the very least redundant, and at the most,\nconfusing. I think that you're right that this is the result of\nsquashing the commits often enough that some of this cruft stuck around\nand got combined in ways that it shouldn't have.\n\n> Maybe collapse them together like:\n>\n>   We might hit an unexpected type in two different ways (imagine we have\n>   a tree entry that claims to be a tree but actually points to a blob):\n>\n>     - when we call lookup_tree(), we might find that we've already seen\n>       the object referenced as a blob, in which case we'd get NULL. We\n>       can exercise this with \"git rev-list --objects $blob $tree\", which\n>       guarantees that the blob will have been parsed before we look in\n>       the tree. These tests are marked as \"seen\" in the test script.\n>\n>     - we call lookup_tree() successfully, but when we try to read the\n>       object, we find out it's something else. We construct our tests\n>       such that $blob is not otherwise mentioned in $tree. These tests\n>       are marked as \"lone\" in the script.\n\nIndeed, this is much cleaner (and thanks for writing it here, and saving\nme the work). I think that I will take this as-is for 2/7 in v2.\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"373277","messageId":"20190406053115.GB37216@Taylors-MBP.hsd1.wa.comcast.net","threadId":"50877","inReplyTo":"20190405182412.GC2284@sigill.intra.peff.net","subject":"Re: [PATCH 2/7] t: introduce tests for unexpected object types","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-04-06T05:31:15Z","receivedAt":"2019-04-06T05:31:20Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Apr 05, 2019 at 02:24:12PM -0400, Jeff King wrote:\n> On Fri, Apr 05, 2019 at 12:50:33PM +0200, SZEDER Gábor wrote:\n>\n> > > +test_expect_failure 'traverse unexpected non-tree entry (seen)' '\n> > > +\ttest_must_fail git rev-list --objects $blob $broken_tree >output 2>&1\n> >\n> > This test saves standard output and error, but doesn't look at them.\n>\n> I think we want to be checking for \"not a tree\" here, which is later\n> added with the fix. But either we should have the test_i18ngrep here\n> initially, or we should add both the redirect and the grep with the fix.\n\nRight, pointing out that saving the standard output and error of 'git\nrev-list' and then doing nothing with it as being redundant is certainly\nright.\n\nI think that the 'fix' here is to write instead:\n\n  +test_expect_failure 'traverse unexpected non-tree entry (seen)' '\n  +\ttest_must_fail git rev-list --objects $blob $broken_tree\n  +'\n\nAnd _then_ add '>output 2>&1 &&' to the end, capturing the output, as\nwell as the appropriate test_i18ngrep. This matches the pattern that\nwe've been otherwise following in the series so far.\n\n(FWIW, I think that this is also the result of squashing the series down\na few times...)\n\n> > > +test_expect_success 'setup unexpected non-commit parent' '\n> > > +\tgit cat-file commit $commit |\n> > > +\t\tperl -lpe \"/^author/ && print q(parent $blob)\" \\\n> > > +\t\t>broken-commit &&\n> >\n> > Don't run git commands upstream of a pipe, because the pipe hides\n> > their exit code.  This applies to several other tests below as well.\n>\n> I disagree with that rule here. We're not testing \"cat-file\" in any\n> meaningful way, but just getting some stock output from a known-good\n> commit.\n>\n> > Wouldn't a 'sed' one-liner suffice, so we won't have yet another perl\n> > dependency?\n>\n> Heh, this was actually the subject of much discussion before the patches\n> hit the list. If you can write such a one-liner that is both readable\n> and portable, please share it. I got disgusted with sed and suggested\n> this perl.\n\nI admit that this gave me a chuckle, too. When preparing this series to\nsend it, I did something like:\n\n  $ git rebase -x 'make && cd t && ./t6102-*.sh\n\nbut found that it was broken on the BSD sed that ships with macOS\n10.14.2. I didn't notice until preparing the series for upstream because\nI wrote it on my Debian 9 VM, with GNU sed (where it is not broken ;-)).\n\nIt was originally written as:\n\n\ttest_expect_success 'setup unexpected non-commit parent' '\n\t\tgit cat-file commit $commit | sed \"/^tree/a\\\n\t\tparent $blob\" >broken-commit &&\n\t\tbroken_commit=\"$(git hash-object -w --literally -t commit \\\n\t\t\tbroken-commit)\"\n\t'\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"373278","messageId":"20190406053308.GC37216@Taylors-MBP.hsd1.wa.comcast.net","threadId":"50877","inReplyTo":"20190405205345.GB8166@sigill.intra.peff.net","subject":"Re: [PATCH 2/7] t: introduce tests for unexpected object types","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-04-06T05:33:08Z","receivedAt":"2019-04-06T05:33:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Apr 05, 2019 at 04:53:45PM -0400, Jeff King wrote:\n> On Fri, Apr 05, 2019 at 03:25:43PM -0400, Eric Sunshine wrote:\n>\n> > On Fri, Apr 5, 2019 at 2:24 PM Jeff King <peff@peff.net> wrote:\n> > > On Fri, Apr 05, 2019 at 12:50:33PM +0200, SZEDER Gábor wrote:\n> > > > > +   git cat-file commit $commit |\n> > > > > +           perl -lpe \"/^author/ && print q(parent $blob)\" \\\n> > > > > +           >broken-commit &&\n> > >\n> > > > Wouldn't a 'sed' one-liner suffice, so we won't have yet another perl\n> > > > dependency?\n> > >\n> > > Heh, this was actually the subject of much discussion before the patches\n> > > hit the list. If you can write such a one-liner that is both readable\n> > > and portable, please share it. I got disgusted with sed and suggested\n> > > this perl.\n> >\n> > Trivial and portable 'sed' equivalent:\n> >\n> > git cat-file commit $commit | sed \"/^author/ { h; s/.*/parent $blob/; G; }\"\n>\n> I always forget about the hold space. That's pretty readable (though\n> being sed, it's terse enough that I actually think the perl is more\n> readable; that may be personal taste, though).\n\nAh, very nice. Thanks Eric, your sed-fu is much stronger than mine ;-).\n\nI share Peff's view that this might be less readable than its Perl\ncounterpart, but am similarly sympathetic to the notion that more Perl\nis a bad example in 't'.\n\nI think that I'll take this for v2 and get rid of the Perl.\n\nThanks,\nTaylor\n"},{"id":"373279","messageId":"20190406053648.GD37216@Taylors-MBP.hsd1.wa.comcast.net","threadId":"50877","inReplyTo":"20190405184111.GE2284@sigill.intra.peff.net","subject":"Re: [PATCH 6/7] rev-list: let traversal die when --missing is not in use","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-04-06T05:36:48Z","receivedAt":"2019-04-06T05:36:51Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Apr 05, 2019 at 02:41:11PM -0400, Jeff King wrote:\n> On Thu, Apr 04, 2019 at 08:37:54PM -0700, Taylor Blau wrote:\n>\n> >  3. have the traversal machinery communicate the failure to the caller,\n> >     so that it can decide how to proceed without re-evaluting the object\n> >     itself.\n> >\n> > Of those, I think (3) is probably the best path forward. However, this\n> > patch does none of them. In the name of expediently fixing the\n> > regression to a normal \"rev-list --objects\" that we use for connectivity\n> > checks, this simply restores the pre-7c0fe330d5 behavior of having the\n> > traversal die as soon as it fails to load a tree (when --missing is set\n> > to MA_ERROR, which is the default).\n>\n> I think this is worth doing, as it restores the earlier behavior. But a\n> few general thoughts (which I've shared already with you, but for the\n> benefit of the list):\n\nI agree that it's worth doing. One question that I have is _when_ you\nfeel it's good to do. I'm happy to write it and include the change in\nv2, but if others would be happy not to grow the series too much between\nre-rolls, I'd be just as pleased to send it in a new series after this\none.\n\n>  - actually doing the \"communicate failure to the caller\" would probably\n>    not be too bad as a single-bit PARSE_FAILED flag in obj->flags. But\n>    it does require the caller understanding which objects the traversal\n>    would try to parse (i.e., rev-list would have to understand that it\n>    is on its own to check blobs, even if they don't have a PARSE_FAILED\n>    flag).\n>\n>  - speaking of blobs, this series does not help rev-list find a\n>    mis-typed or bit-rotted blob at all, because it never opens the\n>    blobs. Does that mean my expectations for rev-list are simply too\n>    high, and that we should be expecting fsck-like checks to catch\n>    these? I dunno.\n>\n>    It would not be too expensive to convert the existing \"do we have the\n>    blob\" check in rev-list to \"do we have it, and is its type correct?\".\n>    But obviously finding bitrot would be super-expensive. Which leads me\n>    to...\n>\n>  - there actually _is_ a --verify-objects option, which would check even\n>    blobs for bitrot. It was added long ago in 5a48d24012 (rev-list\n>    --verify-object, 2011-09-01) for use with check_connected(). But it\n>    was deemed too slow for normal use, and ripped out in d21c463d55\n>    (fetch/receive: remove over-pessimistic connectivity check,\n>    2012-03-15).\n>\n> That last one implies that we're OK relying on the incoming index-pack\n> to catch these cases (which is going to do a sha1 over each object).\n>\n> It does seem like we should bother to notice failures when it's _free_\n> to do so, which is the case with these tree-loading failures. Which is\n> basically what this patch is doing.\n>\n> -Peff\nThanks,\nTaylor\n"},{"id":"373351","messageId":"20190407134113.GA13417@sigill.intra.peff.net","threadId":"50877","inReplyTo":"20190406053648.GD37216@Taylors-MBP.hsd1.wa.comcast.net","subject":"Re: [PATCH 6/7] rev-list: let traversal die when --missing is not in use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-07T13:41:13Z","receivedAt":"2019-04-07T13:41:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 05, 2019 at 10:36:48PM -0700, Taylor Blau wrote:\n\n> > > Of those, I think (3) is probably the best path forward. However, this\n> > > patch does none of them. In the name of expediently fixing the\n> > > regression to a normal \"rev-list --objects\" that we use for connectivity\n> > > checks, this simply restores the pre-7c0fe330d5 behavior of having the\n> > > traversal die as soon as it fails to load a tree (when --missing is set\n> > > to MA_ERROR, which is the default).\n> >\n> > I think this is worth doing, as it restores the earlier behavior. But a\n> > few general thoughts (which I've shared already with you, but for the\n> > benefit of the list):\n> \n> I agree that it's worth doing. One question that I have is _when_ you\n> feel it's good to do. I'm happy to write it and include the change in\n> v2, but if others would be happy not to grow the series too much between\n> re-rolls, I'd be just as pleased to send it in a new series after this\n> one.\n\nI'm not sure what \"it\" is here. My earlier message was admittedly\nrambling, but I think I'm arguing that it's OK to continue to include\nthis patch that you already have, and punt further changes to make\n\"rev-list --objects\" detect blob problems down the road. I.e., leave the\ntwo expect_failures in place that your v1 series ends with.\n\n-Peff\n"},{"id":"373369","messageId":"87a7h1a5uk.fsf@evledraar.gmail.com","threadId":"50877","inReplyTo":"20190405185241.GG2284@sigill.intra.peff.net","subject":"Re: [PATCH 2/7] t: introduce tests for unexpected object types","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-04-07T21:00:19Z","receivedAt":"2019-04-07T21:00:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Apr 05 2019, Jeff King wrote:\n\n> On Fri, Apr 05, 2019 at 08:42:29PM +0200, SZEDER Gábor wrote:\n>\n>> > > Don't run git commands upstream of a pipe, because the pipe hides\n>> > > their exit code.  This applies to several other tests below as well.\n>> >\n>> > I disagree with that rule here. We're not testing \"cat-file\" in any\n>> > meaningful way, but just getting some stock output from a known-good\n>> > commit.\n>>\n>> It makes auditing harder and sets bad example.\n>\n> I disagree that it's a bad example. Just because a reader might not\n> realize that it is sometimes OK and sometimes not, does not make it bad\n> to do so in the OK case. It means the reader needs to understand the\n> rule in order to correctly apply it.\n\nFWIW I thought the rule was something to the effect of \"we're hacking on\ngit, any change might segfault in some unexpected test, let's make sure\nthat fails right away\", hence the blanket rule in t/README against \"!\ngit <cmd>\" in favor of \"test_must_fail git <cmd>\", and \"don't feed the\noutput of a git command to a pipe\" documented right after that.\n\n> I am sympathetic to the auditing issue, though.\n>\n> I dunno. In this case it is not too bad to do:\n>\n>   git cat-file commit $commit >good-commit &&\n>   perl ... <good-commit >broken-commit\n>\n> but I am not sure I am on board with a blanket rule of \"git must never\n> be on the left-hand side of a pipe\".\n>\n>> > > Wouldn't a 'sed' one-liner suffice, so we won't have yet another perl\n>> > > dependency?\n>> >\n>> > Heh, this was actually the subject of much discussion before the patches\n>> > hit the list. If you can write such a one-liner that is both readable\n>> > and portable, please share it. I got disgusted with sed and suggested\n>> > this perl.\n>>\n>> Hm, so the trivial s/// with '\\n' in the replacement part is not\n>> portable, then?  Oh, well.\n>\n> I don't think it is, but I could be wrong. POSIX does say that \"\\n\"\n> matches a newline in the pattern space, but nothing about it on the RHS\n> of a substitution. I have a vague feeling of running into problems in\n> the past, but I could just be misremembering.\n>\n> We also tried matching /^tree/ and using \"a\\\" to append a line, but it\n> was both hard to read and hit portability issues with bsd sed.\n>\n> -Peff\n"},{"id":"373394","messageId":"xmqqsgut9idx.fsf@gitster-ct.c.googlers.com","threadId":"50877","inReplyTo":"20190405185241.GG2284@sigill.intra.peff.net","subject":"Re: [PATCH 2/7] t: introduce tests for unexpected object types","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-08T05:27:06Z","receivedAt":"2019-04-08T05:27:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I don't think it is, but I could be wrong. POSIX does say that \"\\n\"\n> matches a newline in the pattern space, but nothing about it on the RHS\n> of a substitution. I have a vague feeling of running into problems in\n> the past, but I could just be misremembering.\n\nYes, it was quite bad on minority platforms like AIX when I had to\ntouch it the last time.\n"},{"id":"373399","messageId":"xmqqbm1h9et2.fsf@gitster-ct.c.googlers.com","threadId":"50877","inReplyTo":"CAPig+cQ7f6C-YBE5LT3qiXSyjpqQQEknmE8eHdQJvzCAzkBNPA@mail.gmail.com","subject":"Re: [PATCH 2/7] t: introduce tests for unexpected object types","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-08T06:44:25Z","receivedAt":"2019-04-08T06:44:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> On Fri, Apr 5, 2019 at 2:24 PM Jeff King <peff@peff.net> wrote:\n>> On Fri, Apr 05, 2019 at 12:50:33PM +0200, SZEDER Gábor wrote:\n>> > > +   git cat-file commit $commit |\n>> > > +           perl -lpe \"/^author/ && print q(parent $blob)\" \\\n>> > > +           >broken-commit &&\n>>\n>> > Wouldn't a 'sed' one-liner suffice, so we won't have yet another perl\n>> > dependency?\n>>\n>> Heh, this was actually the subject of much discussion before the patches\n>> hit the list. If you can write such a one-liner that is both readable\n>> and portable, please share it. I got disgusted with sed and suggested\n>> this perl.\n>\n> Trivial and portable 'sed' equivalent:\n>\n> git cat-file commit $commit | sed \"/^author/ { h; s/.*/parent $blob/; G; }\"\n\nLooks good.  I had a bit of head scratching moment when I saw that\n\"perl -lpe\" one-liner; this sed expression may not be crystal clear\nto those who are not used to, but it is not so bad, either.\n"},{"id":"373449","messageId":"20190409021138.GD81620@Taylors-MBP.hsd1.wa.comcast.net","threadId":"50877","inReplyTo":"20190407134113.GA13417@sigill.intra.peff.net","subject":"Re: [PATCH 6/7] rev-list: let traversal die when --missing is not in use","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-04-09T02:11:38Z","receivedAt":"2019-04-09T02:11:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Peff,\n\nOn Sun, Apr 07, 2019 at 09:41:13AM -0400, Jeff King wrote:\n> On Fri, Apr 05, 2019 at 10:36:48PM -0700, Taylor Blau wrote:\n>\n> > > > Of those, I think (3) is probably the best path forward. However, this\n> > > > patch does none of them. In the name of expediently fixing the\n> > > > regression to a normal \"rev-list --objects\" that we use for connectivity\n> > > > checks, this simply restores the pre-7c0fe330d5 behavior of having the\n> > > > traversal die as soon as it fails to load a tree (when --missing is set\n> > > > to MA_ERROR, which is the default).\n> > >\n> > > I think this is worth doing, as it restores the earlier behavior. But a\n> > > few general thoughts (which I've shared already with you, but for the\n> > > benefit of the list):\n> >\n> > I agree that it's worth doing. One question that I have is _when_ you\n> > feel it's good to do. I'm happy to write it and include the change in\n> > v2, but if others would be happy not to grow the series too much between\n> > re-rolls, I'd be just as pleased to send it in a new series after this\n> > one.\n>\n> I'm not sure what \"it\" is here.\n\nYes... as I read this email again after the weekend had passed, I found\nmyself a little confused, too.\n\n> My earlier message was admittedly rambling, but I think I'm arguing\n> that it's OK to continue to include this patch that you already have,\n> and punt further changes to make \"rev-list --objects\" detect blob\n> problems down the road. I.e., leave the two expect_failures in place\n> that your v1 series ends with.\n\nI believe that that was the \"it\" that I was talking about it. To be\nexplicit, I think I was suggesting that we should not change this patch\nmuch or add more to the series, and rather address the blob checking in\na new series after this one.\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"373450","messageId":"20190409022936.GE81620@Taylors-MBP.hsd1.wa.comcast.net","threadId":"50877","inReplyTo":"87a7h1a5uk.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/7] t: introduce tests for unexpected object types","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-04-09T02:29:36Z","receivedAt":"2019-04-09T02:29:41Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Ævar,\n\nOn Sun, Apr 07, 2019 at 11:00:19PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n> On Fri, Apr 05 2019, Jeff King wrote:\n>\n> > On Fri, Apr 05, 2019 at 08:42:29PM +0200, SZEDER Gábor wrote:\n> >\n> >> > > Don't run git commands upstream of a pipe, because the pipe hides\n> >> > > their exit code.  This applies to several other tests below as well.\n> >> >\n> >> > I disagree with that rule here. We're not testing \"cat-file\" in any\n> >> > meaningful way, but just getting some stock output from a known-good\n> >> > commit.\n> >>\n> >> It makes auditing harder and sets bad example.\n> >\n> > I disagree that it's a bad example. Just because a reader might not\n> > realize that it is sometimes OK and sometimes not, does not make it bad\n> > to do so in the OK case. It means the reader needs to understand the\n> > rule in order to correctly apply it.\n>\n> FWIW I thought the rule was something to the effect of \"we're hacking on\n> git, any change might segfault in some unexpected test, let's make sure\n> that fails right away\", hence the blanket rule in t/README against \"!\n> git <cmd>\" in favor of \"test_must_fail git <cmd>\", and \"don't feed the\n> output of a git command to a pipe\" documented right after that.\n\nI have some loosely-held opinions on this. Certainly knowing if a change\nto git caused a segfault in some test is something that we would want to\nknow about, though I'm not sure we're loosing anything by putting 'git'\non the left-hand side of a pipe here.\n\n  - If someone wrote a change to git that introduced a segfault in 'git\n    cat-file', I'm sure that this would not be the only place that in\n    the suite that would break because of it. Presumably, at least one\n    of those places uses 'test_must_fail git ...' instead\n\n  - At the very least, 'broken-commit' doesn't contain what it needs to,\n    the test breaks in another way (albeit further from the actual\n    defect), and the developer finds out about their bug that way.\n\nIn any case, these above two might be moot anyways, because I'm almost\ncertain that it will be a rarity for someone to _only_ run t6102, unless\nit is included in a blanket 'make' from within 't'.\n\n> > I am sympathetic to the auditing issue, though.\n\nJust to satisfy my curiosity, I put git on the left-hand side of a pipe\nto see how many such examples exist today:\n\n  ~/g/git (master) $ git grep 'git cat-file .* |' -- t/t*.sh | wc -l\n      179\n\nI'm not going to claim that changing 179 -> 180 is neutral or bad, but\nit's interesting nonetheless.\n\n> > I dunno. In this case it is not too bad to do:\n> >\n> >   git cat-file commit $commit >good-commit &&\n> >   perl ... <good-commit >broken-commit\n> >\n> > but I am not sure I am on board with a blanket rule of \"git must never\n> > be on the left-hand side of a pipe\".\n\nI think that I mostly agree with Peff here for the reasons above.\n\nThat all said, I don't really want to hold up the series for this alone.\nSince there don't seem to be many other comments or issues, I would be\nquite happy to do whatever people are most in favor of.\n\nI basically don't really feel strongly about writing:\n\n  git cat-file commit $commit | sed -e ... >broken-commit\n\nas opposed to:\n\n  git cat-file commit $commit >good-commit &&\n  sed -e '' <commit >bad-commit\n\n> >> > > Wouldn't a 'sed' one-liner suffice, so we won't have yet another perl\n> >> > > dependency?\n> >> >\n> >> > Heh, this was actually the subject of much discussion before the patches\n> >> > hit the list. If you can write such a one-liner that is both readable\n> >> > and portable, please share it. I got disgusted with sed and suggested\n> >> > this perl.\n> >>\n> >> Hm, so the trivial s/// with '\\n' in the replacement part is not\n> >> portable, then?  Oh, well.\n> >\n> > I don't think it is, but I could be wrong. POSIX does say that \"\\n\"\n> > matches a newline in the pattern space, but nothing about it on the RHS\n> > of a substitution. I have a vague feeling of running into problems in\n> > the past, but I could just be misremembering.\n> >\n> > We also tried matching /^tree/ and using \"a\\\" to append a line, but it\n> > was both hard to read and hit portability issues with bsd sed.\n\nI think that all of this discussion is addressed elsewhere in thread, but\nthe gist is that Eric provided a suitable sed invocation that I am going\nto use instead of Peff's Perl.\n\n> > -Peff\n\nThanks,\nTaylor\n"},{"id":"373451","messageId":"20190409023055.GF81620@Taylors-MBP.hsd1.wa.comcast.net","threadId":"50877","inReplyTo":"xmqqbm1h9et2.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/7] t: introduce tests for unexpected object types","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-04-09T02:30:55Z","receivedAt":"2019-04-09T02:31:00Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Junio,\n\n\nOn Mon, Apr 08, 2019 at 03:44:25PM +0900, Junio C Hamano wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n>\n> > On Fri, Apr 5, 2019 at 2:24 PM Jeff King <peff@peff.net> wrote:\n> >> On Fri, Apr 05, 2019 at 12:50:33PM +0200, SZEDER Gábor wrote:\n> >> > > +   git cat-file commit $commit |\n> >> > > +           perl -lpe \"/^author/ && print q(parent $blob)\" \\\n> >> > > +           >broken-commit &&\n> >>\n> >> > Wouldn't a 'sed' one-liner suffice, so we won't have yet another perl\n> >> > dependency?\n> >>\n> >> Heh, this was actually the subject of much discussion before the patches\n> >> hit the list. If you can write such a one-liner that is both readable\n> >> and portable, please share it. I got disgusted with sed and suggested\n> >> this perl.\n> >\n> > Trivial and portable 'sed' equivalent:\n> >\n> > git cat-file commit $commit | sed \"/^author/ { h; s/.*/parent $blob/; G; }\"\n>\n> Looks good.  I had a bit of head scratching moment when I saw that\n> \"perl -lpe\" one-liner; this sed expression may not be crystal clear\n> to those who are not used to, but it is not so bad, either.\n\nShould I take this as your endorsement of putting 'git' on the left-hand\nside of a pipe? ;-).\n\nThanks,\nTaylor\n"},{"id":"373453","messageId":"CAPig+cS+4tTAAWsp+KX2-__kcvgDABQQCmz63d6pQUa6Qu3LKw@mail.gmail.com","threadId":"50877","inReplyTo":"20190409023055.GF81620@Taylors-MBP.hsd1.wa.comcast.net","subject":"Re: [PATCH 2/7] t: introduce tests for unexpected object types","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-04-09T03:28:19Z","receivedAt":"2019-04-09T03:28:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 8, 2019 at 10:31 PM Taylor Blau <me@ttaylorr.com> wrote:\n> On Mon, Apr 08, 2019 at 03:44:25PM +0900, Junio C Hamano wrote:\n> > Eric Sunshine <sunshine@sunshineco.com> writes:\n> > >> > > +   git cat-file commit $commit |\n> > >> > > +           perl -lpe \"/^author/ && print q(parent $blob)\" \\\n> > >> > > +           >broken-commit &&\n> > >\n> > > Trivial and portable 'sed' equivalent:\n> > >\n> > > git cat-file commit $commit | sed \"/^author/ { h; s/.*/parent $blob/; G; }\"\n> >\n> > Looks good.  I had a bit of head scratching moment when I saw that\n> > \"perl -lpe\" one-liner; this sed expression may not be crystal clear\n> > to those who are not used to, but it is not so bad, either.\n>\n> Should I take this as your endorsement of putting 'git' on the left-hand\n> side of a pipe? ;-).\n\nI suspect that Junio's \"Looks good\" was referring to the 'sed expression.\n\nWith all the recent work of moving away from having Git upstream of a\npipe, let's not intentionally introduce a new instance. I wrote the\nexample 'sed' expression that way merely to mirror how the original\n'perl' version was written to make it easier to see the equivalence\n(not because it was intended as an endorsement of having Git upstream\nof a pipe).\n"},{"id":"373454","messageId":"20190409050857.GI81620@Taylors-MBP.hsd1.wa.comcast.net","threadId":"50877","inReplyTo":"CAPig+cS+4tTAAWsp+KX2-__kcvgDABQQCmz63d6pQUa6Qu3LKw@mail.gmail.com","subject":"Re: [PATCH 2/7] t: introduce tests for unexpected object types","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-04-09T05:08:57Z","receivedAt":"2019-04-09T05:09:02Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Eric,\n\nOn Mon, Apr 08, 2019 at 11:28:19PM -0400, Eric Sunshine wrote:\n> On Mon, Apr 8, 2019 at 10:31 PM Taylor Blau <me@ttaylorr.com> wrote:\n> > On Mon, Apr 08, 2019 at 03:44:25PM +0900, Junio C Hamano wrote:\n> > > Eric Sunshine <sunshine@sunshineco.com> writes:\n> > > >> > > +   git cat-file commit $commit |\n> > > >> > > +           perl -lpe \"/^author/ && print q(parent $blob)\" \\\n> > > >> > > +           >broken-commit &&\n> > > >\n> > > > Trivial and portable 'sed' equivalent:\n> > > >\n> > > > git cat-file commit $commit | sed \"/^author/ { h; s/.*/parent $blob/; G; }\"\n> > >\n> > > Looks good.  I had a bit of head scratching moment when I saw that\n> > > \"perl -lpe\" one-liner; this sed expression may not be crystal clear\n> > > to those who are not used to, but it is not so bad, either.\n> >\n> > Should I take this as your endorsement of putting 'git' on the left-hand\n> > side of a pipe? ;-).\n>\n> I suspect that Junio's \"Looks good\" was referring to the 'sed expression.\n\nI think that you are right -- and I'll happily _not_ introduce more Git\non the left-hand-side of a pipe instances.\n\nI noticed a few more instances in t6102 where we do something similar\nto:\n\n  git cat-file -p <something> | sed ... >broken-<something> &&\n\nI wrote the following patch, which I've folded into my local copy (and\nwill send with v2):\n\ndiff --git a/t/t6102-rev-list-unexpected-objects.sh b/t/t6102-rev-list-unexpected-objects.sh\nindex b9d82f9542..15072ecce3 100755\n--- a/t/t6102-rev-list-unexpected-objects.sh\n+++ b/t/t6102-rev-list-unexpected-objects.sh\n@@ -7,7 +7,8 @@ test_description='git rev-list should handle unexpected object types'\n test_expect_success 'setup well-formed objects' '\n \tblob=\"$(printf \"foo\" | git hash-object -w --stdin)\" &&\n \ttree=\"$(printf \"100644 blob $blob\\tfoo\" | git mktree)\" &&\n-\tcommit=\"$(git commit-tree $tree -m \"first commit\")\"\n+\tcommit=\"$(git commit-tree $tree -m \"first commit\")\" &&\n+\tgit cat-file commit $commit >good-commit\n '\n\n test_expect_success 'setup unexpected non-blob entry' '\n@@ -37,8 +38,8 @@ test_expect_failure 'traverse unexpected non-tree entry (seen)' '\n '\n\n test_expect_success 'setup unexpected non-commit parent' '\n-\tgit cat-file commit $commit | \\\n-\t\tsed \"/^author/ { h; s/.*/parent $blob/; G; }\" >broken-commit &&\n+\tsed \"/^author/ { h; s/.*/parent $blob/; G; }\" <good-commit \\\n+\t\t>broken-commit &&\n \tbroken_commit=\"$(git hash-object -w --literally -t commit \\\n \t\tbroken-commit)\"\n '\n@@ -55,8 +56,7 @@ test_expect_success 'traverse unexpected non-commit parent (seen)' '\n '\n\n test_expect_success 'setup unexpected non-tree root' '\n-\tgit cat-file commit $commit |\n-\tsed -e \"s/$tree/$blob/\" >broken-commit &&\n+\tsed -e \"s/$tree/$blob/\" <good-commit >broken-commit &&\n \tbroken_commit=\"$(git hash-object -w --literally -t commit \\\n \t\tbroken-commit)\"\n '\n@@ -71,8 +71,9 @@ test_expect_failure 'traverse unexpected non-tree root (seen)' '\n\n test_expect_success 'setup unexpected non-commit tag' '\n \tgit tag -a -m \"tagged commit\" tag $commit &&\n+\tgit cat-file tag tag >good-tag &&\n \ttest_when_finished \"git tag -d tag\" &&\n-\tgit cat-file -p tag | sed -e \"s/$commit/$blob/\" >broken-tag &&\n+\tsed -e \"s/$commit/$blob/\" <good-tag >broken-tag &&\n \ttag=$(git hash-object -w --literally -t tag broken-tag)\n '\n\n@@ -87,9 +88,9 @@ test_expect_success 'traverse unexpected non-commit tag (seen)' '\n\n test_expect_success 'setup unexpected non-tree tag' '\n \tgit tag -a -m \"tagged tree\" tag $tree &&\n+\tgit cat-file tag tag >good-tag &&\n \ttest_when_finished \"git tag -d tag\" &&\n-\tgit cat-file -p tag |\n-\tsed -e \"s/$tree/$blob/\" >broken-tag &&\n+\tsed -e \"s/$tree/$blob/\" <good-tag >broken-tag &&\n \ttag=$(git hash-object -w --literally -t tag broken-tag)\n '\n\n@@ -104,9 +105,9 @@ test_expect_success 'traverse unexpected non-tree tag (seen)' '\n\n test_expect_success 'setup unexpected non-blob tag' '\n \tgit tag -a -m \"tagged blob\" tag $blob &&\n+\tgit cat-file tag tag >good-tag &&\n \ttest_when_finished \"git tag -d tag\" &&\n-\tgit cat-file -p tag |\n-\tsed -e \"s/$blob/$commit/\" >broken-tag &&\n+\tsed -e \"s/$blob/$commit/\" <good-tag >broken-tag &&\n \ttag=$(git hash-object -w --literally -t tag broken-tag)\n '\n\n> With all the recent work of moving away from having Git upstream of a\n> pipe, let's not intentionally introduce a new instance. I wrote the\n> example 'sed' expression that way merely to mirror how the original\n> 'perl' version was written to make it easier to see the equivalence\n> (not because it was intended as an endorsement of having Git upstream\n> of a pipe).\n\nI see, and thank you for the clarification. Let me know if you like the\npatch above.\n\nThanks,\nTaylor\n"},{"id":"373464","messageId":"CAPig+cR6v_+nmGLsnoSz6K262hQfC1R9Xdk8CfuHcx6MduZfcg@mail.gmail.com","threadId":"50877","inReplyTo":"20190409050857.GI81620@Taylors-MBP.hsd1.wa.comcast.net","subject":"Re: [PATCH 2/7] t: introduce tests for unexpected object types","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-04-09T08:02:23Z","receivedAt":"2019-04-09T08:02:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Apr 9, 2019 at 1:09 AM Taylor Blau <me@ttaylorr.com> wrote:\n> On Mon, Apr 08, 2019 at 11:28:19PM -0400, Eric Sunshine wrote:\n> > I suspect that Junio's \"Looks good\" was referring to the 'sed expression.\n>\n> I think that you are right -- and I'll happily _not_ introduce more Git\n> on the left-hand-side of a pipe instances.\n>\n> I noticed a few more instances in t6102 [...]\n\nIndeed, SZEDER mentioned those in [1]:\n\n    Don't run git commands upstream of a pipe, because the pipe\n    hides their exit code.  This applies to several other tests\n    below as well.\n\n[1]: http://public-inbox.org/git/20190405105033.GT32732@szeder.dev/\n\n> I wrote the following patch, which I've folded into my local copy (and\n> will send with v2):\n>\n> > With all the recent work of moving away from having Git upstream of a\n> > pipe, let's not intentionally introduce a new instance. I wrote the\n> > example 'sed' expression that way merely to mirror how the original\n> > 'perl' version was written to make it easier to see the equivalence\n> > (not because it was intended as an endorsement of having Git upstream\n> > of a pipe).\n>\n> I see, and thank you for the clarification. Let me know if you like the\n> patch above.\n\nLooks fine. Thanks.\n"},{"id":"373471","messageId":"871s2ba6bb.fsf@evledraar.gmail.com","threadId":"50877","inReplyTo":"20190409022936.GE81620@Taylors-MBP.hsd1.wa.comcast.net","subject":"Re: [PATCH 2/7] t: introduce tests for unexpected object types","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-04-09T09:14:48Z","receivedAt":"2019-04-09T09:14:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Apr 09 2019, Taylor Blau wrote:\n\n> Hi Ævar,\n>\n> On Sun, Apr 07, 2019 at 11:00:19PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>>\n>> On Fri, Apr 05 2019, Jeff King wrote:\n>>\n>> > On Fri, Apr 05, 2019 at 08:42:29PM +0200, SZEDER Gábor wrote:\n>> >\n>> >> > > Don't run git commands upstream of a pipe, because the pipe hides\n>> >> > > their exit code.  This applies to several other tests below as well.\n>> >> >\n>> >> > I disagree with that rule here. We're not testing \"cat-file\" in any\n>> >> > meaningful way, but just getting some stock output from a known-good\n>> >> > commit.\n>> >>\n>> >> It makes auditing harder and sets bad example.\n>> >\n>> > I disagree that it's a bad example. Just because a reader might not\n>> > realize that it is sometimes OK and sometimes not, does not make it bad\n>> > to do so in the OK case. It means the reader needs to understand the\n>> > rule in order to correctly apply it.\n>>\n>> FWIW I thought the rule was something to the effect of \"we're hacking on\n>> git, any change might segfault in some unexpected test, let's make sure\n>> that fails right away\", hence the blanket rule in t/README against \"!\n>> git <cmd>\" in favor of \"test_must_fail git <cmd>\", and \"don't feed the\n>> output of a git command to a pipe\" documented right after that.\n>\n> I have some loosely-held opinions on this. Certainly knowing if a change\n> to git caused a segfault in some test is something that we would want to\n> know about, though I'm not sure we're loosing anything by putting 'git'\n> on the left-hand side of a pipe here.\n>\n>   - If someone wrote a change to git that introduced a segfault in 'git\n>     cat-file', I'm sure that this would not be the only place that in\n>     the suite that would break because of it. Presumably, at least one\n>     of those places uses 'test_must_fail git ...' instead\n>\n>   - At the very least, 'broken-commit' doesn't contain what it needs to,\n>     the test breaks in another way (albeit further from the actual\n>     defect), and the developer finds out about their bug that way.\n>\n> In any case, these above two might be moot anyways, because I'm almost\n> certain that it will be a rarity for someone to _only_ run t6102, unless\n> it is included in a blanket 'make' from within 't'.\n\nFirst. I realize we're talking about the light fixtures in the bike shed\nat this point, but with that in mind...\n\nI just think it's useful as a general rule, particularly since with the\n\"special setups\" in the test mode we've found that all sorts of odd\ntests we didn't expect to stress test some features turned out to cover\nedge cases of them, e.g. when \"GIT_TEST_COMMIT_GRAPH\" was added we found\nthat a bunch of random stuff segfaulted all over the place.\n\nSo it's hard to say with any confidence in advance that something isn't\ngoing to stress git in some unusual way and isn't useful to guard for\nsegfaults.\n\nOf course if and when it segfaults it's likely to be seen by subsequent\ntests. In this case I'll note that if git fails we'll happily run not\nonly perl/sed, but then hash-object the subsequent empty file, and then\n(presumably) fail in the next test.\n\n>> > I am sympathetic to the auditing issue, though.\n>\n> Just to satisfy my curiosity, I put git on the left-hand side of a pipe\n> to see how many such examples exist today:\n>\n>   ~/g/git (master) $ git grep 'git cat-file .* |' -- t/t*.sh | wc -l\n>       179\n>\n> I'm not going to claim that changing 179 -> 180 is neutral or bad, but\n> it's interesting nonetheless.\n\nSeparate from the question of if we generally agree that some value of\n\"Y\" makes for good coding style or not, we're always going to be in some\nflux state where a bunch of examples in our existing codebase contradict\nthat, particularly in the test suite.\n\nI think that's a bit unfortunate in some ways. It's the result of the\ndefault \"policy\" that refactoring for refactoring's sakes is generally\nfrowned upon, so we don't tend to go back and mass-fix \"X\" to \"Y\" once\nwe agree \"Y\" is better than \"X\" for new code, just do it as we go when\nnew code is written, or existing tests are updated for other reasons\n\"while we're at it\".\n\n>> > I dunno. In this case it is not too bad to do:\n>> >\n>> >   git cat-file commit $commit >good-commit &&\n>> >   perl ... <good-commit >broken-commit\n>> >\n>> > but I am not sure I am on board with a blanket rule of \"git must never\n>> > be on the left-hand side of a pipe\".\n>\n> I think that I mostly agree with Peff here for the reasons above.\n>\n> That all said, I don't really want to hold up the series for this alone.\n> Since there don't seem to be many other comments or issues, I would be\n> quite happy to do whatever people are most in favor of.\n\nFWIW this series LGTM as a whole. I think it says something about the\ngeneral quality of it that we're all in some giant nitpick thread about\nperl v.s. sed and git on the LHS of a pipe or not :) I'm happy to have\nit queued as-is. These test issues are minor...\n\n> I basically don't really feel strongly about writing:\n>\n>   git cat-file commit $commit | sed -e ... >broken-commit\n>\n> as opposed to:\n>\n>   git cat-file commit $commit >good-commit &&\n>   sed -e '' <commit >bad-commit\n>\n>> >> > > Wouldn't a 'sed' one-liner suffice, so we won't have yet another perl\n>> >> > > dependency?\n>> >> >\n>> >> > Heh, this was actually the subject of much discussion before the patches\n>> >> > hit the list. If you can write such a one-liner that is both readable\n>> >> > and portable, please share it. I got disgusted with sed and suggested\n>> >> > this perl.\n>> >>\n>> >> Hm, so the trivial s/// with '\\n' in the replacement part is not\n>> >> portable, then?  Oh, well.\n>> >\n>> > I don't think it is, but I could be wrong. POSIX does say that \"\\n\"\n>> > matches a newline in the pattern space, but nothing about it on the RHS\n>> > of a substitution. I have a vague feeling of running into problems in\n>> > the past, but I could just be misremembering.\n>> >\n>> > We also tried matching /^tree/ and using \"a\\\" to append a line, but it\n>> > was both hard to read and hit portability issues with bsd sed.\n>\n> I think that all of this discussion is addressed elsewhere in thread, but\n> the gist is that Eric provided a suitable sed invocation that I am going\n> to use instead of Peff's Perl.\n>\n>> > -Peff\n>\n> Thanks,\n> Taylor\n"},{"id":"373551","messageId":"20190410015416.GA56470@Taylors-MBP.hsd1.wa.comcast.net","threadId":"50877","inReplyTo":"CAPig+cR6v_+nmGLsnoSz6K262hQfC1R9Xdk8CfuHcx6MduZfcg@mail.gmail.com","subject":"Re: [PATCH 2/7] t: introduce tests for unexpected object types","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-04-10T01:54:16Z","receivedAt":"2019-04-10T01:54:21Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Eric,\n\nOn Tue, Apr 09, 2019 at 04:02:23AM -0400, Eric Sunshine wrote:\n> On Tue, Apr 9, 2019 at 1:09 AM Taylor Blau <me@ttaylorr.com> wrote:\n> > On Mon, Apr 08, 2019 at 11:28:19PM -0400, Eric Sunshine wrote:\n> > > I suspect that Junio's \"Looks good\" was referring to the 'sed expression.\n> >\n> > I think that you are right -- and I'll happily _not_ introduce more Git\n> > on the left-hand-side of a pipe instances.\n> >\n> > I noticed a few more instances in t6102 [...]\n>\n> Indeed, SZEDER mentioned those in [1]:\n\nAha, thanks -- I'm not sure how I missed that in [1]. Credit is given\nwhere it is due :-).\n\n>     Don't run git commands upstream of a pipe, because the pipe\n>     hides their exit code.  This applies to several other tests\n>     below as well.\n>\n> [1]: http://public-inbox.org/git/20190405105033.GT32732@szeder.dev/\n>\n> > I wrote the following patch, which I've folded into my local copy (and\n> > will send with v2):\n> >\n> > > With all the recent work of moving away from having Git upstream of a\n> > > pipe, let's not intentionally introduce a new instance. I wrote the\n> > > example 'sed' expression that way merely to mirror how the original\n> > > 'perl' version was written to make it easier to see the equivalence\n> > > (not because it was intended as an endorsement of having Git upstream\n> > > of a pipe).\n> >\n> > I see, and thank you for the clarification. Let me know if you like the\n> > patch above.\n>\n> Looks fine. Thanks.\n\nGreat -- I appreciate your review. I'll send v2 shortly with these\nchanges in it.\n\nThanks,\nTaylor\n"},{"id":"373552","messageId":"20190410015934.GB56470@Taylors-MBP.hsd1.wa.comcast.net","threadId":"50877","inReplyTo":"871s2ba6bb.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/7] t: introduce tests for unexpected object types","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-04-10T01:59:34Z","receivedAt":"2019-04-10T01:59:40Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Ævar,\n\nOn Tue, Apr 09, 2019 at 11:14:48AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n> On Tue, Apr 09 2019, Taylor Blau wrote:\n>\n> > Hi Ævar,\n> >\n> > On Sun, Apr 07, 2019 at 11:00:19PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> >>\n> >> On Fri, Apr 05 2019, Jeff King wrote:\n> >>\n> >> > On Fri, Apr 05, 2019 at 08:42:29PM +0200, SZEDER Gábor wrote:\n> >> >\n> >> >> > > Don't run git commands upstream of a pipe, because the pipe hides\n> >> >> > > their exit code.  This applies to several other tests below as well.\n> >> >> >\n> >> >> > I disagree with that rule here. We're not testing \"cat-file\" in any\n> >> >> > meaningful way, but just getting some stock output from a known-good\n> >> >> > commit.\n> >> >>\n> >> >> It makes auditing harder and sets bad example.\n> >> >\n> >> > I disagree that it's a bad example. Just because a reader might not\n> >> > realize that it is sometimes OK and sometimes not, does not make it bad\n> >> > to do so in the OK case. It means the reader needs to understand the\n> >> > rule in order to correctly apply it.\n> >>\n> >> FWIW I thought the rule was something to the effect of \"we're hacking on\n> >> git, any change might segfault in some unexpected test, let's make sure\n> >> that fails right away\", hence the blanket rule in t/README against \"!\n> >> git <cmd>\" in favor of \"test_must_fail git <cmd>\", and \"don't feed the\n> >> output of a git command to a pipe\" documented right after that.\n> >\n> > I have some loosely-held opinions on this. Certainly knowing if a change\n> > to git caused a segfault in some test is something that we would want to\n> > know about, though I'm not sure we're loosing anything by putting 'git'\n> > on the left-hand side of a pipe here.\n> >\n> >   - If someone wrote a change to git that introduced a segfault in 'git\n> >     cat-file', I'm sure that this would not be the only place that in\n> >     the suite that would break because of it. Presumably, at least one\n> >     of those places uses 'test_must_fail git ...' instead\n> >\n> >   - At the very least, 'broken-commit' doesn't contain what it needs to,\n> >     the test breaks in another way (albeit further from the actual\n> >     defect), and the developer finds out about their bug that way.\n> >\n> > In any case, these above two might be moot anyways, because I'm almost\n> > certain that it will be a rarity for someone to _only_ run t6102, unless\n> > it is included in a blanket 'make' from within 't'.\n>\n> First. I realize we're talking about the light fixtures in the bike shed\n> at this point, but with that in mind...\n\nI don't mind some bike-shedding ;-).\n\n> I just think it's useful as a general rule, particularly since with the\n> \"special setups\" in the test mode we've found that all sorts of odd\n> tests we didn't expect to stress test some features turned out to cover\n> edge cases of them, e.g. when \"GIT_TEST_COMMIT_GRAPH\" was added we found\n> that a bunch of random stuff segfaulted all over the place.\n>\n> So it's hard to say with any confidence in advance that something isn't\n> going to stress git in some unusual way and isn't useful to guard for\n> segfaults.\n\nYes, I think that's certainly true. There's something to be said for my\nargument, too, (i.e., that it's ``probably'' just fine), but as I think\nabout it more, I'm less convinced that mine is a position worth holding.\n\nEven if we catch only a bug or two on an off chance, there aren't too\nmany things I'd sacrifice to not make it so. In other words, I'm willing\nto do quite a lot in order to get even a little indication of when\nthings are broken (including replacing 'git ... | sed' with 'git ... >foo &&\nsed ... <foo').\n\n> Of course if and when it segfaults it's likely to be seen by subsequent\n> tests. In this case I'll note that if git fails we'll happily run not\n> only perl/sed, but then hash-object the subsequent empty file, and then\n> (presumably) fail in the next test.\n\nSure, we'll probably find out about it anyway, but still...\n\n> >> > I am sympathetic to the auditing issue, though.\n> >\n> > Just to satisfy my curiosity, I put git on the left-hand side of a pipe\n> > to see how many such examples exist today:\n> >\n> >   ~/g/git (master) $ git grep 'git cat-file .* |' -- t/t*.sh | wc -l\n> >       179\n> >\n> > I'm not going to claim that changing 179 -> 180 is neutral or bad, but\n> > it's interesting nonetheless.\n>\n> Separate from the question of if we generally agree that some value of\n> \"Y\" makes for good coding style or not, we're always going to be in some\n> flux state where a bunch of examples in our existing codebase contradict\n> that, particularly in the test suite.\n>\n> I think that's a bit unfortunate in some ways. It's the result of the\n> default \"policy\" that refactoring for refactoring's sakes is generally\n> frowned upon, so we don't tend to go back and mass-fix \"X\" to \"Y\" once\n> we agree \"Y\" is better than \"X\" for new code, just do it as we go when\n> new code is written, or existing tests are updated for other reasons\n> \"while we're at it\".\n\nIs that refactoring for its own sake, though? I would personally be\nhappy to write a series that either (1) identifies spots in 't' where\n`git` is found on the left-hand side of a pipe, (2) fixes the bad-style\ninvocation in those spots, or (3) both.\n\n> >> > I dunno. In this case it is not too bad to do:\n> >> >\n> >> >   git cat-file commit $commit >good-commit &&\n> >> >   perl ... <good-commit >broken-commit\n> >> >\n> >> > but I am not sure I am on board with a blanket rule of \"git must never\n> >> > be on the left-hand side of a pipe\".\n> >\n> > I think that I mostly agree with Peff here for the reasons above.\n> >\n> > That all said, I don't really want to hold up the series for this alone.\n> > Since there don't seem to be many other comments or issues, I would be\n> > quite happy to do whatever people are most in favor of.\n>\n> FWIW this series LGTM as a whole. I think it says something about the\n> general quality of it that we're all in some giant nitpick thread about\n> perl v.s. sed and git on the LHS of a pipe or not :) I'm happy to have\n> it queued as-is. These test issues are minor...\n\nThanks, I appreciate that you think the series is in good shape. Since\nthere haven't been too many comments other than this thread, I'm going\nto send v2 now.\n\nI decided to ban t6102 of all such 'git ... | sed' invocations, and\ndescribed the change earlier in this thread.\n\n> > I basically don't really feel strongly about writing:\n> >\n> >   git cat-file commit $commit | sed -e ... >broken-commit\n> >\n> > as opposed to:\n> >\n> >   git cat-file commit $commit >good-commit &&\n> >   sed -e '' <commit >bad-commit\n> >\n> >> >> > > Wouldn't a 'sed' one-liner suffice, so we won't have yet another perl\n> >> >> > > dependency?\n> >> >> >\n> >> >> > Heh, this was actually the subject of much discussion before the patches\n> >> >> > hit the list. If you can write such a one-liner that is both readable\n> >> >> > and portable, please share it. I got disgusted with sed and suggested\n> >> >> > this perl.\n> >> >>\n> >> >> Hm, so the trivial s/// with '\\n' in the replacement part is not\n> >> >> portable, then?  Oh, well.\n> >> >\n> >> > I don't think it is, but I could be wrong. POSIX does say that \"\\n\"\n> >> > matches a newline in the pattern space, but nothing about it on the RHS\n> >> > of a substitution. I have a vague feeling of running into problems in\n> >> > the past, but I could just be misremembering.\n> >> >\n> >> > We also tried matching /^tree/ and using \"a\\\" to append a line, but it\n> >> > was both hard to read and hit portability issues with bsd sed.\n> >\n> > I think that all of this discussion is addressed elsewhere in thread, but\n> > the gist is that Eric provided a suitable sed invocation that I am going\n> > to use instead of Peff's Perl.\n> >\n> >> > -Peff\n> >\n> > Thanks,\n> > Taylor\n\nThanks,\nTaylor\n"},{"id":"373553","messageId":"cover.1554861974.git.me@ttaylorr.com","threadId":"50877","inReplyTo":"cover.1554435033.git.me@ttaylorr.com","subject":"[PATCH v2 0/7] harden unexpected object types checks","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-04-10T02:13:06Z","receivedAt":"2019-04-10T02:13:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi everyone,\n\nHere is 'v2' of mine and Peff's series to add tests for and fix some\ncases when object connections are malformed in various ways.\n\nThere aren't many major changes from the initial version, but what has\nchanged is described briefly here, and in complete detail in the\nrange-diff at the bottom of this message:\n\n  - [2/7] was amended with Peff's suggestion to be a little clearer about\n    what 'lone' and 'seen' mean\n\n  - [2/7] was changed to replace all instances of:\n\n      git cat-file ... | sed -e ... >broken-obj\n\n    with:\n\n      git cat-file ... >good-obj &&\n      sed -e ... <good-obj >broken-obj\n\n  - [4/7] was cleaned up to include the right test after a faulty\n    rebase.\n\nThanks as always for your review :-).\n\n\nThanks,\nTaylor\n\nJeff King (3):\n  get_commit_tree(): return NULL for broken tree\n  rev-list: let traversal die when --missing is not in use\n  rev-list: detect broken root trees\n\nTaylor Blau (4):\n  t: move 'hex2oct' into test-lib-functions.sh\n  t: introduce tests for unexpected object types\n  list-objects.c: handle unexpected non-blob entries\n  list-objects.c: handle unexpected non-tree entries\n\n builtin/rev-list.c                     |   4 +-\n commit.c                               |   6 +-\n list-objects.c                         |  13 +++\n t/t1007-hash-object.sh                 |   4 -\n t/t1450-fsck.sh                        |   4 -\n t/t5601-clone.sh                       |   4 -\n t/t6102-rev-list-unexpected-objects.sh | 127 +++++++++++++++++++++++++\n t/test-lib-functions.sh                |   6 ++\n 8 files changed, 152 insertions(+), 16 deletions(-)\n create mode 100755 t/t6102-rev-list-unexpected-objects.sh\n\nRange-diff against v1:\n1:  2104508e42 = 1:  f09c374557 t: move 'hex2oct' into test-lib-functions.sh\n2:  909cbcd4a1 ! 2:  137e2df24d t: introduce tests for unexpected object types\n    @@ -20,21 +20,19 @@\n         cases that are not broken (i.e., they have NULL-ness checks or similar),\n         mark these as expecting success.\n\n    -    Let A be the object referenced with an unexpected type, and B be the\n    -    object doing the referencing. Do the following:\n    -\n    -      - test 'git rev-list --objects A B'. This causes A to be \"cached\", and\n    -        presents the above scenario.\n    -\n    -    Likewise, if we have a tree entry that claims to be a tree (for example)\n    -    but points to another object type (say, a blob), there are two ways we\n    -    might find out:\n    +    We might hit an unexpected type in two different ways (imagine we have a\n    +    tree entry that claims to be a tree but actually points to a blob):\n\n           - when we call lookup_tree(), we might find that we've already seen\n    -        the object referenced as another type, in which case we'd get NULL\n    +        the object referenced as a blob, in which case we'd get NULL. We\n    +        can exercise this with \"git rev-list --objects $blob $tree\", which\n    +        guarantees that the blob will have been parsed before we look in\n    +        the tree. These tests are marked as \"seen\" in the test script.\n\n           - we call lookup_tree() successfully, but when we try to read the\n    -        object, we find out it's something else.\n    +        object, we find out it's something else. We construct our tests\n    +        such that $blob is not otherwise mentioned in $tree. These tests\n    +        are marked as \"lone\" in the script.\n\n         We should check that we behave sensibly in both cases (especially\n         because it is easy for a malicious actor to provoke one case or the\n    @@ -58,7 +56,8 @@\n     +test_expect_success 'setup well-formed objects' '\n     +\tblob=\"$(printf \"foo\" | git hash-object -w --stdin)\" &&\n     +\ttree=\"$(printf \"100644 blob $blob\\tfoo\" | git mktree)\" &&\n    -+\tcommit=\"$(git commit-tree $tree -m \"first commit\")\"\n    ++\tcommit=\"$(git commit-tree $tree -m \"first commit\")\" &&\n    ++\tgit cat-file commit $commit >good-commit\n     +'\n     +\n     +test_expect_success 'setup unexpected non-blob entry' '\n    @@ -84,12 +83,11 @@\n     +'\n     +\n     +test_expect_failure 'traverse unexpected non-tree entry (seen)' '\n    -+\ttest_must_fail git rev-list --objects $blob $broken_tree >output 2>&1\n    ++\ttest_must_fail git rev-list --objects $blob $broken_tree\n     +'\n     +\n     +test_expect_success 'setup unexpected non-commit parent' '\n    -+\tgit cat-file commit $commit |\n    -+\t\tperl -lpe \"/^author/ && print q(parent $blob)\" \\\n    ++\tsed \"/^author/ { h; s/.*/parent $blob/; G; }\" <good-commit \\\n     +\t\t>broken-commit &&\n     +\tbroken_commit=\"$(git hash-object -w --literally -t commit \\\n     +\t\tbroken-commit)\"\n    @@ -107,8 +105,7 @@\n     +'\n     +\n     +test_expect_success 'setup unexpected non-tree root' '\n    -+\tgit cat-file commit $commit |\n    -+\tsed -e \"s/$tree/$blob/\" >broken-commit &&\n    ++\tsed -e \"s/$tree/$blob/\" <good-commit >broken-commit &&\n     +\tbroken_commit=\"$(git hash-object -w --literally -t commit \\\n     +\t\tbroken-commit)\"\n     +'\n    @@ -123,8 +120,9 @@\n     +\n     +test_expect_success 'setup unexpected non-commit tag' '\n     +\tgit tag -a -m \"tagged commit\" tag $commit &&\n    ++\tgit cat-file tag tag >good-tag &&\n     +\ttest_when_finished \"git tag -d tag\" &&\n    -+\tgit cat-file -p tag | sed -e \"s/$commit/$blob/\" >broken-tag &&\n    ++\tsed -e \"s/$commit/$blob/\" <good-tag >broken-tag &&\n     +\ttag=$(git hash-object -w --literally -t tag broken-tag)\n     +'\n     +\n    @@ -139,9 +137,9 @@\n     +\n     +test_expect_success 'setup unexpected non-tree tag' '\n     +\tgit tag -a -m \"tagged tree\" tag $tree &&\n    ++\tgit cat-file tag tag >good-tag &&\n     +\ttest_when_finished \"git tag -d tag\" &&\n    -+\tgit cat-file -p tag |\n    -+\tsed -e \"s/$tree/$blob/\" >broken-tag &&\n    ++\tsed -e \"s/$tree/$blob/\" <good-tag >broken-tag &&\n     +\ttag=$(git hash-object -w --literally -t tag broken-tag)\n     +'\n     +\n    @@ -156,9 +154,9 @@\n     +\n     +test_expect_success 'setup unexpected non-blob tag' '\n     +\tgit tag -a -m \"tagged blob\" tag $blob &&\n    ++\tgit cat-file tag tag >good-tag &&\n     +\ttest_when_finished \"git tag -d tag\" &&\n    -+\tgit cat-file -p tag |\n    -+\tsed -e \"s/$blob/$commit/\" >broken-tag &&\n    ++\tsed -e \"s/$blob/$commit/\" <good-tag >broken-tag &&\n     +\ttag=$(git hash-object -w --literally -t tag broken-tag)\n     +'\n     +\n3:  42f1b5d5fd = 3:  1d1ac9b7a7 list-objects.c: handle unexpected non-blob entries\n4:  62c9a6f2e0 ! 4:  97c7e23ec9 list-objects.c: handle unexpected non-tree entries\n    @@ -31,7 +31,7 @@\n      '\n\n     -test_expect_failure 'traverse unexpected non-tree entry (seen)' '\n    --\ttest_must_fail git rev-list --objects $blob $broken_tree >output 2>&1\n    +-\ttest_must_fail git rev-list --objects $blob $broken_tree\n     +test_expect_success 'traverse unexpected non-tree entry (seen)' '\n     +\ttest_must_fail git rev-list --objects $blob $broken_tree >output 2>&1 &&\n     +\ttest_i18ngrep \"is not a tree\" output\n5:  d0ae70542d = 5:  e9400a9f77 get_commit_tree(): return NULL for broken tree\n6:  a0c23c8c16 = 6:  88ca5dfe68 rev-list: let traversal die when --missing is not in use\n7:  2a25796147 = 7:  e0bd479e82 rev-list: detect broken root trees\n--\n2.21.0.203.g358da99528\n"},{"id":"373554","messageId":"f09c374557070b286391f1f69c4288d7d0769851.1554861974.git.me@ttaylorr.com","threadId":"50877","inReplyTo":"cover.1554861974.git.me@ttaylorr.com","subject":"[PATCH v2 1/7] t: move 'hex2oct' into test-lib-functions.sh","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-04-10T02:13:11Z","receivedAt":"2019-04-10T02:13:16Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The helper 'hex2oct' is used to convert base-16 encoded data into a\nbase-8 binary form, and is useful for preparing data for commands that\naccept input in a binary format, such as 'git hash-object', via\n'printf'.\n\nThis helper is defined identically in three separate places throughout\n't'. Move the definition to test-lib-function.sh, so that it can be used\nin new test suites, and its definition is not redundant.\n\nThis will likewise make our job easier in the subsequent commit, which\nalso uses 'hex2oct'.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/t1007-hash-object.sh  | 4 ----\n t/t1450-fsck.sh         | 4 ----\n t/t5601-clone.sh        | 4 ----\n t/test-lib-functions.sh | 6 ++++++\n 4 files changed, 6 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh\nindex a37753047e..7099d33508 100755\n--- a/t/t1007-hash-object.sh\n+++ b/t/t1007-hash-object.sh\n@@ -199,10 +199,6 @@ test_expect_success 'too-short tree' '\n \ttest_i18ngrep \"too-short tree object\" err\n '\n \n-hex2oct() {\n-    perl -ne 'printf \"\\\\%03o\", hex for /../g'\n-}\n-\n test_expect_success 'malformed mode in tree' '\n \thex_sha1=$(echo foo | git hash-object --stdin -w) &&\n \tbin_sha1=$(echo $hex_sha1 | hex2oct) &&\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 49f08d5b9c..0f268a3664 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -256,10 +256,6 @@ test_expect_success 'unparseable tree object' '\n \ttest_i18ngrep ! \"fatal: empty filename in tree entry\" out\n '\n \n-hex2oct() {\n-\tperl -ne 'printf \"\\\\%03o\", hex for /../g'\n-}\n-\n test_expect_success 'tree entry with type mismatch' '\n \ttest_when_finished \"remove_object \\$blob\" &&\n \ttest_when_finished \"remove_object \\$tree\" &&\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex d6948cbdab..3f49943010 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -611,10 +611,6 @@ test_expect_success 'GIT_TRACE_PACKFILE produces a usable pack' '\n \tgit -C replay.git index-pack -v --stdin <tmp.pack\n '\n \n-hex2oct () {\n-\tperl -ne 'printf \"\\\\%03o\", hex for /../g'\n-}\n-\n test_expect_success 'clone on case-insensitive fs' '\n \tgit init icasefs &&\n \t(\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 80402a428f..349eabe851 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1202,6 +1202,12 @@ depacketize () {\n \t'\n }\n \n+# Converts base-16 data into base-8. The output is given as a sequence of\n+# escaped octals, suitable for consumption by 'printf'.\n+hex2oct () {\n+\tperl -ne 'printf \"\\\\%03o\", hex for /../g'\n+}\n+\n # Set the hash algorithm in use to $1.  Only useful when testing the testsuite.\n test_set_hash () {\n \ttest_hash_algo=\"$1\"\n-- \n2.21.0.203.g358da99528\n\n"},{"id":"373555","messageId":"137e2df24d80f796675691e57d08948b6eede6e3.1554861974.git.me@ttaylorr.com","threadId":"50877","inReplyTo":"cover.1554861974.git.me@ttaylorr.com","subject":"[PATCH v2 2/7] t: introduce tests for unexpected object types","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-04-10T02:13:14Z","receivedAt":"2019-04-10T02:13:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Call an object's type \"unexpected\" when the actual type of an object\ndoes not match Git's contextual expectation. For example, a tree entry\nwhose mode differs from the object's actual type, or a commit's parent\nwhich is not another commit, and so on.\n\nThis can manifest itself in various unfortunate ways, including Git\nSIGSEGV-ing under specific conditions. Consider the following example:\nGit traverses a blob (say, via `git rev-list`), and then tries to read\nout a tree-entry which lists that object as something other than a blob.\nIn this case, `lookup_blob()` will return NULL, and the subsequent\ndereference will result in a SIGSEGV.\n\nIntroduce tests that present objects of \"unexpected\" type in the above\nfashion to 'git rev-list'. Mark as failures the combinations that are\nalready broken (i.e., they exhibit the segfault described above). In the\ncases that are not broken (i.e., they have NULL-ness checks or similar),\nmark these as expecting success.\n\nWe might hit an unexpected type in two different ways (imagine we have a\ntree entry that claims to be a tree but actually points to a blob):\n\n  - when we call lookup_tree(), we might find that we've already seen\n    the object referenced as a blob, in which case we'd get NULL. We\n    can exercise this with \"git rev-list --objects $blob $tree\", which\n    guarantees that the blob will have been parsed before we look in\n    the tree. These tests are marked as \"seen\" in the test script.\n\n  - we call lookup_tree() successfully, but when we try to read the\n    object, we find out it's something else. We construct our tests\n    such that $blob is not otherwise mentioned in $tree. These tests\n    are marked as \"lone\" in the script.\n\nWe should check that we behave sensibly in both cases (especially\nbecause it is easy for a malicious actor to provoke one case or the\nother).\n\nCo-authored-by: Jeff King <peff@peff.net>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t6102-rev-list-unexpected-objects.sh | 123 +++++++++++++++++++++++++\n 1 file changed, 123 insertions(+)\n create mode 100755 t/t6102-rev-list-unexpected-objects.sh\n\ndiff --git a/t/t6102-rev-list-unexpected-objects.sh b/t/t6102-rev-list-unexpected-objects.sh\nnew file mode 100755\nindex 0000000000..15072ecce3\n--- /dev/null\n+++ b/t/t6102-rev-list-unexpected-objects.sh\n@@ -0,0 +1,123 @@\n+#!/bin/sh\n+\n+test_description='git rev-list should handle unexpected object types'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup well-formed objects' '\n+\tblob=\"$(printf \"foo\" | git hash-object -w --stdin)\" &&\n+\ttree=\"$(printf \"100644 blob $blob\\tfoo\" | git mktree)\" &&\n+\tcommit=\"$(git commit-tree $tree -m \"first commit\")\" &&\n+\tgit cat-file commit $commit >good-commit\n+'\n+\n+test_expect_success 'setup unexpected non-blob entry' '\n+\tprintf \"100644 foo\\0$(echo $tree | hex2oct)\" >broken-tree &&\n+\tbroken_tree=\"$(git hash-object -w --literally -t tree broken-tree)\"\n+'\n+\n+test_expect_failure 'traverse unexpected non-blob entry (lone)' '\n+\ttest_must_fail git rev-list --objects $broken_tree\n+'\n+\n+test_expect_failure 'traverse unexpected non-blob entry (seen)' '\n+\ttest_must_fail git rev-list --objects $tree $broken_tree\n+'\n+\n+test_expect_success 'setup unexpected non-tree entry' '\n+\tprintf \"40000 foo\\0$(echo $blob | hex2oct)\" >broken-tree &&\n+\tbroken_tree=\"$(git hash-object -w --literally -t tree broken-tree)\"\n+'\n+\n+test_expect_failure 'traverse unexpected non-tree entry (lone)' '\n+\ttest_must_fail git rev-list --objects $broken_tree\n+'\n+\n+test_expect_failure 'traverse unexpected non-tree entry (seen)' '\n+\ttest_must_fail git rev-list --objects $blob $broken_tree\n+'\n+\n+test_expect_success 'setup unexpected non-commit parent' '\n+\tsed \"/^author/ { h; s/.*/parent $blob/; G; }\" <good-commit \\\n+\t\t>broken-commit &&\n+\tbroken_commit=\"$(git hash-object -w --literally -t commit \\\n+\t\tbroken-commit)\"\n+'\n+\n+test_expect_success 'traverse unexpected non-commit parent (lone)' '\n+\ttest_must_fail git rev-list --objects $broken_commit >output 2>&1 &&\n+\ttest_i18ngrep \"not a commit\" output\n+'\n+\n+test_expect_success 'traverse unexpected non-commit parent (seen)' '\n+\ttest_must_fail git rev-list --objects $commit $broken_commit \\\n+\t\t>output 2>&1 &&\n+\ttest_i18ngrep \"not a commit\" output\n+'\n+\n+test_expect_success 'setup unexpected non-tree root' '\n+\tsed -e \"s/$tree/$blob/\" <good-commit >broken-commit &&\n+\tbroken_commit=\"$(git hash-object -w --literally -t commit \\\n+\t\tbroken-commit)\"\n+'\n+\n+test_expect_failure 'traverse unexpected non-tree root (lone)' '\n+\ttest_must_fail git rev-list --objects $broken_commit\n+'\n+\n+test_expect_failure 'traverse unexpected non-tree root (seen)' '\n+\ttest_must_fail git rev-list --objects $blob $broken_commit\n+'\n+\n+test_expect_success 'setup unexpected non-commit tag' '\n+\tgit tag -a -m \"tagged commit\" tag $commit &&\n+\tgit cat-file tag tag >good-tag &&\n+\ttest_when_finished \"git tag -d tag\" &&\n+\tsed -e \"s/$commit/$blob/\" <good-tag >broken-tag &&\n+\ttag=$(git hash-object -w --literally -t tag broken-tag)\n+'\n+\n+test_expect_success 'traverse unexpected non-commit tag (lone)' '\n+\ttest_must_fail git rev-list --objects $tag\n+'\n+\n+test_expect_success 'traverse unexpected non-commit tag (seen)' '\n+\ttest_must_fail git rev-list --objects $blob $tag >output 2>&1 &&\n+\ttest_i18ngrep \"not a commit\" output\n+'\n+\n+test_expect_success 'setup unexpected non-tree tag' '\n+\tgit tag -a -m \"tagged tree\" tag $tree &&\n+\tgit cat-file tag tag >good-tag &&\n+\ttest_when_finished \"git tag -d tag\" &&\n+\tsed -e \"s/$tree/$blob/\" <good-tag >broken-tag &&\n+\ttag=$(git hash-object -w --literally -t tag broken-tag)\n+'\n+\n+test_expect_success 'traverse unexpected non-tree tag (lone)' '\n+\ttest_must_fail git rev-list --objects $tag\n+'\n+\n+test_expect_success 'traverse unexpected non-tree tag (seen)' '\n+\ttest_must_fail git rev-list --objects $blob $tag >output 2>&1 &&\n+\ttest_i18ngrep \"not a tree\" output\n+'\n+\n+test_expect_success 'setup unexpected non-blob tag' '\n+\tgit tag -a -m \"tagged blob\" tag $blob &&\n+\tgit cat-file tag tag >good-tag &&\n+\ttest_when_finished \"git tag -d tag\" &&\n+\tsed -e \"s/$blob/$commit/\" <good-tag >broken-tag &&\n+\ttag=$(git hash-object -w --literally -t tag broken-tag)\n+'\n+\n+test_expect_failure 'traverse unexpected non-blob tag (lone)' '\n+\ttest_must_fail git rev-list --objects $tag\n+'\n+\n+test_expect_success 'traverse unexpected non-blob tag (seen)' '\n+\ttest_must_fail git rev-list --objects $commit $tag >output 2>&1 &&\n+\ttest_i18ngrep \"not a blob\" output\n+'\n+\n+test_done\n-- \n2.21.0.203.g358da99528\n\n"},{"id":"373556","messageId":"1d1ac9b7a74fefccb6003f4ab8eb1a6d3660dff7.1554861974.git.me@ttaylorr.com","threadId":"50877","inReplyTo":"cover.1554861974.git.me@ttaylorr.com","subject":"[PATCH v2 3/7] list-objects.c: handle unexpected non-blob entries","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-04-10T02:13:17Z","receivedAt":"2019-04-10T02:13:21Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Fix one of the cases described in the previous commit where a tree-entry\nthat is promised to a blob is in fact a non-blob.\n\nWhen 'lookup_blob()' returns NULL, it is because Git has cached the\nrequested object as a non-blob. In this case, prevent a SIGSEGV by\n'die()'-ing immediately before attempting to dereference the result.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n list-objects.c                         | 5 +++++\n t/t6102-rev-list-unexpected-objects.sh | 5 +++--\n 2 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/list-objects.c b/list-objects.c\nindex dc77361e11..ea04bbdee6 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -133,6 +133,11 @@ static void process_tree_contents(struct traversal_context *ctx,\n \t\t\t\t\tbase, entry.path);\n \t\telse {\n \t\t\tstruct blob *b = lookup_blob(ctx->revs->repo, &entry.oid);\n+\t\t\tif (!b) {\n+\t\t\t\tdie(_(\"entry '%s' in tree %s has blob mode, \"\n+\t\t\t\t      \"but is not a blob\"),\n+\t\t\t\t    entry.path, oid_to_hex(&tree->object.oid));\n+\t\t\t}\n \t\t\tb->object.flags |= NOT_USER_GIVEN;\n \t\t\tprocess_blob(ctx, b, base, entry.path);\n \t\t}\ndiff --git a/t/t6102-rev-list-unexpected-objects.sh b/t/t6102-rev-list-unexpected-objects.sh\nindex 15072ecce3..1377c60378 100755\n--- a/t/t6102-rev-list-unexpected-objects.sh\n+++ b/t/t6102-rev-list-unexpected-objects.sh\n@@ -20,8 +20,9 @@ test_expect_failure 'traverse unexpected non-blob entry (lone)' '\n \ttest_must_fail git rev-list --objects $broken_tree\n '\n \n-test_expect_failure 'traverse unexpected non-blob entry (seen)' '\n-\ttest_must_fail git rev-list --objects $tree $broken_tree\n+test_expect_success 'traverse unexpected non-blob entry (seen)' '\n+\ttest_must_fail git rev-list --objects $tree $broken_tree >output 2>&1 &&\n+\ttest_i18ngrep \"is not a blob\" output\n '\n \n test_expect_success 'setup unexpected non-tree entry' '\n-- \n2.21.0.203.g358da99528\n\n"},{"id":"373557","messageId":"97c7e23ec9c2ac33abb2e110c5425482ecfd1925.1554861974.git.me@ttaylorr.com","threadId":"50877","inReplyTo":"cover.1554861974.git.me@ttaylorr.com","subject":"[PATCH v2 4/7] list-objects.c: handle unexpected non-tree entries","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-04-10T02:13:19Z","receivedAt":"2019-04-10T02:13:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Apply similar treatment as the previous commit for non-tree entries,\ntoo.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n list-objects.c                         | 5 +++++\n t/t6102-rev-list-unexpected-objects.sh | 5 +++--\n 2 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/list-objects.c b/list-objects.c\nindex ea04bbdee6..bb7e61ef4b 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -125,6 +125,11 @@ static void process_tree_contents(struct traversal_context *ctx,\n \n \t\tif (S_ISDIR(entry.mode)) {\n \t\t\tstruct tree *t = lookup_tree(ctx->revs->repo, &entry.oid);\n+\t\t\tif (!t) {\n+\t\t\t\tdie(_(\"entry '%s' in tree %s has tree mode, \"\n+\t\t\t\t      \"but is not a tree\"),\n+\t\t\t\t    entry.path, oid_to_hex(&tree->object.oid));\n+\t\t\t}\n \t\t\tt->object.flags |= NOT_USER_GIVEN;\n \t\t\tprocess_tree(ctx, t, base, entry.path);\n \t\t}\ndiff --git a/t/t6102-rev-list-unexpected-objects.sh b/t/t6102-rev-list-unexpected-objects.sh\nindex 1377c60378..e3ec195532 100755\n--- a/t/t6102-rev-list-unexpected-objects.sh\n+++ b/t/t6102-rev-list-unexpected-objects.sh\n@@ -34,8 +34,9 @@ test_expect_failure 'traverse unexpected non-tree entry (lone)' '\n \ttest_must_fail git rev-list --objects $broken_tree\n '\n \n-test_expect_failure 'traverse unexpected non-tree entry (seen)' '\n-\ttest_must_fail git rev-list --objects $blob $broken_tree\n+test_expect_success 'traverse unexpected non-tree entry (seen)' '\n+\ttest_must_fail git rev-list --objects $blob $broken_tree >output 2>&1 &&\n+\ttest_i18ngrep \"is not a tree\" output\n '\n \n test_expect_success 'setup unexpected non-commit parent' '\n-- \n2.21.0.203.g358da99528\n\n"},{"id":"373558","messageId":"e9400a9f773adb62315f75347ad7231b4476a2f4.1554861974.git.me@ttaylorr.com","threadId":"50877","inReplyTo":"cover.1554861974.git.me@ttaylorr.com","subject":"[PATCH v2 5/7] get_commit_tree(): return NULL for broken tree","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-04-10T02:13:20Z","receivedAt":"2019-04-10T02:13:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"From: Jeff King <peff@peff.net>\n\nReturn NULL from 'get_commit_tree()' when a commit's root tree is\ncorrupt, doesn't exist, or points to an object which is not a tree.\n\nIn [1], this situation became a BUG(), but it can certainly occur in\ncases which are not a bug in Git, for e.g., if a caller manually crafts\na commit whose tree is corrupt in any of the above ways.\n\nNote that the expect_failure test in t6102 triggers this BUG(), but we\ncan't flip it to expect_success yet. Solving this problem actually\nreveals a second bug.\n\n[1]: 7b8a21dba1 (commit-graph: lazy-load trees for commits, 2018-04-06)\n\nCo-authored-by: Taylor Blau <me@ttaylorr.com>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n commit.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex a5333c7ac6..e2cde566a9 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -345,10 +345,10 @@ struct tree *get_commit_tree(const struct commit *commit)\n \tif (commit->maybe_tree || !commit->object.parsed)\n \t\treturn commit->maybe_tree;\n \n-\tif (commit->graph_pos == COMMIT_NOT_FROM_GRAPH)\n-\t\tBUG(\"commit has NULL tree, but was not loaded from commit-graph\");\n+\tif (commit->graph_pos != COMMIT_NOT_FROM_GRAPH)\n+\t\treturn get_commit_tree_in_graph(the_repository, commit);\n \n-\treturn get_commit_tree_in_graph(the_repository, commit);\n+\treturn NULL;\n }\n \n struct object_id *get_commit_tree_oid(const struct commit *commit)\n-- \n2.21.0.203.g358da99528\n\n"},{"id":"373559","messageId":"88ca5dfe68c4cdb558aff6e90a525ec06f15dbd6.1554861974.git.me@ttaylorr.com","threadId":"50877","inReplyTo":"cover.1554861974.git.me@ttaylorr.com","subject":"[PATCH v2 6/7] rev-list: let traversal die when --missing is not in use","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-04-10T02:13:23Z","receivedAt":"2019-04-10T02:13:27Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"From: Jeff King <peff@peff.net>\n\nCommit 7c0fe330d5 (rev-list: handle missing tree objects properly,\n2018-10-05) taught the traversal machinery used by git-rev-list to\nignore missing trees, so that rev-list could handle them itself.\n\nHowever, it does so only by checking via oid_object_info_extended() that\nthe object exists at all. This can miss several classes of errors that\nwere previously detected by rev-list:\n\n  - type mismatches (e.g., we expected a tree but got a blob)\n\n  - failure to read the object data (e.g., due to bitrot on disk)\n\nThis is especially important because we use \"rev-list --objects\" as our\nconnectivity check to admit new objects to the repository, and it will\nnow miss these cases (though the bitrot one is less important here,\nbecause we'd typically have just hashed and stored the object).\n\nThere are a few options to fix this:\n\n 1. we could check these properties in rev-list when we do the existence\n    check. This is probably too expensive in practice (perhaps even for\n    a type check, but definitely for checking the whole content again,\n    which implies loading each object into memory twice).\n\n 2. teach the traversal machinery to differentiate between a missing\n    object, and one that could not be loaded as expected. This probably\n    wouldn't be too hard to detect type mismatches, but detecting bitrot\n    versus a truly missing object would require deep changes to the\n    object-loading code.\n\n 3. have the traversal machinery communicate the failure to the caller,\n    so that it can decide how to proceed without re-evaluting the object\n    itself.\n\nOf those, I think (3) is probably the best path forward. However, this\npatch does none of them. In the name of expediently fixing the\nregression to a normal \"rev-list --objects\" that we use for connectivity\nchecks, this simply restores the pre-7c0fe330d5 behavior of having the\ntraversal die as soon as it fails to load a tree (when --missing is set\nto MA_ERROR, which is the default).\n\nNote that we can't get rid of the object-existence check in\nfinish_object(), because this also handles blobs (which are not\notherwise checked at all by the traversal code).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/rev-list.c                     | 4 +++-\n t/t6102-rev-list-unexpected-objects.sh | 4 ++--\n 2 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 425a5774db..9f31837d30 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -379,7 +379,6 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \trepo_init_revisions(the_repository, &revs, prefix);\n \trevs.abbrev = DEFAULT_ABBREV;\n \trevs.commit_format = CMIT_FMT_UNSPECIFIED;\n-\trevs.do_not_die_on_missing_tree = 1;\n \n \t/*\n \t * Scan the argument list before invoking setup_revisions(), so that we\n@@ -409,6 +408,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n+\tif (arg_missing_action)\n+\t\trevs.do_not_die_on_missing_tree = 1;\n+\n \targc = setup_revisions(argc, argv, &revs, &s_r_opt);\n \n \tmemset(&info, 0, sizeof(info));\ndiff --git a/t/t6102-rev-list-unexpected-objects.sh b/t/t6102-rev-list-unexpected-objects.sh\nindex e3ec195532..28ee1bcb07 100755\n--- a/t/t6102-rev-list-unexpected-objects.sh\n+++ b/t/t6102-rev-list-unexpected-objects.sh\n@@ -30,7 +30,7 @@ test_expect_success 'setup unexpected non-tree entry' '\n \tbroken_tree=\"$(git hash-object -w --literally -t tree broken-tree)\"\n '\n \n-test_expect_failure 'traverse unexpected non-tree entry (lone)' '\n+test_expect_success 'traverse unexpected non-tree entry (lone)' '\n \ttest_must_fail git rev-list --objects $broken_tree\n '\n \n@@ -63,7 +63,7 @@ test_expect_success 'setup unexpected non-tree root' '\n \t\tbroken-commit)\"\n '\n \n-test_expect_failure 'traverse unexpected non-tree root (lone)' '\n+test_expect_success 'traverse unexpected non-tree root (lone)' '\n \ttest_must_fail git rev-list --objects $broken_commit\n '\n \n-- \n2.21.0.203.g358da99528\n\n"},{"id":"373560","messageId":"e0bd479e822ce81de280ee6fdd07f608a96b7836.1554861974.git.me@ttaylorr.com","threadId":"50877","inReplyTo":"cover.1554861974.git.me@ttaylorr.com","subject":"[PATCH v2 7/7] rev-list: detect broken root trees","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-04-10T02:13:25Z","receivedAt":"2019-04-10T02:13:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"From: Jeff King <peff@peff.net>\n\nWhen the traversal machinery sees a commit without a root tree, it\nassumes that the tree was part of a BOUNDARY commit, and quietly ignores\nthe tree. But it could also be caused by a commit whose root tree is\nbroken or missing.\n\nInstead, let's die() when we see a NULL root tree. We can differentiate\nit from the BOUNDARY case by seeing if the commit was actually parsed.\nThis covers that case, plus future-proofs us against any others where we\nmight try to show an unparsed commit.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n list-objects.c                         | 3 +++\n t/t6102-rev-list-unexpected-objects.sh | 6 ++++--\n 2 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/list-objects.c b/list-objects.c\nindex bb7e61ef4b..b5651ddd5b 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -374,6 +374,9 @@ static void do_traverse(struct traversal_context *ctx)\n \t\t\tstruct tree *tree = get_commit_tree(commit);\n \t\t\ttree->object.flags |= NOT_USER_GIVEN;\n \t\t\tadd_pending_tree(ctx->revs, tree);\n+\t\t} else if (commit->object.parsed) {\n+\t\t\tdie(_(\"unable to load root tree for commit %s\"),\n+\t\t\t      oid_to_hex(&commit->object.oid));\n \t\t}\n \t\tctx->show_commit(commit, ctx->show_data);\n \ndiff --git a/t/t6102-rev-list-unexpected-objects.sh b/t/t6102-rev-list-unexpected-objects.sh\nindex 28ee1bcb07..28611c978e 100755\n--- a/t/t6102-rev-list-unexpected-objects.sh\n+++ b/t/t6102-rev-list-unexpected-objects.sh\n@@ -67,8 +67,10 @@ test_expect_success 'traverse unexpected non-tree root (lone)' '\n \ttest_must_fail git rev-list --objects $broken_commit\n '\n \n-test_expect_failure 'traverse unexpected non-tree root (seen)' '\n-\ttest_must_fail git rev-list --objects $blob $broken_commit\n+test_expect_success 'traverse unexpected non-tree root (seen)' '\n+\ttest_must_fail git rev-list --objects $blob $broken_commit \\\n+\t\t>output 2>&1 &&\n+\ttest_i18ngrep \"not a tree\" output\n '\n \n test_expect_success 'setup unexpected non-commit tag' '\n-- \n2.21.0.203.g358da99528\n"}]}