On Thu, Jun 25, 2015 at 12:05 AM, Junio C Hamano <gits...@pobox.com> wrote: > Internal "diff-index --cached" is used for another reason you did > not mention (and scripted Porcelains literally use that externally > for the same reason). When we start a mergy operation, we say it is > OK if the working tree has local changes relative to the index, but > we require the index does not have any modifications since the HEAD > was read. > > Side note: some codepaths insist that "diff-index --cached" > and "diff-files" are both clean, so d95d728a is harmless; > the former may say "clean" on i-t-a entries more than > before, but the latter will still catch the difference > between the index and the working tree and stop the caller > from going forward. > > With d95d728a (diff-lib.c: adjust position of i-t-a entries in diff, > 2015-03-16)'s world view, an empty output from "diff-index --cached" > no longer means that. Entries added with any "git add -N" are not > reported, so we would go ahead to record the merge result on top of > that "half-dirty" index. > > Side note: a merge based on unpack-trees has an extra layer > of safety that unpack_trees() does not ignore i-t-a entry as > "not exist (yet)" and notices that the path does exist in > the index but not in HEAD. But that just shows that some > parts of the world do need to consider that having an i-t-a > in the index makes it different from HEAD. > > If the mergy operation goes without any conflict, the next thing we > do typically is to write the index out as a tree (to record in a new > commit, etc.) and we are OK only in that particular case, because > i-t-a entries are ignored. But what would happen when the mergy > operation conflicts? I haven't thought about that fully, but I > doubt it is a safe thing to do in general. > > But that is just one example that there are also other codepaths > that do not want to be fooled into thinking that i-t-a entries do > not exist in the index at all.
I think it's clear that you need to revert that commit. I didn't see this at all when I made the commit. > All we learned from the above discussion is that unconditionally > declaring that adding i-t-a entries to the index without doing > anything else should keep the index compare the same to HEAD. > > If d95d728a were only about what wt_status.c sees (and gets reported > in "git status" output), which was what the change wanted to address > anyway, we didn't have to have this discussion. Without realizing > that two kinds of callers want different view out of "diff-index > --cached" and "diff-files", we broke half the world by changing the > world order unconditionally for everybody, I am afraid. > > Perhaps a good and safe way forward to resurrect what d95d728a > wanted to do is to first add an option to tell run_diff_index() and > run_diff_files() which behaviour the caller wants to see, add that > only to the caller in wt-status.c? Then incrementally pass that > option from more callsites that we are absolutely certain that want > this different worldview with respect to i-t-a? Agreed. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
