{"thread":{"id":"18253","subject":"[BUG] - git-read-tree segfaults","startedAt":"2009-03-10T19:34:28Z","lastAt":"2009-03-14T06:41:56Z","messageCount":11,"participants":["Jiri Olsa","Johannes Schindelin","Junio C Hamano","Daniel Barkalow","Linus Torvalds"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"107602","messageId":"35476bd20903101234q71bc520fh18828d7170a4a2f5@mail.gmail.com","threadId":"18253","inReplyTo":null,"subject":"[BUG] - git-read-tree segfaults","fromName":"Jiri Olsa","fromEmail":"olsajiri@gmail.com","sentAt":"2009-03-10T19:34:28Z","receivedAt":"2009-03-10T19:34:28Z","isPatch":false,"sender":{"key":"olsajiri@gmail.com","avatar":null},"body":"Hi,\n\nI was following the git core tutorial and git-read-tree got me segfault.\nI accidentaly executed the git-read-tree without \"-m -u\" options and\ngot segfault.\n\nI can reproduce with the latest git using following script.\n\n-----------------------------------------------\n#!/bin/sh\n\nGIT=$1\n\nrm -rf crash; mkdir -p crash; cd crash\n$GIT init\necho \"xxx\" > xxx\n$GIT add xxx\n$GIT commit -m \"xxx\"\n$GIT checkout -b yyy\necho \"yyy\" > yyy\n$GIT add yyy\n$GIT commit -m \"yyy\"\necho \"yyy1\" >> yyy\n$GIT commit -a -m \"yyy1\"\n$GIT checkout master\necho \"xxx1\" >> xxx\n$GIT commit -a -m \"xxx1\"\nmb=$($GIT merge-base HEAD yyy)\n$GIT read-tree $mb HEAD yyy\n-----------------------------------------------\n\nregards,\njirka\n"},{"id":"107607","messageId":"alpine.DEB.1.00.0903102050090.14295@intel-tinevez-2-302","threadId":"18253","inReplyTo":"35476bd20903101234q71bc520fh18828d7170a4a2f5@mail.gmail.com","subject":"Re: [BUG] - git-read-tree segfaults","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-03-10T19:50:38Z","receivedAt":"2009-03-10T19:50:38Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 10 Mar 2009, Jiri Olsa wrote:\n\n> Hi,\n> \n> I was following the git core tutorial and git-read-tree got me segfault.\n> I accidentaly executed the git-read-tree without \"-m -u\" options and\n> got segfault.\n> \n> I can reproduce with the latest git using following script.\n\nI can reproduce with the script, too...\n\nWorking on it,\nDscho\n"},{"id":"107608","messageId":"alpine.DEB.1.00.0903102120290.14295@intel-tinevez-2-302","threadId":"18253","inReplyTo":"35476bd20903101234q71bc520fh18828d7170a4a2f5@mail.gmail.com","subject":"Re: [BUG] - git-read-tree segfaults","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-03-10T20:21:28Z","receivedAt":"2009-03-10T20:21:28Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 10 Mar 2009, Jiri Olsa wrote:\n\n> mb=$($GIT merge-base HEAD yyy)\n> $GIT read-tree $mb HEAD yyy\n\nWhile I agree that it is a bad thing for Git to segfault, I think this \nhere is a pilot error. You try to read 3 trees at the same time, but not \nperform a merge. IMHO you want to add -m at least.\n\nCiao,\nDscho\n"},{"id":"107652","messageId":"35476bd20903110059v7d6d8aa0g93af17a940c91919@mail.gmail.com","threadId":"18253","inReplyTo":"alpine.DEB.1.00.0903102120290.14295@intel-tinevez-2-302","subject":"Re: [BUG] - git-read-tree segfaults","fromName":"Jiri Olsa","fromEmail":"olsajiri@gmail.com","sentAt":"2009-03-11T07:59:06Z","receivedAt":"2009-03-11T07:59:06Z","isPatch":false,"sender":{"key":"olsajiri@gmail.com","avatar":null},"body":"On Tue, Mar 10, 2009 at 9:21 PM, Johannes Schindelin\n wrote:\n> Hi,\n>\n> On Tue, 10 Mar 2009, Jiri Olsa wrote:\n>\n>> mb=$($GIT merge-base HEAD yyy)\n>> $GIT read-tree $mb HEAD yyy\n>\n> While I agree that it is a bad thing for Git to segfault, I think this\n> here is a pilot error.  You try to read 3 trees at the same time, but not\n> perform a merge.  IMHO you want to add -m at least.\n\nagreed, I've already said I executed it like this by an accident...\nit was easy to recreate so I shared... I'm not saying it is a show stopper :)\n\njirka\n\n>\n> Ciao,\n> Dscho\n>\n>\n"},{"id":"107676","messageId":"alpine.DEB.1.00.0903111300330.10279@pacific.mpi-cbg.de","threadId":"18253","inReplyTo":"35476bd20903110059v7d6d8aa0g93af17a940c91919@mail.gmail.com","subject":"Re: [BUG] - git-read-tree segfaults","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-03-11T12:04:15Z","receivedAt":"2009-03-11T12:04:15Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 11 Mar 2009, Jiri Olsa wrote:\n\n> On Tue, Mar 10, 2009 at 9:21 PM, Johannes Schindelin\n> wrote:\n>\n> > On Tue, 10 Mar 2009, Jiri Olsa wrote:\n> >\n> >> mb=$($GIT merge-base HEAD yyy)\n> >> $GIT read-tree $mb HEAD yyy\n> >\n> > While I agree that it is a bad thing for Git to segfault, I think this \n> > here is a pilot error.  You try to read 3 trees at the same time, but \n> > not perform a merge.  IMHO you want to add -m at least.\n> \n> agreed, I've already said I executed it like this by an accident...\n\nHey, you did the right thing! And you even provided a script to recreate, \nso that it was really easy to see what is happening.\n\n> it was easy to recreate so I shared... I'm not saying it is a show \n> stopper :)\n\nWell, Git should not crash. But read-tree is real low-level, so I am \ntorn. OTOH, something like this may be the correct thing to do:\n\n-- snipsnap --\nSubject: [PATCH] Make read-tree with multiple trees imply -m\n\nNoticed by Jiri Olsa.\n\nSigned-off-by: Johannes Schindelin \n\n---\n\n builtin-read-tree.c | 5 +++++\n 1 files changed, 5 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-read-tree.c b/builtin-read-tree.c\nindex 8e02738..547ac25 100644\n--- a/builtin-read-tree.c\n+++ b/builtin-read-tree.c\n@@ -221,6 +221,11 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \t\t\topts.head_idx = 1;\n \t}\n \n+\tif (nr_trees > 1 && !o->merge) {\n+\t\twarning(\"Assuming -m with multiple trees\");\n+\t\to->merge = 1;\n+\t}\n+\n \tfor (i = 0; i < nr_trees; i++) {\n \t\tstruct tree *tree = trees[i];\n \t\tparse_tree(tree);"},{"id":"107762","messageId":"7vtz5zjnai.fsf@gitster.siamese.dyndns.org","threadId":"18253","inReplyTo":"alpine.DEB.1.00.0903111300330.10279@pacific.mpi-cbg.de","subject":"Re: [BUG] - git-read-tree segfaults","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-12T05:57:41Z","receivedAt":"2009-03-12T05:57:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Wed, 11 Mar 2009, Jiri Olsa wrote:\n>\n>> On Tue, Mar 10, 2009 at 9:21 PM, Johannes Schindelin\n>> wrote:\n>>\n>> > On Tue, 10 Mar 2009, Jiri Olsa wrote:\n>> >\n>> >> mb=$($GIT merge-base HEAD yyy)\n>> >> $GIT read-tree $mb HEAD yyy\n>> >\n>> > While I agree that it is a bad thing for Git to segfault, I think this \n>> > here is a pilot error.  You try to read 3 trees at the same time, but \n>> > not perform a merge.  IMHO you want to add -m at least.\n>> \n>> agreed, I've already said I executed it like this by an accident...\n>\n> Hey, you did the right thing! And you even provided a script to recreate, \n> so that it was really easy to see what is happening.\n>\n>> it was easy to recreate so I shared... I'm not saying it is a show \n>> stopper :)\n>\n> Well, Git should not crash. But read-tree is real low-level, so I am \n> torn. OTOH, something like this may be the correct thing to do:\n\nThat's a bogus \"fix\".\n\n\"git read-tree\" without \"-m\" is supposed to behave as an overlay of the\ngiven trees. Try it with any git older than 1.5.5 and you should see one\nentry for xxx and yyy each from Jiri's example. Somewhere between 1.5.4\nand 1.5.5 we seem to have broken it.\n\nHaving said that, I do not think \"read-tree A B C\" to overlay these trees\nhas never worked reliably. For one thing, I do not think the code never\ntried to avoid D/F conflicts in the index, and because of that, you can\nend up with a bogus index that looks like this:\n\n\t$ git ls-files -s\n 100644 58b2ca10b6b032c114cb934c012c3743e34e0e7a 0\txxx\n 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0\txxx/zzz\n 100644 363a9e8d3b0b423eab41fd12ebed489004ab3c2e 0\tyyy\n\nand trying to write it out as a tree object will produce an even more\nbogus result.\n\nI think the attached patch fixes the segfault, and also fixes the\nlongstanding lack of D/F conflict detection, but it needs a bit of\ncommentary.\n\nThe first hunk fixes the D/F conflict issue. After reading three trees\nthat has (xxx), (xxx, yyy) and (xxx/zzz, yyy) in this order, the resulting\nindex should look like this with this patch, instead of the broken index\ndepicted above:\n\n\t$ git ls-files -s\n 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0\txxx/zzz\n 100644 363a9e8d3b0b423eab41fd12ebed489004ab3c2e 0\tyyy\n\nI suspect that the reason add_entry() passed SKIP_DFCHECK was to work\naround an old bug in add_index_entry() that considered it a D/F conflict\nif you have a file D at stage N and a file D/F at stage M when N and M are\ndifferent. I think such a bug has been fixed long time ago, and there is\nno reason for such a workaround. Besides, OK_TO_REPLACE only makes sense\nwhen you do check D/F conflict (\"replace\" in the name of the flag means\n\"If you want to add 'xxx/zzz' when the index has 'xxx', it is Ok to drop\nthe existing 'xxx' in order to add your 'xxx/zzz''); it makes no sense to\ngive it if you are giving SKIP_DFCHECK at the same time.\n\nThe second hunk removes a noop increment of n with o->merge (at this point\nwe know o->merge is zero) and then makes sure we only send an existing\nentry taken from the tree to add_entry().\n\nA NULL src[i] entry is obviously a missing entry from i-th tree, and an\nentry that is o->df_conflict_entry is just a stand-in phantom entry the\nunpack machinery uses when i-th tree has \"xxx/zzz\" (hence it cannot have\n\"xxx\") and the unpacker is looking at path \"xxx\". In either case, i-th\ntree does not have an entry \"xxx\" and we should skip add_entry() for\nsuch a src[i].\n\n unpack-trees.c | 6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex e547282..5820ce4 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -49,7 +49,7 @@ static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,\n \tmemcpy(new, ce, size);\n \tnew->next = NULL;\n \tnew->ce_flags = (new->ce_flags & ~clear) | set;\n-\tadd_index_entry(&o->result, new, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE|ADD_CACHE_SKIP_DFCHECK);\n+\tadd_index_entry(&o->result, new, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n }\n \n /* Unlink the last component and attempt to remove leading\n@@ -286,9 +286,9 @@ static int unpack_nondirectories(int n, unsigned long mask,\n \tif (o->merge)\n \t\treturn call_unpack_fn(src, o);\n \n-\tn += o->merge;\n \tfor (i = 0; i < n; i++)\n-\t\tadd_entry(o, src[i], 0, 0);\n+\t\tif (src[i] && src[i] != o->df_conflict_entry)\n+\t\t\tadd_entry(o, src[i], 0, 0);\n \treturn 0;\n }\n \n"},{"id":"107768","messageId":"7vfxhjjkcm.fsf@gitster.siamese.dyndns.org","threadId":"18253","inReplyTo":"7vtz5zjnai.fsf@gitster.siamese.dyndns.org","subject":"Re: [BUG] - git-read-tree segfaults","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-12T07:01:13Z","receivedAt":"2009-03-12T07:01:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I suspect that the reason add_entry() passed SKIP_DFCHECK was to work\n> around an old bug in add_index_entry() that considered it a D/F conflict\n> if you have a file D at stage N and a file D/F at stage M when N and M are\n> different. I think such a bug has been fixed long time ago, and there is\n> no reason for such a workaround. Besides, OK_TO_REPLACE only makes sense\n> when you do check D/F conflict (\"replace\" in the name of the flag means\n> \"If you want to add 'xxx/zzz' when the index has 'xxx', it is Ok to drop\n> the existing 'xxx' in order to add your 'xxx/zzz''); it makes no sense to\n> give it if you are giving SKIP_DFCHECK at the same time.\n\nThe ancient D/F conflict detection bug in add_cache_entry() I had in mind\nwas fixed by b155725 ([PATCH] Fix oversimplified optimization for\nadd_cache_entry()., 2005-06-25). The use of SKIP_DFCHECK turns out to\nhave nothing to do with working that ancient bug around.\n\nThe real culprit seems to be 34110cd (Make 'unpack_trees()' have a\nseparate source and destination index, 2008-03-06).\n\nBefore that commit:\n\n * merge_entry() that records the final tree-level 3-way merge decision\n used to pass OK_TO_REPLACE without SKIP_DFCHECK.\n\n * unpack_nondirectories() codepath for non-merge multi-tree read-tree\n (the one under discussion in this thread) used to pass SKIP_DFCHECK but\n did not pass OK_TO_REPLACE.\n\nThe fact that even merge_entry() side passes SKIP_DFCHECK these days does\nnot appear to be a workaround for an old bug in D/F conflict detection\ncode after all; it simply is a bug in the refactoring done with the said\ncommit.\n\nThe unpack_nondirectories() codepath passed SKIP_DFCHECK from ee6566e\n(Rewrite read-tree, 2005-09-05), which is the very original implementation\nof the modern \"read-tree A B C\" code. The ancient bug in the D/F conflict\ndetection code was killed way before that commit, and SKIP_DFCHECK in the\ncommit is not a workaround either; it also simply is a bug.\n\n\tSide note: I was somewhat surprised that \"make test\" of that old\n\tcommit dates from September 2005 runs _much_ faster than the test\n\tsuite we have these days.\n\nThe only sane use of SKIP_DFCHECK is when the caller knows the addition is\nnot introducing D/F conflict in the index to avoid the overhead.\n"},{"id":"107769","messageId":"7v3adjjj1y.fsf_-_@gitster.siamese.dyndns.org","threadId":"18253","inReplyTo":"7vfxhjjkcm.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] read-tree A B C: do not create a bogus index and do not segfault","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-12T07:29:13Z","receivedAt":"2009-03-12T07:29:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"git read-tree A B C...\" without the \"-m\" (merge) option is a way to read\nthese trees on top of each other to get an overlay of them.\n\nAn ancient commit ee6566e (Rewrite read-tree, 2005-09-05) passed the\nADD_CACHE_SKIP_DFCHECK flag when calling add_index_entry() to add the\npaths obtained from these trees to the index, but it is an incorrect use\nof the flag. The flag is meant to be used by callers who know the\naddition of the entry does not introduce a D/F conflict to the index in\norder to avoid the overhead of checking.\n\nThis bug resulted in a bogus index that records both \"x\" and \"x/z\" as a\nblob after reading three trees that have paths (\"x\"), (\"x\", \"y\"), and\n(\"x/z\", \"y\") respectively. 34110cd (Make 'unpack_trees()' have a separate\nsource and destination index, 2008-03-06) refactored the callsites of\nadd_index_entry() incorrectly and added more codepaths that use this flag\nwhen it shouldn't be used.\n\nAlso, 0190457 (Move 'unpack_trees()' over to 'traverse_trees()' interface,\n2008-03-05) introduced a bug to call add_index_entry() for the tree that\ndoes not have the path in it, passing NULL as a cache entry. This caused\nreading multiple trees, one of which has path \"x\" but another doesn't, to\nsegfault.\n\nSigned-off-by: Junio C Hamano \n---\n * I suspect that we can probably remove SKIP_DFCHECK option by fixing\n tree.c::read_tree(); the only caller the big comment at the beginning\n of it talks about is overlay_tree_on_cache() in builtin-ls-files.c and\n we haven't gained any new callers of the function.\n\n It is a bit sad that a very good looking refactoring and rewriting\n patch introduced this kind of regression that has gone unnoticed for a\n long time. I managed to point three fingers and they turn out to be\n all ancient commits before v1.5.5.\n\n unpack-trees.c | 6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex cba0aca..5b0a8c1 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -49,7 +49,7 @@ static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,\n \tmemcpy(new, ce, size);\n \tnew->next = NULL;\n \tnew->ce_flags = (new->ce_flags & ~clear) | set;\n-\tadd_index_entry(&o->result, new, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE|ADD_CACHE_SKIP_DFCHECK);\n+\tadd_index_entry(&o->result, new, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n }\n \n /* Unlink the last component and attempt to remove leading\n@@ -283,9 +283,9 @@ static int unpack_nondirectories(int n, unsigned long mask, unsigned long dirmas\n \tif (o->merge)\n \t\treturn call_unpack_fn(src, o);\n \n-\tn += o->merge;\n \tfor (i = 0; i < n; i++)\n-\t\tadd_entry(o, src[i], 0, 0);\n+\t\tif (src[i] && src[i] != o->df_conflict_entry)\n+\t\t\tadd_entry(o, src[i], 0, 0);\n \treturn 0;\n }\n \n-- \n1.6.2.249.g770a0\n"},{"id":"107833","messageId":"alpine.LNX.1.00.0903121024400.19665@iabervon.org","threadId":"18253","inReplyTo":"7v3adjjj1y.fsf_-_@gitster.siamese.dyndns.org","subject":"Re: [PATCH] read-tree A B C: do not create a bogus index and do not segfault","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2009-03-12T14:46:45Z","receivedAt":"2009-03-12T14:46:45Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Thu, 12 Mar 2009, Junio C Hamano wrote:\n\n> \"git read-tree A B C...\" without the \"-m\" (merge) option is a way to read\n> these trees on top of each other to get an overlay of them.\n> \n> An ancient commit ee6566e (Rewrite read-tree, 2005-09-05) passed the\n> ADD_CACHE_SKIP_DFCHECK flag when calling add_index_entry() to add the\n> paths obtained from these trees to the index, but it is an incorrect use\n> of the flag. The flag is meant to be used by callers who know the\n> addition of the entry does not introduce a D/F conflict to the index in\n> order to avoid the overhead of checking.\n\nI think that this implementation actually thought it knew that the entry \ncouldn't cause a D/F conflict. (Of course, it turned out 2.5 years later \nthat it was wrong, on account of missing an implication of the way the \nindex is sorted.)\n\n> This bug resulted in a bogus index that records both \"x\" and \"x/z\" as a\n> blob after reading three trees that have paths (\"x\"), (\"x\", \"y\"), and\n> (\"x/z\", \"y\") respectively. 34110cd (Make 'unpack_trees()' have a separate\n> source and destination index, 2008-03-06) refactored the callsites of\n> add_index_entry() incorrectly and added more codepaths that use this flag\n> when it shouldn't be used.\n>\n> Also, 0190457 (Move 'unpack_trees()' over to 'traverse_trees()' interface,\n> 2008-03-05) introduced a bug to call add_index_entry() for the tree that\n> does not have the path in it, passing NULL as a cache entry. This caused\n> reading multiple trees, one of which has path \"x\" but another doesn't, to\n> segfault.\n\nI think one of the later refactorings may have given up on seeing \nconflicts while reading trees, but didn't drop the flag (perhaps because \nLinus knew at the time that my assumption that conflicts would actually \nhave been recognized was false, and didn't realize that it was also the \nsource of the flag).\n\n> Signed-off-by: Junio C Hamano \n> ---\n> * I suspect that we can probably remove SKIP_DFCHECK option by fixing\n> tree.c::read_tree(); the only caller the big comment at the beginning\n> of it talks about is overlay_tree_on_cache() in builtin-ls-files.c and\n> we haven't gained any new callers of the function.\n> \n> It is a bit sad that a very good looking refactoring and rewriting\n> patch introduced this kind of regression that has gone unnoticed for a\n> long time. I managed to point three fingers and they turn out to be\n> all ancient commits before v1.5.5.\n\nI'm not too surprised that \"read-tree\" with multiple trees without -m \ndoesn't actually work, because I don't think this has ever been useful for \nanything, and nobody's been able to come up with an implementation \nof read-tree that's particularly easy to understand. So regular testing \nwouldn't find this problem, and inspection wouldn't find it, either.\n\nI think it might be a good idea to take this as evidence that nobody is \nusing read-tree with multiple trees without merge, and just disallow it. \nIf it takes a year to hit a pretty big bug in a code path, and using the \ncode path was due to a typo anyway, it's unlikely that the code path is \nactually used for anything.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"107845","messageId":"alpine.LFD.2.00.0903120929250.32478@localhost.localdomain","threadId":"18253","inReplyTo":"alpine.LNX.1.00.0903121024400.19665@iabervon.org","subject":"Re: [PATCH] read-tree A B C: do not create a bogus index and do not segfault","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-03-12T16:34:49Z","receivedAt":"2009-03-12T16:34:49Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 12 Mar 2009, Daniel Barkalow wrote:\n> \n> I think one of the later refactorings may have given up on seeing \n> conflicts while reading trees, but didn't drop the flag (perhaps because \n> Linus knew at the time that my assumption that conflicts would actually \n> have been recognized was false, and didn't realize that it was also the \n> source of the flag).\n\nThat is very thoughtful of you, but I suspect the real reason was a much \nsimpler one: redoing the read-tree logic was a huge pain in the posterior, \nand my primary goal at the time was to make the code more readable while \npassing all the tests.\n\nSo while I tried to make patches minimal (which may well explain why the \nflag remained), it wasn't the main goal, and trying to sort out the \nread-tree logic into something more understandable was quite test-driven.\n\nSo I think the big issue to take away from this is that I think this is \nsuch an odd case that if we want to support it, it would need explicit \ntests. Or:\n\n> I think it might be a good idea to take this as evidence that nobody is \n> using read-tree with multiple trees without merge, and just disallow it. \n\nHmm. It _has_ been used. It's been useful for really odd things \nhistorically, namely trying to merge different trees by hand. So while I \nagree that we could probably remove it, it _is_ a very interesting \nfeature in theory, and since we have the code.. \n\nSo I'd say \"add a few tests for the known horrible cases\" should be the \nfirst approach. If it ever actually breaks again and becomes a big \nmaintenance headache, maybe _then_ remove the feature as not being worth \nthe pain?\n\n\t\t\tLinus\n"},{"id":"107997","messageId":"7v7i2sehcb.fsf@gitster.siamese.dyndns.org","threadId":"18253","inReplyTo":"alpine.LFD.2.00.0903120929250.32478@localhost.localdomain","subject":"Re: [PATCH] read-tree A B C: do not create a bogus index and do not segfault","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-14T06:41:56Z","receivedAt":"2009-03-14T06:41:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Thu, 12 Mar 2009, Daniel Barkalow wrote:\n>\n>> I think it might be a good idea to take this as evidence that nobody is \n>> using read-tree with multiple trees without merge, and just disallow it. \n>\n> Hmm. It _has_ been used. It's been useful for really odd things \n> historically, namely trying to merge different trees by hand. So while I \n> agree that we could probably remove it, it _is_ a very interesting \n> feature in theory, and since we have the code.. \n>\n> So I'd say \"add a few tests for the known horrible cases\" should be the \n> first approach. If it ever actually breaks again and becomes a big \n> maintenance headache, maybe _then_ remove the feature as not being worth \n> the pain?\n\nI've added trivial tests and will start cooking it by letting it go\nthrough the usual pu/next/master/maint cycle.\n\nI think you are thinking about something like the \"gitk\" merge (aka \"The\ncoolest merge EVER!\" [*1*]), but you can do the same thing more safely\nwith -m option, giving an empty tree as the common ancestor. Especially\nif you run it in the aggressive mode, \"addition\" from our tree and their\ntree, when it is an overlay, will not overlap, and will all cleanly\nresolve to stage #0.\n\nI suspect you can also use a single tree \"read-tree\", immediately followed\nby another \"read-tree --prefix=''\" to read in two trees into the index and\nwrite the results out.\n\n[Footnote]\n\n*1* http://article.gmane.org/gmane.comp.version-control.git/5126\n"}]}