threads / rfc / 29340

RFC patchcommit: allow to commit even if there are intent-to-add entries

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

## tl;dr

12 messages between Jan 11, 2012 and Jan 12, 2012. Diffs are folded; open one to read it.

replies: 11people: 3as markdown or json

Nguyễn Thái Ngọc Duy· Jan 11, 2012, 06:01 UTC · lore

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

builtin/commit.c, builtin/write-tree.c, cache-tree.c, t/t2203-add-intent.sh

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
Junio C Hamano· Jan 11, 2012, 08:08 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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.
Jonathan Nieder· Jan 11, 2012, 11:02 UTC · re: Junio C Hamano · lore

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

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.

Junio C Hamano· Jan 11, 2012, 21:08 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 9 quoted lines
>> 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.

Show 5 quoted lines
> 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"?

Nguyen Thai Ngoc Duy· Jan 12, 2012, 02:53 UTC · re: Junio C Hamano · lore

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

2012/1/12 Junio C Hamano <gitster@pobox.com>:
Show 18 quoted lines
> 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.

Show 8 quoted lines
>> (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
Junio C Hamano· Jan 12, 2012, 03:05 UTC · re: Nguyen Thai Ngoc Duy · lore

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

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

Nguyễn Thái Ngọc Duy· Jan 11, 2012, 09:59 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

2012/1/11 Junio C Hamano <gitster@pobox.com>:
Show 6 quoted lines
> 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
Nguyễn Thái Ngọc Duy· Jan 11, 2012, 09:59 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

---
 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(-)
Show changes to 5 files +19 −20

builtin/commit.c, cache-tree.c, cache-tree.h, merge-recursive.c, test-dump-cache-tree.c

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
Nguyen Thai Ngoc Duy· Jan 12, 2012, 01:20 UTC · re: Junio C Hamano · lore

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

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
Nguyễn Thái Ngọc Duy· Jan 11, 2012, 09:59 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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

builtin/commit.c, cache-tree.c, cache-tree.h, t/t2203-add-intent.sh

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
Junio C Hamano· Jan 11, 2012, 23:55 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [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  <pclouds@gmail.com> writes:
Show 24 quoted lines
> ---
>  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."

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

← back to recent threads