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

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

[Nguyễn Thái Ngọc Duy]

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.

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(-)

diff --git a/builtin/diff.c b/builtin/diff.c
index 16bfb22f73..00424c296d 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -352,6 +352,13 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	rev.diffopt.flags.allow_external = 1;
 	rev.diffopt.flags.allow_textconv = 1;
 
+	/*
+	 * Default to intent-to-add entries invisible in the
+	 * index. This makes them show up as new files in diff-files
+	 * and not at all in diff-cached.
+	 */
+	rev.diffopt.ita_invisible_in_index = 1;
+
 	if (nongit)
 		die(_("Not a git repository"));
 	argc = setup_revisions(argc, argv, &rev, NULL);
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 78236dc7d8..31bf50082f 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -69,9 +69,9 @@ test_expect_success 'i-t-a entry is simply ignored' '
 	git add -N nitfol &&
 	git commit -m second &&
 	test $(git ls-tree HEAD -- nitfol | wc -l) = 0 &&
-	test $(git diff --name-only HEAD -- nitfol | wc -l) = 1 &&
-	test $(git diff --name-only --ita-invisible-in-index HEAD -- nitfol | wc -l) = 0 &&
-	test $(git diff --name-only --ita-invisible-in-index -- nitfol | wc -l) = 1
+	test $(git diff --name-only --ita-visible-in-index HEAD -- nitfol | wc -l) = 1 &&
+	test $(git diff --name-only HEAD -- nitfol | wc -l) = 0 &&
+	test $(git diff --name-only -- nitfol | wc -l) = 1
 '
 
 test_expect_success 'can commit with an unrelated i-t-a entry in index' '
@@ -99,13 +99,13 @@ test_expect_success 'cache-tree invalidates i-t-a paths' '
 
 	: >dir/bar &&
 	git add -N dir/bar &&
-	git diff --cached --name-only >actual &&
+	git diff --name-only >actual &&
 	echo dir/bar >expect &&
 	test_cmp expect actual &&
 
 	git write-tree >/dev/null &&
 
-	git diff --cached --name-only >actual &&
+	git diff --name-only >actual &&
 	echo dir/bar >expect &&
 	test_cmp expect actual
 '
@@ -186,7 +186,19 @@ test_expect_success 'rename detection finds the right names' '
 		cat >expected.3 <<-EOF &&
 		2 .R N... 100644 100644 100644 $hash $hash R100 third	first
 		EOF
-		test_cmp expected.3 actual.3
+		test_cmp expected.3 actual.3 &&
+
+		git diff --stat >actual.4 &&
+		cat >expected.4 <<-EOF &&
+		 first => third | 0
+		 1 file changed, 0 insertions(+), 0 deletions(-)
+		EOF
+		test_cmp expected.4 actual.4 &&
+
+		git diff --cached --stat >actual.5 &&
+		: >expected.5 &&
+		test_cmp expected.5 actual.5
+
 	)
 '
 
@@ -222,5 +234,16 @@ test_expect_success 'double rename detection in status' '
 	)
 '
 
-test_done
+test_expect_success 'diff-files/diff-cached shows ita as new/not-new files' '
+	git reset --hard &&
+	echo new >new-ita &&
+	git add -N new-ita &&
+	git diff --summary >actual &&
+	echo " create mode 100644 new-ita" >expected &&
+	test_cmp expected actual &&
+	git diff --cached --summary >actual2 &&
+	: >expected2 &&
+	test_cmp expected2 actual2
+'
 
+test_done
diff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh
index cf0f3a1ee7..108c012a3a 100755
--- a/t/t4011-diff-symlink.sh
+++ b/t/t4011-diff-symlink.sh
@@ -139,11 +139,13 @@ test_expect_success SYMLINKS 'setup symlinks with attributes' '
 test_expect_success SYMLINKS 'symlinks do not respect userdiff config by path' '
 	cat >expect <<-\EOF &&
 	diff --git a/file.bin b/file.bin
-	index e69de29..d95f3ad 100644
-	Binary files a/file.bin and b/file.bin differ
+	new file mode 100644
+	index 0000000..d95f3ad
+	Binary files /dev/null and b/file.bin differ
 	diff --git a/link.bin b/link.bin
-	index e69de29..dce41ec 120000
-	--- a/link.bin
+	new file mode 120000
+	index 0000000..dce41ec
+	--- /dev/null
 	+++ b/link.bin
 	@@ -0,0 +1 @@
 	+file.bin
-- 
2.17.0.705.g3525833791

[PATCH 2/2] apply: add --intent-to-add

[Nguyễn Thái Ngọc Duy]

Similar to 'git reset -N', this option makes 'git apply' automatically
mark new files as intent-to-add so they are visible in the following
'git diff' command and could also be committed with 'git commit -a'.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-apply.txt |  9 ++++++++-
 apply.c                     | 38 +++++++++++++++++++++++++++++++------
 apply.h                     |  1 +
 t/t2203-add-intent.sh       | 12 ++++++++++++
 4 files changed, 53 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index 4ebc3d3271..2374f64b51 100644
--- a/Documentation/git-apply.txt
+++ b/Documentation/git-apply.txt
@@ -9,7 +9,7 @@ git-apply - Apply a patch to files and/or to the index
 SYNOPSIS
 --------
 [verse]
-'git apply' [--stat] [--numstat] [--summary] [--check] [--index] [--3way]
+'git apply' [--stat] [--numstat] [--summary] [--check] [--index | --intent-to-add] [--3way]
 	  [--apply] [--no-add] [--build-fake-ancestor=<file>] [-R | --reverse]
 	  [--allow-binary-replacement | --binary] [--reject] [-z]
 	  [-p<n>] [-C<n>] [--inaccurate-eof] [--recount] [--cached]
@@ -74,6 +74,13 @@ OPTIONS
 	cached data, apply the patch, and store the result in the index
 	without using the working tree. This implies `--index`.
 
+--intent-to-add::
+	When applying the patch only to the working tree, mark new
+	files to be added to the index later (see `--intent-to-add`
+	option in linkgit:git-add[1]). This option is ignored if
+	`--index` is present or the command is not run in a Git
+	repository.
+
 -3::
 --3way::
 	When the patch does not apply cleanly, fall back on 3-way merge if
diff --git a/apply.c b/apply.c
index 7e5792c996..31d3e50401 100644
--- a/apply.c
+++ b/apply.c
@@ -136,6 +136,8 @@ int check_apply_state(struct apply_state *state, int force_apply)
 		state->apply = 0;
 	if (state->check_index && is_not_gitdir)
 		return error(_("--index outside a repository"));
+	if (state->set_ita && is_not_gitdir)
+		state->set_ita = 0;
 	if (state->cached) {
 		if (is_not_gitdir)
 			return error(_("--cached outside a repository"));
@@ -4265,9 +4267,6 @@ static int add_index_file(struct apply_state *state,
 	int namelen = strlen(path);
 	unsigned ce_size = cache_entry_size(namelen);
 
-	if (!state->update_index)
-		return 0;
-
 	ce = xcalloc(1, ce_size);
 	memcpy(ce->name, path, namelen);
 	ce->ce_mode = create_ce_mode(mode);
@@ -4305,6 +4304,27 @@ static int add_index_file(struct apply_state *state,
 	return 0;
 }
 
+static int add_ita_file(struct apply_state *state,
+			const char *path, unsigned mode)
+{
+	struct cache_entry *ce;
+	int namelen = strlen(path);
+	unsigned ce_size = cache_entry_size(namelen);
+
+	ce = xcalloc(1, ce_size);
+	memcpy(ce->name, path, namelen);
+	ce->ce_mode = create_ce_mode(mode);
+	ce->ce_flags = create_ce_flags(0) | CE_INTENT_TO_ADD;
+	ce->ce_namelen = namelen;
+	set_object_name_for_intent_to_add_entry(ce);
+	if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD) < 0) {
+		free(ce);
+		return error(_("unable to add cache entry for %s"), path);
+	}
+
+	return 0;
+}
+
 /*
  * Returns:
  *  -1 if an unrecoverable error happened
@@ -4465,8 +4485,11 @@ static int create_file(struct apply_state *state, struct patch *patch)
 
 	if (patch->conflicted_threeway)
 		return add_conflicted_stages_file(state, patch);
-	else
+	else if (state->update_index)
 		return add_index_file(state, path, mode, buf, size);
+	else if (state->set_ita)
+		return add_ita_file(state, path, mode);
+	return 0;
 }
 
 /* phase zero is to remove, phase one is to create */
@@ -4687,7 +4710,8 @@ static int apply_patch(struct apply_state *state,
 		state->apply = 0;
 
 	state->update_index = state->check_index && state->apply;
-	if (state->update_index && !is_lock_file_locked(&state->lock_file)) {
+	if ((state->update_index || state->set_ita) &&
+	    !is_lock_file_locked(&state->lock_file)) {
 		if (state->index_file)
 			hold_lock_file_for_update(&state->lock_file,
 						  state->index_file,
@@ -4888,7 +4912,7 @@ int apply_all_patches(struct apply_state *state,
 				state->whitespace_error);
 	}
 
-	if (state->update_index) {
+	if (state->update_index || state->set_ita) {
 		res = write_locked_index(&the_index, &state->lock_file, COMMIT_LOCK);
 		if (res) {
 			error(_("Unable to write new index file"));
@@ -4941,6 +4965,8 @@ int apply_parse_options(int argc, const char **argv,
 			N_("instead of applying the patch, see if the patch is applicable")),
 		OPT_BOOL(0, "index", &state->check_index,
 			N_("make sure the patch is applicable to the current index")),
+		OPT_BOOL('N', "intent-to-add", &state->set_ita,
+			N_("mark new files with `git add --intent-to-add`")),
 		OPT_BOOL(0, "cached", &state->cached,
 			N_("apply a patch without touching the working tree")),
 		OPT_BOOL_F(0, "unsafe-paths", &state->unsafe_paths,
diff --git a/apply.h b/apply.h
index dc4a019057..94b38533a2 100644
--- a/apply.h
+++ b/apply.h
@@ -45,6 +45,7 @@ struct apply_state {
 	int check; /* preimage must match working tree, don't actually apply */
 	int check_index; /* preimage must match the indexed version */
 	int update_index; /* check_index && apply */
+	int set_ita;	  /* add intent-to-add entries to the index */
 
 	/* These control cosmetic aspect of the output */
 	int diffstat; /* just show a diffstat, and don't actually apply */
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 31bf50082f..1d640a33f0 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -246,4 +246,16 @@ test_expect_success 'diff-files/diff-cached shows ita as new/not-new files' '
 	test_cmp expected2 actual2
 '
 
+test_expect_success 'apply --intent-to-add' '
+	git reset --hard &&
+	echo new >new-ita &&
+	git add -N new-ita &&
+	git diff >expected &&
+	grep "new file" expected &&
+	git reset --hard &&
+	git apply --intent-to-add expected &&
+	git diff >actual &&
+	test_cmp expected actual
+'
+
 test_done
-- 
2.17.0.705.g3525833791

Re: [PATCH 2/2] apply: add --intent-to-add

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Similar to 'git reset -N', this option makes 'git apply' automatically
> mark new files as intent-to-add so they are visible in the following
> 'git diff' command and could also be committed with 'git commit -a'.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Documentation/git-apply.txt |  9 ++++++++-
>  apply.c                     | 38 +++++++++++++++++++++++++++++++------
>  apply.h                     |  1 +
>  t/t2203-add-intent.sh       | 12 ++++++++++++
>  4 files changed, 53 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
> index 4ebc3d3271..2374f64b51 100644
> --- a/Documentation/git-apply.txt
> +++ b/Documentation/git-apply.txt
> @@ -9,7 +9,7 @@ git-apply - Apply a patch to files and/or to the index
>  SYNOPSIS
>  --------
>  [verse]
> -'git apply' [--stat] [--numstat] [--summary] [--check] [--index] [--3way]
> +'git apply' [--stat] [--numstat] [--summary] [--check] [--index | --intent-to-add] [--3way]
>  	  [--apply] [--no-add] [--build-fake-ancestor=<file>] [-R | --reverse]
>  	  [--allow-binary-replacement | --binary] [--reject] [-z]
>  	  [-p<n>] [-C<n>] [--inaccurate-eof] [--recount] [--cached]
> @@ -74,6 +74,13 @@ OPTIONS
>  	cached data, apply the patch, and store the result in the index
>  	without using the working tree. This implies `--index`.
>  
> +--intent-to-add::
> +	When applying the patch only to the working tree, mark new
> +	files to be added to the index later (see `--intent-to-add`
> +	option in linkgit:git-add[1]). This option is ignored if
> +	`--index` is present or the command is not run in a Git
> +	repository.

It may make sense to make it incompatible with "--index" like you
did, but how does this interact with "--cached" or "--3way"?  It is
unclear from the above documentation.

> diff --git a/apply.c b/apply.c
> index 7e5792c996..31d3e50401 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -136,6 +136,8 @@ int check_apply_state(struct apply_state *state, int force_apply)
>  		state->apply = 0;
>  	if (state->check_index && is_not_gitdir)
>  		return error(_("--index outside a repository"));
> +	if (state->set_ita && is_not_gitdir)
> +		state->set_ita = 0;

I think this should error out, just like one line above does.
"I-t-a" is impossible without having the index, just like "--index"
is impossible without having the index.

> @@ -4265,9 +4267,6 @@ static int add_index_file(struct apply_state *state,
>  	int namelen = strlen(path);
>  	unsigned ce_size = cache_entry_size(namelen);
>  
> -	if (!state->update_index)
> -		return 0;
> -

OK, with this change, only "index-affecting" mode will call into the
function, in the first place.  The current code was wasteful in that
the caller always called and forced this function to do a few useless
computation before it realized that it is not going to touch the index
at all.

> @@ -4305,6 +4304,27 @@ static int add_index_file(struct apply_state *state,
>  	return 0;
>  }
>  
> +static int add_ita_file(struct apply_state *state,
> +			const char *path, unsigned mode)
> +{
> +	struct cache_entry *ce;
> +	int namelen = strlen(path);
> +	unsigned ce_size = cache_entry_size(namelen);
> +
> +	ce = xcalloc(1, ce_size);
> +	memcpy(ce->name, path, namelen);
> +	ce->ce_mode = create_ce_mode(mode);
> +	ce->ce_flags = create_ce_flags(0) | CE_INTENT_TO_ADD;
> +	ce->ce_namelen = namelen;
> +	set_object_name_for_intent_to_add_entry(ce);
> +	if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD) < 0) {
> +		free(ce);
> +		return error(_("unable to add cache entry for %s"), path);
> +	}
> +
> +	return 0;
> +}

It is somewhat unsatisfactory that we need a whole new function to
record a new path in the index.  IOW, I have a feeling that a bit of
refactoring of add_index_file() should allow us to share more code
between it and this new function.

> @@ -4465,8 +4485,11 @@ static int create_file(struct apply_state *state, struct patch *patch)
>  
>  	if (patch->conflicted_threeway)
>  		return add_conflicted_stages_file(state, patch);
> -	else
> +	else if (state->update_index)
>  		return add_index_file(state, path, mode, buf, size);
> +	else if (state->set_ita)
> +		return add_ita_file(state, path, mode);
> +	return 0;

It is very unfortunate that you need to do (update_index||set_ita)
everywhere else only bevause you want to do this else/if cascade.
I'd rather redefine the bits to mean

    - update-index: we will do something that touches the index
      (hence we need to have the repository, we need to lock the
      index, etc.).

    - ita-only: changes to the existing paths are only reflected to
      the working tree, but new paths are added to the index as
      i-t-a entries.

and make add_index_file() more intelligent, without having to add a
new add_ita_file().

Of course, setting only ita-only without setting update-index is an
inconsistent state.  "--index" would set only update-index, "--ita"
would set both, "--ita --index" would either be an error, or set
only update-index and clear ita-only if we adopt "last one wins"
semantics.

Re: [PATCH 2/2] apply: add --intent-to-add

[Duy Nguyen]

On Mon, May 14, 2018 at 11:33:48AM +0900, Junio C Hamano wrote:
> > diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
> > index 4ebc3d3271..2374f64b51 100644
> > --- a/Documentation/git-apply.txt
> > +++ b/Documentation/git-apply.txt
> > @@ -9,7 +9,7 @@ git-apply - Apply a patch to files and/or to the index
> >  SYNOPSIS
> >  --------
> >  [verse]
> > -'git apply' [--stat] [--numstat] [--summary] [--check] [--index] [--3way]
> > +'git apply' [--stat] [--numstat] [--summary] [--check] [--index | --intent-to-add] [--3way]
> >  	  [--apply] [--no-add] [--build-fake-ancestor=<file>] [-R | --reverse]
> >  	  [--allow-binary-replacement | --binary] [--reject] [-z]
> >  	  [-p<n>] [-C<n>] [--inaccurate-eof] [--recount] [--cached]
> > @@ -74,6 +74,13 @@ OPTIONS
> >  	cached data, apply the patch, and store the result in the index
> >  	without using the working tree. This implies `--index`.
> >  
> > +--intent-to-add::
> > +	When applying the patch only to the working tree, mark new
> > +	files to be added to the index later (see `--intent-to-add`
> > +	option in linkgit:git-add[1]). This option is ignored if
> > +	`--index` is present or the command is not run in a Git
> > +	repository.
> 
> It may make sense to make it incompatible with "--index" like you
> did, but how does this interact with "--cached" or "--3way"?  It is
> unclear from the above documentation.

I did check --cached and it mentioned about implying --index so I
thought that was enough. Will elaborate a bit more.

> > diff --git a/apply.c b/apply.c
> > index 7e5792c996..31d3e50401 100644
> > --- a/apply.c
> > +++ b/apply.c
> > @@ -136,6 +136,8 @@ int check_apply_state(struct apply_state *state, int force_apply)
> >  		state->apply = 0;
> >  	if (state->check_index && is_not_gitdir)
> >  		return error(_("--index outside a repository"));
> > +	if (state->set_ita && is_not_gitdir)
> > +		state->set_ita = 0;
> 
> I think this should error out, just like one line above does.
> "I-t-a" is impossible without having the index, just like "--index"
> is impossible without having the index.

I was hoping to put this in an alias that works both with or without a
repository. Do you feel strongly about this? I may need to find
another way to achieve that instead.
--
Duy

Re: [PATCH 2/2] apply: add --intent-to-add

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

>> >  	if (state->check_index && is_not_gitdir)
>> >  		return error(_("--index outside a repository"));
>> > +	if (state->set_ita && is_not_gitdir)
>> > +		state->set_ita = 0;
>> 
>> I think this should error out, just like one line above does.
>> "I-t-a" is impossible without having the index, just like "--index"
>> is impossible without having the index.
>
> I was hoping to put this in an alias that works both with or without a
> repository. Do you feel strongly about this? 

"git apply --index" that silently applied only to the filesystem
files without telling me that I am in a wrong directory by mistake
is a UI disaster, and that is what the existing error we see in the
pre context is about.  I do not think of a good reason why "git
apply --i-t-a" should behave any differently.

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

[Jonathan Nieder]

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?

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?

> 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

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

[Duy Nguyen]

On Thu, May 24, 2018 at 08:39:42PM -0700, Jonathan Nieder wrote:
> 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

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

[Jonathan Nieder]

Hi,

Duy Nguyen wrote:

>     $ 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
[...]
> 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.

Thankds for this context.  That helps a lot.

[...]
> Since this commit is already in 'next', it's too late to update the
> commit message now.

I think it should be reverted from 'next' because of the unintended
change to the behavior of "git diff HEAD".  Here is what I have
applied internally.

-- >8 -- 
Subject: Revert "diff: turn --ita-invisible-in-index on by default"

This reverts commit 992118dd95b052bc82a795fd3a8978bea0fe5c0e.  That
change is promising, since it improves the behavior of "git diff" and
"git diff --cached" by correctly showing an intent-to-add entry as no
file.  Unfortunately, though, it breaks "git diff HEAD" in the
process:

	echo hi >new-file
	git add -N new-file
	git diff HEAD

Before: shows addition of the new file
After: shows no change

Noticed because the new --ita-invisible-in-index default broke a
script using "git diff --name-only --diff-filter=A master" to get a
list of added files.  Arguably that script should use diff-index
instead, which would have sidestepped the issue, but the new behavior
is unexpected in interactive use as well.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/diff.c          |  7 -------
 t/t2203-add-intent.sh   | 40 ++++++++--------------------------------
 t/t4011-diff-symlink.sh | 10 ++++------
 3 files changed, 12 insertions(+), 45 deletions(-)

diff --git a/builtin/diff.c b/builtin/diff.c
index b709b6e984..bfefff3a84 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -352,13 +352,6 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	rev.diffopt.flags.allow_external = 1;
 	rev.diffopt.flags.allow_textconv = 1;
 
-	/*
-	 * Default to intent-to-add entries invisible in the
-	 * index. This makes them show up as new files in diff-files
-	 * and not at all in diff-cached.
-	 */
-	rev.diffopt.ita_invisible_in_index = 1;
-
 	if (nongit)
 		die(_("Not a git repository"));
 	argc = setup_revisions(argc, argv, &rev, NULL);
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 1d640a33f0..d9fb151d52 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -69,9 +69,9 @@ test_expect_success 'i-t-a entry is simply ignored' '
 	git add -N nitfol &&
 	git commit -m second &&
 	test $(git ls-tree HEAD -- nitfol | wc -l) = 0 &&
-	test $(git diff --name-only --ita-visible-in-index HEAD -- nitfol | wc -l) = 1 &&
-	test $(git diff --name-only HEAD -- nitfol | wc -l) = 0 &&
-	test $(git diff --name-only -- nitfol | wc -l) = 1
+	test $(git diff --name-only HEAD -- nitfol | wc -l) = 1 &&
+	test $(git diff --name-only --ita-invisible-in-index HEAD -- nitfol | wc -l) = 0 &&
+	test $(git diff --name-only --ita-invisible-in-index -- nitfol | wc -l) = 1
 '
 
 test_expect_success 'can commit with an unrelated i-t-a entry in index' '
@@ -99,13 +99,13 @@ test_expect_success 'cache-tree invalidates i-t-a paths' '
 
 	: >dir/bar &&
 	git add -N dir/bar &&
-	git diff --name-only >actual &&
+	git diff --cached --name-only >actual &&
 	echo dir/bar >expect &&
 	test_cmp expect actual &&
 
 	git write-tree >/dev/null &&
 
-	git diff --name-only >actual &&
+	git diff --cached --name-only >actual &&
 	echo dir/bar >expect &&
 	test_cmp expect actual
 '
@@ -186,19 +186,7 @@ test_expect_success 'rename detection finds the right names' '
 		cat >expected.3 <<-EOF &&
 		2 .R N... 100644 100644 100644 $hash $hash R100 third	first
 		EOF
-		test_cmp expected.3 actual.3 &&
-
-		git diff --stat >actual.4 &&
-		cat >expected.4 <<-EOF &&
-		 first => third | 0
-		 1 file changed, 0 insertions(+), 0 deletions(-)
-		EOF
-		test_cmp expected.4 actual.4 &&
-
-		git diff --cached --stat >actual.5 &&
-		: >expected.5 &&
-		test_cmp expected.5 actual.5
-
+		test_cmp expected.3 actual.3
 	)
 '
 
@@ -234,27 +222,15 @@ test_expect_success 'double rename detection in status' '
 	)
 '
 
-test_expect_success 'diff-files/diff-cached shows ita as new/not-new files' '
-	git reset --hard &&
-	echo new >new-ita &&
-	git add -N new-ita &&
-	git diff --summary >actual &&
-	echo " create mode 100644 new-ita" >expected &&
-	test_cmp expected actual &&
-	git diff --cached --summary >actual2 &&
-	: >expected2 &&
-	test_cmp expected2 actual2
-'
-
 test_expect_success 'apply --intent-to-add' '
 	git reset --hard &&
 	echo new >new-ita &&
 	git add -N new-ita &&
-	git diff >expected &&
+	git diff --ita-invisible-in-index >expected &&
 	grep "new file" expected &&
 	git reset --hard &&
 	git apply --intent-to-add expected &&
-	git diff >actual &&
+	git diff --ita-invisible-in-index >actual &&
 	test_cmp expected actual
 '
 
diff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh
index 108c012a3a..cf0f3a1ee7 100755
--- a/t/t4011-diff-symlink.sh
+++ b/t/t4011-diff-symlink.sh
@@ -139,13 +139,11 @@ test_expect_success SYMLINKS 'setup symlinks with attributes' '
 test_expect_success SYMLINKS 'symlinks do not respect userdiff config by path' '
 	cat >expect <<-\EOF &&
 	diff --git a/file.bin b/file.bin
-	new file mode 100644
-	index 0000000..d95f3ad
-	Binary files /dev/null and b/file.bin differ
+	index e69de29..d95f3ad 100644
+	Binary files a/file.bin and b/file.bin differ
 	diff --git a/link.bin b/link.bin
-	new file mode 120000
-	index 0000000..dce41ec
-	--- /dev/null
+	index e69de29..dce41ec 120000
+	--- a/link.bin
 	+++ b/link.bin
 	@@ -0,0 +1 @@
 	+file.bin
-- 
2.17.0.921.gf22659ad46

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

[Duy Nguyen]

On Fri, May 25, 2018 at 6:43 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> I think it should be reverted from 'next' because of the unintended
> change to the behavior of "git diff HEAD".

Ah. That is indeed unintended. I still don't know how this change
affects that (but that's probably why it slipped through in the first
place). While there, perhaps you could add a test in t2203 just to
make sure I will not break it again in the next attempt?

I agree that it should be reverted. Or if it's possible to eject it
from next, then it's even better since I will need to fix up the
second patch in that series anyway. Junio?

> Here is what I have applied internally.
>
> -- >8 --
> Subject: Revert "diff: turn --ita-invisible-in-index on by default"
>
> This reverts commit 992118dd95b052bc82a795fd3a8978bea0fe5c0e.  That
> change is promising, since it improves the behavior of "git diff" and
> "git diff --cached" by correctly showing an intent-to-add entry as no
> file.  Unfortunately, though, it breaks "git diff HEAD" in the
> process:
>
>         echo hi >new-file
>         git add -N new-file
>         git diff HEAD
>
> Before: shows addition of the new file
> After: shows no change
>
> Noticed because the new --ita-invisible-in-index default broke a
> script using "git diff --name-only --diff-filter=A master" to get a
> list of added files.  Arguably that script should use diff-index
> instead, which would have sidestepped the issue, but the new behavior
> is unexpected in interactive use as well.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
>  builtin/diff.c          |  7 -------
>  t/t2203-add-intent.sh   | 40 ++++++++--------------------------------
>  t/t4011-diff-symlink.sh | 10 ++++------
>  3 files changed, 12 insertions(+), 45 deletions(-)
>
> diff --git a/builtin/diff.c b/builtin/diff.c
> index b709b6e984..bfefff3a84 100644
> --- a/builtin/diff.c
> +++ b/builtin/diff.c
> @@ -352,13 +352,6 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>         rev.diffopt.flags.allow_external = 1;
>         rev.diffopt.flags.allow_textconv = 1;
>
> -       /*
> -        * Default to intent-to-add entries invisible in the
> -        * index. This makes them show up as new files in diff-files
> -        * and not at all in diff-cached.
> -        */
> -       rev.diffopt.ita_invisible_in_index = 1;
> -
>         if (nongit)
>                 die(_("Not a git repository"));
>         argc = setup_revisions(argc, argv, &rev, NULL);
> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
> index 1d640a33f0..d9fb151d52 100755
> --- a/t/t2203-add-intent.sh
> +++ b/t/t2203-add-intent.sh
> @@ -69,9 +69,9 @@ test_expect_success 'i-t-a entry is simply ignored' '
>         git add -N nitfol &&
>         git commit -m second &&
>         test $(git ls-tree HEAD -- nitfol | wc -l) = 0 &&
> -       test $(git diff --name-only --ita-visible-in-index HEAD -- nitfol | wc -l) = 1 &&
> -       test $(git diff --name-only HEAD -- nitfol | wc -l) = 0 &&
> -       test $(git diff --name-only -- nitfol | wc -l) = 1
> +       test $(git diff --name-only HEAD -- nitfol | wc -l) = 1 &&
> +       test $(git diff --name-only --ita-invisible-in-index HEAD -- nitfol | wc -l) = 0 &&
> +       test $(git diff --name-only --ita-invisible-in-index -- nitfol | wc -l) = 1
>  '
>
>  test_expect_success 'can commit with an unrelated i-t-a entry in index' '
> @@ -99,13 +99,13 @@ test_expect_success 'cache-tree invalidates i-t-a paths' '
>
>         : >dir/bar &&
>         git add -N dir/bar &&
> -       git diff --name-only >actual &&
> +       git diff --cached --name-only >actual &&
>         echo dir/bar >expect &&
>         test_cmp expect actual &&
>
>         git write-tree >/dev/null &&
>
> -       git diff --name-only >actual &&
> +       git diff --cached --name-only >actual &&
>         echo dir/bar >expect &&
>         test_cmp expect actual
>  '
> @@ -186,19 +186,7 @@ test_expect_success 'rename detection finds the right names' '
>                 cat >expected.3 <<-EOF &&
>                 2 .R N... 100644 100644 100644 $hash $hash R100 third   first
>                 EOF
> -               test_cmp expected.3 actual.3 &&
> -
> -               git diff --stat >actual.4 &&
> -               cat >expected.4 <<-EOF &&
> -                first => third | 0
> -                1 file changed, 0 insertions(+), 0 deletions(-)
> -               EOF
> -               test_cmp expected.4 actual.4 &&
> -
> -               git diff --cached --stat >actual.5 &&
> -               : >expected.5 &&
> -               test_cmp expected.5 actual.5
> -
> +               test_cmp expected.3 actual.3
>         )
>  '
>
> @@ -234,27 +222,15 @@ test_expect_success 'double rename detection in status' '
>         )
>  '
>
> -test_expect_success 'diff-files/diff-cached shows ita as new/not-new files' '
> -       git reset --hard &&
> -       echo new >new-ita &&
> -       git add -N new-ita &&
> -       git diff --summary >actual &&
> -       echo " create mode 100644 new-ita" >expected &&
> -       test_cmp expected actual &&
> -       git diff --cached --summary >actual2 &&
> -       : >expected2 &&
> -       test_cmp expected2 actual2
> -'
> -
>  test_expect_success 'apply --intent-to-add' '
>         git reset --hard &&
>         echo new >new-ita &&
>         git add -N new-ita &&
> -       git diff >expected &&
> +       git diff --ita-invisible-in-index >expected &&
>         grep "new file" expected &&
>         git reset --hard &&
>         git apply --intent-to-add expected &&
> -       git diff >actual &&
> +       git diff --ita-invisible-in-index >actual &&
>         test_cmp expected actual
>  '
>
> diff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh
> index 108c012a3a..cf0f3a1ee7 100755
> --- a/t/t4011-diff-symlink.sh
> +++ b/t/t4011-diff-symlink.sh
> @@ -139,13 +139,11 @@ test_expect_success SYMLINKS 'setup symlinks with attributes' '
>  test_expect_success SYMLINKS 'symlinks do not respect userdiff config by path' '
>         cat >expect <<-\EOF &&
>         diff --git a/file.bin b/file.bin
> -       new file mode 100644
> -       index 0000000..d95f3ad
> -       Binary files /dev/null and b/file.bin differ
> +       index e69de29..d95f3ad 100644
> +       Binary files a/file.bin and b/file.bin differ
>         diff --git a/link.bin b/link.bin
> -       new file mode 120000
> -       index 0000000..dce41ec
> -       --- /dev/null
> +       index e69de29..dce41ec 120000
> +       --- a/link.bin
>         +++ b/link.bin
>         @@ -0,0 +1 @@
>         +file.bin
> --
> 2.17.0.921.gf22659ad46
-- 
Duy

[PATCH v2 0/4] Fix i-t-a entries in git-diff and git-apply

[Nguyễn Thái Ngọc Duy]

v2 first fixes a bug in --ita-invisible-in-index that accidentally
affects diffing a worktree and a tree. This caused a regression when
v1's 1/2 turned this option on by default.

Other than that, 4/4 should address Junio's comments on v1's 2/2.

Nguyễn Thái Ngọc Duy (4):
  diff: ignore --ita-[in]visible-in-index when diffing worktree-to-tree
  diff: turn --ita-invisible-in-index on by default
  t2203: add a test about "diff HEAD" case
  apply: add --intent-to-add

 Documentation/git-apply.txt | 10 +++++-
 apply.c                     | 19 +++++++----
 apply.h                     |  1 +
 builtin/diff.c              |  7 ++++
 diff-lib.c                  |  8 +++--
 t/t2203-add-intent.sh       | 64 +++++++++++++++++++++++++++++++++----
 t/t4011-diff-symlink.sh     | 10 +++---
 7 files changed, 99 insertions(+), 20 deletions(-)

-- 
2.17.0.705.g3525833791

[PATCH v2 1/4] diff: ignore --ita-[in]visible-in-index when diffing worktree-to-tree

[Nguyễn Thái Ngọc Duy]

This option is supposed to fix the diff of "diff-files" (not reporting
ita entries as new files) and "diff-index --cached <tree>" ( showing
ita entries as present in the index with empty content) but not
"diff-index <tree>".

When --ita-invisible-in-index is set on "git diff-index <tree>",
unpack_trees() will eventually call oneway_diff() on the ita entry
with the same code flow as "diff-index --cached <tree>". We want to
ignore the ita entry for "diff-index --cached <tree>" but not
"diff-index <tree>" since the latter will examine and produce a diff
based on worktree entry's (real) content, not ita index entry's
(empty) content.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff-lib.c            | 8 ++++++--
 t/t2203-add-intent.sh | 2 +-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 104f954a25..a9f38eb5a3 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -389,8 +389,12 @@ static void do_oneway_diff(struct unpack_trees_options *o,
 	struct rev_info *revs = o->unpack_data;
 	int match_missing, cached;
 
-	/* i-t-a entries do not actually exist in the index */
-	if (revs->diffopt.ita_invisible_in_index &&
+	/*
+	 * i-t-a entries do not actually exist in the index (if we're
+	 * looking at its content)
+	 */
+	if (o->index_only &&
+	    revs->diffopt.ita_invisible_in_index &&
 	    idx && ce_intent_to_add(idx)) {
 		idx = NULL;
 		if (!tree)
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 78236dc7d8..3ab07cb404 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -70,7 +70,7 @@ test_expect_success 'i-t-a entry is simply ignored' '
 	git commit -m second &&
 	test $(git ls-tree HEAD -- nitfol | wc -l) = 0 &&
 	test $(git diff --name-only HEAD -- nitfol | wc -l) = 1 &&
-	test $(git diff --name-only --ita-invisible-in-index HEAD -- nitfol | wc -l) = 0 &&
+	test $(git diff --name-only --ita-invisible-in-index HEAD -- nitfol | wc -l) = 1 &&
 	test $(git diff --name-only --ita-invisible-in-index -- nitfol | wc -l) = 1
 '
 
-- 
2.17.0.705.g3525833791

Re: [PATCH v2 1/4] diff: ignore --ita-[in]visible-in-index when diffing worktree-to-tree

[Eric Sunshine]

On Sat, May 26, 2018 at 8:08 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> This option is supposed to fix the diff of "diff-files" (not reporting
> ita entries as new files) and "diff-index --cached <tree>" ( showing

s/(\s/(/

> ita entries as present in the index with empty content) but not
> "diff-index <tree>".
>
> When --ita-invisible-in-index is set on "git diff-index <tree>",
> unpack_trees() will eventually call oneway_diff() on the ita entry
> with the same code flow as "diff-index --cached <tree>". We want to
> ignore the ita entry for "diff-index --cached <tree>" but not
> "diff-index <tree>" since the latter will examine and produce a diff
> based on worktree entry's (real) content, not ita index entry's
> (empty) content.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

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

[Nguyễn Thái Ngọc Duy]

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.

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

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. The result
is "new file" line moving from "git diff --cached" to "git diff".

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

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   | 34 ++++++++++++++++++++++++++++------
 t/t4011-diff-symlink.sh | 10 ++++++----
 3 files changed, 41 insertions(+), 10 deletions(-)

diff --git a/builtin/diff.c b/builtin/diff.c
index 16bfb22f73..00424c296d 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -352,6 +352,13 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	rev.diffopt.flags.allow_external = 1;
 	rev.diffopt.flags.allow_textconv = 1;
 
+	/*
+	 * Default to intent-to-add entries invisible in the
+	 * index. This makes them show up as new files in diff-files
+	 * and not at all in diff-cached.
+	 */
+	rev.diffopt.ita_invisible_in_index = 1;
+
 	if (nongit)
 		die(_("Not a git repository"));
 	argc = setup_revisions(argc, argv, &rev, NULL);
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 3ab07cb404..1115347712 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -70,8 +70,7 @@ test_expect_success 'i-t-a entry is simply ignored' '
 	git commit -m second &&
 	test $(git ls-tree HEAD -- nitfol | wc -l) = 0 &&
 	test $(git diff --name-only HEAD -- nitfol | wc -l) = 1 &&
-	test $(git diff --name-only --ita-invisible-in-index HEAD -- nitfol | wc -l) = 1 &&
-	test $(git diff --name-only --ita-invisible-in-index -- nitfol | wc -l) = 1
+	test $(git diff --name-only -- nitfol | wc -l) = 1
 '
 
 test_expect_success 'can commit with an unrelated i-t-a entry in index' '
@@ -99,13 +98,13 @@ test_expect_success 'cache-tree invalidates i-t-a paths' '
 
 	: >dir/bar &&
 	git add -N dir/bar &&
-	git diff --cached --name-only >actual &&
+	git diff --name-only >actual &&
 	echo dir/bar >expect &&
 	test_cmp expect actual &&
 
 	git write-tree >/dev/null &&
 
-	git diff --cached --name-only >actual &&
+	git diff --name-only >actual &&
 	echo dir/bar >expect &&
 	test_cmp expect actual
 '
@@ -186,7 +185,19 @@ test_expect_success 'rename detection finds the right names' '
 		cat >expected.3 <<-EOF &&
 		2 .R N... 100644 100644 100644 $hash $hash R100 third	first
 		EOF
-		test_cmp expected.3 actual.3
+		test_cmp expected.3 actual.3 &&
+
+		git diff --stat >actual.4 &&
+		cat >expected.4 <<-EOF &&
+		 first => third | 0
+		 1 file changed, 0 insertions(+), 0 deletions(-)
+		EOF
+		test_cmp expected.4 actual.4 &&
+
+		git diff --cached --stat >actual.5 &&
+		: >expected.5 &&
+		test_cmp expected.5 actual.5
+
 	)
 '
 
@@ -222,5 +233,16 @@ test_expect_success 'double rename detection in status' '
 	)
 '
 
-test_done
+test_expect_success 'diff-files/diff-cached shows ita as new/not-new files' '
+	git reset --hard &&
+	echo new >new-ita &&
+	git add -N new-ita &&
+	git diff --summary >actual &&
+	echo " create mode 100644 new-ita" >expected &&
+	test_cmp expected actual &&
+	git diff --cached --summary >actual2 &&
+	: >expected2 &&
+	test_cmp expected2 actual2
+'
 
+test_done
diff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh
index cf0f3a1ee7..108c012a3a 100755
--- a/t/t4011-diff-symlink.sh
+++ b/t/t4011-diff-symlink.sh
@@ -139,11 +139,13 @@ test_expect_success SYMLINKS 'setup symlinks with attributes' '
 test_expect_success SYMLINKS 'symlinks do not respect userdiff config by path' '
 	cat >expect <<-\EOF &&
 	diff --git a/file.bin b/file.bin
-	index e69de29..d95f3ad 100644
-	Binary files a/file.bin and b/file.bin differ
+	new file mode 100644
+	index 0000000..d95f3ad
+	Binary files /dev/null and b/file.bin differ
 	diff --git a/link.bin b/link.bin
-	index e69de29..dce41ec 120000
-	--- a/link.bin
+	new file mode 120000
+	index 0000000..dce41ec
+	--- /dev/null
 	+++ b/link.bin
 	@@ -0,0 +1 @@
 	+file.bin
-- 
2.17.0.705.g3525833791

[PATCH v2 3/4] t2203: add a test about "diff HEAD" case

[Nguyễn Thái Ngọc Duy]

Previous attempts to fix ita-related diffs breaks this case. To make
sure that does not happen again, add a test to verify the behavior
wrt. ita entries when we diff a worktree and a tree.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t2203-add-intent.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 1115347712..546fead6ad 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -245,4 +245,21 @@ test_expect_success 'diff-files/diff-cached shows ita as new/not-new files' '
 	test_cmp expected2 actual2
 '
 
+test_expect_success '"diff HEAD" includes ita as new files' '
+	git reset --hard &&
+	echo new >new-ita &&
+	git add -N new-ita &&
+	git diff HEAD >actual &&
+	cat >expected <<-\EOF &&
+	diff --git a/new-ita b/new-ita
+	new file mode 100644
+	index 0000000..3e75765
+	--- /dev/null
+	+++ b/new-ita
+	@@ -0,0 +1 @@
+	+new
+	EOF
+	test_cmp expected actual
+'
+
 test_done
-- 
2.17.0.705.g3525833791

[PATCH v2 4/4] apply: add --intent-to-add

[Nguyễn Thái Ngọc Duy]

Similar to 'git reset -N', this option makes 'git apply' automatically
mark new files as intent-to-add so they are visible in the following
'git diff' command and could also be committed with 'git commit -a'.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-apply.txt | 10 +++++++++-
 apply.c                     | 19 ++++++++++++-------
 apply.h                     |  1 +
 t/t2203-add-intent.sh       | 13 +++++++++++++
 4 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index 4ebc3d3271..e3b966c656 100644
--- a/Documentation/git-apply.txt
+++ b/Documentation/git-apply.txt
@@ -9,7 +9,7 @@ git-apply - Apply a patch to files and/or to the index
 SYNOPSIS
 --------
 [verse]
-'git apply' [--stat] [--numstat] [--summary] [--check] [--index] [--3way]
+'git apply' [--stat] [--numstat] [--summary] [--check] [--index | --intent-to-add] [--3way]
 	  [--apply] [--no-add] [--build-fake-ancestor=<file>] [-R | --reverse]
 	  [--allow-binary-replacement | --binary] [--reject] [-z]
 	  [-p<n>] [-C<n>] [--inaccurate-eof] [--recount] [--cached]
@@ -74,6 +74,14 @@ OPTIONS
 	cached data, apply the patch, and store the result in the index
 	without using the working tree. This implies `--index`.
 
+--intent-to-add::
+	When applying the patch only to the working tree, mark new
+	files to be added to the index later (see `--intent-to-add`
+	option in linkgit:git-add[1]). This option is ignored unless
+	running in a Git repository and `--index` is not specified.
+	Note that `--index` could be implied by other options such
+	as `--cached` or `--3way`.
+
 -3::
 --3way::
 	When the patch does not apply cleanly, fall back on 3-way merge if
diff --git a/apply.c b/apply.c
index 7e5792c996..899c5cc0ac 100644
--- a/apply.c
+++ b/apply.c
@@ -141,6 +141,8 @@ int check_apply_state(struct apply_state *state, int force_apply)
 			return error(_("--cached outside a repository"));
 		state->check_index = 1;
 	}
+	if (state->ita_only && (state->check_index || is_not_gitdir))
+		state->ita_only = 0;
 	if (state->check_index)
 		state->unsafe_paths = 0;
 
@@ -4242,7 +4244,7 @@ static void patch_stats(struct apply_state *state, struct patch *patch)
 
 static int remove_file(struct apply_state *state, struct patch *patch, int rmdir_empty)
 {
-	if (state->update_index) {
+	if (state->update_index && !state->ita_only) {
 		if (remove_file_from_cache(patch->old_name) < 0)
 			return error(_("unable to remove %s from index"), patch->old_name);
 	}
@@ -4265,15 +4267,15 @@ static int add_index_file(struct apply_state *state,
 	int namelen = strlen(path);
 	unsigned ce_size = cache_entry_size(namelen);
 
-	if (!state->update_index)
-		return 0;
-
 	ce = xcalloc(1, ce_size);
 	memcpy(ce->name, path, namelen);
 	ce->ce_mode = create_ce_mode(mode);
 	ce->ce_flags = create_ce_flags(0);
 	ce->ce_namelen = namelen;
-	if (S_ISGITLINK(mode)) {
+	if (state->ita_only) {
+		ce->ce_flags |= CE_INTENT_TO_ADD;
+		set_object_name_for_intent_to_add_entry(ce);
+	} else if (S_ISGITLINK(mode)) {
 		const char *s;
 
 		if (!skip_prefix(buf, "Subproject commit ", &s) ||
@@ -4465,8 +4467,9 @@ static int create_file(struct apply_state *state, struct patch *patch)
 
 	if (patch->conflicted_threeway)
 		return add_conflicted_stages_file(state, patch);
-	else
+	else if (state->update_index)
 		return add_index_file(state, path, mode, buf, size);
+	return 0;
 }
 
 /* phase zero is to remove, phase one is to create */
@@ -4686,7 +4689,7 @@ static int apply_patch(struct apply_state *state,
 	if (state->whitespace_error && (state->ws_error_action == die_on_ws_error))
 		state->apply = 0;
 
-	state->update_index = state->check_index && state->apply;
+	state->update_index = (state->check_index || state->ita_only) && state->apply;
 	if (state->update_index && !is_lock_file_locked(&state->lock_file)) {
 		if (state->index_file)
 			hold_lock_file_for_update(&state->lock_file,
@@ -4941,6 +4944,8 @@ int apply_parse_options(int argc, const char **argv,
 			N_("instead of applying the patch, see if the patch is applicable")),
 		OPT_BOOL(0, "index", &state->check_index,
 			N_("make sure the patch is applicable to the current index")),
+		OPT_BOOL('N', "intent-to-add", &state->ita_only,
+			N_("mark new files with `git add --intent-to-add`")),
 		OPT_BOOL(0, "cached", &state->cached,
 			N_("apply a patch without touching the working tree")),
 		OPT_BOOL_F(0, "unsafe-paths", &state->unsafe_paths,
diff --git a/apply.h b/apply.h
index dc4a019057..b80d8ba736 100644
--- a/apply.h
+++ b/apply.h
@@ -45,6 +45,7 @@ struct apply_state {
 	int check; /* preimage must match working tree, don't actually apply */
 	int check_index; /* preimage must match the indexed version */
 	int update_index; /* check_index && apply */
+	int ita_only;	  /* add intent-to-add entries to the index */
 
 	/* These control cosmetic aspect of the output */
 	int diffstat; /* just show a diffstat, and don't actually apply */
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 546fead6ad..0891827863 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -245,6 +245,7 @@ test_expect_success 'diff-files/diff-cached shows ita as new/not-new files' '
 	test_cmp expected2 actual2
 '
 
+
 test_expect_success '"diff HEAD" includes ita as new files' '
 	git reset --hard &&
 	echo new >new-ita &&
@@ -262,4 +263,16 @@ test_expect_success '"diff HEAD" includes ita as new files' '
 	test_cmp expected actual
 '
 
+test_expect_success 'apply --intent-to-add' '
+	git reset --hard &&
+	echo new >new-ita &&
+	git add -N new-ita &&
+	git diff >expected &&
+	grep "new file" expected &&
+	git reset --hard &&
+	git apply --intent-to-add expected &&
+	git diff >actual &&
+	test_cmp expected actual
+'
+
 test_done
-- 
2.17.0.705.g3525833791