[PATCH RFC] commit: allow to commit even if there are intent-to-add entries

[PATCH RFC] commit: allow to commit even if there are intent-to-add entries

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

This patch replaces the approach in 331fcb5 (git add --intent-to-add:
do not let an empty blob be committed by accident) regarding i-t-a
entries: instead of forbidding i-t-a entries at commit time, we can
simply ignore them.

We already ignore CE_REMOVE entries while updating cache-tree. Putting
CE_INTENT_TO_ADD ones in the same category should not cause any negative
effects regarding cache-tree.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 On the few chances I have to use "git add -N" it does not fit well
 with "git add -p; git diff --cached; git commit -m foo" style. I
 think this may be a good thing to do.

 builtin/commit.c      |    2 +-
 builtin/write-tree.c  |    2 +-
 cache-tree.c          |   14 +++++---------
 t/t2203-add-intent.sh |   10 +++++++++-
 4 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index eba1377..767b78a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -871,7 +871,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	discard_cache();
 	read_cache_from(index_file);
 	if (update_main_cache_tree(0)) {
-		error(_("Error building trees"));
+		error(_("Error building trees; the index is unmerged?"));
 		return 0;
 	}
 
diff --git a/builtin/write-tree.c b/builtin/write-tree.c
index b223af4..68baa24 100644
--- a/builtin/write-tree.c
+++ b/builtin/write-tree.c
@@ -46,7 +46,7 @@ int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)
 		die("%s: error reading the index", me);
 		break;
 	case WRITE_TREE_UNMERGED_INDEX:
-		die("%s: error building trees", me);
+		die("%s: error building trees; the index is unmerged?", me);
 		break;
 	case WRITE_TREE_PREFIX_ERROR:
 		die("%s: prefix %s not found", me, prefix);
diff --git a/cache-tree.c b/cache-tree.c
index 8de3959..47defd1 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -158,19 +158,15 @@ static int verify_cache(struct cache_entry **cache,
 	funny = 0;
 	for (i = 0; i < entries; i++) {
 		struct cache_entry *ce = cache[i];
-		if (ce_stage(ce) || (ce->ce_flags & CE_INTENT_TO_ADD)) {
+		if (ce_stage(ce)) {
 			if (silent)
 				return -1;
 			if (10 < ++funny) {
 				fprintf(stderr, "...\n");
 				break;
 			}
-			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);
+			fprintf(stderr, "%s: unmerged (%s)\n",
+				ce->name, sha1_to_hex(ce->sha1));
 		}
 	}
 	if (funny)
@@ -338,8 +334,8 @@ static int update_one(struct cache_tree *it,
 				mode, sha1_to_hex(sha1), entlen+baselen, path);
 		}
 
-		if (ce->ce_flags & CE_REMOVE)
-			continue; /* entry being removed */
+		if (ce->ce_flags & (CE_REMOVE | CE_INTENT_TO_ADD))
+			continue; /* entry being removed or just placeholder */
 
 		strbuf_grow(&buffer, entlen + 100);
 		strbuf_addf(&buffer, "%o %.*s%c", mode, entlen, path + baselen, '\0');
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 2543529..65430e4 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -41,7 +41,15 @@ test_expect_success 'cannot commit with i-t-a entry' '
 	echo frotz >nitfol &&
 	git add rezrov &&
 	git add -N nitfol &&
-	test_must_fail git commit -m initial
+	git commit -m initial &&
+	git ls-tree -r HEAD >actual &&
+	cat >expected <<EOF &&
+100644 blob ce013625030ba8dba906f756967f9e9ca394464a	elif
+100644 blob ce013625030ba8dba906f756967f9e9ca394464a	file
+100644 blob cf7711b63209d0dbc2d030f7fe3513745a9880e4	rezrov
+EOF
+	test_cmp expected actual &&
+	git reset HEAD^
 '
 
 test_expect_success 'can commit with an unrelated i-t-a entry in index' '
-- 
1.7.3.1.256.g2539c.dirty

Re: [PATCH RFC] commit: allow to commit even if there are intent-to-add entries

[Junio C Hamano]

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

> This patch replaces the approach in 331fcb5 (git add --intent-to-add:
> do not let an empty blob be committed by accident) regarding i-t-a
> entries: instead of forbidding i-t-a entries at commit time, we can
> simply ignore them.

I have a mild suspicion that in earlier incarnation of the patch we used
to let empty blobs committed, and then we used to instead not commit
anything at all for such a path, and the real users were bitten by either
of these approaches, forgetting to add the contents to the final commit.

So I am not sure if this is such a good idea.

Re: [PATCH RFC] commit: allow to commit even if there are intent-to-add entries

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

2012/1/11 Junio C Hamano <gitster@pobox.com>:
> I have a mild suspicion that in earlier incarnation of the patch we used
> to let empty blobs committed, and then we used to instead not commit
> anything at all for such a path, and the real users were bitten by either
> of these approaches, forgetting to add the contents to the final commit.
>
> So I am not sure if this is such a good idea.

I found your elaborate writing [1] about it. These are the
interpretations listed in that post:

-- 8< --
When running "commit" and "status" with files marked with "intent to add",
I think there are three possible interpretations of what the user wants to
do.

 (1) I earlier said "I'll decide the exact contents to be committed for
     these paths and tell you by running 'git add' later." when I said
     'git add -N'.  But I forgot to do so before running "git commit".
     Thanks for catching this mistake and erroring out.

 (2) I said "I'll decide the exact content to be committed ... later."
     when I said 'git add -N'. I am running "git commit" now, but I still
     don't know what the final contents for this path should be.  I
     changed my mind, and I do not want to include the addition of these
     paths in the commit I am making.  Please do not error out, but just
     ignore the earlier 'add -N' for now.

 (3) I said "I'll decide the exact content to be committed ... later."
     when I said 'git add -N'. I am running "git commit" now, without
     explicitly telling you with 'git add' about the final contents for
     these paths.  Please take it as an implicit hint that I am happy with
     the contents in the working tree and commit them as if I said 'git
     add' on these paths, but do leave modifications to already tracked
     paths that I haven't added with 'git add'.
-- 8< --

So (1) may be the safe and sane interpretation and should be the
default. But perhaps we should allow (2) also, for example with
--skip-intent-to-add option? It's really frustrating to remove all
i-t-a, commit (I don't do "commit <path>"), then add them back.

[1] http://article.gmane.org/gmane.comp.version-control.git/170658

Nguyễn Thái Ngọc Duy (2):
  cache-tree: update API to take abitrary flags
  commit: add --skip-intent-to-add to allow commit with i-t-a entries
    in index

 builtin/commit.c       |   10 +++++++---
 cache-tree.c           |   35 +++++++++++++++++------------------
 cache-tree.h           |    5 ++++-
 merge-recursive.c      |    2 +-
 t/t2203-add-intent.sh  |   17 +++++++++++++++++
 test-dump-cache-tree.c |    2 +-
 6 files changed, 47 insertions(+), 24 deletions(-)

-- 
1.7.3.1.256.g2539c.dirty

[PATCH 1/2] cache-tree: update API to take abitrary flags

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

---
 builtin/commit.c       |    4 ++--
 cache-tree.c           |   27 ++++++++++++---------------
 cache-tree.h           |    4 +++-
 merge-recursive.c      |    2 +-
 test-dump-cache-tree.c |    2 +-
 5 files changed, 19 insertions(+), 20 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index eba1377..bf42bb3 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -400,7 +400,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
 		fd = hold_locked_index(&index_lock, 1);
 		add_files_to_cache(also ? prefix : NULL, pathspec, 0);
 		refresh_cache_or_die(refresh_flags);
-		update_main_cache_tree(1);
+		update_main_cache_tree(WRITE_TREE_SILENT);
 		if (write_cache(fd, active_cache, active_nr) ||
 		    close_lock_file(&index_lock))
 			die(_("unable to write new_index file"));
@@ -421,7 +421,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
 		fd = hold_locked_index(&index_lock, 1);
 		refresh_cache_or_die(refresh_flags);
 		if (active_cache_changed) {
-			update_main_cache_tree(1);
+			update_main_cache_tree(WRITE_TREE_SILENT);
 			if (write_cache(fd, active_cache, active_nr) ||
 			    commit_locked_index(&index_lock))
 				die(_("unable to write new_index file"));
diff --git a/cache-tree.c b/cache-tree.c
index 8de3959..16355d6 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -150,9 +150,10 @@ void cache_tree_invalidate_path(struct cache_tree *it, const char *path)
 }
 
 static int verify_cache(struct cache_entry **cache,
-			int entries, int silent)
+			int entries, int flags)
 {
 	int i, funny;
+	int silent = flags & WRITE_TREE_SILENT;
 
 	/* Verify that the tree is merged */
 	funny = 0;
@@ -241,10 +242,11 @@ static int update_one(struct cache_tree *it,
 		      int entries,
 		      const char *base,
 		      int baselen,
-		      int missing_ok,
-		      int dryrun)
+		      int flags)
 {
 	struct strbuf buffer;
+	int missing_ok = flags & WRITE_TREE_MISSING_OK;
+	int dryrun = flags & WRITE_TREE_DRY_RUN;
 	int i;
 
 	if (0 <= it->entry_count && has_sha1_file(it->sha1))
@@ -288,8 +290,7 @@ static int update_one(struct cache_tree *it,
 				    cache + i, entries - i,
 				    path,
 				    baselen + sublen + 1,
-				    missing_ok,
-				    dryrun);
+				    flags);
 		if (subcnt < 0)
 			return subcnt;
 		i += subcnt - 1;
@@ -371,15 +372,13 @@ static int update_one(struct cache_tree *it,
 int cache_tree_update(struct cache_tree *it,
 		      struct cache_entry **cache,
 		      int entries,
-		      int missing_ok,
-		      int dryrun,
-		      int silent)
+		      int flags)
 {
 	int i;
-	i = verify_cache(cache, entries, silent);
+	i = verify_cache(cache, entries, flags);
 	if (i)
 		return i;
-	i = update_one(it, cache, entries, "", 0, missing_ok, dryrun);
+	i = update_one(it, cache, entries, "", 0, flags);
 	if (i < 0)
 		return i;
 	return 0;
@@ -572,11 +571,9 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)
 
 	was_valid = cache_tree_fully_valid(active_cache_tree);
 	if (!was_valid) {
-		int missing_ok = flags & WRITE_TREE_MISSING_OK;
-
 		if (cache_tree_update(active_cache_tree,
 				      active_cache, active_nr,
-				      missing_ok, 0, 0) < 0)
+				      flags) < 0)
 			return WRITE_TREE_UNMERGED_INDEX;
 		if (0 <= newfd) {
 			if (!write_cache(newfd, active_cache, active_nr) &&
@@ -672,10 +669,10 @@ int cache_tree_matches_traversal(struct cache_tree *root,
 	return 0;
 }
 
-int update_main_cache_tree (int silent)
+int update_main_cache_tree (int flags)
 {
 	if (!the_index.cache_tree)
 		the_index.cache_tree = cache_tree();
 	return cache_tree_update(the_index.cache_tree,
-				 the_index.cache, the_index.cache_nr, 0, 0, silent);
+				 the_index.cache, the_index.cache_nr, flags);
 }
diff --git a/cache-tree.h b/cache-tree.h
index 0ec0b2a..d8cb2e9 100644
--- a/cache-tree.h
+++ b/cache-tree.h
@@ -29,13 +29,15 @@ void cache_tree_write(struct strbuf *, struct cache_tree *root);
 struct cache_tree *cache_tree_read(const char *buffer, unsigned long size);
 
 int cache_tree_fully_valid(struct cache_tree *);
-int cache_tree_update(struct cache_tree *, struct cache_entry **, int, int, int, int);
+int cache_tree_update(struct cache_tree *, struct cache_entry **, int, int);
 
 int update_main_cache_tree(int);
 
 /* bitmasks to write_cache_as_tree flags */
 #define WRITE_TREE_MISSING_OK 1
 #define WRITE_TREE_IGNORE_CACHE_TREE 2
+#define WRITE_TREE_DRY_RUN 4
+#define WRITE_TREE_SILENT 8
 
 /* error return codes */
 #define WRITE_TREE_UNREADABLE_INDEX (-1)
diff --git a/merge-recursive.c b/merge-recursive.c
index d83cd6c..6479a60 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -264,7 +264,7 @@ struct tree *write_tree_from_memory(struct merge_options *o)
 
 	if (!cache_tree_fully_valid(active_cache_tree) &&
 	    cache_tree_update(active_cache_tree,
-			      active_cache, active_nr, 0, 0, 0) < 0)
+			      active_cache, active_nr, 0) < 0)
 		die("error building trees");
 
 	result = lookup_tree(active_cache_tree->sha1);
diff --git a/test-dump-cache-tree.c b/test-dump-cache-tree.c
index e6c2923..a6ffdf3 100644
--- a/test-dump-cache-tree.c
+++ b/test-dump-cache-tree.c
@@ -59,6 +59,6 @@ int main(int ac, char **av)
 	struct cache_tree *another = cache_tree();
 	if (read_cache() < 0)
 		die("unable to read index file");
-	cache_tree_update(another, active_cache, active_nr, 0, 1, 0);
+	cache_tree_update(another, active_cache, active_nr, WRITE_TREE_DRY_RUN);
 	return dump_cache_tree(active_cache_tree, another, "");
 }
-- 
1.7.3.1.256.g2539c.dirty

[PATCH 2/2] commit: add --skip-intent-to-add to allow commit with i-t-a entries in index

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

---
 builtin/commit.c      |   10 +++++++---
 cache-tree.c          |    8 +++++---
 cache-tree.h          |    1 +
 t/t2203-add-intent.sh |   17 +++++++++++++++++
 4 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index bf42bb3..021206e 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -86,6 +86,7 @@ static int all, also, interactive, patch_interactive, only, amend, signoff;
 static int edit_flag = -1; /* unspecified */
 static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
 static int no_post_rewrite, allow_empty_message;
+static int cache_tree_flags, skip_intent_to_add;
 static char *untracked_files_arg, *force_date, *ignore_submodule_arg;
 static char *sign_commit;
 
@@ -170,6 +171,7 @@ static struct option builtin_commit_options[] = {
 	OPT_BOOLEAN(0, "amend", &amend, "amend previous commit"),
 	OPT_BOOLEAN(0, "no-post-rewrite", &no_post_rewrite, "bypass post-rewrite hook"),
 	{ OPTION_STRING, 'u', "untracked-files", &untracked_files_arg, "mode", "show untracked files, optional modes: all, normal, no. (Default: all)", PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
+	OPT_BOOL(0, "skip-intent-to-add", &skip_intent_to_add, "allow intent-to-add entries in index"),
 	/* end commit contents options */
 
 	{ OPTION_BOOLEAN, 0, "allow-empty", &allow_empty, NULL,
@@ -400,7 +402,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
 		fd = hold_locked_index(&index_lock, 1);
 		add_files_to_cache(also ? prefix : NULL, pathspec, 0);
 		refresh_cache_or_die(refresh_flags);
-		update_main_cache_tree(WRITE_TREE_SILENT);
+		update_main_cache_tree(cache_tree_flags | WRITE_TREE_SILENT);
 		if (write_cache(fd, active_cache, active_nr) ||
 		    close_lock_file(&index_lock))
 			die(_("unable to write new_index file"));
@@ -421,7 +423,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
 		fd = hold_locked_index(&index_lock, 1);
 		refresh_cache_or_die(refresh_flags);
 		if (active_cache_changed) {
-			update_main_cache_tree(WRITE_TREE_SILENT);
+			update_main_cache_tree(cache_tree_flags | WRITE_TREE_SILENT);
 			if (write_cache(fd, active_cache, active_nr) ||
 			    commit_locked_index(&index_lock))
 				die(_("unable to write new_index file"));
@@ -870,7 +872,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	 */
 	discard_cache();
 	read_cache_from(index_file);
-	if (update_main_cache_tree(0)) {
+	if (update_main_cache_tree(cache_tree_flags)) {
 		error(_("Error building trees"));
 		return 0;
 	}
@@ -1088,6 +1090,8 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		cleanup_mode = CLEANUP_ALL;
 	else
 		die(_("Invalid cleanup mode %s"), cleanup_arg);
+	if (skip_intent_to_add)
+		cache_tree_flags = WRITE_TREE_INTENT_TO_ADD_OK;
 
 	handle_untracked_files_arg(s);
 
diff --git a/cache-tree.c b/cache-tree.c
index 16355d6..b8593e0 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -159,7 +159,9 @@ static int verify_cache(struct cache_entry **cache,
 	funny = 0;
 	for (i = 0; i < entries; i++) {
 		struct cache_entry *ce = cache[i];
-		if (ce_stage(ce) || (ce->ce_flags & CE_INTENT_TO_ADD)) {
+		if (ce_stage(ce) ||
+		    ((flags & WRITE_TREE_INTENT_TO_ADD_OK) == 0 &&
+		     (ce->ce_flags & CE_INTENT_TO_ADD))) {
 			if (silent)
 				return -1;
 			if (10 < ++funny) {
@@ -339,8 +341,8 @@ static int update_one(struct cache_tree *it,
 				mode, sha1_to_hex(sha1), entlen+baselen, path);
 		}
 
-		if (ce->ce_flags & CE_REMOVE)
-			continue; /* entry being removed */
+		if (ce->ce_flags & (CE_REMOVE | CE_INTENT_TO_ADD))
+			continue; /* entry being removed or placeholder */
 
 		strbuf_grow(&buffer, entlen + 100);
 		strbuf_addf(&buffer, "%o %.*s%c", mode, entlen, path + baselen, '\0');
diff --git a/cache-tree.h b/cache-tree.h
index d8cb2e9..865733c 100644
--- a/cache-tree.h
+++ b/cache-tree.h
@@ -38,6 +38,7 @@ int update_main_cache_tree(int);
 #define WRITE_TREE_IGNORE_CACHE_TREE 2
 #define WRITE_TREE_DRY_RUN 4
 #define WRITE_TREE_SILENT 8
+#define WRITE_TREE_INTENT_TO_ADD_OK 16
 
 /* error return codes */
 #define WRITE_TREE_UNREADABLE_INDEX (-1)
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 2543529..990c765 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -44,6 +44,23 @@ test_expect_success 'cannot commit with i-t-a entry' '
 	test_must_fail git commit -m initial
 '
 
+test_expect_success 'can commit with i-t-a entry' '
+	git reset --hard &&
+	echo xyzzy >rezrov &&
+	echo frotz >nitfol &&
+	git add rezrov &&
+	git add -N nitfol &&
+	git commit --skip-intent-to-add -m initial &&
+	git ls-tree -r HEAD >actual &&
+	cat >expected <<EOF &&
+100644 blob ce013625030ba8dba906f756967f9e9ca394464a	elif
+100644 blob ce013625030ba8dba906f756967f9e9ca394464a	file
+100644 blob cf7711b63209d0dbc2d030f7fe3513745a9880e4	rezrov
+EOF
+	test_cmp expected actual &&
+	git reset HEAD^
+'
+
 test_expect_success 'can commit with an unrelated i-t-a entry in index' '
 	git reset --hard &&
 	echo xyzzy >rezrov &&
-- 
1.7.3.1.256.g2539c.dirty

Re: [PATCH RFC] commit: allow to commit even if there are intent-to-add entries

[Jonathan Nieder]

Hi,

Junio C Hamano wrote:

> I have a mild suspicion that in earlier incarnation of the patch we used
> to let empty blobs committed, and then we used to instead not commit
> anything at all for such a path, and the real users were bitten by either
> of these approaches, forgetting to add the contents to the final commit.

I remember the empty blob era. :)  I don't think I ever saw something
like this patch, though, and a quick search finds that the first
iteration of the bugfix to stop commiting empty blobs was the one that
was used:

 http://thread.gmane.org/gmane.comp.version-control.git/101881/focus=101894

I suspect that at the time, introducing an intent-to-add flag (which
was always the right thing to do) provided enough momentum to avoid
any worries about smaller details like whether to error out or skip
those entries on commit, which could always be changed later (today).

> So I am not sure if this is such a good idea.

My first reaction was the same, but on reflection, I think this is a
good idea as long as the "git status" output in the editor says
something appropriate.

The response Duy mentioned[1] to a report about the unenlightening
message "error building trees" was also memorable:

> When running "commit" and "status" with files marked with "intent to add",
> I think there are three possible interpretations of what the user
> wants to do.
[ (1) thanks for stopping me, I had forgotten about that file;
  (2) I changed my mind, please leave that file out; or (3) please
  dwim and add the file ]

I think (3) was a trick --- no one that does not use the "-a" option
would want that. :)

At the time, I did not understand what (2) meant.  Now I see why ---
in interpretation (2), the user did not change her mind at all.  She
said "I will be adding this file at some point, so please keep track
of it along with the others for the sake of commands like 'git diff'
and 'git add -u', but that does not mean "I will be adding this file
at some point _before the next commit_".

So at the time I thought (1) was the only sensible behavior but kept
my mouth shut; and now I see that (1) and (2) both fit into reasonable
workflows.

However.  A person using "git diff" to review remaining changes and
"git add" to mark files once they have reached their final form would
benefit even more from a switch for "git commit" to error out if _any_
files in the worktree do not match the index.  So if we are to take
this workflow seriously, treating "git add -N" as a special case is
not helping.  What we currently provide for this workflow is a
reminder in the status area of changes that were not marked with "git
add".

I suspect no longer erroring out might feel eerie for a period for
people that were relying on "git add -N" as a reminder but that as
long as the "git status" output is sensible enough, Duy's proposed
behavior would end up seeming just as natural.

(2) makes intent-to-add entries just like any other tracked index
entry with some un-added content.  It is conceptually pleasant and
fits well in all workflows I can imagine[2].

Hope that helps, and sorry for the ramble,
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/170651/focus=170658
[2] Ok, that is a small lie.  In "git stash", a commit is used to save
the state of the index, so the user would want the presence of the
intent-to-add entry to be stored somehow in the commit, and none of
(1), (2), or (3) will make her happy.  Using "git commit" this way is
not going to work when there are unmerged entries, for example,
either, so I think it is okay to ignore that problem here.

Re: [PATCH RFC] commit: allow to commit even if there are intent-to-add entries

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>> When running "commit" and "status" with files marked with "intent to add",
>> I think there are three possible interpretations of what the user
>> wants to do.
> [ (1) thanks for stopping me, I had forgotten about that file;
>   (2) I changed my mind, please leave that file out; or (3) please
>   dwim and add the file ]
>
> I think (3) was a trick --- no one that does not use the "-a" option
> would want that. :)

I really wish it were the case, but I doubt it.

People from other VCS background seem to still think that "commit" without
paths should commit everything; after getting told that "what you add to
the index is what you will commit", I can easily see this commint: but but
but I told Git that I _want_ to add with -N! Why aren't you committing it?

> At the time, I did not understand what (2) meant.  Now I see why ---
> in interpretation (2), the user did not change her mind at all.

You are correct. "I still cannot make up my mind" is what is happening in
that situation.

The user explicitly said "I cannot decide about this path right now" when
she said "add -N". And we haven't heard from the user what should happen
to the path. Now we have to make a commit so somebody needs to decide.

> She
> said "I will be adding this file at some point, so please keep track
> of it along with the others for the sake of commands like 'git diff'
> and 'git add -u', but that does not mean "I will be adding this file
> at some point _before the next commit_".

Correct. She only said "I cannot decide right now" when she said "add -N"
and hasn't gave us any more hint as to what should happen now we have to
make a commit.

It is _wrong_ for us to unilaterally decide for the user that she does not
want the path in the commit. The last we heard from her was that she does
not know what should happen to the path.

> (2) makes intent-to-add entries just like any other tracked index
> entry with some un-added content.

You are comparing files edited in the working tree without the user
telling anything about them to Git (both tracked and untracked) and files
the user explicitly told Git that the user hasn't made up her mind
about. Why is it a good thing to make the latter behave "just like any
other"?

Re: [PATCH 1/2] cache-tree: update API to take abitrary flags

[Junio C Hamano]

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

> ---

Thanks; this one looks very sensible regardless of what follows (or does
not follow).  Forgot to sign-off?

Re: [PATCH 2/2] commit: add --skip-intent-to-add to allow commit with i-t-a entries in index

[Junio C Hamano]

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

> ---
>  builtin/commit.c      |   10 +++++++---
>  cache-tree.c          |    8 +++++---
>  cache-tree.h          |    1 +
>  t/t2203-add-intent.sh |   17 +++++++++++++++++
>  4 files changed, 30 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index bf42bb3..021206e 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -86,6 +86,7 @@ static int all, also, interactive, patch_interactive, only, amend, signoff;
>  static int edit_flag = -1; /* unspecified */
>  static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
>  static int no_post_rewrite, allow_empty_message;
> +static int cache_tree_flags, skip_intent_to_add;
>  static char *untracked_files_arg, *force_date, *ignore_submodule_arg;
>  static char *sign_commit;
>  
> @@ -170,6 +171,7 @@ static struct option builtin_commit_options[] = {
>  	OPT_BOOLEAN(0, "amend", &amend, "amend previous commit"),
>  	OPT_BOOLEAN(0, "no-post-rewrite", &no_post_rewrite, "bypass post-rewrite hook"),
>  	{ OPTION_STRING, 'u', "untracked-files", &untracked_files_arg, "mode", "show untracked files, optional modes: all, normal, no. (Default: all)", PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
> +	OPT_BOOL(0, "skip-intent-to-add", &skip_intent_to_add, "allow intent-to-add entries in index"),

This is more like "ignore", not "allow", from end user's point of view,
no? The user earlier said "I cannot decide what contents to put in the
commit yet for this path", and normally we catch it and remind the user
that she needs to decide. This option gives her a quick way to say "I
decide that I do not want to add this path at all to this commit I am
creating, so please ignore it in the meantime."

> @@ -1088,6 +1090,8 @@ static int parse_and_validate_options(int argc, const char *argv[],
>  		cleanup_mode = CLEANUP_ALL;
>  	else
>  		die(_("Invalid cleanup mode %s"), cleanup_arg);
> +	if (skip_intent_to_add)
> +		cache_tree_flags = WRITE_TREE_INTENT_TO_ADD_OK;

The name WRITE_TREE_INTENT_TO_ADD_OK says "it is OK to call write-tree
with i-t-a entries in the index, please do not barf", but I think "when
writing a tree, ignore i-t-a entries" would be a more appropriate way to
say the same thing, i.e. WRITE_TREE_IGNORE_INTENT_TO_ADD.

Other than that, I do not see an issue in the implementation of the
patch. It is a separate design level issue if we want to worsen
proliferation of the options, though.

Thanks.

Re: [PATCH 1/2] cache-tree: update API to take abitrary flags

[Nguyen Thai Ngoc Duy]

2012/1/12 Junio C Hamano <gitster@pobox.com>:
> Thanks; this one looks very sensible regardless of what follows (or does
> not follow).  Forgot to sign-off?

Deliberately to stop you from using it because I did not test it
carefully. It was created as material for the discussion only. Will
resubmit later.
-- 
Duy

Re: [PATCH RFC] commit: allow to commit even if there are intent-to-add entries

[Nguyen Thai Ngoc Duy]

2012/1/12 Junio C Hamano <gitster@pobox.com>:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>>> When running "commit" and "status" with files marked with "intent to add",
>>> I think there are three possible interpretations of what the user
>>> wants to do.
>> [ (1) thanks for stopping me, I had forgotten about that file;
>>   (2) I changed my mind, please leave that file out; or (3) please
>>   dwim and add the file ]
>>
>> I think (3) was a trick --- no one that does not use the "-a" option
>> would want that. :)
>
> I really wish it were the case, but I doubt it.
>
> People from other VCS background seem to still think that "commit" without
> paths should commit everything; after getting told that "what you add to
> the index is what you will commit", I can easily see this commint: but but
> but I told Git that I _want_ to add with -N! Why aren't you committing it?

I see "-N" just as an indication, not really an "add" operation.
Perhaps update-index is a better place for it.

>> (2) makes intent-to-add entries just like any other tracked index
>> entry with some un-added content.
>
> You are comparing files edited in the working tree without the user
> telling anything about them to Git (both tracked and untracked) and files
> the user explicitly told Git that the user hasn't made up her mind
> about. Why is it a good thing to make the latter behave "just like any
> other"?

The way I see this flag is "include these files in my diff in addition
to tracked files", and therefore should not have any effects at commit
time. I might turn some of those extra files to tracked some time if I
want to commit.
-- 
Duy

Re: [PATCH RFC] commit: allow to commit even if there are intent-to-add entries

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

>> I really wish it were the case, but I doubt it.
>>
>> People from other VCS background seem to still think that "commit" without
>> paths should commit everything; after getting told that "what you add to
>> the index is what you will commit", I can easily see this commint: but but
>> but I told Git that I _want_ to add with -N! Why aren't you committing it?
>
> I see "-N" just as an indication, not really an "add" operation.

But the thing is, what _you_, who knows a bit more than these new people
about Git, see does not matter, because they would not listen to you.