[PATCH 0/2] nd/commit-ignore-i-t-a replacement

[PATCH 0/2] nd/commit-ignore-i-t-a replacement

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

This replaces current topic branch in pu with a more sensible
approach using config key. write-tree also learns about this.

Nguyễn Thái Ngọc Duy (2):
  cache-tree: update API to take abitrary flags
  commit, write-tree: allow to ignore CE_INTENT_TO_ADD while writing
    trees

 Documentation/config.txt         |    5 +++++
 Documentation/git-add.txt        |   12 ++++++++++--
 Documentation/git-write-tree.txt |    8 +++++++-
 builtin/commit.c                 |   13 ++++++++++---
 builtin/write-tree.c             |    2 ++
 cache-tree.c                     |   35 +++++++++++++++++------------------
 cache-tree.h                     |    5 ++++-
 merge-recursive.c                |    2 +-
 t/t2203-add-intent.sh            |   30 ++++++++++++++++++++++++++++++
 test-dump-cache-tree.c           |    2 +-
 10 files changed, 87 insertions(+), 27 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]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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, write-tree: allow to ignore CE_INTENT_TO_ADD while writing trees

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

Normally cache-tree will not produce trees from an index that has
CE_INTENT_TO_ADD entries. This is a safe measure to avoid
mis-interpreting user's intention regarding this flag.

There are situations however where users want to create trees/commits
regardless i-t-a entries. Allow such cases with commit.ignoreIntentToAdd
for git-commit and --ignore-intent-to-add for git-write-tree.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/config.txt         |    5 +++++
 Documentation/git-add.txt        |   12 ++++++++++--
 Documentation/git-write-tree.txt |    8 +++++++-
 builtin/commit.c                 |   13 ++++++++++---
 builtin/write-tree.c             |    2 ++
 cache-tree.c                     |    8 +++++---
 cache-tree.h                     |    1 +
 t/t2203-add-intent.sh            |   30 ++++++++++++++++++++++++++++++
 8 files changed, 70 insertions(+), 9 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index abeb82b..10c3767 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -831,6 +831,11 @@ commit.template::
 	"{tilde}/" is expanded to the value of `$HOME` and "{tilde}user/" to the
 	specified user's home directory.
 
+commit.ignoreIntentToAdd::
+	Allow to commit the index as-is even if there are
+	intent-to-add entries (see option `-N` in linkgit:git-add[1])
+	in index.
+
 credential.helper::
 	Specify an external helper to be called when a username or
 	password credential is needed; the helper may consult external
diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 9c1d395..ec548ea 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -123,8 +123,16 @@ subdirectories.
 	Record only the fact that the path will be added later. An entry
 	for the path is placed in the index with no content. This is
 	useful for, among other things, showing the unstaged content of
-	such files with `git diff` and committing them with `git commit
-	-a`.
+	such files with `git diff`.
++
+Paths added with this option have intent-to-add flag in index. The
+flag is removed once real content is added or updated. By default you
+cannot commit the index as-is from until this flag is removed from all
+entries (i.e. all entries have real content). See commit.ignoreIntentToAdd
+regardless the flag.
++
+Committing with `git commit -a` or with selected paths works
+regardless the config key and the flag.
 
 --refresh::
 	Don't add the file(s), but only refresh their stat()
diff --git a/Documentation/git-write-tree.txt b/Documentation/git-write-tree.txt
index f22041a..d511d3a 100644
--- a/Documentation/git-write-tree.txt
+++ b/Documentation/git-write-tree.txt
@@ -9,7 +9,7 @@ git-write-tree - Create a tree object from the current index
 SYNOPSIS
 --------
 [verse]
-'git write-tree' [--missing-ok] [--prefix=<prefix>/]
+'git write-tree' [--missing-ok] [--ignore-intent-to-add] [--prefix=<prefix>/]
 
 DESCRIPTION
 -----------
@@ -32,6 +32,12 @@ OPTIONS
 	directory exist in the object database.  This option disables this
 	check.
 
+--ignore-intent-to-add::
+	Normally 'git write-tree' will refuse to proceed if there are any
+	intent-to-add entries (see `-N` option in linkgit:git-add[1]).
+	This option continues to create object tree as if there are no
+	intent-to-add entries in index.
+
 --prefix=<prefix>/::
 	Writes a tree object that represents a subdirectory
 	`<prefix>`.  This can be used to write the tree object
diff --git a/builtin/commit.c b/builtin/commit.c
index bf42bb3..097699e 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;
 static char *untracked_files_arg, *force_date, *ignore_submodule_arg;
 static char *sign_commit;
 
@@ -400,7 +401,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 +422,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 +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)) {
+	if (update_main_cache_tree(cache_tree_flags)) {
 		error(_("Error building trees"));
 		return 0;
 	}
@@ -1338,6 +1339,12 @@ static int git_commit_config(const char *k, const char *v, void *cb)
 		include_status = git_config_bool(k, v);
 		return 0;
 	}
+	if (!strcmp(k, "commit.ignoreintenttoadd")) {
+		if (git_config_bool(k, v))
+			cache_tree_flags |= WRITE_TREE_IGNORE_INTENT_TO_ADD;
+		else
+			cache_tree_flags &= ~WRITE_TREE_IGNORE_INTENT_TO_ADD;
+	}
 
 	status = git_gpg_config(k, v, NULL);
 	if (status)
diff --git a/builtin/write-tree.c b/builtin/write-tree.c
index b223af4..07797bb 100644
--- a/builtin/write-tree.c
+++ b/builtin/write-tree.c
@@ -23,6 +23,8 @@ int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)
 	struct option write_tree_options[] = {
 		OPT_BIT(0, "missing-ok", &flags, "allow missing objects",
 			WRITE_TREE_MISSING_OK),
+		OPT_BIT(0, "ignore-intent-to-add", &flags, "ignore i-t-a entries in index",
+			WRITE_TREE_IGNORE_INTENT_TO_ADD ),
 		{ OPTION_STRING, 0, "prefix", &prefix, "<prefix>/",
 		  "write tree object for a subdirectory <prefix>" ,
 		  PARSE_OPT_LITERAL_ARGHELP },
diff --git a/cache-tree.c b/cache-tree.c
index 16355d6..d0be159 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_IGNORE_INTENT_TO_ADD) == 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..af3b917 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_IGNORE_INTENT_TO_ADD 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 58a3299..7b447d3 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -44,6 +44,36 @@ test_expect_success 'cannot commit with i-t-a entry' '
 	test_must_fail git commit
 '
 
+test_expect_success 'can write-tree with i-t-a entry' '
+	git reset --hard &&
+	echo xyzzy >rezrov &&
+	echo frotz >nitfol &&
+	git add rezrov &&
+	git add -N nitfol &&
+	git write-tree --ignore-intent-to-add >actual &&
+	echo 150389afa1ccf46e4104667c741cd0e598269511 >expected &&
+	test_cmp expected actual
+'
+
+test_expect_success 'can commit tree with i-t-a entry' '
+	git reset --hard &&
+	echo xyzzy >rezrov &&
+	echo frotz >nitfol &&
+	git add rezrov &&
+	git add -N nitfol &&
+	git config commit.ignoreIntentToAdd true &&
+	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 config commit.ignoreIntentToAdd false &&
+	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 2/2] commit, write-tree: allow to ignore CE_INTENT_TO_ADD while writing trees

[Pete Wyckoff]

pclouds@gmail.com wrote on Mon, 16 Jan 2012 09:36 +0700:
> Normally cache-tree will not produce trees from an index that has
> CE_INTENT_TO_ADD entries. This is a safe measure to avoid
> mis-interpreting user's intention regarding this flag.
> 
> There are situations however where users want to create trees/commits
> regardless i-t-a entries. Allow such cases with commit.ignoreIntentToAdd
> for git-commit and --ignore-intent-to-add for git-write-tree.

Recently I tried to use "--intent-to-add" on a new file, but when
committing was annoyed by the confusing error, and that I was
forced to do something with that new file.

With commit.ignoreIntentToAdd I can happily commit while leaving
the new file for later.  It stays in the index and is easy to see
in "git status".

I don't understand the need for an option in write-tree; just the
configuration variable is required.

Here's some changes to the docs you might squash in.  It took me
a while to figure out what this variable was about, and I tried
to explain it more clearly for a non-developer audience.

		-- Pete

--------8<------------
From 2471de7083ca3198f59a4734c0d11e9446874de1 Mon Sep 17 00:00:00 2001
From: Pete Wyckoff <pw@padd.com>
Date: Mon, 16 Jan 2012 11:44:26 -0500
Subject: [PATCH] documentation for commit.ignoreIntentToAdd

---
 Documentation/config.txt  |   13 ++++++++-----
 Documentation/git-add.txt |   15 ++++++++-------
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 7ba8777..a2cbb50 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -821,6 +821,14 @@ color.ui::
 	`never` if you prefer git commands not to use color unless enabled
 	explicitly with some other configuration or the `--color` option.
 
+commit.ignoreIntentToAdd::
+	When 'git add' is invoked with `-N`, an "intent-to-add" entry is
+	made in the index.  At commit time, these entries must be removed
+	from the index ("git reset ...") or added ("git add ...").  This
+	boolean variable makes it possible to commit while leaving the
+	"intent-to-add" entries still in the index.  See the description
+	of the `-N` option in linkgit:git-add[1] for details.
+
 commit.status::
 	A boolean to enable/disable inclusion of status information in the
 	commit message template when using an editor to prepare the commit
@@ -831,11 +839,6 @@ commit.template::
 	"{tilde}/" is expanded to the value of `$HOME` and "{tilde}user/" to the
 	specified user's home directory.
 
-commit.ignoreIntentToAdd::
-	Allow to commit the index as-is even if there are
-	intent-to-add entries (see option `-N` in linkgit:git-add[1])
-	in index.
-
 credential.helper::
 	Specify an external helper to be called when a username or
 	password credential is needed; the helper may consult external
diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index ec548ea..1c2ac44 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -125,14 +125,15 @@ subdirectories.
 	useful for, among other things, showing the unstaged content of
 	such files with `git diff`.
 +
-Paths added with this option have intent-to-add flag in index. The
-flag is removed once real content is added or updated. By default you
-cannot commit the index as-is from until this flag is removed from all
-entries (i.e. all entries have real content). See commit.ignoreIntentToAdd
-regardless the flag.
+When committing, these paths must be either added to the index (without
+the `-N` flag) or removed (with "git reset").  However, a configuration
+variable `commit.ignoreIntentToAdd` can be set to allow commits to
+proceed, while the intent-to-add paths remain in the index.
 +
-Committing with `git commit -a` or with selected paths works
-regardless the config key and the flag.
+Regardless of the configuration variable, invoking `git commit -a` will
+commit all files including the ones marked with intent-to-add.
+Specifying a <filepattern> can be used to commit files other than the
+ones marked with intent-to-add.
 
 --refresh::
 	Don't add the file(s), but only refresh their stat()
-- 
1.7.9.rc0.18.gdae96

Re: [PATCH 2/2] commit, write-tree: allow to ignore CE_INTENT_TO_ADD while writing trees

[Junio C Hamano]

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

> Normally cache-tree will not produce trees from an index that has
> CE_INTENT_TO_ADD entries. This is a safe measure to avoid
> mis-interpreting user's intention regarding this flag.

s/safe/safety/;

> There are situations however where users want to create trees/commits
> regardless i-t-a entries.

A new command line option "--no-check-intent-to-add" to "commit" without
any configuration bit may be a useful addition as a first cut, and in
order to help users to which "there are situations" is more than 50% of
the time, a configuration that can be overriden by "--check-intent-to-add"
may be a usability improvement over that first cut, but if this is really
about "there are situations", then a configuration that cannot be
overriden by command line option feels a wrong way to go about it.

Is this really about "there are situations" to begin with? I am suspecting
that this patch is either:

 (1) making it easier to use a wrong workflow, by promoting a way to
     bypass a useful safety measure;

 (2) fixing an earlier UI mistake (iow, the interpretation #2 in the old
     discussion is always the right one and the existing safety measure is
     misguided) in such a way that allows you to work around an objection
     from a bonehead maintainer who refuses to admit that earlier mistake;
     and/or

 (3) splitting the Git userbase into two and making the resulting system
     harder to teach.

If it is (2), and I suspect it may be the case, we might want to rather
honestly describe that the future direction is to fix it, and describe the
configuration option as "an early opt-in" switch, together with the usual
three-step deprecation and migration schedule to make the new behaviour
the default in a future version. From the timeline point of view, it
probably can coincide with the change to always start an editor when
recording a merge commit.

In any case, for this change to help people who add more than one paths
with "add -N" and want to include only a subset of them in the commit, we
may want to explicitly teach them to add what they want to before
committing with the new command line option in the documentation.

Re: [PATCH 2/2] commit, write-tree: allow to ignore CE_INTENT_TO_ADD while writing trees

[Nguyen Thai Ngoc Duy]

2012/1/17 Junio C Hamano <gitster@pobox.com>:
> A new command line option "--no-check-intent-to-add" to "commit" without
> any configuration bit may be a useful addition as a first cut, and in
> order to help users to which "there are situations" is more than 50% of
> the time, a configuration that can be overriden by "--check-intent-to-add"
> may be a usability improvement over that first cut, but if this is really
> about "there are situations", then a configuration that cannot be
> overriden by command line option feels a wrong way to go about it.

"git -c key=value commit" may help.

> Is this really about "there are situations" to begin with? I am suspecting
> that this patch is either:
>
>  (1) making it easier to use a wrong workflow, by promoting a way to
>     bypass a useful safety measure;
>
>  (2) fixing an earlier UI mistake (iow, the interpretation #2 in the old
>     discussion is always the right one and the existing safety measure is
>     misguided) in such a way that allows you to work around an objection
>     from a bonehead maintainer who refuses to admit that earlier mistake;
>     and/or
>
>  (3) splitting the Git userbase into two and making the resulting system
>     harder to teach.

This patch is towards (3). I agree that "add -N" can be confusing to
new users because it does not actually add anything. Those who do not
read manual carefully and commit without checking the result may fall
into that trap. So a user is expected to "level up" a bit and
hopefully by the time he/she feels the need to get around the safety
check and discover this flag, they should be ready to go without the
safety check.

Although I would not oppose deprecating the old behavior if you go with (2) ;)

> In any case, for this change to help people who add more than one paths
> with "add -N" and want to include only a subset of them in the commit, we
> may want to explicitly teach them to add what they want to before
> committing with the new command line option in the documentation.

yeah, keep telling people "this does not add any thing, you need to
git-add again without -N" after running "add -N" using advice
framework when this config is on?
-- 
Duy

Re: [PATCH 2/2] commit, write-tree: allow to ignore CE_INTENT_TO_ADD while writing trees

[Jonathan Nieder]

Nguyen Thai Ngoc Duy wrote:

> Although I would not oppose deprecating the old behavior if you go with (2) ;)

My reaction on reading the patch and clarifying followup was similar
to Junio's.  I don't think a configuration item makes sense unless we
are planning to flip the default in the future, and it would probably
be clearer to document it that way.

By the way, thanks very much for working on this.  I wish I had more
time to offer more than comments.

Sincerely,
Jonathan

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

[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