splitting a commit that adds new files

splitting a commit that adds new files

[Duy Nguyen]

I usually start splitting a commit with "reset @^" then "add -p" back.
The problem is "reset @^" does not keep track of new files added in
HEAD, so I often end up forgetting to add new files back (with "add
-p"). I'm thinking of making "reset" to do "add -N" automatically for
me so I won't miss changes in "add -p". But maybe people already know
how to deal with this case without adding more code?
-- 
Duy

Re: splitting a commit that adds new files

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> I usually start splitting a commit with "reset @^" then "add -p" back.
> The problem is "reset @^" does not keep track of new files added in
> HEAD, so I often end up forgetting to add new files back (with "add
> -p"). I'm thinking of making "reset" to do "add -N" automatically for
> me so I won't miss changes in "add -p". But maybe people already know
> how to deal with this case without adding more code?

Is "reset -p" what you are looking for?  I do not use that myself,
though.

Re: splitting a commit that adds new files

[Jeff King]

On Sun, Feb 02, 2014 at 10:15:07AM -0800, Junio C Hamano wrote:

> Duy Nguyen <pclouds@gmail.com> writes:
> 
> > I usually start splitting a commit with "reset @^" then "add -p" back.
> > The problem is "reset @^" does not keep track of new files added in
> > HEAD, so I often end up forgetting to add new files back (with "add
> > -p"). I'm thinking of making "reset" to do "add -N" automatically for
> > me so I won't miss changes in "add -p". But maybe people already know
> > how to deal with this case without adding more code?
> 
> Is "reset -p" what you are looking for?  I do not use that myself,
> though.

I don't think so. He is at a commit that needs split, so the first thing
to do is to shift the HEAD back. "reset -p" is only about changing the
index[1]. I suppose you could start with a soft reset, and then "reset
-p" away the changes. But then you are going backwards ("this does not
belong in the commit, remove it" rather than "this does belong in the
commit, add it").

I don't typically have a problem with this because I keep my directory
free of untracked files (they are either marked by .gitignore, or
something that I am planning to commit). So just running "git status"
gives me a neat list of what needs to be added (and typically "git add
-N ." is a good starting point upon which to build a "git add -p"
session).

-Peff

[1] I _do_ use "reset -p" when splitting commits, but I do not think it
    is useful here. I use it for "oops, I staged this change, but it
    actually belongs in the next commit. Undo my staging, but leave the
    changes in the working tree for the next one".

Re: splitting a commit that adds new files

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> [1] I _do_ use "reset -p" when splitting commits, but I do not think it
>     is useful here. I use it for "oops, I staged this change, but it
>     actually belongs in the next commit. Undo my staging, but leave the
>     changes in the working tree for the next one".

Sure.  I thought that was exactly what Duy was attempting to do when
he splitted a commit into two (or more).

Re: splitting a commit that adds new files

[Duy Nguyen]

On Tue, Feb 4, 2014 at 1:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> [1] I _do_ use "reset -p" when splitting commits, but I do not think it
>>     is useful here. I use it for "oops, I staged this change, but it
>>     actually belongs in the next commit. Undo my staging, but leave the
>>     changes in the working tree for the next one".
>
> Sure.  I thought that was exactly what Duy was attempting to do when
> he splitted a commit into two (or more).

For splitting into two commits, "reset -p" or "reset @^; add -p" would
be more or less the same, although I still prefer to think "this is
what I need" than "this is what I do _not_ need". "add -p" is more
convenient when the commit is big and you need to split into more than
two because the number of revert chunks may be higher than the number
of added chunks. I recall editing a patch with "checkout -p" sometimes
does not work, not sure it happens with "reset -p" too.
-- 
Duy

[PATCH 1/2] t7101, t7014: rename test files to indicate what that file is for

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Looks like a good thing to do.. Three files with the same "-reset.sh"
 suffix could be confusing.

 t/t7101-reset-empty-subdirs.sh (new +x) | 63 +++++++++++++++++++++++++++++++++
 t/t7101-reset.sh (gone)                 | 63 ---------------------------------
 t/t7104-reset-hard.sh (new +x)          | 46 ++++++++++++++++++++++++
 t/t7104-reset.sh (gone)                 | 46 ------------------------
 4 files changed, 109 insertions(+), 109 deletions(-)
 create mode 100755 t/t7101-reset-empty-subdirs.sh
 delete mode 100755 t/t7101-reset.sh
 create mode 100755 t/t7104-reset-hard.sh
 delete mode 100755 t/t7104-reset.sh

diff --git a/t/t7101-reset-empty-subdirs.sh b/t/t7101-reset-empty-subdirs.sh
new file mode 100755
index 0000000..96e163f
--- /dev/null
+++ b/t/t7101-reset-empty-subdirs.sh
@@ -0,0 +1,63 @@
+#!/bin/sh
+#
+# Copyright (c) 2006 Shawn Pearce
+#
+
+test_description='git reset should cull empty subdirs'
+. ./test-lib.sh
+
+test_expect_success \
+    'creating initial files' \
+    'mkdir path0 &&
+     cp "$TEST_DIRECTORY"/../COPYING path0/COPYING &&
+     git add path0/COPYING &&
+     git commit -m add -a'
+
+test_expect_success \
+    'creating second files' \
+    'mkdir path1 &&
+     mkdir path1/path2 &&
+     cp "$TEST_DIRECTORY"/../COPYING path1/path2/COPYING &&
+     cp "$TEST_DIRECTORY"/../COPYING path1/COPYING &&
+     cp "$TEST_DIRECTORY"/../COPYING COPYING &&
+     cp "$TEST_DIRECTORY"/../COPYING path0/COPYING-TOO &&
+     git add path1/path2/COPYING &&
+     git add path1/COPYING &&
+     git add COPYING &&
+     git add path0/COPYING-TOO &&
+     git commit -m change -a'
+
+test_expect_success \
+    'resetting tree HEAD^' \
+    'git reset --hard HEAD^'
+
+test_expect_success \
+    'checking initial files exist after rewind' \
+    'test -d path0 &&
+     test -f path0/COPYING'
+
+test_expect_success \
+    'checking lack of path1/path2/COPYING' \
+    '! test -f path1/path2/COPYING'
+
+test_expect_success \
+    'checking lack of path1/COPYING' \
+    '! test -f path1/COPYING'
+
+test_expect_success \
+    'checking lack of COPYING' \
+    '! test -f COPYING'
+
+test_expect_success \
+    'checking checking lack of path1/COPYING-TOO' \
+    '! test -f path0/COPYING-TOO'
+
+test_expect_success \
+    'checking lack of path1/path2' \
+    '! test -d path1/path2'
+
+test_expect_success \
+    'checking lack of path1' \
+    '! test -d path1'
+
+test_done
diff --git a/t/t7101-reset.sh b/t/t7101-reset.sh
deleted file mode 100755
index 96e163f..0000000
--- a/t/t7101-reset.sh
+++ /dev/null
@@ -1,63 +0,0 @@
-#!/bin/sh
-#
-# Copyright (c) 2006 Shawn Pearce
-#
-
-test_description='git reset should cull empty subdirs'
-. ./test-lib.sh
-
-test_expect_success \
-    'creating initial files' \
-    'mkdir path0 &&
-     cp "$TEST_DIRECTORY"/../COPYING path0/COPYING &&
-     git add path0/COPYING &&
-     git commit -m add -a'
-
-test_expect_success \
-    'creating second files' \
-    'mkdir path1 &&
-     mkdir path1/path2 &&
-     cp "$TEST_DIRECTORY"/../COPYING path1/path2/COPYING &&
-     cp "$TEST_DIRECTORY"/../COPYING path1/COPYING &&
-     cp "$TEST_DIRECTORY"/../COPYING COPYING &&
-     cp "$TEST_DIRECTORY"/../COPYING path0/COPYING-TOO &&
-     git add path1/path2/COPYING &&
-     git add path1/COPYING &&
-     git add COPYING &&
-     git add path0/COPYING-TOO &&
-     git commit -m change -a'
-
-test_expect_success \
-    'resetting tree HEAD^' \
-    'git reset --hard HEAD^'
-
-test_expect_success \
-    'checking initial files exist after rewind' \
-    'test -d path0 &&
-     test -f path0/COPYING'
-
-test_expect_success \
-    'checking lack of path1/path2/COPYING' \
-    '! test -f path1/path2/COPYING'
-
-test_expect_success \
-    'checking lack of path1/COPYING' \
-    '! test -f path1/COPYING'
-
-test_expect_success \
-    'checking lack of COPYING' \
-    '! test -f COPYING'
-
-test_expect_success \
-    'checking checking lack of path1/COPYING-TOO' \
-    '! test -f path0/COPYING-TOO'
-
-test_expect_success \
-    'checking lack of path1/path2' \
-    '! test -d path1/path2'
-
-test_expect_success \
-    'checking lack of path1' \
-    '! test -d path1'
-
-test_done
diff --git a/t/t7104-reset-hard.sh b/t/t7104-reset-hard.sh
new file mode 100755
index 0000000..f136ee7
--- /dev/null
+++ b/t/t7104-reset-hard.sh
@@ -0,0 +1,46 @@
+#!/bin/sh
+
+test_description='reset --hard unmerged'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+
+	mkdir before later &&
+	>before/1 &&
+	>before/2 &&
+	>hello &&
+	>later/3 &&
+	git add before hello later &&
+	git commit -m world &&
+
+	H=$(git rev-parse :hello) &&
+	git rm --cached hello &&
+	echo "100644 $H 2	hello" | git update-index --index-info &&
+
+	rm -f hello &&
+	mkdir -p hello &&
+	>hello/world &&
+	test "$(git ls-files -o)" = hello/world
+
+'
+
+test_expect_success 'reset --hard should restore unmerged ones' '
+
+	git reset --hard &&
+	git ls-files --error-unmatch before/1 before/2 hello later/3 &&
+	test -f hello
+
+'
+
+test_expect_success 'reset --hard did not corrupt index nor cached-tree' '
+
+	T=$(git write-tree) &&
+	rm -f .git/index &&
+	git add before hello later &&
+	U=$(git write-tree) &&
+	test "$T" = "$U"
+
+'
+
+test_done
diff --git a/t/t7104-reset.sh b/t/t7104-reset.sh
deleted file mode 100755
index f136ee7..0000000
--- a/t/t7104-reset.sh
+++ /dev/null
@@ -1,46 +0,0 @@
-#!/bin/sh
-
-test_description='reset --hard unmerged'
-
-. ./test-lib.sh
-
-test_expect_success setup '
-
-	mkdir before later &&
-	>before/1 &&
-	>before/2 &&
-	>hello &&
-	>later/3 &&
-	git add before hello later &&
-	git commit -m world &&
-
-	H=$(git rev-parse :hello) &&
-	git rm --cached hello &&
-	echo "100644 $H 2	hello" | git update-index --index-info &&
-
-	rm -f hello &&
-	mkdir -p hello &&
-	>hello/world &&
-	test "$(git ls-files -o)" = hello/world
-
-'
-
-test_expect_success 'reset --hard should restore unmerged ones' '
-
-	git reset --hard &&
-	git ls-files --error-unmatch before/1 before/2 hello later/3 &&
-	test -f hello
-
-'
-
-test_expect_success 'reset --hard did not corrupt index nor cached-tree' '
-
-	T=$(git write-tree) &&
-	rm -f .git/index &&
-	git add before hello later &&
-	U=$(git write-tree) &&
-	test "$T" = "$U"
-
-'
-
-test_done
-- 
1.8.5.2.240.g8478abd

[PATCH 2/2] reset: support "--mixed --intent-to-add" mode

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

When --mixed is used, entries could be removed from index if the
target ref does not have them. When "reset" is used in preparation for
commit spliting (in a dirty worktree), it could be hard to track what
files to be added back. The new option --intent-to-add simplifies it
by marking all removed files intent-to-add.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-reset.txt |  5 ++++-
 builtin/reset.c             | 19 +++++++++++++++++--
 cache.h                     |  1 +
 read-cache.c                |  9 +++++++++
 t/t7102-reset.sh            |  9 +++++++++
 5 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index f445cb3..a077ba0 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 [verse]
 'git reset' [-q] [<tree-ish>] [--] <paths>...
 'git reset' (--patch | -p) [<tree-ish>] [--] [<paths>...]
-'git reset' [--soft | --mixed | --hard | --merge | --keep] [-q] [<commit>]
+'git reset' [--soft | --mixed [-N] | --hard | --merge | --keep] [-q] [<commit>]
 
 DESCRIPTION
 -----------
@@ -60,6 +60,9 @@ section of linkgit:git-add[1] to learn how to operate the `--patch` mode.
 	Resets the index but not the working tree (i.e., the changed files
 	are preserved but not marked for commit) and reports what has not
 	been updated. This is the default action.
++
+If `-N` is specified, removed paths are marked as intent-to-add (see
+linkgit:git-add[1]).
 
 --hard::
 	Resets the index and working tree. Any changes to tracked files in the
diff --git a/builtin/reset.c b/builtin/reset.c
index 6004803..f34eab4 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -116,6 +116,7 @@ static void update_index_from_diff(struct diff_queue_struct *q,
 		struct diff_options *opt, void *data)
 {
 	int i;
+	int *intent_to_add = data;
 
 	for (i = 0; i < q->nr; i++) {
 		struct diff_filespec *one = q->queue[i]->one;
@@ -128,13 +129,20 @@ static void update_index_from_diff(struct diff_queue_struct *q,
 				    one->path);
 			add_cache_entry(ce, ADD_CACHE_OK_TO_ADD |
 				ADD_CACHE_OK_TO_REPLACE);
+		} else if (*intent_to_add) {
+			int pos = cache_name_pos(one->path, strlen(one->path));
+			if (pos < 0)
+				die(_("%s does not exist in index"),
+				    one->path);
+			set_intent_to_add(&the_index, active_cache[pos]);
 		} else
 			remove_file_from_cache(one->path);
 	}
 }
 
 static int read_from_tree(const struct pathspec *pathspec,
-			  unsigned char *tree_sha1)
+			  unsigned char *tree_sha1,
+			  int intent_to_add)
 {
 	struct diff_options opt;
 
@@ -142,6 +150,7 @@ static int read_from_tree(const struct pathspec *pathspec,
 	copy_pathspec(&opt.pathspec, pathspec);
 	opt.output_format = DIFF_FORMAT_CALLBACK;
 	opt.format_callback = update_index_from_diff;
+	opt.format_callback_data = &intent_to_add;
 
 	if (do_diff_cache(tree_sha1, &opt))
 		return 1;
@@ -258,6 +267,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	const char *rev;
 	unsigned char sha1[20];
 	struct pathspec pathspec;
+	int intent_to_add = 0;
 	const struct option options[] = {
 		OPT__QUIET(&quiet, N_("be quiet, only report errors")),
 		OPT_SET_INT(0, "mixed", &reset_type,
@@ -270,6 +280,8 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		OPT_SET_INT(0, "keep", &reset_type,
 				N_("reset HEAD but keep local changes"), KEEP),
 		OPT_BOOL('p', "patch", &patch_mode, N_("select hunks interactively")),
+		OPT_BOOL('N', "intent-to-add", &intent_to_add,
+				N_("record only the fact that removed paths will be added later")),
 		OPT_END()
 	};
 
@@ -327,6 +339,9 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		die(_("%s reset is not allowed in a bare repository"),
 		    _(reset_type_names[reset_type]));
 
+	if (intent_to_add && reset_type != MIXED)
+		die(_("-N can only be used with --mixed"));
+
 	/* Soft reset does not touch the index file nor the working tree
 	 * at all, but requires them in a good order.  Other resets reset
 	 * the index file to the tree object we are switching to. */
@@ -338,7 +353,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		int newfd = hold_locked_index(lock, 1);
 		if (reset_type == MIXED) {
 			int flags = quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN;
-			if (read_from_tree(&pathspec, sha1))
+			if (read_from_tree(&pathspec, sha1, intent_to_add))
 				return 1;
 			refresh_index(&the_index, flags, NULL, NULL,
 				      _("Unstaged changes after reset:"));
diff --git a/cache.h b/cache.h
index dc040fb..9d5e09e 100644
--- a/cache.h
+++ b/cache.h
@@ -479,6 +479,7 @@ extern void rename_index_entry_at(struct index_state *, int pos, const char *new
 extern int remove_index_entry_at(struct index_state *, int pos);
 extern void remove_marked_cache_entries(struct index_state *istate);
 extern int remove_file_from_index(struct index_state *, const char *path);
+extern int set_intent_to_add(struct index_state *, struct cache_entry *);
 #define ADD_CACHE_VERBOSE 1
 #define ADD_CACHE_PRETEND 2
 #define ADD_CACHE_IGNORE_ERRORS	4
diff --git a/read-cache.c b/read-cache.c
index 33dd676..dc160a3 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -587,6 +587,15 @@ static void record_intent_to_add(struct cache_entry *ce)
 	hashcpy(ce->sha1, sha1);
 }
 
+int set_intent_to_add(struct index_state *istate, struct cache_entry *ce)
+{
+	record_intent_to_add(ce);
+	ce->ce_flags |= CE_INTENT_TO_ADD;
+	cache_tree_invalidate_path(istate->cache_tree, ce->name);
+	istate->cache_changed = 1;
+	return 0;
+}
+
 int add_to_index(struct index_state *istate, const char *path, struct stat *st, int flags)
 {
 	int size, namelen, was_same;
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 8d4b50d..642920a 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -535,4 +535,13 @@ test_expect_success 'reset with paths accepts tree' '
 	git diff HEAD --exit-code
 '
 
+test_expect_success 'reset -N keeps removed files as intent-to-add' '
+	echo new-file >new-file &&
+	git add new-file &&
+	git reset -N HEAD &&
+	git diff --name-only >actual &&
+	echo new-file >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.8.5.2.240.g8478abd

Re: [PATCH 1/2] t7101, t7014: rename test files to indicate what that file is for

[Jonathan Nieder]

Nguyễn Thái Ngọc Duy wrote:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Makes sense.

[...]
>  t/t7101-reset-empty-subdirs.sh (new +x) | 63 +++++++++++++++++++++++++++++++++
>  t/t7101-reset.sh (gone)                 | 63 ---------------------------------
>  t/t7104-reset-hard.sh (new +x)          | 46 ++++++++++++++++++++++++
>  t/t7104-reset.sh (gone)                 | 46 ------------------------

Hm, summary incorporated in the diffstat.  Neat. :)

format-patch -M tells me that this indeed just renamed the files, so
fwiw

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH 2/2] reset: support "--mixed --intent-to-add" mode

[Junio C Hamano]

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

> @@ -128,13 +129,20 @@ static void update_index_from_diff(struct diff_queue_struct *q,
>  				    one->path);
>  			add_cache_entry(ce, ADD_CACHE_OK_TO_ADD |
>  				ADD_CACHE_OK_TO_REPLACE);
> +		} else if (*intent_to_add) {
> +			int pos = cache_name_pos(one->path, strlen(one->path));
> +			if (pos < 0)
> +				die(_("%s does not exist in index"),
> +				    one->path);
> +			set_intent_to_add(&the_index, active_cache[pos]);

While I do not have any problem with adding an optional "keep lost
paths as intent-to-add entries" feature, I am not sure why this has
to be so different from the usual add-cache-entry codepath.  The
if/elseif chain you are touching inside this loop does:

 - If the tree you are resetting to has something at the path
   (which is different from the current index, obviously), create
   a cache entry to represent that state from the tree and stuff
   it in the index;

 - Otherwise, the tree you are resetting to does not have that
   path.  We used to say "remove it from the index", but now we have
   an option to instead add it as an intent-to-add entry.

So, why doesn't the new codepath do exactly the same thing as the
first branch of the if/else chain and call add_cache_entry but with
a ce marked with CE_INTENT_TO_ADD?  That would parallel what happens
in "git add -N" better, I would think, no?

Re: [PATCH 2/2] reset: support "--mixed --intent-to-add" mode

[Junio C Hamano]

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

> While I do not have any problem with adding an optional "keep lost
> paths as intent-to-add entries" feature, I am not sure why this has
> to be so different from the usual add-cache-entry codepath.  The
> if/elseif chain you are touching inside this loop does:
>
>  - If the tree you are resetting to has something at the path
>    (which is different from the current index, obviously), create
>    a cache entry to represent that state from the tree and stuff
>    it in the index;
>
>  - Otherwise, the tree you are resetting to does not have that
>    path.  We used to say "remove it from the index", but now we have
>    an option to instead add it as an intent-to-add entry.
>
> So, why doesn't the new codepath do exactly the same thing as the
> first branch of the if/else chain and call add_cache_entry but with
> a ce marked with CE_INTENT_TO_ADD?  That would parallel what happens
> in "git add -N" better, I would think, no?

In other words, something along this line, perhaps?

-- >8 --
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Date: Tue, 4 Feb 2014 09:20:09 +0700

When --mixed is used, entries could be removed from index if the
target ref does not have them. When "reset" is used in preparation for
commit spliting (in a dirty worktree), it could be hard to track what
files to be added back. The new option --intent-to-add simplifies it
by marking all removed files intent-to-add.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-reset.txt |  5 ++++-
 builtin/reset.c             | 38 ++++++++++++++++++++++++++------------
 cache.h                     |  1 +
 read-cache.c                |  4 ++--
 t/t7102-reset.sh            |  9 +++++++++
 5 files changed, 42 insertions(+), 15 deletions(-)

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index f445cb3..a077ba0 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 [verse]
 'git reset' [-q] [<tree-ish>] [--] <paths>...
 'git reset' (--patch | -p) [<tree-ish>] [--] [<paths>...]
-'git reset' [--soft | --mixed | --hard | --merge | --keep] [-q] [<commit>]
+'git reset' [--soft | --mixed [-N] | --hard | --merge | --keep] [-q] [<commit>]
 
 DESCRIPTION
 -----------
@@ -60,6 +60,9 @@ section of linkgit:git-add[1] to learn how to operate the `--patch` mode.
 	Resets the index but not the working tree (i.e., the changed files
 	are preserved but not marked for commit) and reports what has not
 	been updated. This is the default action.
++
+If `-N` is specified, removed paths are marked as intent-to-add (see
+linkgit:git-add[1]).
 
 --hard::
 	Resets the index and working tree. Any changes to tracked files in the
diff --git a/builtin/reset.c b/builtin/reset.c
index 6004803..d363bc5 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -116,25 +116,32 @@ static void update_index_from_diff(struct diff_queue_struct *q,
 		struct diff_options *opt, void *data)
 {
 	int i;
+	int intent_to_add = *(int *)data;
 
 	for (i = 0; i < q->nr; i++) {
 		struct diff_filespec *one = q->queue[i]->one;
-		if (one->mode && !is_null_sha1(one->sha1)) {
-			struct cache_entry *ce;
-			ce = make_cache_entry(one->mode, one->sha1, one->path,
-				0, 0);
-			if (!ce)
-				die(_("make_cache_entry failed for path '%s'"),
-				    one->path);
-			add_cache_entry(ce, ADD_CACHE_OK_TO_ADD |
-				ADD_CACHE_OK_TO_REPLACE);
-		} else
+		int is_missing = !(one->mode && !is_null_sha1(one->sha1));
+		struct cache_entry *ce;
+
+		if (is_missing && !intent_to_add) {
 			remove_file_from_cache(one->path);
+			continue;
+		}
+
+		ce = make_cache_entry(one->mode, one->sha1, one->path,
+				      0, 0);
+		if (!ce)
+			die(_("make_cache_entry failed for path '%s'"),
+			    one->path);
+		if (is_missing)
+			mark_intent_to_add(ce);
+		add_cache_entry(ce, ADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE);
 	}
 }
 
 static int read_from_tree(const struct pathspec *pathspec,
-			  unsigned char *tree_sha1)
+			  unsigned char *tree_sha1,
+			  int intent_to_add)
 {
 	struct diff_options opt;
 
@@ -142,6 +149,7 @@ static int read_from_tree(const struct pathspec *pathspec,
 	copy_pathspec(&opt.pathspec, pathspec);
 	opt.output_format = DIFF_FORMAT_CALLBACK;
 	opt.format_callback = update_index_from_diff;
+	opt.format_callback_data = &intent_to_add;
 
 	if (do_diff_cache(tree_sha1, &opt))
 		return 1;
@@ -258,6 +266,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	const char *rev;
 	unsigned char sha1[20];
 	struct pathspec pathspec;
+	int intent_to_add = 0;
 	const struct option options[] = {
 		OPT__QUIET(&quiet, N_("be quiet, only report errors")),
 		OPT_SET_INT(0, "mixed", &reset_type,
@@ -270,6 +279,8 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		OPT_SET_INT(0, "keep", &reset_type,
 				N_("reset HEAD but keep local changes"), KEEP),
 		OPT_BOOL('p', "patch", &patch_mode, N_("select hunks interactively")),
+		OPT_BOOL('N', "intent-to-add", &intent_to_add,
+				N_("record only the fact that removed paths will be added later")),
 		OPT_END()
 	};
 
@@ -327,6 +338,9 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		die(_("%s reset is not allowed in a bare repository"),
 		    _(reset_type_names[reset_type]));
 
+	if (intent_to_add && reset_type != MIXED)
+		die(_("-N can only be used with --mixed"));
+
 	/* Soft reset does not touch the index file nor the working tree
 	 * at all, but requires them in a good order.  Other resets reset
 	 * the index file to the tree object we are switching to. */
@@ -338,7 +352,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		int newfd = hold_locked_index(lock, 1);
 		if (reset_type == MIXED) {
 			int flags = quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN;
-			if (read_from_tree(&pathspec, sha1))
+			if (read_from_tree(&pathspec, sha1, intent_to_add))
 				return 1;
 			refresh_index(&the_index, flags, NULL, NULL,
 				      _("Unstaged changes after reset:"));
diff --git a/cache.h b/cache.h
index dc040fb..20aa73f 100644
--- a/cache.h
+++ b/cache.h
@@ -489,6 +489,7 @@ extern int add_to_index(struct index_state *, const char *path, struct stat *, i
 extern int add_file_to_index(struct index_state *, const char *path, int flags);
 extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, int refresh);
 extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b);
+extern void mark_intent_to_add(struct cache_entry *ce);
 extern int index_name_is_other(const struct index_state *, const char *, int);
 extern void *read_blob_data_from_index(struct index_state *, const char *, unsigned long *);
 
diff --git a/read-cache.c b/read-cache.c
index 33dd676..325d193 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -579,7 +579,7 @@ static struct cache_entry *create_alias_ce(struct cache_entry *ce, struct cache_
 	return new;
 }
 
-static void record_intent_to_add(struct cache_entry *ce)
+void mark_intent_to_add(struct cache_entry *ce)
 {
 	unsigned char sha1[20];
 	if (write_sha1_file("", 0, blob_type, sha1))
@@ -665,7 +665,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
 		if (index_path(ce->sha1, path, st, HASH_WRITE_OBJECT))
 			return error("unable to index file %s", path);
 	} else
-		record_intent_to_add(ce);
+		mark_intent_to_add(ce);
 
 	if (ignore_case && alias && different_name(ce, alias))
 		ce = create_alias_ce(ce, alias);
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 8d4b50d..642920a 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -535,4 +535,13 @@ test_expect_success 'reset with paths accepts tree' '
 	git diff HEAD --exit-code
 '
 
+test_expect_success 'reset -N keeps removed files as intent-to-add' '
+	echo new-file >new-file &&
+	git add new-file &&
+	git reset -N HEAD &&
+	git diff --name-only >actual &&
+	echo new-file >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.9-rc2-217-g24a8b2e

Re: [PATCH 2/2] reset: support "--mixed --intent-to-add" mode

[Duy Nguyen]

On Tue, Feb 04, 2014 at 02:25:25PM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > While I do not have any problem with adding an optional "keep lost
> > paths as intent-to-add entries" feature, I am not sure why this has
> > to be so different from the usual add-cache-entry codepath.  The
> > if/elseif chain you are touching inside this loop does:
> >
> >  - If the tree you are resetting to has something at the path
> >    (which is different from the current index, obviously), create
> >    a cache entry to represent that state from the tree and stuff
> >    it in the index;
> >
> >  - Otherwise, the tree you are resetting to does not have that
> >    path.  We used to say "remove it from the index", but now we have
> >    an option to instead add it as an intent-to-add entry.
> >
> > So, why doesn't the new codepath do exactly the same thing as the
> > first branch of the if/else chain and call add_cache_entry but with
> > a ce marked with CE_INTENT_TO_ADD?  That would parallel what happens
> > in "git add -N" better, I would think, no?
> 
> In other words, something along this line, perhaps?

<snip>

Yes. But you need something like this on top to actually set
CE_INTENT_TO_ADD

-- 8< --
diff --git a/read-cache.c b/read-cache.c
index 325d193..87f1367 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -585,6 +585,7 @@ void mark_intent_to_add(struct cache_entry *ce)
 	if (write_sha1_file("", 0, blob_type, sha1))
 		die("cannot create an empty blob in the object database");
 	hashcpy(ce->sha1, sha1);
+	ce->ce_flags |= CE_INTENT_TO_ADD;
 }
 
 int add_to_index(struct index_state *istate, const char *path, struct stat *st, int flags)
-- 8< --

Re: [PATCH 2/2] reset: support "--mixed --intent-to-add" mode

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Tue, Feb 04, 2014 at 02:25:25PM -0800, Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>> 
>> > While I do not have any problem with adding an optional "keep lost
>> > paths as intent-to-add entries" feature, I am not sure why this has
>> > to be so different from the usual add-cache-entry codepath.  The
>> > if/elseif chain you are touching inside this loop does:
>> >
>> >  - If the tree you are resetting to has something at the path
>> >    (which is different from the current index, obviously), create
>> >    a cache entry to represent that state from the tree and stuff
>> >    it in the index;
>> >
>> >  - Otherwise, the tree you are resetting to does not have that
>> >    path.  We used to say "remove it from the index", but now we have
>> >    an option to instead add it as an intent-to-add entry.
>> >
>> > So, why doesn't the new codepath do exactly the same thing as the
>> > first branch of the if/else chain and call add_cache_entry but with
>> > a ce marked with CE_INTENT_TO_ADD?  That would parallel what happens
>> > in "git add -N" better, I would think, no?
>> 
>> In other words, something along this line, perhaps?
>
> <snip>
>
> Yes. But you need something like this on top to actually set
> CE_INTENT_TO_ADD

Yes, indeed.  I wonder why your new test did not notice it, though
;-)


>
> -- 8< --
> diff --git a/read-cache.c b/read-cache.c
> index 325d193..87f1367 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -585,6 +585,7 @@ void mark_intent_to_add(struct cache_entry *ce)
>  	if (write_sha1_file("", 0, blob_type, sha1))
>  		die("cannot create an empty blob in the object database");
>  	hashcpy(ce->sha1, sha1);
> +	ce->ce_flags |= CE_INTENT_TO_ADD;
>  }
>  
>  int add_to_index(struct index_state *istate, const char *path, struct stat *st, int flags)
> -- 8< --

Re: [PATCH 2/2] reset: support "--mixed --intent-to-add" mode

[Junio C Hamano]

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

> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Tue, Feb 04, 2014 at 02:25:25PM -0800, Junio C Hamano wrote:
>>> Junio C Hamano <gitster@pobox.com> writes:
>>> 
>>> > While I do not have any problem with adding an optional "keep lost
>>> > paths as intent-to-add entries" feature, I am not sure why this has
>>> > to be so different from the usual add-cache-entry codepath.  The
>>> > if/elseif chain you are touching inside this loop does:
>>> >
>>> >  - If the tree you are resetting to has something at the path
>>> >    (which is different from the current index, obviously), create
>>> >    a cache entry to represent that state from the tree and stuff
>>> >    it in the index;
>>> >
>>> >  - Otherwise, the tree you are resetting to does not have that
>>> >    path.  We used to say "remove it from the index", but now we have
>>> >    an option to instead add it as an intent-to-add entry.
>>> >
>>> > So, why doesn't the new codepath do exactly the same thing as the
>>> > first branch of the if/else chain and call add_cache_entry but with
>>> > a ce marked with CE_INTENT_TO_ADD?  That would parallel what happens
>>> > in "git add -N" better, I would think, no?
>>> 
>>> In other words, something along this line, perhaps?
>>
>> <snip>
>>
>> Yes. But you need something like this on top to actually set
>> CE_INTENT_TO_ADD
>
> Yes, indeed.  I wonder why your new test did not notice it, though
> ;-)

... and the answer turns out to be that it was not testing the right
thing.  On top of that faulty version, this will fix it.

Your suggestion to move CE_INTENT_TO_ADD to mark-intent-to-add makes
sense but a caller needs to be adjusted to drop the duplicated flag
manipulation.

 read-cache.c     | 3 +--
 t/t7102-reset.sh | 6 ++++++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 325d193..5b8102a 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -584,6 +584,7 @@ void mark_intent_to_add(struct cache_entry *ce)
 	unsigned char sha1[20];
 	if (write_sha1_file("", 0, blob_type, sha1))
 		die("cannot create an empty blob in the object database");
+	ce->ce_flags |= CE_INTENT_TO_ADD;
 	hashcpy(ce->sha1, sha1);
 }
 
@@ -613,8 +614,6 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
 	ce->ce_namelen = namelen;
 	if (!intent_only)
 		fill_stat_cache_info(ce, st);
-	else
-		ce->ce_flags |= CE_INTENT_TO_ADD;
 
 	if (trust_executable_bit && has_symlinks)
 		ce->ce_mode = create_ce_mode(st_mode);
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 642920a..bc0846f 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -539,6 +539,12 @@ test_expect_success 'reset -N keeps removed files as intent-to-add' '
 	echo new-file >new-file &&
 	git add new-file &&
 	git reset -N HEAD &&
+
+	tree=$(git write-tree) &&
+	git ls-tree $tree new-file >actual &&
+	>expect &&
+	test_cmp expect actual &&
+
 	git diff --name-only >actual &&
 	echo new-file >expect &&
 	test_cmp expect actual

Re: [PATCH 2/2] reset: support "--mixed --intent-to-add" mode

[Duy Nguyen]

On Thu, Feb 6, 2014 at 1:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Yes, indeed.  I wonder why your new test did not notice it, though
>> ;-)
>
> ... and the answer turns out to be that it was not testing the right
> thing.  On top of that faulty version, this will fix it.

Yes, write-tree should test that well.

> Your suggestion to move CE_INTENT_TO_ADD to mark-intent-to-add makes
> sense but a caller needs to be adjusted to drop the duplicated flag
> manipulation.

No no. I found that duplicate, but I did not suggest removing it
because it is needed there..

> @@ -613,8 +614,6 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
>         ce->ce_namelen = namelen;
>         if (!intent_only)
>                 fill_stat_cache_info(ce, st);
> -       else
> -               ce->ce_flags |= CE_INTENT_TO_ADD;
>
>         if (trust_executable_bit && has_symlinks)
>                 ce->ce_mode = create_ce_mode(st_mode);

A few lines down, there's ie_match_stat() call that will check
CE_INTENT_TO_ADD and returns "changed" immediately without looking at
stat data. If stat info is used, it may (not so sure) return "not
changed", the exit path is taken and mark_intent_to_add() is ignored.
-- 
Duy

Re: [PATCH 2/2] reset: support "--mixed --intent-to-add" mode

[Junio C Hamano]

On Wed, Feb 5, 2014 at 3:48 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> No no. I found that duplicate, but I did not suggest removing it
> because it is needed there..

Hmph, if that is the case, we probably should make it the
responsibility of the calling side to actually mark ce->flags with the
bit (which would also mean the function must be renamed to make it
clear that it does not mark).

Re: [PATCH 2/2] reset: support "--mixed --intent-to-add" mode

[Junio C Hamano]

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

> On Wed, Feb 5, 2014 at 3:48 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>> No no. I found that duplicate, but I did not suggest removing it
>> because it is needed there..
>
> Hmph, if that is the case, we probably should make it the
> responsibility of the calling side to actually mark ce->flags with the
> bit (which would also mean the function must be renamed to make it
> clear that it does not mark).

After looking at the codepath that uses the record_intent_to_add()
before this patch, I am coming to the conclusion that it is the
right thing to do after all.  The code appears in this section:

	if (!intent_only) {
		if (index_path(ce->sha1, path, st, HASH_WRITE_OBJECT))
			return error("unable to index file %s", path);
	} else
		record_intent_to_add(ce);

which tells (at least) me: "We are not adding the contents of this
path, so we do not run index_path(); instead we call this helper
function to set the object name in ce to represent an intent-to-add
entry".

So I'll rename it to set_object_name_for_intent_to_add_entry() or
something, restore that flag manipulation back to the caller, and
add another to the new caller, and requeue.

Re: [PATCH 1/2] t7101, t7014: rename test files to indicate what that file is for

[Duy Nguyen]

On Tue, Feb 4, 2014 at 11:05 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>  t/t7101-reset-empty-subdirs.sh (new +x) | 63 +++++++++++++++++++++++++++++++++
>>  t/t7101-reset.sh (gone)                 | 63 ---------------------------------
>>  t/t7104-reset-hard.sh (new +x)          | 46 ++++++++++++++++++++++++
>>  t/t7104-reset.sh (gone)                 | 46 ------------------------
>
> Hm, summary incorporated in the diffstat.  Neat. :)

Yeah, it's from this patch [1]. Maybe I should give it another push,
then remove --summary from format-patch diffstat.

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