Re: [PATCH] apply: fix adding new files on i-t-a entries

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Jun 25, 2015 at 12:05 AM, Junio C Hamano <gitster@xxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]