{"thread":{"id":"31122","subject":"[PATCH 0/3] null sha1 in trees","startedAt":"2012-07-28T15:01:32Z","lastAt":"2013-01-07T13:46:54Z","messageCount":22,"participants":["Jeff King","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"196026","messageId":"20120728150132.GA25042@sigill.intra.peff.net","threadId":"31122","inReplyTo":null,"subject":"[PATCH 0/3] null sha1 in trees","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-28T15:01:32Z","receivedAt":"2012-07-28T15:01:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I recently came across a tree in the wild that had a submodule entry\nwhose sha1 was the null sha1 (i.e., all-zeros). It triggered an\ninteresting bug in the diff code, which is fixed by patch 1.\n\nUnfortunately, I have no clue how this tree came about. I'm assuming it\nwas simply a bug somewhere in git, where the entry should have had\nanother sha1, or possibly been removed from the tree entirely.. Patches\n2 and 3 tighten up our checks for null sha1s in a few places, which\nmight help detect such a bug earlier.\n\nI assume I have never seen this with a non-submodule entry because such\na tree would fail the usual connectivity checks during fsck or during a\ntransfer. However, since we don't enforce connectivity on submodule\nentries, nothing blocked the creation and propagation of such an entry.\n\nI'm not at liberty to share the repository in question, but if anybody\nhas specific things to look for, I'd be happy to investigate further.\n\nThe patches are:\n\n  [1/3]: diff: do not use null sha1 as a sentinel value\n\nThis is the actual bug-fix, and I hope is obviously a good thing to do.\n\n  [2/3]: do not write null sha1s to on-disk index\n\nThis one tries to tighten our writing a bit. There are unfortunately a\nlot of different code paths that create trees in git. I hope by catching\nthe index write as a choke-point, we can prevent bugs from spreading.\nHowever, there are a lot of tree-writers that update an index in-core\nand then write a tree out directly. I would not be surprised if this\ndoes not catch the bug by itself, but I think it is a step in the right\ndirection.\n\n  [3/3]: fsck: detect null sha1 in tree entries\n\nAnd this one will at least let us notice the bug once it has happened.\nAnd if transfer.fsckObjects is set, it will prevent bogus trees from\npassing between repositories, containing any damage.\n\n-Peff\n"},{"id":"196027","messageId":"20120728150301.GA25269@sigill.intra.peff.net","threadId":"31122","inReplyTo":"20120728150132.GA25042@sigill.intra.peff.net","subject":"[PATCH 1/3] diff: do not use null sha1 as a sentinel value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-28T15:03:01Z","receivedAt":"2012-07-28T15:03:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The diff code represents paths using the diff_filespec\nstruct. This struct has a sha1 to represent the sha1 of the\ncontent at that path, as well as a sha1_valid member which\nindicates whether its sha1 field is actually useful. If\nsha1_valid is not true, then the filespec represents a\nworking tree file (e.g., for the no-index case, or for when\nthe index is not up-to-date).\n\nThe diff_filespec is only used internally, though. At the\ninterfaces to the diff subsystem, callers feed the sha1\ndirectly, and we create a diff_filespec from it. It's at\nthat point that we look at the sha1 and decide whether it is\nvalid or not; callers may pass the null sha1 as a sentinel\nvalue to indicate that it is not.\n\nWe should not typically see the null sha1 coming from any\nother source (e.g., in the index itself, or from a tree).\nHowever, a corrupt tree might have a null sha1, which would\ncause \"diff --patch\" to accidentally diff the working tree\nversion of a file instead of treating it as a blob.\n\nThis patch extends the edges of the diff interface to accept\na \"sha1_valid\" flag whenever we accept a sha1, and to use\nthat flag when creating a filespec. In some cases, this\nmeans passing the flag through several layers, making the\ncode change larger than would be desirable.\n\nOne alternative would be to simply die() upon seeing\ncorrupted trees with null sha1s. However, this fix more\ndirectly addresses the problem (while bogus sha1s in a tree\nare probably a bad thing, it is really the sentinel\nconfusion sending us down the wrong code path that is what\nmakes it devastating). And it means that git is more capable\nof examining and debugging these corrupted trees. For\nexample, you can still \"diff --raw\" such a tree to find out\nwhen the bogus entry was introduced; you just cannot do a\n\"--patch\" diff (just as you could not with any other\ncorrupted tree, as we do not have any content to diff).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin.h                  |  2 +-\n builtin/blame.c            |  9 ++---\n builtin/cat-file.c         |  2 +-\n builtin/diff.c             |  8 +++--\n combine-diff.c             |  4 +--\n diff-lib.c                 | 20 ++++++-----\n diff-no-index.c            |  2 +-\n diff.c                     | 16 +++++----\n diff.h                     |  5 +++\n diffcore-rename.c          |  2 +-\n diffcore.h                 |  2 +-\n revision.c                 |  2 ++\n t/t4054-diff-bogus-tree.sh | 83 ++++++++++++++++++++++++++++++++++++++++++++++\n tree-diff.c                |  8 ++---\n 14 files changed, 134 insertions(+), 31 deletions(-)\n create mode 100755 t/t4054-diff-bogus-tree.sh\n\ndiff --git a/builtin.h b/builtin.h\nindex ba6626b..8e37752 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -43,7 +43,7 @@ extern int check_pager_config(const char *cmd);\n struct diff_options;\n extern void setup_diff_pager(struct diff_options *);\n \n-extern int textconv_object(const char *path, unsigned mode, const unsigned char *sha1, char **buf, unsigned long *buf_size);\n+extern int textconv_object(const char *path, unsigned mode, const unsigned char *sha1, int sha1_valid, char **buf, unsigned long *buf_size);\n \n extern int cmd_add(int argc, const char **argv, const char *prefix);\n extern int cmd_annotate(int argc, const char **argv, const char *prefix);\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 0d50273..f2c48ef 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -110,6 +110,7 @@ static int diff_hunks(mmfile_t *file_a, mmfile_t *file_b, long ctxlen,\n int textconv_object(const char *path,\n \t\t    unsigned mode,\n \t\t    const unsigned char *sha1,\n+\t\t    int sha1_valid,\n \t\t    char **buf,\n \t\t    unsigned long *buf_size)\n {\n@@ -117,7 +118,7 @@ int textconv_object(const char *path,\n \tstruct userdiff_driver *textconv;\n \n \tdf = alloc_filespec(path);\n-\tfill_filespec(df, sha1, mode);\n+\tfill_filespec(df, sha1, sha1_valid, mode);\n \ttextconv = get_textconv(df);\n \tif (!textconv) {\n \t\tfree_filespec(df);\n@@ -142,7 +143,7 @@ static void fill_origin_blob(struct diff_options *opt,\n \n \t\tnum_read_blob++;\n \t\tif (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) &&\n-\t\t    textconv_object(o->path, o->mode, o->blob_sha1, &file->ptr, &file_size))\n+\t\t    textconv_object(o->path, o->mode, o->blob_sha1, 1, &file->ptr, &file_size))\n \t\t\t;\n \t\telse\n \t\t\tfile->ptr = read_sha1_file(o->blob_sha1, &type, &file_size);\n@@ -2123,7 +2124,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n \t\tswitch (st.st_mode & S_IFMT) {\n \t\tcase S_IFREG:\n \t\t\tif (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) &&\n-\t\t\t    textconv_object(read_from, mode, null_sha1, &buf_ptr, &buf_len))\n+\t\t\t    textconv_object(read_from, mode, null_sha1, 0, &buf_ptr, &buf_len))\n \t\t\t\tstrbuf_attach(&buf, buf_ptr, buf_len, buf_len + 1);\n \t\t\telse if (strbuf_read_file(&buf, read_from, st.st_size) != st.st_size)\n \t\t\t\tdie_errno(\"cannot open or read '%s'\", read_from);\n@@ -2516,7 +2517,7 @@ parse_done:\n \t\t\tdie(\"no such path %s in %s\", path, final_commit_name);\n \n \t\tif (DIFF_OPT_TST(&sb.revs->diffopt, ALLOW_TEXTCONV) &&\n-\t\t    textconv_object(path, o->mode, o->blob_sha1, (char **) &sb.final_buf,\n+\t\t    textconv_object(path, o->mode, o->blob_sha1, 1, (char **) &sb.final_buf,\n \t\t\t\t    &sb.final_buf_size))\n \t\t\t;\n \t\telse\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex af74e77..0eca2d7 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -146,7 +146,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)\n \t\t\tdie(\"git cat-file --textconv %s: <object> must be <sha1:path>\",\n \t\t\t    obj_name);\n \n-\t\tif (!textconv_object(obj_context.path, obj_context.mode, sha1, &buf, &size))\n+\t\tif (!textconv_object(obj_context.path, obj_context.mode, sha1, 1, &buf, &size))\n \t\t\tdie(\"git cat-file --textconv: unable to run textconv on %s\",\n \t\t\t    obj_name);\n \t\tbreak;\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex da8f6aa..bf72298 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -29,6 +29,8 @@ static void stuff_change(struct diff_options *opt,\n \t\t\t unsigned old_mode, unsigned new_mode,\n \t\t\t const unsigned char *old_sha1,\n \t\t\t const unsigned char *new_sha1,\n+\t\t\t int old_sha1_valid,\n+\t\t\t int new_sha1_valid,\n \t\t\t const char *old_name,\n \t\t\t const char *new_name)\n {\n@@ -54,8 +56,8 @@ static void stuff_change(struct diff_options *opt,\n \n \tone = alloc_filespec(old_name);\n \ttwo = alloc_filespec(new_name);\n-\tfill_filespec(one, old_sha1, old_mode);\n-\tfill_filespec(two, new_sha1, new_mode);\n+\tfill_filespec(one, old_sha1, old_sha1_valid, old_mode);\n+\tfill_filespec(two, new_sha1, new_sha1_valid, new_mode);\n \n \tdiff_queue(&diff_queued_diff, one, two);\n }\n@@ -84,6 +86,7 @@ static int builtin_diff_b_f(struct rev_info *revs,\n \tstuff_change(&revs->diffopt,\n \t\t     blob[0].mode, canon_mode(st.st_mode),\n \t\t     blob[0].sha1, null_sha1,\n+\t\t     1, 0,\n \t\t     path, path);\n \tdiffcore_std(&revs->diffopt);\n \tdiff_flush(&revs->diffopt);\n@@ -108,6 +111,7 @@ static int builtin_diff_blobs(struct rev_info *revs,\n \tstuff_change(&revs->diffopt,\n \t\t     blob[0].mode, blob[1].mode,\n \t\t     blob[0].sha1, blob[1].sha1,\n+\t\t     1, 1,\n \t\t     blob[0].name, blob[1].name);\n \tdiffcore_std(&revs->diffopt);\n \tdiff_flush(&revs->diffopt);\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 9786680..bb1cc96 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -111,7 +111,7 @@ static char *grab_blob(const unsigned char *sha1, unsigned int mode,\n \t\treturn xcalloc(1, 1);\n \t} else if (textconv) {\n \t\tstruct diff_filespec *df = alloc_filespec(path);\n-\t\tfill_filespec(df, sha1, mode);\n+\t\tfill_filespec(df, sha1, 1, mode);\n \t\t*size = fill_textconv(textconv, df, &blob);\n \t\tfree_filespec(df);\n \t} else {\n@@ -823,7 +823,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\t\t\t\t   &result_size, NULL, NULL);\n \t\t} else if (textconv) {\n \t\t\tstruct diff_filespec *df = alloc_filespec(elem->path);\n-\t\t\tfill_filespec(df, null_sha1, st.st_mode);\n+\t\t\tfill_filespec(df, null_sha1, 0, st.st_mode);\n \t\t\tresult_size = fill_textconv(textconv, df, &result);\n \t\t\tfree_filespec(df);\n \t\t} else if (0 <= (fd = open(elem->path, O_RDONLY))) {\ndiff --git a/diff-lib.c b/diff-lib.c\nindex fc0dff3..f35de0f 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -206,7 +206,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\tif (silent_on_removed)\n \t\t\t\tcontinue;\n \t\t\tdiff_addremove(&revs->diffopt, '-', ce->ce_mode,\n-\t\t\t\t       ce->sha1, ce->name, 0);\n+\t\t\t\t       ce->sha1, !is_null_sha1(ce->sha1),\n+\t\t\t\t       ce->name, 0);\n \t\t\tcontinue;\n \t\t}\n \t\tchanged = match_stat_with_submodule(&revs->diffopt, ce, &st,\n@@ -220,6 +221,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\tnewmode = ce_mode_from_stat(ce, st.st_mode);\n \t\tdiff_change(&revs->diffopt, oldmode, newmode,\n \t\t\t    ce->sha1, (changed ? null_sha1 : ce->sha1),\n+\t\t\t    !is_null_sha1(ce->sha1), (changed ? 0 : !is_null_sha1(ce->sha1)),\n \t\t\t    ce->name, 0, dirty_submodule);\n \n \t}\n@@ -236,11 +238,12 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n static void diff_index_show_file(struct rev_info *revs,\n \t\t\t\t const char *prefix,\n \t\t\t\t struct cache_entry *ce,\n-\t\t\t\t const unsigned char *sha1, unsigned int mode,\n+\t\t\t\t const unsigned char *sha1, int sha1_valid,\n+\t\t\t\t unsigned int mode,\n \t\t\t\t unsigned dirty_submodule)\n {\n \tdiff_addremove(&revs->diffopt, prefix[0], mode,\n-\t\t       sha1, ce->name, dirty_submodule);\n+\t\t       sha1, sha1_valid, ce->name, dirty_submodule);\n }\n \n static int get_stat_data(struct cache_entry *ce,\n@@ -295,7 +298,7 @@ static void show_new_file(struct rev_info *revs,\n \t    &dirty_submodule, &revs->diffopt) < 0)\n \t\treturn;\n \n-\tdiff_index_show_file(revs, \"+\", new, sha1, mode, dirty_submodule);\n+\tdiff_index_show_file(revs, \"+\", new, sha1, !is_null_sha1(sha1), mode, dirty_submodule);\n }\n \n static int show_modified(struct rev_info *revs,\n@@ -312,7 +315,7 @@ static int show_modified(struct rev_info *revs,\n \t\t\t  &dirty_submodule, &revs->diffopt) < 0) {\n \t\tif (report_missing)\n \t\t\tdiff_index_show_file(revs, \"-\", old,\n-\t\t\t\t\t     old->sha1, old->ce_mode, 0);\n+\t\t\t\t\t     old->sha1, 1, old->ce_mode, 0);\n \t\treturn -1;\n \t}\n \n@@ -347,7 +350,8 @@ static int show_modified(struct rev_info *revs,\n \t\treturn 0;\n \n \tdiff_change(&revs->diffopt, oldmode, mode,\n-\t\t    old->sha1, sha1, old->name, 0, dirty_submodule);\n+\t\t    old->sha1, sha1, 1, !is_null_sha1(sha1),\n+\t\t    old->name, 0, dirty_submodule);\n \treturn 0;\n }\n \n@@ -380,7 +384,7 @@ static void do_oneway_diff(struct unpack_trees_options *o,\n \t\tstruct diff_filepair *pair;\n \t\tpair = diff_unmerge(&revs->diffopt, idx->name);\n \t\tif (tree)\n-\t\t\tfill_filespec(pair->one, tree->sha1, tree->ce_mode);\n+\t\t\tfill_filespec(pair->one, tree->sha1, 1, tree->ce_mode);\n \t\treturn;\n \t}\n \n@@ -396,7 +400,7 @@ static void do_oneway_diff(struct unpack_trees_options *o,\n \t * Something removed from the tree?\n \t */\n \tif (!idx) {\n-\t\tdiff_index_show_file(revs, \"-\", tree, tree->sha1, tree->ce_mode, 0);\n+\t\tdiff_index_show_file(revs, \"-\", tree, tree->sha1, 1, tree->ce_mode, 0);\n \t\treturn;\n \t}\n \ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 7d805a0..0b46a0f 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -82,7 +82,7 @@ static struct diff_filespec *noindex_filespec(const char *name, int mode)\n \tif (!name)\n \t\tname = \"/dev/null\";\n \ts = alloc_filespec(name);\n-\tfill_filespec(s, null_sha1, mode);\n+\tfill_filespec(s, null_sha1, 0, mode);\n \tif (name == file_from_standard_input)\n \t\tpopulate_from_stdin(s);\n \treturn s;\ndiff --git a/diff.c b/diff.c\nindex 62cbe14..15125e8 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2541,12 +2541,12 @@ void free_filespec(struct diff_filespec *spec)\n }\n \n void fill_filespec(struct diff_filespec *spec, const unsigned char *sha1,\n-\t\t   unsigned short mode)\n+\t\t   int sha1_valid, unsigned short mode)\n {\n \tif (mode) {\n \t\tspec->mode = canon_mode(mode);\n \t\thashcpy(spec->sha1, sha1);\n-\t\tspec->sha1_valid = !is_null_sha1(sha1);\n+\t\tspec->sha1_valid = sha1_valid;\n \t}\n }\n \n@@ -4693,6 +4693,7 @@ static int is_submodule_ignored(const char *path, struct diff_options *options)\n void diff_addremove(struct diff_options *options,\n \t\t    int addremove, unsigned mode,\n \t\t    const unsigned char *sha1,\n+\t\t    int sha1_valid,\n \t\t    const char *concatpath, unsigned dirty_submodule)\n {\n \tstruct diff_filespec *one, *two;\n@@ -4724,9 +4725,9 @@ void diff_addremove(struct diff_options *options,\n \ttwo = alloc_filespec(concatpath);\n \n \tif (addremove != '+')\n-\t\tfill_filespec(one, sha1, mode);\n+\t\tfill_filespec(one, sha1, sha1_valid, mode);\n \tif (addremove != '-') {\n-\t\tfill_filespec(two, sha1, mode);\n+\t\tfill_filespec(two, sha1, sha1_valid, mode);\n \t\ttwo->dirty_submodule = dirty_submodule;\n \t}\n \n@@ -4739,6 +4740,7 @@ void diff_change(struct diff_options *options,\n \t\t unsigned old_mode, unsigned new_mode,\n \t\t const unsigned char *old_sha1,\n \t\t const unsigned char *new_sha1,\n+\t\t int old_sha1_valid, int new_sha1_valid,\n \t\t const char *concatpath,\n \t\t unsigned old_dirty_submodule, unsigned new_dirty_submodule)\n {\n@@ -4753,6 +4755,8 @@ void diff_change(struct diff_options *options,\n \t\tconst unsigned char *tmp_c;\n \t\ttmp = old_mode; old_mode = new_mode; new_mode = tmp;\n \t\ttmp_c = old_sha1; old_sha1 = new_sha1; new_sha1 = tmp_c;\n+\t\ttmp = old_sha1_valid; old_sha1_valid = new_sha1_valid;\n+\t\t\tnew_sha1_valid = tmp;\n \t\ttmp = old_dirty_submodule; old_dirty_submodule = new_dirty_submodule;\n \t\t\tnew_dirty_submodule = tmp;\n \t}\n@@ -4763,8 +4767,8 @@ void diff_change(struct diff_options *options,\n \n \tone = alloc_filespec(concatpath);\n \ttwo = alloc_filespec(concatpath);\n-\tfill_filespec(one, old_sha1, old_mode);\n-\tfill_filespec(two, new_sha1, new_mode);\n+\tfill_filespec(one, old_sha1, old_sha1_valid, old_mode);\n+\tfill_filespec(two, new_sha1, new_sha1_valid, new_mode);\n \tone->dirty_submodule = old_dirty_submodule;\n \ttwo->dirty_submodule = new_dirty_submodule;\n \ndiff --git a/diff.h b/diff.h\nindex e027650..815dd7a 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -19,12 +19,14 @@ typedef void (*change_fn_t)(struct diff_options *options,\n \t\t unsigned old_mode, unsigned new_mode,\n \t\t const unsigned char *old_sha1,\n \t\t const unsigned char *new_sha1,\n+\t\t int old_sha1_valid, int new_sha1_valid,\n \t\t const char *fullpath,\n \t\t unsigned old_dirty_submodule, unsigned new_dirty_submodule);\n \n typedef void (*add_remove_fn_t)(struct diff_options *options,\n \t\t    int addremove, unsigned mode,\n \t\t    const unsigned char *sha1,\n+\t\t    int sha1_valid,\n \t\t    const char *fullpath, unsigned dirty_submodule);\n \n typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,\n@@ -214,12 +216,15 @@ extern void diff_addremove(struct diff_options *,\n \t\t\t   int addremove,\n \t\t\t   unsigned mode,\n \t\t\t   const unsigned char *sha1,\n+\t\t\t   int sha1_valid,\n \t\t\t   const char *fullpath, unsigned dirty_submodule);\n \n extern void diff_change(struct diff_options *,\n \t\t\tunsigned mode1, unsigned mode2,\n \t\t\tconst unsigned char *sha1,\n \t\t\tconst unsigned char *sha2,\n+\t\t\tint sha1_valid,\n+\t\t\tint sha2_valid,\n \t\t\tconst char *fullpath,\n \t\t\tunsigned dirty_submodule1, unsigned dirty_submodule2);\n \ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex 216a7a4..512d0ac 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -48,7 +48,7 @@ static struct diff_rename_dst *locate_rename_dst(struct diff_filespec *two,\n \t\tmemmove(rename_dst + first + 1, rename_dst + first,\n \t\t\t(rename_dst_nr - first - 1) * sizeof(*rename_dst));\n \trename_dst[first].two = alloc_filespec(two->path);\n-\tfill_filespec(rename_dst[first].two, two->sha1, two->mode);\n+\tfill_filespec(rename_dst[first].two, two->sha1, two->sha1_valid, two->mode);\n \trename_dst[first].pair = NULL;\n \treturn &(rename_dst[first]);\n }\ndiff --git a/diffcore.h b/diffcore.h\nindex be0739c..1c16c85 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -55,7 +55,7 @@ struct diff_filespec {\n extern struct diff_filespec *alloc_filespec(const char *);\n extern void free_filespec(struct diff_filespec *);\n extern void fill_filespec(struct diff_filespec *, const unsigned char *,\n-\t\t\t  unsigned short);\n+\t\t\t  int, unsigned short);\n \n extern int diff_populate_filespec(struct diff_filespec *, int);\n extern void diff_free_filespec_data(struct diff_filespec *);\ndiff --git a/revision.c b/revision.c\nindex 9e8f47a..04c7de8 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -345,6 +345,7 @@ static int tree_difference = REV_TREE_SAME;\n static void file_add_remove(struct diff_options *options,\n \t\t    int addremove, unsigned mode,\n \t\t    const unsigned char *sha1,\n+\t\t    int sha1_valid,\n \t\t    const char *fullpath, unsigned dirty_submodule)\n {\n \tint diff = addremove == '+' ? REV_TREE_NEW : REV_TREE_OLD;\n@@ -358,6 +359,7 @@ static void file_change(struct diff_options *options,\n \t\t unsigned old_mode, unsigned new_mode,\n \t\t const unsigned char *old_sha1,\n \t\t const unsigned char *new_sha1,\n+\t\t int old_sha1_valid, int new_sha1_valid,\n \t\t const char *fullpath,\n \t\t unsigned old_dirty_submodule, unsigned new_dirty_submodule)\n {\ndiff --git a/t/t4054-diff-bogus-tree.sh b/t/t4054-diff-bogus-tree.sh\nnew file mode 100755\nindex 0000000..0843c87\n--- /dev/null\n+++ b/t/t4054-diff-bogus-tree.sh\n@@ -0,0 +1,83 @@\n+#!/bin/sh\n+\n+test_description='test diff with a bogus tree containing the null sha1'\n+. ./test-lib.sh\n+\n+empty_tree=4b825dc642cb6eb9a060e54bf8d69288fbee4904\n+\n+test_expect_success 'create bogus tree' '\n+\tbogus_tree=$(\n+\t\tprintf \"100644 fooQQQQQQQQQQQQQQQQQQQQQ\" |\n+\t\tq_to_nul |\n+\t\tgit hash-object -w --stdin -t tree\n+\t)\n+'\n+\n+test_expect_success 'create tree with matching file' '\n+\techo bar >foo &&\n+\tgit add foo &&\n+\tgood_tree=$(git write-tree)\n+\tblob=$(git rev-parse :foo)\n+'\n+\n+test_expect_success 'raw diff shows null sha1 (addition)' '\n+\techo \":000000 100644 $_z40 $_z40 A\tfoo\" >expect &&\n+\tgit diff-tree $empty_tree $bogus_tree >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'raw diff shows null sha1 (removal)' '\n+\techo \":100644 000000 $_z40 $_z40 D\tfoo\" >expect &&\n+\tgit diff-tree $bogus_tree $empty_tree >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'raw diff shows null sha1 (modification)' '\n+\techo \":100644 100644 $blob $_z40 M\tfoo\" >expect &&\n+\tgit diff-tree $good_tree $bogus_tree >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'raw diff shows null sha1 (other direction)' '\n+\techo \":100644 100644 $_z40 $blob M\tfoo\" >expect &&\n+\tgit diff-tree $bogus_tree $good_tree >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'raw diff shows null sha1 (reverse)' '\n+\techo \":100644 100644 $_z40 $blob M\tfoo\" >expect &&\n+\tgit diff-tree -R $good_tree $bogus_tree >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'raw diff shows null sha1 (index)' '\n+\techo \":100644 100644 $_z40 $blob M\tfoo\" >expect &&\n+\tgit diff-index $bogus_tree >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'patch fails due to bogus sha1 (addition)' '\n+\ttest_must_fail git diff-tree -p $empty_tree $bogus_tree\n+'\n+\n+test_expect_success 'patch fails due to bogus sha1 (removal)' '\n+\ttest_must_fail git diff-tree -p $bogus_tree $empty_tree\n+'\n+\n+test_expect_success 'patch fails due to bogus sha1 (modification)' '\n+\ttest_must_fail git diff-tree -p $good_tree $bogus_tree\n+'\n+\n+test_expect_success 'patch fails due to bogus sha1 (other direction)' '\n+\ttest_must_fail git diff-tree -p $bogus_tree $good_tree\n+'\n+\n+test_expect_success 'patch fails due to bogus sha1 (reverse)' '\n+\ttest_must_fail git diff-tree -R -p $good_tree $bogus_tree\n+'\n+\n+test_expect_success 'patch fails due to bogus sha1 (index)' '\n+\ttest_must_fail git diff-index -p $bogus_tree\n+'\n+\n+test_done\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 28ad6db..7e54833 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -49,12 +49,12 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2,\n \tif (DIFF_OPT_TST(opt, RECURSIVE) && S_ISDIR(mode1)) {\n \t\tif (DIFF_OPT_TST(opt, TREE_IN_RECURSIVE)) {\n \t\t\topt->change(opt, mode1, mode2,\n-\t\t\t\t    sha1, sha2, base->buf, 0, 0);\n+\t\t\t\t    sha1, sha2, 1, 1, base->buf, 0, 0);\n \t\t}\n \t\tstrbuf_addch(base, '/');\n \t\tdiff_tree_sha1(sha1, sha2, base->buf, opt);\n \t} else {\n-\t\topt->change(opt, mode1, mode2, sha1, sha2, base->buf, 0, 0);\n+\t\topt->change(opt, mode1, mode2, sha1, sha2, 1, 1, base->buf, 0, 0);\n \t}\n \tstrbuf_setlen(base, old_baselen);\n \treturn 0;\n@@ -100,7 +100,7 @@ static void show_entry(struct diff_options *opt, const char *prefix,\n \t\t\tdie(\"corrupt tree sha %s\", sha1_to_hex(sha1));\n \n \t\tif (DIFF_OPT_TST(opt, TREE_IN_RECURSIVE))\n-\t\t\topt->add_remove(opt, *prefix, mode, sha1, base->buf, 0);\n+\t\t\topt->add_remove(opt, *prefix, mode, sha1, 1, base->buf, 0);\n \n \t\tstrbuf_addch(base, '/');\n \n@@ -108,7 +108,7 @@ static void show_entry(struct diff_options *opt, const char *prefix,\n \t\tshow_tree(opt, prefix, &inner, base);\n \t\tfree(tree);\n \t} else\n-\t\topt->add_remove(opt, prefix[0], mode, sha1, base->buf, 0);\n+\t\topt->add_remove(opt, prefix[0], mode, sha1, 1, base->buf, 0);\n \n \tstrbuf_setlen(base, old_baselen);\n }\n-- \n1.7.11.3.42.g90758bf\n"},{"id":"196028","messageId":"20120728150524.GB25269@sigill.intra.peff.net","threadId":"31122","inReplyTo":"20120728150132.GA25042@sigill.intra.peff.net","subject":"[PATCH 2/3] do not write null sha1s to on-disk index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-28T15:05:24Z","receivedAt":"2012-07-28T15:05:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We should never need to write the null sha1 into an index\nentry (short of the 1 in 2^160 chance that somebody actually\nhas content that hashes to it). If we attempt to do so, it\nis much more likely that it is a bug, since we use the null\nsha1 as a sentinel value to mean \"not valid\".\n\nThe presence of null sha1s in the index (which can come\nfrom, among other things, \"update-index --cacheinfo\", or by\nreading a corrupted tree) can cause problems for later\nreaders, because they cannot distinguish the literal null\nsha1 from its use a sentinel value.  For example, \"git\ndiff-files\" on such an entry would make it appear as if it\nis stat-dirty, and until recently, the diff code assumed\nsuch an entry meant that we should be diffing a working tree\nfile rather than a blob.\n\nIdeally, we would stop such entries from entering even our\nin-core index. However, we do sometimes legitimately add\nentries with null sha1s in order to represent these sentinel\nsituations; simply forbidding them in add_index_entry breaks\na lot of the existing code. However, we can at least make\nsure that our in-core sentinel representation never makes it\nto disk.\n\nTo be thorough, we will test an attempt to add both a blob\nand a submodule entry. In the former case, we might run into\nproblems anyway because we will be missing the blob object.\nBut in the latter case, we do not enforce connectivity\nacross gitlink entries, making this our only point of\nenforcement. The current implementation does not care which\ntype of entry we are seeing, but testing both cases helps\nfuture-proof the test suite in case that changes.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI confess to not being clear on all of the instances in which a null\nsha1 might enter the in-core index. I did try modifying add_index_entry,\nbut the breakage was pretty severe. I traced through a few instances,\nand it seemed to be mostly related to unmerged entries.\n\n read-cache.c                  |  2 ++\n t/t2107-update-index-basic.sh | 19 +++++++++++++++++++\n 2 files changed, 21 insertions(+)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 2f8159f..d2be78e 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1800,6 +1800,8 @@ int write_index(struct index_state *istate, int newfd)\n \t\t\tcontinue;\n \t\tif (!ce_uptodate(ce) && is_racy_timestamp(istate, ce))\n \t\t\tce_smudge_racily_clean_entry(ce);\n+\t\tif (is_null_sha1(ce->sha1))\n+\t\t\treturn error(\"cache entry has null sha1: %s\", ce->name);\n \t\tif (ce_write_entry(&c, newfd, ce, previous_name) < 0)\n \t\t\treturn -1;\n \t}\ndiff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh\nindex 809fafe..0dbbb00 100755\n--- a/t/t2107-update-index-basic.sh\n+++ b/t/t2107-update-index-basic.sh\n@@ -29,4 +29,23 @@ test_expect_success 'update-index -h with corrupt index' '\n \tgrep \"[Uu]sage: git update-index\" broken/usage\n '\n \n+test_expect_success '--cacheinfo does not accept blob null sha1' '\n+\techo content >file &&\n+\tgit add file &&\n+\tgit rev-parse :file >expect &&\n+\ttest_must_fail git update-index --cacheinfo 100644 $_z40 file &&\n+\tgit rev-parse :file >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--cacheinfo does not accept gitlink null sha1' '\n+\tgit init submodule &&\n+\t(cd submodule && test_commit foo) &&\n+\tgit add submodule &&\n+\tgit rev-parse :submodule >expect &&\n+\ttest_must_fail git update-index --cacheinfo 160000 $_z40 submodule &&\n+\tgit rev-parse :submodule >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.7.11.3.42.g90758bf\n"},{"id":"196029","messageId":"20120728150629.GC25269@sigill.intra.peff.net","threadId":"31122","inReplyTo":"20120728150132.GA25042@sigill.intra.peff.net","subject":"[PATCH 3/3] fsck: detect null sha1 in tree entries","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-28T15:06:29Z","receivedAt":"2012-07-28T15:06:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Short of somebody happening to beat the 1 in 2^160 odds of\nactually generating content that hashes to the null sha1, we\nshould never see this value in a tree entry. So let's have\nfsck warn if it it seen.\n\nAs in the previous commit, we test both blob and submodule\nentries to future-proof the test suite against the\nimplementation depending on connectivity to notice the\nerror.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI left this as a warning, because git can still mostly handle such\ntrees, which may aid in debugging or salvaging data.\n\n fsck.c          |  8 +++++++-\n t/t1450-fsck.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 33 insertions(+), 1 deletion(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 4c63b2c..7395ef6 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -139,6 +139,7 @@ static int verify_ordered(unsigned mode1, const char *name1, unsigned mode2, con\n static int fsck_tree(struct tree *item, int strict, fsck_error error_func)\n {\n \tint retval;\n+\tint has_null_sha1 = 0;\n \tint has_full_path = 0;\n \tint has_empty_name = 0;\n \tint has_zero_pad = 0;\n@@ -157,9 +158,12 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)\n \twhile (desc.size) {\n \t\tunsigned mode;\n \t\tconst char *name;\n+\t\tconst unsigned char *sha1;\n \n-\t\ttree_entry_extract(&desc, &name, &mode);\n+\t\tsha1 = tree_entry_extract(&desc, &name, &mode);\n \n+\t\tif (is_null_sha1(sha1))\n+\t\t\thas_null_sha1 = 1;\n \t\tif (strchr(name, '/'))\n \t\t\thas_full_path = 1;\n \t\tif (!*name)\n@@ -207,6 +211,8 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)\n \t}\n \n \tretval = 0;\n+\tif (has_null_sha1)\n+\t\tretval += error_func(&item->object, FSCK_WARN, \"contains entries pointing to null sha1\");\n \tif (has_full_path)\n \t\tretval += error_func(&item->object, FSCK_WARN, \"contains full pathnames\");\n \tif (has_empty_name)\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 5b79c51..bf7a2cd 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -213,4 +213,30 @@ test_expect_success 'rev-list --verify-objects with bad sha1' '\n \tgrep -q \"error: sha1 mismatch 63ffffffffffffffffffffffffffffffffffffff\" out\n '\n \n+_bz='\\0'\n+_bz5=\"$_bz$_bz$_bz$_bz$_bz\"\n+_bz20=\"$_bz5$_bz5$_bz5$_bz5\"\n+\n+test_expect_success 'fsck notices blob entry pointing to null sha1' '\n+\t(git init null-blob &&\n+\t cd null-blob &&\n+\t sha=$(printf \"100644 file$_bz$_bz20\" |\n+\t       git hash-object -w --stdin -t tree) &&\n+\t  git fsck 2>out &&\n+\t  cat out &&\n+\t  grep \"warning.*null sha1\" out\n+\t)\n+'\n+\n+test_expect_success 'fsck notices submodule entry pointing to null sha1' '\n+\t(git init null-commit &&\n+\t cd null-commit &&\n+\t sha=$(printf \"160000 submodule$_bz$_bz20\" |\n+\t       git hash-object -w --stdin -t tree) &&\n+\t  git fsck 2>out &&\n+\t  cat out &&\n+\t  grep \"warning.*null sha1\" out\n+\t)\n+'\n+\n test_done\n-- \n1.7.11.3.42.g90758bf\n"},{"id":"196086","messageId":"7v8ve2uscw.fsf@alter.siamese.dyndns.org","threadId":"31122","inReplyTo":"20120728150132.GA25042@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] null sha1 in trees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-29T22:15:11Z","receivedAt":"2012-07-29T22:15:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"All looked reasonable, even though I'd want to read the\nsurrounding codepath over for 2/3 a few more times.\n\nWill queue; thanks.\n"},{"id":"196146","messageId":"20120730154214.GA16701@sigill.intra.peff.net","threadId":"31122","inReplyTo":"7v8ve2uscw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/3] null sha1 in trees","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-30T15:42:14Z","receivedAt":"2012-07-30T15:42:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 29, 2012 at 03:15:11PM -0700, Junio C Hamano wrote:\n\n> All looked reasonable, even though I'd want to read the\n> surrounding codepath over for 2/3 a few more times.\n> \n> Will queue; thanks.\n\nYeah, 2/3 is the one that gives me the most pause. I originally assumed\nwe would never want to put a null-sha1 entry into the in-core index, but\nthat turned out to be very wrong. So I softened my assumption to \"we\nwould never want to put a null-sha1 entry on disk\". Which seems like the\nright thing to me and passes the test suite, but there might be an\nunexercised code path in there.\n\nIn an ideal world, I would trace each code path that inserts a cache\nentry all the way back to its original sha1, but when I tried it turned\nout too complex. The educated-guess-and-see-if-it-passes-tests approach\nto coding does not make me happy, but I don't see a good alternative.\n\nPatch 1 is a pure bug-fix, and it would be nice for it to make it onto a\nmaint- track or into 1.7.12 (though I can't guarantee that I got it 100%\nright, I am confident that it is at least trying to do the right thing,\nand any fixes would be incremental on top).  The other two should be\nfine to cook longer and go in post-1.7.12.\n\n-Peff\n"},{"id":"205646","messageId":"20121229100130.GA31497@elie.Belkin","threadId":"31122","inReplyTo":"20120728150524.GB25269@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] do not write null sha1s to on-disk index","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-12-29T10:03:46Z","receivedAt":"2012-12-29T10:03:46Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Peff,\n\nJeff King wrote:\n\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1800,6 +1800,8 @@ int write_index(struct index_state *istate, int newfd)\n>  \t\t\tcontinue;\n>  \t\tif (!ce_uptodate(ce) && is_racy_timestamp(istate, ce))\n>  \t\t\tce_smudge_racily_clean_entry(ce);\n> +\t\tif (is_null_sha1(ce->sha1))\n> +\t\t\treturn error(\"cache entry has null sha1: %s\", ce->name);\n>  \t\tif (ce_write_entry(&c, newfd, ce, previous_name) < 0)\n>  \t\t\treturn -1;\n\nQuick heads up: this is tripping for me in the \"git am --abort\"\ncodepath:\n\n  $ cd ~/src/linux\n  $ git checkout v3.2.35\n  $ git am -3sc /tmp/alsa-usb-audio-fix-missing-autopm-for-midi-input.patch \n  Applying: ALSA: usb-audio: Fix missing autopm for MIDI input\n  Using index info to reconstruct a base tree...\n  Falling back to patching base and 3-way merge...\n  error: Your local changes to the following files would be overwritten by merge:\n\tsound/usb/midi.c\n  Please, commit your changes or stash them before you can merge.\n  Aborting\n  Failed to merge in the changes.\n  Patch failed at 0001 ALSA: usb-audio: Fix missing autopm for MIDI input\n  When you have resolved this problem run \"git am --resolved\".\n  If you would prefer to skip this patch, instead run \"git am --skip\".\n  To restore the original branch and stop patching run \"git am --abort\".\n  $ git am --abort\n  error: cache entry has null sha1: sound/usb/midi.c\n  fatal: unable to write new index file\n  Unstaged changes after reset:\n  M       sound/usb/midi.c\n  $\n\nReproducible using v1.8.1-rc3 and master.  Bisects to 4337b585 (do not\nwrite null sha1s to on-disk index, 2012-07-28).  For comparison, older\ngits produced\n\n  $ git am --abort\n  Unstaged changes after reset:\n  M       sound/usb/midi.c\n\nwhich is also not great but at least didn't involve any obviously\ninvalid behavior.  Known problem?\n\nThanks,\nJonathan\n"},{"id":"205647","messageId":"20121229102707.GA26730@sigill.intra.peff.net","threadId":"31122","inReplyTo":"20121229100130.GA31497@elie.Belkin","subject":"Re: [PATCH 2/3] do not write null sha1s to on-disk index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-12-29T10:27:07Z","receivedAt":"2012-12-29T10:27:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 29, 2012 at 02:03:46AM -0800, Jonathan Nieder wrote:\n\n> > --- a/read-cache.c\n> > +++ b/read-cache.c\n> > @@ -1800,6 +1800,8 @@ int write_index(struct index_state *istate, int newfd)\n> >  \t\t\tcontinue;\n> >  \t\tif (!ce_uptodate(ce) && is_racy_timestamp(istate, ce))\n> >  \t\t\tce_smudge_racily_clean_entry(ce);\n> > +\t\tif (is_null_sha1(ce->sha1))\n> > +\t\t\treturn error(\"cache entry has null sha1: %s\", ce->name);\n> >  \t\tif (ce_write_entry(&c, newfd, ce, previous_name) < 0)\n> >  \t\t\treturn -1;\n> \n> Quick heads up: this is tripping for me in the \"git am --abort\"\n> codepath:\n\nThanks. The intent was that this should never happen, and we are\nprotecting against bogus sha1s slipping into the index. So either we\nhave found a bug in \"am --abort\", or the assumption that it should never\nhappen was wrong. :)\n\n>   $ cd ~/src/linux\n>   $ git checkout v3.2.35\n>   $ git am -3sc /tmp/alsa-usb-audio-fix-missing-autopm-for-midi-input.patch \n>   Applying: ALSA: usb-audio: Fix missing autopm for MIDI input\n>   Using index info to reconstruct a base tree...\n>   Falling back to patching base and 3-way merge...\n>   error: Your local changes to the following files would be overwritten by merge:\n> \tsound/usb/midi.c\n>   Please, commit your changes or stash them before you can merge.\n>   Aborting\n>   Failed to merge in the changes.\n>   Patch failed at 0001 ALSA: usb-audio: Fix missing autopm for MIDI input\n>   When you have resolved this problem run \"git am --resolved\".\n>   If you would prefer to skip this patch, instead run \"git am --skip\".\n>   To restore the original branch and stop patching run \"git am --abort\".\n>   $ git am --abort\n>   error: cache entry has null sha1: sound/usb/midi.c\n>   fatal: unable to write new index file\n>   Unstaged changes after reset:\n>   M       sound/usb/midi.c\n>   $\n\nI can't reproduce here. I can checkout v3.2.35, and I guess that the\npatch you are applying comes from f5f1654, but I don't know your\nlocal modification to sound/usb/midi.c. If I just make a fake\nmodification, I get this:\n\n  $ git checkout v3.2.35\n  $ echo fake >sound/usb/midi.c\n  $ git format-patch --stdout -1 f5f1654 | git am -3 ~/patch\n  [same errors as you]\n  $ git am --abort\n  Unstaged changes after reset:\n  M       sound/usb/midi.c\n\nSo I wonder if there is something in the way your working tree is\nmodified. Can you give more details?\n\n> Reproducible using v1.8.1-rc3 and master.  Bisects to 4337b585 (do not\n> write null sha1s to on-disk index, 2012-07-28).  For comparison, older\n> gits produced\n> \n>   $ git am --abort\n>   Unstaged changes after reset:\n>   M       sound/usb/midi.c\n\nWhat does your index look like afterwards? Does it have a null sha1 in\nit (check \"ls-files -s\")?\n\n-Peff\n"},{"id":"205648","messageId":"20121229103430.GG18903@elie.Belkin","threadId":"31122","inReplyTo":"20121229102707.GA26730@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] do not write null sha1s to on-disk index","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-12-29T10:34:30Z","receivedAt":"2012-12-29T10:34:30Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> I can't reproduce here. I can checkout v3.2.35, and I guess that the\n> patch you are applying comes from f5f1654, but I don't know your\n> local modification to sound/usb/midi.c.\n\nNo local modification.  The unstaged change after \"git am --abort\" to\nrecover from a conflicted git am is a longstanding bug (at least a\ncouple of years old).\n\nThe patch creating conflicts is\n\n\tqueue-3.2/alsa-usb-audio-fix-missing-autopm-for-midi-input.patch\n\nfrom git://git.kernel.org/pub/scm/linux/kernel/git/bwh/linux-3.2.y-queue.git\n\n[...]\n>>   $ git am --abort\n>>   Unstaged changes after reset:\n>>   M       sound/usb/midi.c\n>\n> What does your index look like afterwards? Does it have a null sha1 in\n> it (check \"ls-files -s\")?\n\n$ git diff-index --abbrev HEAD\n:100644 100644 eeefbce3873c... 000000000000... M\tsound/usb/midi.c\n$ git ls-files --abbrev -s sound/usb/midi.c\n100644 eeefbce3873c 0\tsound/usb/midi.c\n"},{"id":"205649","messageId":"20121229104048.GH18903@elie.Belkin","threadId":"31122","inReplyTo":"20121229102707.GA26730@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] do not write null sha1s to on-disk index","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-12-29T10:40:48Z","receivedAt":"2012-12-29T10:40:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Can you give more details?\n\n$ GIT_TRACE=1 git am --abort\ntrace: exec: 'git-am' '--abort'\ntrace: run_command: 'git-am' '--abort'\ntrace: built-in: git 'rev-parse' '--parseopt' '--' '--abort'\ntrace: built-in: git 'rev-parse' '--git-dir'\ntrace: built-in: git 'rev-parse' '--show-prefix'\ntrace: built-in: git 'rev-parse' '--show-toplevel'\ntrace: built-in: git 'var' 'GIT_COMMITTER_IDENT'\ntrace: built-in: git 'rev-parse' '--verify' '-q' 'HEAD'\ntrace: built-in: git 'config' '--bool' '--get' 'am.keepcr'\ntrace: built-in: git 'rerere' 'clear'\ntrace: built-in: git 'rev-parse' '--verify' '-q' 'HEAD'\ntrace: built-in: git 'read-tree' '--reset' '-u' 'HEAD' 'ORIG_HEAD'\nerror: cache entry has null sha1: sound/usb/midi.c\nfatal: unable to write new index file\ntrace: built-in: git 'reset' 'ORIG_HEAD'\nUnstaged changes after reset:\nM       sound/usb/midi.c\n"},{"id":"205650","messageId":"20121229104247.GA30283@sigill.intra.peff.net","threadId":"31122","inReplyTo":"20121229103430.GG18903@elie.Belkin","subject":"Re: [PATCH 2/3] do not write null sha1s to on-disk index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-12-29T10:42:47Z","receivedAt":"2012-12-29T10:42:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 29, 2012 at 02:34:30AM -0800, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > I can't reproduce here. I can checkout v3.2.35, and I guess that the\n> > patch you are applying comes from f5f1654, but I don't know your\n> > local modification to sound/usb/midi.c.\n> \n> No local modification.  The unstaged change after \"git am --abort\" to\n> recover from a conflicted git am is a longstanding bug (at least a\n> couple of years old).\n> \n> The patch creating conflicts is\n> \n> \tqueue-3.2/alsa-usb-audio-fix-missing-autopm-for-midi-input.patch\n> \n> from git://git.kernel.org/pub/scm/linux/kernel/git/bwh/linux-3.2.y-queue.git\n\nHrm. But your output does not say there is a conflict. It says you have\na local modification and it does not try the merge:\n\n> $ git am -3sc /tmp/alsa-usb-audio-fix-missing-autopm-for-midi-input.patch\n> Applying: ALSA: usb-audio: Fix missing autopm for MIDI input\n> Using index info to reconstruct a base tree...\n> Falling back to patching base and 3-way merge...\n> error: Your local changes to the following files would be overwritten by merge:\n>       sound/usb/midi.c\n> Please, commit your changes or stash them before you can merge.\n> Aborting\n\nIf I try to apply it, I get a real conflict:\n\n  $ git checkout v3.2.35\n  $ git am -3sc linux-3.2.y-queue/queue-3.2/alsa-usb-audio-fix-missing-autopm-for-midi-input.patch\n  Applying: ALSA: usb-audio: Fix missing autopm for MIDI input\n  Using index info to reconstruct a base tree...\n  M       sound/usb/midi.c\n  Falling back to patching base and 3-way merge...\n  Auto-merging sound/usb/midi.c\n  CONFLICT (content): Merge conflict in sound/usb/midi.c\n\nAlthough running \"git am --abort\" after that does seem to produce the\n\"cache entry has null sha1\" error. So I can start investigating from\nthere.\n\n-Peff\n"},{"id":"205651","messageId":"20121229105132.GA31505@elie.Belkin","threadId":"31122","inReplyTo":"20121229104247.GA30283@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] do not write null sha1s to on-disk index","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-12-29T10:51:32Z","receivedAt":"2012-12-29T10:51:32Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Hrm. But your output does not say there is a conflict. It says you have\n> a local modification and it does not try the merge:\n\nThat's probably operator error on my part when gathering output to\npaste into the email.\n\nIn other words, nothing to see there. :)  Sorry for the confusion.\n\n[...]\n> If I try to apply it, I get a real conflict:\n[...]\n> Although running \"git am --abort\" after that does seem to produce the\n> \"cache entry has null sha1\" error.\n\nYep, that is what my report should have said.\n\n'night,\nJonathan\n"},{"id":"205652","messageId":"20121229110541.GA1408@sigill.intra.peff.net","threadId":"31122","inReplyTo":"20121229103430.GG18903@elie.Belkin","subject":"Re: [PATCH 2/3] do not write null sha1s to on-disk index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-12-29T11:05:41Z","receivedAt":"2012-12-29T11:05:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 29, 2012 at 02:34:30AM -0800, Jonathan Nieder wrote:\n\n> >>   $ git am --abort\n> >>   Unstaged changes after reset:\n> >>   M       sound/usb/midi.c\n> >\n> > What does your index look like afterwards? Does it have a null sha1 in\n> > it (check \"ls-files -s\")?\n> \n> $ git diff-index --abbrev HEAD\n> :100644 100644 eeefbce3873c... 000000000000... M\tsound/usb/midi.c\n> $ git ls-files --abbrev -s sound/usb/midi.c\n> 100644 eeefbce3873c 0\tsound/usb/midi.c\n\nHmm. It looks like \"am --abort\" overwrites the index again after the\nread-tree which complains.  If I downgrade the error in write_index to a\nwarning, like this:\n\ndiff --git a/read-cache.c b/read-cache.c\nindex fda78bc..70a6d86 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1797,7 +1797,7 @@ int write_index(struct index_state *istate, int newfd)\n \t\tif (!ce_uptodate(ce) && is_racy_timestamp(istate, ce))\n \t\t\tce_smudge_racily_clean_entry(ce);\n \t\tif (is_null_sha1(ce->sha1))\n-\t\t\treturn error(\"cache entry has null sha1: %s\", ce->name);\n+\t\t\twarning(\"cache entry has null sha1: %s\", ce->name);\n \t\tif (ce_write_entry(&c, newfd, ce, previous_name) < 0)\n \t\t\treturn -1;\n \t}\n\nand then just run this:\n\n  [clear state from last run]\n  $ rm -rf .git/rebase-apply\n  $ git reset --hard\n\n  [apply the patch; we get a conflict]\n  $ git am -3sc queue-3.2/alsa-usb-audio-fix-missing-autopm-for-midi-input.patch\n\n  [now run just the read-tree from \"am --abort\"]\n  $ git.compile read-tree --reset -u HEAD ORIG_HEAD\n  warning: cache entry has null sha1: sound/usb/midi.c\n\n  [and now check our index]\n  $ git ls-files -s sound/usb/midi.c\n  100644 0000000000000000000000000000000000000000 0 sound/usb/midi.c\n\n  [yes, this index is bogus]\n  $ git write-tree\n  error: invalid object 100644 0000000000000000000000000000000000000000 for 'sound/usb/midi.c'\n  fatal: git-write-tree: error building trees\n\nSo I think this check may actually be finding a real bug. I have seen\nthese null sha1s in the wild, but I was never able to track down the\nactual cause. Maybe this will give us a clue. Now we just need to work\nbackwards and figure out who is putting it in the in-memory index and\nwhy.\n\nI'll try to work on it tomorrow, but please don't let that stop you if\nyou want to keep digging in the meantime.\n\n-Peff\n"},{"id":"205659","messageId":"20121229205154.GA21058@sigill.intra.peff.net","threadId":"31122","inReplyTo":"20121229110541.GA1408@sigill.intra.peff.net","subject":"[BUG] two-way read-tree can write null sha1s into index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-12-29T20:51:54Z","receivedAt":"2012-12-29T20:51:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 29, 2012 at 06:05:41AM -0500, Jeff King wrote:\n\n>   [clear state from last run]\n>   $ rm -rf .git/rebase-apply\n>   $ git reset --hard\n> \n>   [apply the patch; we get a conflict]\n>   $ git am -3sc queue-3.2/alsa-usb-audio-fix-missing-autopm-for-midi-input.patch\n> \n>   [now run just the read-tree from \"am --abort\"]\n>   $ git.compile read-tree --reset -u HEAD ORIG_HEAD\n>   warning: cache entry has null sha1: sound/usb/midi.c\n> \n>   [and now check our index]\n>   $ git ls-files -s sound/usb/midi.c\n>   100644 0000000000000000000000000000000000000000 0 sound/usb/midi.c\n> \n>   [yes, this index is bogus]\n>   $ git write-tree\n>   error: invalid object 100644 0000000000000000000000000000000000000000 for 'sound/usb/midi.c'\n>   fatal: git-write-tree: error building trees\n> \n> So I think this check may actually be finding a real bug. I have seen\n> these null sha1s in the wild, but I was never able to track down the\n> actual cause. Maybe this will give us a clue. Now we just need to work\n> backwards and figure out who is putting it in the in-memory index and\n> why.\n\nI made some progress on this, but I'd like a sanity check from others\n(especially Junio). As far as I can tell, this is a bug in read-tree.\n\nWhen we call \"read-tree --reset -u HEAD ORIG_HEAD\", the first thing we\ndo with the index is call read_cache_unmerged. Originally that would\nread the index, leaving aside any unmerged entries. However, as of\nd1a43f2 (reset --hard/read-tree --reset -u: remove unmerged new paths,\n2008-10-15), it actually creates a new cache entry. This serves as a\nplaceholder, so that we later know to update the working tree.\n\nHowever, we later noticed that the sha1 of that unmerged entry was\njust copied from some higher stage, leaving you with random content in\nthe index.  That was fixed by e11d7b5 (\"reset --merge\": fix unmerged\ncase, 2009-12-31), which instead puts the null sha1 into the newly\ncreated entry, and sets a CE_CONFLICTED flag. At the same time, it\nteaches the unpack-trees machinery to pay attention to this flag, so\nthat oneway_merge throws away the current value.\n\nHowever, it did not update the code paths for  twoway_merge, which is\nwhere we end up in the read-tree above. We notice that the HEAD and\nORIG_HEAD versions are the same, and say \"oh, we can just reuse the\ncurrent version\". But that's not true. The current version is bogus.\n\nSo I think we need to update twoway_merge to recognize unmerged entries,\nwhich gives us two options:\n\n  1. Reject the merge.\n\n  2. Throw away the current unmerged entry in favor of the \"new\" entry\n     (when old and new are the same, of course; otherwise we would\n     reject).\n\nI think (2) is the right thing. It fixes the entry of the bogus sha1\ninto the index, _and_ it solves the problem that \"git am --abort\" leaves\nthe conflicted entry as a modification. It should just go away. But\nmaybe I am forgetting some other case where read-tree should be more\nconservative, and (1) is a safer choice.\n\nSomething like this patch:\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 6d96366..e06e01f 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1746,14 +1746,19 @@ int twoway_merge(struct cache_entry **src, struct unpack_trees_options *o)\n \t\tnewtree = NULL;\n \n \tif (current) {\n-\t\tif ((!oldtree && !newtree) || /* 4 and 5 */\n-\t\t    (!oldtree && newtree &&\n-\t\t     same(current, newtree)) || /* 6 and 7 */\n-\t\t    (oldtree && newtree &&\n-\t\t     same(oldtree, newtree)) || /* 14 and 15 */\n-\t\t    (oldtree && newtree &&\n-\t\t     !same(oldtree, newtree) && /* 18 and 19 */\n-\t\t     same(current, newtree))) {\n+\t\tif (current->ce_flags & CE_CONFLICTED) {\n+\t\t\tif (same(oldtree, newtree))\n+\t\t\t\treturn merged_entry(newtree, current, o);\n+\t\t\treturn o->gently ? -1 : reject_merge(current, o);\n+\t\t}\n+\t\telse if ((!oldtree && !newtree) || /* 4 and 5 */\n+\t\t\t (!oldtree && newtree &&\n+\t\t\t  same(current, newtree)) || /* 6 and 7 */\n+\t\t\t (oldtree && newtree &&\n+\t\t\t  same(oldtree, newtree)) || /* 14 and 15 */\n+\t\t\t (oldtree && newtree &&\n+\t\t\t  !same(oldtree, newtree) && /* 18 and 19 */\n+\t\t\t  same(current, newtree))) {\n \t\t\treturn keep_entry(current, o);\n \t\t}\n \t\telse if (oldtree && !newtree && same(current, oldtree)) {\n\nI suspect threeway_merge may need a similar update, but I haven't looked\ntoo carefully yet.\n\n-Peff\n"},{"id":"205802","messageId":"7vvcbg7d8x.fsf@alter.siamese.dyndns.org","threadId":"31122","inReplyTo":"20121229205154.GA21058@sigill.intra.peff.net","subject":"Re: [BUG] two-way read-tree can write null sha1s into index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-01T22:24:46Z","receivedAt":"2013-01-01T22:24:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So I think we need to update twoway_merge to recognize unmerged entries,\n> which gives us two options:\n>\n>   1. Reject the merge.\n>\n>   2. Throw away the current unmerged entry in favor of the \"new\" entry\n>      (when old and new are the same, of course; otherwise we would\n>      reject).\n>\n> I think (2) is the right thing.\n\nAs \"--reset\" in \"read-tree --reset -u A B\" is a way to say \"we know\nwe are based on A and we want to go to B no matter what\", I agree we\nshould not reject the merge.\n\nWith -m instead of --reset, I am not sure what the right semantics\nshould be, though.\n"},{"id":"205916","messageId":"20130103083712.GC32377@sigill.intra.peff.net","threadId":"31122","inReplyTo":"7vvcbg7d8x.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] two-way read-tree can write null sha1s into index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-03T08:37:12Z","receivedAt":"2013-01-03T08:37:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 01, 2013 at 02:24:46PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > So I think we need to update twoway_merge to recognize unmerged entries,\n> > which gives us two options:\n> >\n> >   1. Reject the merge.\n> >\n> >   2. Throw away the current unmerged entry in favor of the \"new\" entry\n> >      (when old and new are the same, of course; otherwise we would\n> >      reject).\n> >\n> > I think (2) is the right thing.\n> \n> As \"--reset\" in \"read-tree --reset -u A B\" is a way to say \"we know\n> we are based on A and we want to go to B no matter what\", I agree we\n> should not reject the merge.\n> \n> With -m instead of --reset, I am not sure what the right semantics\n> should be, though.\n\nGood point; I was just thinking about the --reset case.\n\nWith \"-m\", though, we could in theory carry over the unmerged entries\n(again, assuming that \"old\" and \"new\" are the same; otherwise it is an\nobvious reject). But those entries would be confused with any new\nunmerged entries we create. It seems we already protect against this,\nthough: \"read-tree -m\" will not run at all if you have unmerged entries.\n\nLikewise, \"checkout\" seems to have similar protections.\n\nSo I think it may be a non-issue.\n\n-Peff\n"},{"id":"205931","messageId":"7vehi2xote.fsf@alter.siamese.dyndns.org","threadId":"31122","inReplyTo":"20130103083712.GC32377@sigill.intra.peff.net","subject":"Re: [BUG] two-way read-tree can write null sha1s into index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-03T15:34:53Z","receivedAt":"2013-01-03T15:34:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Jan 01, 2013 at 02:24:46PM -0800, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>> \n>> > So I think we need to update twoway_merge to recognize unmerged entries,\n>> > which gives us two options:\n>> >\n>> >   1. Reject the merge.\n>> >\n>> >   2. Throw away the current unmerged entry in favor of the \"new\" entry\n>> >      (when old and new are the same, of course; otherwise we would\n>> >      reject).\n>> >\n>> > I think (2) is the right thing.\n>> \n>> As \"--reset\" in \"read-tree --reset -u A B\" is a way to say \"we know\n>> we are based on A and we want to go to B no matter what\", I agree we\n>> should not reject the merge.\n>> \n>> With -m instead of --reset, I am not sure what the right semantics\n>> should be, though.\n>\n> Good point; I was just thinking about the --reset case.\n>\n> With \"-m\", though, we could in theory carry over the unmerged entries\n> (again, assuming that \"old\" and \"new\" are the same; otherwise it is an\n> obvious reject). But those entries would be confused with any new\n> unmerged entries we create. It seems we already protect against this,\n> though: \"read-tree -m\" will not run at all if you have unmerged entries.\n>\n> Likewise, \"checkout\" seems to have similar protections.\n>\n> So I think it may be a non-issue.\n\nYeah.  Also earlier in the thread you mentioned three-way case, but\nI do not think we ever would want --reset with three trees, so I\nthink that too is a non-issue for the same reason.\n\nI would still feel safer if we expressed the expectation of\nthe callee in the code, perhaps like this in the two-way case:\n\n\tif (current->ce_flags & CE_CONFLICTED) {\n        \tif (!o->reset) {\n                \t... either die or fail ...\n\t\t} else {\n                \t... your fix ...\n\t\t}\n\t}\n"},{"id":"205946","messageId":"20130103202343.GA4632@sigill.intra.peff.net","threadId":"31122","inReplyTo":"7vehi2xote.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] two-way read-tree can write null sha1s into index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-03T20:23:43Z","receivedAt":"2013-01-03T20:23:43Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 03, 2013 at 07:34:53AM -0800, Junio C Hamano wrote:\n\n> > Good point; I was just thinking about the --reset case.\n> >\n> > With \"-m\", though, we could in theory carry over the unmerged entries\n> > (again, assuming that \"old\" and \"new\" are the same; otherwise it is an\n> > obvious reject). But those entries would be confused with any new\n> > unmerged entries we create. It seems we already protect against this,\n> > though: \"read-tree -m\" will not run at all if you have unmerged entries.\n> >\n> > Likewise, \"checkout\" seems to have similar protections.\n> >\n> > So I think it may be a non-issue.\n> \n> Yeah.  Also earlier in the thread you mentioned three-way case, but\n> I do not think we ever would want --reset with three trees, so I\n> think that too is a non-issue for the same reason.\n\nYeah, agreed; we should always reject in the three-way case. I would\nworry more that it has a bug where something is _not_ rejected, and we\nend up putting a bogus null sha1 entry into the index (which is the\nactual problem with twoway_merge). IOW, if we have the bogus sha1 in the\nindex (because we marked it with CE_CONFLICTED), and the two sides and\nthe common ancestor are all the same, would we blindly carry through the\nbogus conflicted entry (which we would prefer, because it has the\nup-to-date stat information)?\n\nOr are you suggesting that the three-way case should always be protected\nby checking that there are no unmerged entries before we start it? That\nseems sane to me, but I haven't confirmed that that is the case.\n\n> I would still feel safer if we expressed the expectation of\n> the callee in the code, perhaps like this in the two-way case:\n> \n> \tif (current->ce_flags & CE_CONFLICTED) {\n>         \tif (!o->reset) {\n>                 \t... either die or fail ...\n> \t\t} else {\n>                 \t... your fix ...\n> \t\t}\n> \t}\n\nAgreed. I looked at that, but it seemed like it was going to involve\nrepeating a lot of the \"are the two trees the same\" logic. Let me see if\nI can refactor it to avoid that.\n\n-Peff\n"},{"id":"205948","messageId":"7vip7evwdo.fsf@alter.siamese.dyndns.org","threadId":"31122","inReplyTo":"20130103202343.GA4632@sigill.intra.peff.net","subject":"Re: [BUG] two-way read-tree can write null sha1s into index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-03T20:34:27Z","receivedAt":"2013-01-03T20:34:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Or are you suggesting that the three-way case should always be protected\n> by checking that there are no unmerged entries before we start it? That\n> seems sane to me, but I haven't confirmed that that is the case.\n\nI think the normal (and hopefully only) \"-m -u O A B\" use case of\nthreeway is the bog-standard \"git merge\", which requires even more:\nyour index must exactly match HEAD, even though you are allowed to\nhave local changes in the working tree.\n\nThat requirement is not likely to change, as cleanly merged paths\nare automatically added to the index, so \"diff --cached\" should show\nonly the changes from cleanly merged part, while \"diff\" should show\npaths that still needs user's help.\n\nIf we allowed local modification to the index (let alone conflicted\nentries in it) before the merge begins, the users would not be able\nto tell which paths are in what state after a half-merge stops and\nasks for help.  Updated paths may not have anything to do with the\nmerge (i.e. earlier \"git add\" before the merge started), conflicting\npaths may not have anything to do with the merge (i.e. leftover\nconflicts before the merge started).\n"},{"id":"205949","messageId":"20130103203606.GA8188@sigill.intra.peff.net","threadId":"31122","inReplyTo":"7vip7evwdo.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] two-way read-tree can write null sha1s into index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-03T20:36:07Z","receivedAt":"2013-01-03T20:36:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 03, 2013 at 12:34:27PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Or are you suggesting that the three-way case should always be protected\n> > by checking that there are no unmerged entries before we start it? That\n> > seems sane to me, but I haven't confirmed that that is the case.\n> \n> I think the normal (and hopefully only) \"-m -u O A B\" use case of\n> threeway is the bog-standard \"git merge\", which requires even more:\n> your index must exactly match HEAD, even though you are allowed to\n> have local changes in the working tree.\n> \n> That requirement is not likely to change, as cleanly merged paths\n> are automatically added to the index, so \"diff --cached\" should show\n> only the changes from cleanly merged part, while \"diff\" should show\n> paths that still needs user's help.\n> \n> If we allowed local modification to the index (let alone conflicted\n> entries in it) before the merge begins, the users would not be able\n> to tell which paths are in what state after a half-merge stops and\n> asks for help.  Updated paths may not have anything to do with the\n> merge (i.e. earlier \"git add\" before the merge started), conflicting\n> paths may not have anything to do with the merge (i.e. leftover\n> conflicts before the merge started).\n\nOh, I agree it's insane to try to carry through unmerged entries. I'm\njust concerned that not all code paths are careful enough to check. They\nprobably are, though, as a null sha1 in your index would be the least of\nyour worries, and somebody would have noticed in the last 7 years.\n\n-Peff\n"},{"id":"205959","messageId":"7vzk0pvqvu.fsf@alter.siamese.dyndns.org","threadId":"31122","inReplyTo":"20130103203606.GA8188@sigill.intra.peff.net","subject":"Re: [BUG] two-way read-tree can write null sha1s into index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-03T22:33:09Z","receivedAt":"2013-01-03T22:33:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Oh, I agree it's insane to try to carry through unmerged entries. I'm\n> just concerned that not all code paths are careful enough to check.\n\nI would actually be surprised if some code path do assume somebody\nmight give them an index with conflicting entries in it and guard\nagainst it.  We have been coding under the \"index must exactly match\nthe second tree when three-way unpack_trees() begin\" requirement\nsince day one.  An conflicted entry will appear as \"index and HEAD\nnot matching\" and will cause reject_merge() early in threeway_merge()\nanyway, no?\n"},{"id":"206225","messageId":"20130107134654.GA22692@sigill.intra.peff.net","threadId":"31122","inReplyTo":"7vzk0pvqvu.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] two-way read-tree can write null sha1s into index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-07T13:46:54Z","receivedAt":"2013-01-07T13:46:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 03, 2013 at 02:33:09PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Oh, I agree it's insane to try to carry through unmerged entries. I'm\n> > just concerned that not all code paths are careful enough to check.\n> \n> I would actually be surprised if some code path do assume somebody\n> might give them an index with conflicting entries in it and guard\n> against it.  We have been coding under the \"index must exactly match\n> the second tree when three-way unpack_trees() begin\" requirement\n> since day one.  An conflicted entry will appear as \"index and HEAD\n> not matching\" and will cause reject_merge() early in threeway_merge()\n> anyway, no?\n\nHmm. There is code in threeway_merge to handle a few cases:\n\n        /*\n         * We start with cases where the index is allowed to match\n         * something other than the head: #14(ALT) and #2ALT, where it\n         * is permitted to match the result instead.\n         */\n        /* #14, #14ALT, #2ALT */\n        if (remote && !df_conflict_head && head_match && !remote_match) {\n                if (index && !same(index, remote) && !same(index, head))\n                        return o->gently ? -1 : reject_merge(index, o);\n                return merged_entry(remote, index, o);\n        }\n\nbut I do not think we have to worry, because we know that the index will\nnever match remote, either (and merged_entry has already been taught to\nbe wary of the conflicted bit, anyway). I'm not entirely clear on how\nthat would get triggered if all of the callers avoid operating on a\nmodified index.\n\n-Peff\n"}]}