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

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

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

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

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

[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
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
-- 
2.8.2.524.g6ff3d78

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

[Junio C Hamano]

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

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

[Duy Nguyen]

On Tue, Jun 7, 2016 at 2:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

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

[stefan.naewe]

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".

> 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

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

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

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

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

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

[Junio C Hamano]

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, 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.

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

[Duy Nguyen]

On Tue, Jun 7, 2016 at 3:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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

> 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

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 888 bytes --]

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

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

[Duy Nguyen]

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

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

[Duy Nguyen]

On Tue, Sep 27, 2016 at 5:58 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>> 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

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

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);
 		}
 	}
-- 
2.8.2.524.g6ff3d78

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

[Junio C Hamano]

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

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

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

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

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

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

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