threads / patch / 48482

patch, 2 partsdiff: turn --ita-invisible-in-index on by default

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

## tl;dr

15 messages between May 13, 2018 and May 27, 2018. Diffs are folded; open one to read it.

replies: 14people: 4as markdown or json

Nguyễn Thái Ngọc Duy· May 13, 2018, 17:54 UTC · lore

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(-)
Show changes to 3 files +43 −11

builtin/diff.c, t/t2203-add-intent.sh, t/t4011-diff-symlink.sh

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
Nguyễn Thái Ngọc Duy· May 13, 2018, 17:54 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to 4 files +53 −7

Documentation/git-apply.txt, apply.c, apply.h, t/t2203-add-intent.sh

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
Junio C Hamano· May 14, 2018, 02:33 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 35 quoted lines
> 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.

Show 10 quoted lines
> 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.

Show 7 quoted lines
> @@ -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.

Show 24 quoted lines
> @@ -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.

Show 10 quoted lines
> @@ -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.

Duy Nguyen· May 20, 2018, 06:34 UTC · re: Junio C Hamano · lore

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

On Mon, May 14, 2018 at 11:33:48AM +0900, Junio C Hamano wrote:
Show 27 quoted lines
> > 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.

Show 14 quoted lines
> > 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

Junio C Hamano· May 21, 2018, 03:14 UTC · re: Duy Nguyen · lore

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

Duy Nguyen <pclouds@gmail.com> writes:
Show 11 quoted lines
>> >  	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.

Jonathan Nieder· May 25, 2018, 03:39 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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

Duy Nguyen· May 25, 2018, 14:30 UTC · re: Jonathan Nieder · lore

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

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

Jonathan Nieder· May 25, 2018, 16:43 UTC · re: Duy Nguyen · lore

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

Hi,
Duy Nguyen wrote:
Show 20 quoted lines
>     $ 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
[...]
Show 7 quoted lines
> 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(-)
Show changes to 3 files +12 −45

builtin/diff.c, t/t2203-add-intent.sh, t/t4011-diff-symlink.sh

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 Nguyen· May 25, 2018, 16:59 UTC · re: Jonathan Nieder · lore

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

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?

Show 157 quoted lines
> 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
Nguyễn Thái Ngọc Duy· May 26, 2018, 12:08 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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
Nguyễn Thái Ngọc Duy· May 26, 2018, 12:08 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to 3 files +41 −10

builtin/diff.c, t/t2203-add-intent.sh, t/t4011-diff-symlink.sh

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
Nguyễn Thái Ngọc Duy· May 26, 2018, 12:08 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to 2 files +7 −3

diff-lib.c, t/t2203-add-intent.sh

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
Eric Sunshine· May 27, 2018, 07:18 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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/(/
Show 12 quoted lines
> 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>
Nguyễn Thái Ngọc Duy· May 26, 2018, 12:08 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(+)
Show changes to t/t2203-add-intent.sh +17 −0
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
Nguyễn Thái Ngọc Duy· May 26, 2018, 12:08 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to 4 files +35 −8

Documentation/git-apply.txt, apply.c, apply.h, t/t2203-add-intent.sh

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

← back to recent threads