threads / rfc / 42536

RFC patch, 4 partsi-t-a entries again...

Subject: [PATCH/RFC 0/4] i-t-a entries again...

## tl;dr

15 messages between Jun 6, 2016 and Sep 27, 2016. Diffs are folded; open one to read it.

replies: 14people: 4as markdown or json

Nguyễn Thái Ngọc Duy· Jun 6, 2016, 11:16 UTC · lore

The first two patches bring back a patch does fixes "git status" when i-t-a entries are present (i.e. "git diff" should show new files while "git diff --cached" should show no changes).

The third commit fixes "git commit" creating empty commits when i-t-a entries are the only changes compared to HEAD.

The last one brings back the old behavior before 3f6d56d (commit: ignore intent-to-add entries instead of refusing - 2012-02-07). I still believe that current behavior has its uses. So instead of reverting the behavior unconditonally, the user can choose to switch back to the old behavior via config key.

The last two do not have have proper tests added because I just created them in the last hours. This is mainly to bring back the old discussion to see what should be the way forward.

Nguyễn Thái Ngọc Duy (4):
  diff.h: extend "flags" field to 64 bits because we're out of bits
  Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"
  commit: don't count i-t-a entries when checking if the new commit is empty
  commit: reinstate commit behavior before 3f6d56d via a config option
 Documentation/diff-options.txt |  6 ++++++
 builtin/commit.c               | 14 ++++++++++----
 cache-tree.c                   | 14 +++++++++++---
 diff-lib.c                     | 18 ++++++++++++++++--
 diff.c                         |  4 +++-
 diff.h                         |  7 ++++---
 t/t2203-add-intent.sh          | 20 ++++++++++++++++++--
 wt-status.c                    |  5 +++++
 8 files changed, 73 insertions(+), 15 deletions(-)
-- 
2.8.2.524.g6ff3d78
Nguyễn Thái Ngọc Duy· Jun 6, 2016, 11:16 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 1/4] diff.h: extend "flags" field to 64 bits because we're out of bits

Current flags field is 32-bits, all used except one bit and we need one more bit is needed for to toggle i-t-a behavior. The 9th bit could be reused for this, but we could just extend it to 64 bits now to give room for more future flags.

gcc -Wconversion is used to catch assignments that truncate bits. No new warning was introduced (in fact one in index_differs_from() was eliminated),

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/commit.c | 2 +-
 diff-lib.c       | 4 ++--
 diff.c           | 2 +-
 diff.h           | 6 +++---
 4 files changed, 7 insertions(+), 7 deletions(-)
Show changes to 4 files +7 −7

builtin/commit.c, diff-lib.c, diff.c, diff.h

diff --git a/builtin/commit.c b/builtin/commit.c
index 443ff91..fcfaa2b 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -906,7 +906,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 			 * submodules which were manually staged, which would
 			 * be really confusing.
 			 */
-			int diff_flags = DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
+			uint64_t diff_flags = DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
 			if (ignore_submodule_arg &&
 			    !strcmp(ignore_submodule_arg, "all"))
 				diff_flags |= DIFF_OPT_IGNORE_SUBMODULES;
diff --git a/diff-lib.c b/diff-lib.c
index bc49c70..27887d0 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -71,7 +71,7 @@ static int match_stat_with_submodule(struct diff_options *diffopt,
 {
 	int changed = ce_match_stat(ce, st, ce_option);
 	if (S_ISGITLINK(ce->ce_mode)) {
-		unsigned orig_flags = diffopt->flags;
+		uint64_t orig_flags = diffopt->flags;
 		if (!DIFF_OPT_TST(diffopt, OVERRIDE_SUBMODULE_CONFIG))
 			set_diffopt_flags_from_submodule_config(diffopt, ce->name);
 		if (DIFF_OPT_TST(diffopt, IGNORE_SUBMODULES))
@@ -516,7 +516,7 @@ int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)
 	return 0;
 }
 
-int index_differs_from(const char *def, int diff_flags)
+int index_differs_from(const char *def, uint64_t diff_flags)
 {
 	struct rev_info rev;
 	struct setup_revision_opt opt;
diff --git a/diff.c b/diff.c
index d3734d3..f70425f 100644
--- a/diff.c
+++ b/diff.c
@@ -4936,7 +4936,7 @@ int diff_can_quit_early(struct diff_options *opt)
 static int is_submodule_ignored(const char *path, struct diff_options *options)
 {
 	int ignored = 0;
-	unsigned orig_flags = options->flags;
+	uint64_t orig_flags = options->flags;
 	if (!DIFF_OPT_TST(options, OVERRIDE_SUBMODULE_CONFIG))
 		set_diffopt_flags_from_submodule_config(options, path);
 	if (DIFF_OPT_TST(options, IGNORE_SUBMODULES))
diff --git a/diff.h b/diff.h
index 125447b..b497078 100644
--- a/diff.h
+++ b/diff.h
@@ -115,8 +115,8 @@ struct diff_options {
 	const char *pickaxe;
 	const char *single_follow;
 	const char *a_prefix, *b_prefix;
-	unsigned flags;
-	unsigned touched_flags;
+	uint64_t flags;
+	uint64_t touched_flags;
 
 	/* diff-filter bits */
 	unsigned int filter;
@@ -348,7 +348,7 @@ extern int diff_result_code(struct diff_options *, int);
 
 extern void diff_no_index(struct rev_info *, int, const char **);
 
-extern int index_differs_from(const char *def, int diff_flags);
+extern int index_differs_from(const char *def, uint64_t diff_flags);
 
 /*
  * Fill the contents of the filespec "df", respecting any textconv defined by
-- 
2.8.2.524.g6ff3d78
Junio C Hamano· Jun 6, 2016, 19:45 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 1/4] diff.h: extend "flags" field to 64 bits because we're out of bits

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
> Current flags field is 32-bits, all used except one bit and we need one
> more bit is needed for to toggle i-t-a behavior. The 9th bit could be
> reused for this, but we could just extend it to 64 bits now to give room
> for more future flags.

Isn't it a better option to add new things as separate words (or a separate bit:1 field)? Is this 32nd thing envisioned to be added going to be used everywhere like the existing flag bits, or can it be handled like "use_color", "break_opt", "show_rename_progress"?

Duy Nguyen· Jun 7, 2016, 00:40 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/4] diff.h: extend "flags" field to 64 bits because we're out of bits

On Tue, Jun 7, 2016 at 2:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> Current flags field is 32-bits, all used except one bit and we need one
>> more bit is needed for to toggle i-t-a behavior. The 9th bit could be
>> reused for this, but we could just extend it to 64 bits now to give room
>> for more future flags.
>
> Isn't it a better option to add new things as separate words (or a
> separate bit:1 field)?  Is this 32nd thing envisioned to be added
> going to be used everywhere like the existing flag bits, or can it
> be handled like "use_color", "break_opt", "show_rename_progress"?

It could probably live as a separate bitfield, or an int. What scares me is "flags" sometimes is backed up then restored, but it's probably ok. But we need to extend this "flags" eventually, don't we? I don't think people will stop adding new DIFF_OPT_ flags.

-- 
Duy
stefan.naewe@atlas-elektronik.com· Jun 7, 2016, 06:40 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 1/4] diff.h: extend "flags" field to 64 bits because we're out of bits

Am 06.06.2016 um 13:16 schrieb Nguyễn Thái Ngọc Duy:
> Current flags field is 32-bits, all used except one bit and we need one
> more bit is needed for to toggle i-t-a behavior. The 9th bit could be
Something's wrong here. Either drop the "is needed" or the "we need".
Show 88 quoted lines
> reused for this, but we could just extend it to 64 bits now to give room
> for more future flags.
> 
> gcc -Wconversion is used to catch assignments that truncate bits. No new
> warning was introduced (in fact one in index_differs_from() was
> eliminated),
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/commit.c | 2 +-
>  diff-lib.c       | 4 ++--
>  diff.c           | 2 +-
>  diff.h           | 6 +++---
>  4 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 443ff91..fcfaa2b 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -906,7 +906,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  			 * submodules which were manually staged, which would
>  			 * be really confusing.
>  			 */
> -			int diff_flags = DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
> +			uint64_t diff_flags = DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
>  			if (ignore_submodule_arg &&
>  			    !strcmp(ignore_submodule_arg, "all"))
>  				diff_flags |= DIFF_OPT_IGNORE_SUBMODULES;
> diff --git a/diff-lib.c b/diff-lib.c
> index bc49c70..27887d0 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -71,7 +71,7 @@ static int match_stat_with_submodule(struct diff_options *diffopt,
>  {
>  	int changed = ce_match_stat(ce, st, ce_option);
>  	if (S_ISGITLINK(ce->ce_mode)) {
> -		unsigned orig_flags = diffopt->flags;
> +		uint64_t orig_flags = diffopt->flags;
>  		if (!DIFF_OPT_TST(diffopt, OVERRIDE_SUBMODULE_CONFIG))
>  			set_diffopt_flags_from_submodule_config(diffopt, ce->name);
>  		if (DIFF_OPT_TST(diffopt, IGNORE_SUBMODULES))
> @@ -516,7 +516,7 @@ int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)
>  	return 0;
>  }
>  
> -int index_differs_from(const char *def, int diff_flags)
> +int index_differs_from(const char *def, uint64_t diff_flags)
>  {
>  	struct rev_info rev;
>  	struct setup_revision_opt opt;
> diff --git a/diff.c b/diff.c
> index d3734d3..f70425f 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4936,7 +4936,7 @@ int diff_can_quit_early(struct diff_options *opt)
>  static int is_submodule_ignored(const char *path, struct diff_options *options)
>  {
>  	int ignored = 0;
> -	unsigned orig_flags = options->flags;
> +	uint64_t orig_flags = options->flags;
>  	if (!DIFF_OPT_TST(options, OVERRIDE_SUBMODULE_CONFIG))
>  		set_diffopt_flags_from_submodule_config(options, path);
>  	if (DIFF_OPT_TST(options, IGNORE_SUBMODULES))
> diff --git a/diff.h b/diff.h
> index 125447b..b497078 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -115,8 +115,8 @@ struct diff_options {
>  	const char *pickaxe;
>  	const char *single_follow;
>  	const char *a_prefix, *b_prefix;
> -	unsigned flags;
> -	unsigned touched_flags;
> +	uint64_t flags;
> +	uint64_t touched_flags;
>  
>  	/* diff-filter bits */
>  	unsigned int filter;
> @@ -348,7 +348,7 @@ extern int diff_result_code(struct diff_options *, int);
>  
>  extern void diff_no_index(struct rev_info *, int, const char **);
>  
> -extern int index_differs_from(const char *def, int diff_flags);
> +extern int index_differs_from(const char *def, uint64_t diff_flags);
>  
>  /*
>   * Fill the contents of the filespec "df", respecting any textconv defined by
> 
Stefan
-- 
----------------------------------------------------------------
/dev/random says: Make Headlines..use a corduroy pillow....
python -c "print '73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')" 
GPG Key fingerprint = 2DF5 E01B 09C3 7501 BCA9  9666 829B 49C5 9221 27AF
Nguyễn Thái Ngọc Duy· Jun 6, 2016, 11:16 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 2/4] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"

The original commit d95d728aba06a34394d15466045cbdabdada58a2 was reverted in commit 78cc1a540ba127b13f2f3fd531777b57f3a9cd46 because we were (and still are) not ready for a new world order. A lot more investigation must be done to see what is impacted. See the 78cc1a5 for details.

This patch takes a smaller and safer step. The new behavior is controlled by SHIFT_INTENT_TO_ADD flag. We can gradually move more diff users to the new behavior after we are sure it's safe to do so. This flag is exposed to outside as "--shift-ita".

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/diff-options.txt |  6 ++++++
 diff-lib.c                     | 14 ++++++++++++++
 diff.c                         |  2 ++
 diff.h                         |  1 +
 t/t2203-add-intent.sh          | 20 ++++++++++++++++++--
 wt-status.c                    |  5 +++++
 6 files changed, 46 insertions(+), 2 deletions(-)
Show changes to 6 files +46 −2

Documentation/diff-options.txt, diff-lib.c, diff.c, diff.h, t/t2203-add-intent.sh, wt-status.c

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 3cb3015..904438b 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -559,5 +559,11 @@ endif::git-format-patch[]
 --no-prefix::
 	Do not show any source or destination prefix.
 
+--shift-ita::
+	By default entries added by "git add -N" appear as an existing
+	empty file in "git diff" and a new file in "git diff --cached".
+	This option makes the entry appear as a new file in "git diff"
+	and non-existent in "git diff --cached".
+
 For more detailed explanation on these common options, see also
 linkgit:gitdiffcore[7].
diff --git a/diff-lib.c b/diff-lib.c
index 27887d0..1248970 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -212,6 +212,12 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 					       ce->sha1, !is_null_sha1(ce->sha1),
 					       ce->name, 0);
 				continue;
+			} else if (DIFF_OPT_TST(&revs->diffopt, SHIFT_INTENT_TO_ADD) &&
+				   ce_intent_to_add(ce)) {
+				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
+					       EMPTY_BLOB_SHA1_BIN, 0,
+					       ce->name, 0);
+				continue;
 			}
 
 			changed = match_stat_with_submodule(&revs->diffopt, ce, &st,
@@ -376,6 +382,14 @@ 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 (DIFF_OPT_TST(&revs->diffopt, SHIFT_INTENT_TO_ADD) &&
+	    idx && ce_intent_to_add(idx)) {
+		idx = NULL;
+		if (!tree)
+			return;	/* nothing to diff.. */
+	}
+
 	/* if the entry is not checked out, don't examine work tree */
 	cached = o->index_only ||
 		(idx && ((idx->ce_flags & CE_VALID) || ce_skip_worktree(idx)));
diff --git a/diff.c b/diff.c
index f70425f..b624e64 100644
--- a/diff.c
+++ b/diff.c
@@ -3910,6 +3910,8 @@ int diff_opt_parse(struct diff_options *options,
 		return parse_submodule_opt(options, arg);
 	else if (skip_prefix(arg, "--ws-error-highlight=", &arg))
 		return parse_ws_error_highlight(options, arg);
+	else if (!strcmp(arg, "--shift-ita"))
+		DIFF_OPT_SET(options, SHIFT_INTENT_TO_ADD);
 
 	/* misc options */
 	else if (!strcmp(arg, "-z"))
diff --git a/diff.h b/diff.h
index b497078..9e42556 100644
--- a/diff.h
+++ b/diff.h
@@ -92,6 +92,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
 #define DIFF_OPT_FUNCCONTEXT         (1 << 29)
 #define DIFF_OPT_PICKAXE_IGNORE_CASE (1 << 30)
 #define DIFF_OPT_DEFAULT_FOLLOW_RENAMES (1U << 31)
+#define DIFF_OPT_SHIFT_INTENT_TO_ADD (1UL << 32)
 
 #define DIFF_OPT_TST(opts, flag)    ((opts)->flags & DIFF_OPT_##flag)
 #define DIFF_OPT_TOUCHED(opts, flag)    ((opts)->touched_flags & DIFF_OPT_##flag)
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 2a4a749..4ae98f4 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -5,10 +5,24 @@ test_description='Intent to add'
 . ./test-lib.sh
 
 test_expect_success 'intent to add' '
+	test_commit 1 &&
+	git rm 1.t &&
+	echo hello >1.t &&
 	echo hello >file &&
 	echo hello >elif &&
 	git add -N file &&
-	git add elif
+	git add elif &&
+	git add -N 1.t
+'
+
+test_expect_success 'git status' '
+	git status --porcelain | grep -v actual >actual &&
+	cat >expect <<-\EOF &&
+	DA 1.t
+	A  elif
+	 A file
+	EOF
+	test_cmp expect actual
 '
 
 test_expect_success 'check result of "add -N"' '
@@ -43,7 +57,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 HEAD -- nitfol | wc -l) = 1 &&
+	test $(git diff --name-only --shift-ita HEAD -- nitfol | wc -l) = 0 &&
+	test $(git diff --name-only --shift-ita -- nitfol | wc -l) = 1
 '
 
 test_expect_success 'can commit with an unrelated i-t-a entry in index' '
diff --git a/wt-status.c b/wt-status.c
index 4f27bd6..cb89005 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -497,6 +497,7 @@ static void wt_status_collect_changes_worktree(struct wt_status *s)
 	setup_revisions(0, NULL, &rev, NULL);
 	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
 	DIFF_OPT_SET(&rev.diffopt, DIRTY_SUBMODULES);
+	DIFF_OPT_SET(&rev.diffopt, SHIFT_INTENT_TO_ADD);
 	if (!s->show_untracked_files)
 		DIFF_OPT_SET(&rev.diffopt, IGNORE_UNTRACKED_IN_SUBMODULES);
 	if (s->ignore_submodule_arg) {
@@ -520,6 +521,7 @@ static void wt_status_collect_changes_index(struct wt_status *s)
 	setup_revisions(0, NULL, &rev, &opt);
 
 	DIFF_OPT_SET(&rev.diffopt, OVERRIDE_SUBMODULE_CONFIG);
+	DIFF_OPT_SET(&rev.diffopt, SHIFT_INTENT_TO_ADD);
 	if (s->ignore_submodule_arg) {
 		handle_ignore_submodules_arg(&rev.diffopt, s->ignore_submodule_arg);
 	} else {
@@ -555,6 +557,8 @@ static void wt_status_collect_changes_initial(struct wt_status *s)
 
 		if (!ce_path_match(ce, &s->pathspec, NULL))
 			continue;
+		if (ce_intent_to_add(ce))
+			continue;
 		it = string_list_insert(&s->change, ce->name);
 		d = it->util;
 		if (!d) {
@@ -853,6 +857,7 @@ static void wt_status_print_verbose(struct wt_status *s)
 
 	init_revisions(&rev, NULL);
 	DIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);
+	DIFF_OPT_SET(&rev.diffopt, SHIFT_INTENT_TO_ADD);
 
 	memset(&opt, 0, sizeof(opt));
 	opt.def = s->is_initial ? EMPTY_TREE_SHA1_HEX : s->reference;
-- 
2.8.2.524.g6ff3d78
Junio C Hamano· Jun 6, 2016, 20:42 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 2/4] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 5 quoted lines
> +--shift-ita::
> +	By default entries added by "git add -N" appear as an existing
> +	empty file in "git diff" and a new file in "git diff --cached".
> +	This option makes the entry appear as a new file in "git diff"
> +	and non-existent in "git diff --cached".

I do not think this should exist at the UI level, even though the use of it in wt-status.c (below) makes a very good sense at least as a temporary band-aid.

At the philosophical level, I however think this "I-T-A does not logically exist in the index (yet)" is a mistake, and "an option controls if I-T-A does or does not exist depending on who calls it" is even worse; it is a road to insanity.

For example, because I-T-A does not logically exist in the index, "git reset --hard" should not remove it but make it untracked again (but I do not think it does). After "git add -N foo", because "foo" does not exist in the index, "git clean" should remove it for the definition of what's in the index to be logically consistent, but the whole intent of "add -N" is that the user meant it is worth checking into sometime in the future, which contradicts with its removal upon "clean".

So, I dunno.
Duy Nguyen· Jun 7, 2016, 12:04 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/4] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"

On Tue, Jun 7, 2016 at 3:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 9 quoted lines
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> +--shift-ita::
>> +     By default entries added by "git add -N" appear as an existing
>> +     empty file in "git diff" and a new file in "git diff --cached".
>> +     This option makes the entry appear as a new file in "git diff"
>> +     and non-existent in "git diff --cached".
>
> I do not think this should exist at the UI level,

I need it. I do "git diff --stat" and "git diff --cached --stat" a lot more often than "git status". Without this option, I'm stuck with the old behavior.

Show 8 quoted lines
> even though the
> use of it in wt-status.c (below) makes a very good sense at least
> as a temporary band-aid.
>
> At the philosophical level, I however think this "I-T-A does not
> logically exist in the index (yet)" is a mistake, and "an option
> controls if I-T-A does or does not exist depending on who calls it"
> is even worse; it is a road to insanity.

i-t-a entries have dual personality (perhaps because it's implemented as an index entry). Although I think the "does not exist" aspect will win in most cases. The intention behind the revert is we have more time to examine case by case and gradually convert them. Maybe in the end one behavior wins and we no longer need another. A thought of keeping i-t-a entries in an index extension instead crossed my mind. It may simplify things a bit (e.g. there's no "ghost" entries any more and active_nr in 3/4 can remain "the number of _real_ entries"). The few parts that do need to know about i-t-a entries need explict modification (probably git-reset and git-diff). But I don't know yet if it would just lead to another nightmare.

Show 8 quoted lines
> For example, because I-T-A does not logically exist in the index,
> "git reset --hard" should not remove it but make it untracked again
> (but I do not think it does). After "git add -N foo", because "foo"
> does not exist in the index, "git clean" should remove it for the
> definition of what's in the index to be logically consistent, but
> the whole intent of "add -N" is that the user meant it is worth
> checking into sometime in the future, which contradicts with its
> removal upon "clean".

I think we should fix them. I started that and so far only 4d55200 (grep: make it clear i-t-a entries are ignored - 2015-12-27) has made it to 'master'.

> So, I dunno.

I just remembered why the old behavior (abort to commit if i-t-a entries are present) bugged me: it does not work well with splitting changes in worktree into multiple commits (e.g. with "git add -p"). Even though I want git remind me to commit an i-t-a entry in the end, it does not necessarily mean I have to do it in the next commit, which may cover a bunch of files except that i-t-a file. I don't see any way around that except ignoring i-t-a entries at commit time. If there's another way, I'm all ears.

-- 
Duy
Johannes Schindelin· Jun 9, 2016, 16:18 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 2/4] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"

Hi Duy,
On Mon, 6 Jun 2016, Nguyễn Thái Ngọc Duy wrote:
Show 9 quoted lines
> diff --git a/diff.h b/diff.h
> index b497078..9e42556 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -92,6 +92,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
>  #define DIFF_OPT_FUNCCONTEXT         (1 << 29)
>  #define DIFF_OPT_PICKAXE_IGNORE_CASE (1 << 30)
>  #define DIFF_OPT_DEFAULT_FOLLOW_RENAMES (1U << 31)
> +#define DIFF_OPT_SHIFT_INTENT_TO_ADD (1UL << 32)

I am afraid that this is not enough. On Windows, sizeof(unsigned long) is 4 (and it is perfectly legal). That means that you would have to use at least (1ULL << 32), but then you also have to change the type of xdl_opts to uint64_t, which in turn means that you will have to change the definition of xpparam_t's "flags" field from unsigned long to uint64_t.

Maybe this can be avoided?

Ciao, Johannes

Duy Nguyen· Sep 27, 2016, 10:58 UTC · re: Johannes Schindelin · lore

Re: [PATCH 2/4] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"

(sorry for a very late reply, I'm just picking this series up again)

On Thu, Jun 9, 2016 at 11:18 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 17 quoted lines
> Hi Duy,
>
> On Mon, 6 Jun 2016, Nguyễn Thái Ngọc Duy wrote:
>
>> diff --git a/diff.h b/diff.h
>> index b497078..9e42556 100644
>> --- a/diff.h
>> +++ b/diff.h
>> @@ -92,6 +92,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
>>  #define DIFF_OPT_FUNCCONTEXT         (1 << 29)
>>  #define DIFF_OPT_PICKAXE_IGNORE_CASE (1 << 30)
>>  #define DIFF_OPT_DEFAULT_FOLLOW_RENAMES (1U << 31)
>> +#define DIFF_OPT_SHIFT_INTENT_TO_ADD (1UL << 32)
>
> I am afraid that this is not enough. On Windows, sizeof(unsigned long) is
> 4 (and it is perfectly legal). That means that you would have to use at
> least (1ULL << 32),
OK.
> but then you also have to change the type of xdl_opts
> to uint64_t, which in turn means that you will have to change the
> definition of xpparam_t's "flags" field from unsigned long to uint64_t.

I miss a connection here. This new flag is intended to be used in "flags" field in struct diff_options. Is there any chance it can be set on xdl_opts (of the same struct, I assume)?

> Maybe this can be avoided?

I don't see a good way to avoid it. We normally enable or disable diff features as bit flags and now we run out of bits. Adding something like "flags2" works, but not pretty. Any suggestion is welcome.

-- 
Duy
Duy Nguyen· Sep 27, 2016, 12:27 UTC · re: Duy Nguyen · lore

Re: [PATCH 2/4] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"

On Tue, Sep 27, 2016 at 5:58 PM, Duy Nguyen <pclouds@gmail.com> wrote:
Show 13 quoted lines
>> but then you also have to change the type of xdl_opts
>> to uint64_t, which in turn means that you will have to change the
>> definition of xpparam_t's "flags" field from unsigned long to uint64_t.
>
> I miss a connection here. This new flag is intended to be used in
> "flags" field in struct diff_options. Is there any chance it can be
> set on xdl_opts (of the same struct, I assume)?
>
>> Maybe this can be avoided?
>
> I don't see a good way to avoid it. We normally enable or disable diff
> features as bit flags and now we run out of bits. Adding something
> like "flags2" works, but not pretty. Any suggestion is welcome.
Never mind. I think I found some way that does not look particularly bad.
-- 
Duy
Nguyễn Thái Ngọc Duy· Jun 6, 2016, 11:16 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 3/4] commit: don't count i-t-a entries when checking if the new commit is empty

i-t-a entries are excluded from tree building. Relying solely on active_nr (or diff without --shift-ita) may lead to empty commits sometimes, when i-t-a entries are the only ones "changed" in the index.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/commit.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)
Show changes to builtin/commit.c +9 −3
diff --git a/builtin/commit.c b/builtin/commit.c
index fcfaa2b..e98ca8a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -894,9 +894,14 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		if (amend)
 			parent = "HEAD^1";
 
-		if (get_sha1(parent, sha1))
-			commitable = !!active_nr;
-		else {
+		if (get_sha1(parent, sha1)) {
+			int i, ita_nr = 0;
+
+			for (i = 0; i < active_nr; i++)
+				if (ce_intent_to_add(active_cache[i]))
+					ita_nr++;
+			commitable = active_nr - ita_nr == 0;
+		} else {
 			/*
 			 * Unless the user did explicitly request a submodule
 			 * ignore mode by passing a command line option we do
@@ -910,6 +915,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 			if (ignore_submodule_arg &&
 			    !strcmp(ignore_submodule_arg, "all"))
 				diff_flags |= DIFF_OPT_IGNORE_SUBMODULES;
+			diff_flags |= DIFF_OPT_SHIFT_INTENT_TO_ADD;
 			commitable = index_differs_from(parent, diff_flags);
 		}
 	}
-- 
2.8.2.524.g6ff3d78
Junio C Hamano· Jun 6, 2016, 19:58 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 3/4] commit: don't count i-t-a entries when checking if the new commit is empty

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 37 quoted lines
> i-t-a entries are excluded from tree building. Relying solely on active_nr
> (or diff without --shift-ita) may lead to empty commits sometimes, when
> i-t-a entries are the only ones "changed" in the index.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/commit.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index fcfaa2b..e98ca8a 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -894,9 +894,14 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  		if (amend)
>  			parent = "HEAD^1";
>  
> -		if (get_sha1(parent, sha1))
> -			commitable = !!active_nr;
> -		else {
> +		if (get_sha1(parent, sha1)) {
> +			int i, ita_nr = 0;
> +
> +			for (i = 0; i < active_nr; i++)
> +				if (ce_intent_to_add(active_cache[i]))
> +					ita_nr++;
> +			commitable = active_nr - ita_nr == 0;
> +		} else {
>  			/*
>  			 * Unless the user did explicitly request a submodule
>  			 * ignore mode by passing a command line option we do
> @@ -910,6 +915,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  			if (ignore_submodule_arg &&
>  			    !strcmp(ignore_submodule_arg, "all"))
>  				diff_flags |= DIFF_OPT_IGNORE_SUBMODULES;
> +			diff_flags |= DIFF_OPT_SHIFT_INTENT_TO_ADD;
>  			commitable = index_differs_from(parent, diff_flags);

This feels like an ugly hack. Doesn't wt-status do a separate "diff" that enumerates what's different between the index and the HEAD?

I am wondering if this "we do not include status and do not ask run_status() about commitable bit" codepath should share more with the other side, which you do not touch at all with this series, which in turn must be doing the right thing already, apparently without having to know anything about the SHIFT_INTENT_TO_ADD hackery.

Or does "git commit" that uses run_status() still allow an empty commit after this patch?

Junio C Hamano· Jun 6, 2016, 20:28 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/4] commit: don't count i-t-a entries when checking if the new commit is empty

Junio C Hamano <gitster@pobox.com> writes:
Show 9 quoted lines
> I am wondering if this "we do not include status and do not ask
> run_status() about commitable bit" codepath should share more with
> the other side, which you do not touch at all with this series,
> which in turn must be doing the right thing already, apparently
> without having to know anything about the SHIFT_INTENT_TO_ADD
> hackery.
>
> Or does "git commit" that uses run_status() still allow an empty
> commit after this patch?

Ah, I didn't see what 2/4 does. The duplication still does make this look an ugly hack, but I can see that the series is at least internally consistent.

Nguyễn Thái Ngọc Duy· Jun 6, 2016, 11:16 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH/RFC 4/4] commit: reinstate commit behavior before 3f6d56d via a config option

There are two groups of users with regards to "git add -N". One uses i-t-a as a reminder that certain untracked files must be added before the next commit. Erroring when i-t-a entries are still present (i.e. real content not added) out gives the user a chance to add real content.

The other group that sees i-t-a entries as a way to make Git aware of some untracked paths so that "git diff" and "git status" show them correctly. If they decide that some content should be staged for the next commit, they "git add" again for real. If content in these paths is never added, the paths are simply ignored.

3f6d56d (commit: ignore intent-to-add entries instead of refusing - 2012-02-07) changes the behavior from the former to the latter. This patch brings bach the former behavior if core.errorCommitIta is true.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 cache-tree.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)
Show changes to cache-tree.c +11 −3
diff --git a/cache-tree.c b/cache-tree.c
index ddf0cc9..b87bd3c 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -155,20 +155,28 @@ static int verify_cache(struct cache_entry **cache,
 {
 	int i, funny;
 	int silent = flags & WRITE_TREE_SILENT;
+	int core_error_commit_ita = 0;
+
+	git_config_get_bool("core.errorcommitita", &core_error_commit_ita);
 
 	/* Verify that the tree is merged */
 	funny = 0;
 	for (i = 0; i < entries; i++) {
 		const struct cache_entry *ce = cache[i];
-		if (ce_stage(ce)) {
+		if (ce_stage(ce) ||
+		    (core_error_commit_ita && ce_intent_to_add(ce))) {
 			if (silent)
 				return -1;
 			if (10 < ++funny) {
 				fprintf(stderr, "...\n");
 				break;
 			}
-			fprintf(stderr, "%s: unmerged (%s)\n",
-				ce->name, sha1_to_hex(ce->sha1));
+			if (ce_stage(ce))
+				fprintf(stderr, "%s: unmerged (%s)\n",
+					ce->name, sha1_to_hex(ce->sha1));
+			else
+				fprintf(stderr, "%s: not added yet\n",
+					ce->name);
 		}
 	}
 	if (funny)
-- 
2.8.2.524.g6ff3d78

← back to recent threads