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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: May 25, 2018, 03:39 UTC
Message-ID: <20180525033942.GA234191@aiede.svl.corp.google.com>
In-Reply-To: <20180513175438.32152-1-pclouds@gmail.com>

Hi Duy,

Nguyễn Thái Ngọc Duy wrote:

Show 11 quoted lines

> 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?

To put it another way, the basic aspects I look for in a commit message are:

 - the motivation behind the change (a wrong behavior, a task that isn't
   possible, some excessive complexity, or whatever).  The reader
   doesn't know your motivation so their default attitude will be to
   assume that nothing should change

 - a little more detail about the why and how behind the current
   behavior, to put the proposed in context.  This makes it easier for
   the reader to understand how the change will affect users of that
   behavior that don't necessarily have the same motivation.

   An example illustrating the behavior can work well here.

 - any interesting details of implementation or alternatives considered
   that can make the patch easier to read now that the motivation is out
   of the way.

 - a word or two on what this makes possible

I'm having trouble pulling apart these pieces in this commit message. Can you give an example of a command's output before and after this change so I can understand better why it's a good one?

Show 16 quoted lines

> This option is on by default in git-status [1] but we need more fixup
> in rename detection code [3]. Luckily we don't need to do anything
> else for the rename detection code in diff.c (wt-status.c uses a
> customized one).
>
> [1] 425a28e0a4 (diff-lib: allow ita entries treated as "not yet exist
>     in index" - 2016-10-24)
> [2] b42b451919 (diff: add --ita-[in]visible-in-index - 2016-10-24)
> [3] bc3dca07f4 (Merge branch 'nd/ita-wt-renames-in-status' - 2018-01-23)
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/diff.c          |  7 +++++++
>  t/t2203-add-intent.sh   | 37 ++++++++++++++++++++++++++++++-------
>  t/t4011-diff-symlink.sh | 10 ++++++----
>  3 files changed, 43 insertions(+), 11 deletions(-)

This flips the default as announced but I'm not sure yet whether it's a good thing. After all, an intent-to-add entry is a real entry in the index; why wouldn't it show up in "git diff --cached"?

Is the idea that it shouldn't show up there because "git commit" would not include the intent-to-add entry? That makes some sense to me.

What does the endgame look like? Would we flip the default to --ita-invisible and then remove the option?

Context is that an internal script does something like

	echo 'This file is added!' >added
	git add --intent-to-add added
	git diff --name-only --no-renames --diff-filter=A master

to list added files and started failing with this patch (in "next"). Arguably the script should use diff-index for a stronger stability guarantee. Should the script use --ita-visible as a futureproofing measure as well?

Actually, why is this "git diff" output changing at all, given that the script doesn't pass --cached? I would expect "git diff" to show the ITA entry because it affects the result of "git commit -a".

Thanks, Jonathan