Re: [PATCH 1/2] diff: turn --ita-invisible-in-index on by default

From: Duy Nguyen <pclouds@gmail.com>
Date: May 25, 2018, 14:30 UTC
Message-ID: <20180525143042.GA10607@duynguyen.home>
In-Reply-To: <20180525033942.GA234191@aiede.svl.corp.google.com>

On Thu, May 24, 2018 at 08:39:42PM -0700, Jonathan Nieder wrote:

Show 24 quoted lines

> Hi Duy,
> 
> Nguyễn Thái Ngọc Duy wrote:
> 
> > Due to the implementation detail of intent-to-add entries. The current
> > "git diff" (i.e. no treeish or --cached argument) would show the
> > changes in the i-t-a file, but it does not mark the file as new, while
> > "diff --cached" would mark the file as new while showing its content
> > as empty.
> >
> > One evidence of the current output being wrong is that, the output
> > from "git diff" (with ita entries) cannot be applied because it
> > assumes empty files exist before applying.
> >
> > Turning on --ita-invisible-in-index [1] [2] would fix this.
> 
> I'm having a lot of trouble understanding the above.  Part of my
> confusion may be grammatical: for example, the first sentence is a
> sentence fragment.  Another part is that the commit message doesn't tell
> me a story: what does the user try to do and fail that is not possible
> without this?  What is the intention or effect behind the commits
> mentioned that leads to them being cited?
>
> ...

Sorry, this i-t-a thing had been on my mind for too long I mistakenly thought this problem was common knowledge. Reference [1] in that commit points to a previous attempt d95d728aba (diff-lib.c: adjust position of i-t-a entries in diff - 2015-03-16) that was reverted, which gives more info.

Anyway this is the diff we see today

    $ echo haha > new; git add -N
    $ git diff
    diff --git a/new b/new
    index e69de29..5ad28e2 100644
    --- a/new
    +++ b/new
    @@ -0,0 +1 @@
    +haha

Notice that the diff does not tell you that 'new' is a new file. The diff with this patch gives you this

    $ git diff
    diff --git a/new b/new
    new file mode 100644
    index 0000000..5ad28e2
    --- /dev/null
    +++ b/new
    @@ -0,0 +1 @@
    +haha

You may argue that an intent-to-add entry is a real entry in the index and showing "new file mode 100644" here is wrong. I beg to differ. That just happens to be how you mark an ita entry. If Junio chose to record ita entries as an index extension, then they would not be "real" and could still be used to remind users about things to add.

From the user perspective, as a user I do not care if it's a "real entry". I want git to _remind_ myself that I need to add that file. That file should not truly exist in the index because I have not actually added it. I did not tell git to add anything to the index.

One consequence of this is you can't apply the diff generated with ita entries because the diff expects empty files to be already in the worktree. This to me does not make sense.

Of course there's other things that also go along this line. Like if "git commit" does not add an ita entry, why should it appear in 'git diff --cached', which should show you what's to be committed. Right know it shows

    $ git diff --cached
    diff --git a/new b/new
    new file mode 100644
    index 0000000..e69de29

which to me is ridiculous. Why would it show me a diff of a new file with empty content? I as a user have not mentioned "empty content" anywhere through the commands I have used.

Since this commit is already in 'next', it's too late to update the commit message now. Maybe I can elaborate about this more in git-add.txt if needed (and then I can add more explanation in the commit message that updates that file). -- Duy