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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 1, 2020, 21:02 UTC
Message-ID: <xmqqeepvc5k5.fsf@gitster.c.googlers.com>
In-Reply-To: <nycvar.QRO.7.76.6.2007011144410.56@tvgsbejvaqbjf.bet>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 27 quoted lines

> On Thu, 25 Jun 2020, Junio C Hamano wrote:
>
>> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
>> writes:
>>
>> > diff --git a/diff-lib.c b/diff-lib.c
>> > index 61812f48c2..25fd2dee19 100644
>> > --- a/diff-lib.c
>> > +++ b/diff-lib.c
>> > @@ -220,8 +220,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
>> >  			} else if (revs->diffopt.ita_invisible_in_index &&
>> >  				   ce_intent_to_add(ce)) {
>> >  				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
>> > -					       the_hash_algo->empty_tree, 0,
>> > -					       ce->name, 0);
>> > +					       &null_oid, 0, ce->name, 0);
>>
>> This (even if the preimage were correctly using empty_blob) is *so*
>> simple a change that it is very surprising that the new test in
>> [2/2] passes without any other code change.
>>
>> It means that difftool was written correctly in the first place,
>> right?
>
> Well, it means that difftool does not even consume the OID. Sure, it
> parses it, but then it ignores it. All it _really_ is interested in is
> that status flag (`A`),

Ah, that's an ultimately defensive code. No matter what is on the right hand side of the raw output, as long as it knows that the right hand side is a file on the working tree, it can safely ignore the object name and directly go to the working tree file.

Nice ;-)

> so technically, my fix in 1/2 is not even needed
> after `sk/diff-files-show-i-t-a-as-new` to let 2/2 pass.

Sure, but I think it is the right thing to do nevertheless. Giving the object name for an empty blob would be wrong unless the working tree file is known to be empty (and empty tree? what were we even thinking, gee...).