Re: [PATCH v4 1/2] diff-files --raw: show correct post-image of intent-to-add files

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Junio C Hamano <gitster@pobox.com>
Cc: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org,
	Srinidhi Kaushik <shrinidhi.kaushik@gmail.com>
Subject: Re: [PATCH v4 1/2] diff-files --raw: show correct post-image of intent-to-add files
Date: Wed, 1 Jul 2020 11:52:12 +0200 (CEST)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.2007011149000.56@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <xmqqeeq33t2q.fsf@gitster.c.googlers.com>

Hi Junio,

On Thu, 25 Jun 2020, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > The documented behavior of `git diff-files --raw` is to display
> >
> > 	[...] 0{40} if creation, unmerged or "look at work tree".
>
> "on the right hand (i.e. postimage) side" is missing, which is
> crucial in this description.

True. Sorry for omitting that.

> > This happens for example when showing modified, unstaged files.
>
> Modified I would understand.  We notice that the contents on the
> work tree is different from what is in the index, and we tell the
> consumer "look at work tree".  I do not think you meant "unstaged",

Right. I probably should have written "[...] when showing files with
unstaged modifications".

> though.  If truly removed from the index, diff-files won't even know
> about the path.  You probably meant "removed from the working tree",
> but 0{40} in that case means totally different thing (i.e. it would
> be a (D)eletion record, so 0{40} on the postimage side is a filler,
> not even "look at work tree").  What would make more sense to
> describe might be
>
> 	This happens for paths that are modified, or unmodified but
> 	stat-dirty.

Yes, but that includes more than I wanted to describe because the
stat-dirty does not matter for intent-to-add files, and I wanted to point
out the analogy between unstaged modifications and intent-to-add-files.

I noticed that you merged this commit into `next` already, so I am
assuming that you do not want me to send a fifth iteration ;-)

Ciao,
Dscho

> Either case, we tell the consumer to check the "work tree".
>
> > For intent-to-add files, we used to show the empty blob's hash instead.
> > In c26022ea8f5 (diff: convert diff_addremove to struct object_id,
> > 2017-05-30), we made that worse by inadvertently changing that to the
> > hash of the empty tree.
> >
> > Let's make the behavior consistent with modified files by showing
> > all-zero values also for intent-to-add files.
>
> Well described.
>
> Thanks.
>