[PATCH v2 1/4] builtin-add.c: restructure the code for maintainability

[PATCH v2 1/4] builtin-add.c: restructure the code for maintainability

[Junio C Hamano]

The implementation of "git add" has four major codepaths that are mutually
exclusive:

 - if "--interactive" or "--patch" is given, spawn "git add--interactive"
   and exit without doing anything else.  Otherwise things are handled
   internally in this C code;

 - if "--update" is given, update the modified files and exit without
   doing anything else;

 - if "--refresh" is given, do refresh and exit without doing anything
   else;

 - otherwise, find the paths that match pathspecs and stage their
   contents.

It led to an unholy mess in the code structure; each of the latter three
codepaths has a separate call to read_cache(), even though they are all
about "read the current index, update it and write it back", and logically
they should read the index once _anyway_.

This cleans up the latter three cases by introducing a pair of helper
variables:

 - "add_new_files" is set if we need to scan the working tree for paths
   that match the pathspec.  This variable is false for "--update" and
   "--refresh", because they only work on already tracked files.

 - "require_pathspec" is set if the user must give at least one pathspec.
   "--update" does not need it but all the other cases do.

This is in preparation for introducing a new option "--all", that does the
equivalent of "git add -u && git add ." (aka "addremove").

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-add.c |   75 ++++++++++++++++++++++++++++++++------------------------
 1 files changed, 43 insertions(+), 32 deletions(-)

diff --git a/builtin-add.c b/builtin-add.c
index bf13aa3..9b2ee8c 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -140,8 +140,6 @@ static void refresh(int verbose, const char **pathspec)
 	for (specs = 0; pathspec[specs];  specs++)
 		/* nothing */;
 	seen = xcalloc(specs, 1);
-	if (read_cache() < 0)
-		die("index file corrupt");
 	refresh_index(&the_index, verbose ? 0 : REFRESH_QUIET, pathspec, seen);
 	for (i = 0; i < specs; i++) {
 		if (!seen[i])
@@ -216,13 +214,36 @@ static int add_config(const char *var, const char *value, void *cb)
 	return git_default_config(var, value, cb);
 }
 
+static int add_files(struct dir_struct *dir, int flags)
+{
+	int i, exit_status = 0;
+
+	if (dir->ignored_nr) {
+		fprintf(stderr, ignore_error);
+		for (i = 0; i < dir->ignored_nr; i++)
+			fprintf(stderr, "%s\n", dir->ignored[i]->name);
+		fprintf(stderr, "Use -f if you really want to add them.\n");
+		die("no files added");
+	}
+
+	for (i = 0; i < dir->nr; i++)
+		if (add_file_to_cache(dir->entries[i]->name, flags)) {
+			if (!ignore_add_errors)
+				die("adding files failed");
+			exit_status = 1;
+		}
+	return exit_status;
+}
+
 int cmd_add(int argc, const char **argv, const char *prefix)
 {
 	int exit_status = 0;
-	int i, newfd;
+	int newfd;
 	const char **pathspec;
 	struct dir_struct dir;
 	int flags;
+	int add_new_files;
+	int require_pathspec;
 
 	argc = parse_options(argc, argv, builtin_add_options,
 			  builtin_add_usage, 0);
@@ -233,53 +254,43 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	git_config(add_config, NULL);
 
+	add_new_files = !take_worktree_changes && !refresh_only;
+	require_pathspec = !take_worktree_changes;
+
 	newfd = hold_locked_index(&lock_file, 1);
 
 	flags = ((verbose ? ADD_CACHE_VERBOSE : 0) |
 		 (show_only ? ADD_CACHE_PRETEND : 0) |
 		 (ignore_add_errors ? ADD_CACHE_IGNORE_ERRORS : 0));
 
-	if (take_worktree_changes) {
-		const char **pathspec;
-		if (read_cache() < 0)
-			die("index file corrupt");
-		pathspec = get_pathspec(prefix, argv);
-		exit_status = add_files_to_cache(prefix, pathspec, flags);
-		goto finish;
-	}
-
-	if (argc == 0) {
+	if (require_pathspec && argc == 0) {
 		fprintf(stderr, "Nothing specified, nothing added.\n");
 		fprintf(stderr, "Maybe you wanted to say 'git add .'?\n");
 		return 0;
 	}
 	pathspec = get_pathspec(prefix, argv);
 
-	if (refresh_only) {
-		refresh(verbose, pathspec);
-		goto finish;
-	}
-
-	fill_directory(&dir, pathspec, ignored_too);
+	/*
+	 * If we are adding new files, we need to scan the working
+	 * tree to find the ones that match pathspecs; this needs
+	 * to be done before we read the index.
+	 */
+	if (add_new_files)
+		fill_directory(&dir, pathspec, ignored_too);
 
 	if (read_cache() < 0)
 		die("index file corrupt");
 
-	if (dir.ignored_nr) {
-		fprintf(stderr, ignore_error);
-		for (i = 0; i < dir.ignored_nr; i++) {
-			fprintf(stderr, "%s\n", dir.ignored[i]->name);
-		}
-		fprintf(stderr, "Use -f if you really want to add them.\n");
-		die("no files added");
+	if (refresh_only) {
+		refresh(verbose, pathspec);
+		goto finish;
 	}
 
-	for (i = 0; i < dir.nr; i++)
-		if (add_file_to_cache(dir.entries[i]->name, flags)) {
-			if (!ignore_add_errors)
-				die("adding files failed");
-			exit_status = 1;
-		}
+	if (take_worktree_changes)
+		exit_status |= add_files_to_cache(prefix, pathspec, flags);
+
+	if (add_new_files)
+		exit_status |= add_files(&dir, flags);
 
  finish:
 	if (active_cache_changed) {
-- 
1.5.6.4.570.g052e6

[PATCH v2 2/4] git-add --all: add all files

[Junio C Hamano]

People sometimes find that "git add -u && git add ." are 13 keystrokes too
many.  This reduces it by nine.

The support of this has been very low priority for me personally, because
I almost never do "git add ." in a directory with already tracked files,
and in a new directory, there is no point saying "git add -u".

However, for two types of people (that are very different from me), this
mode of operation may make sense and there is no reason to leave it
unsupported.  That is:

 (1) If you are extremely well disciplined and keep perfect .gitignore, it
     always is safe to say "git add ."; or

 (2) If you are extremely undisciplined and do not even know what files
     you created, and you do not very much care what goes in your history,
     it does not matter if "git add ." included everything.

So there it is, although I suspect I will not use it myself, ever.

It will be too much of a change that is against the expectation of the
existing users to allow "git commit -a" to include untracked files, and
it would be inconsistent if we named this new option "-a", so the short
option is "-A".  We _might_ want to later add "git commit -A" but that is
a separate topic.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This option is different from what "commit -a" does, so it must be
   named differently.  v2 patch uses -A as the short option.

 builtin-add.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/builtin-add.c b/builtin-add.c
index 9b2ee8c..6f5672a 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -190,7 +190,7 @@ static const char ignore_error[] =
 "The following paths are ignored by one of your .gitignore files:\n";
 
 static int verbose = 0, show_only = 0, ignored_too = 0, refresh_only = 0;
-static int ignore_add_errors;
+static int ignore_add_errors, addremove;
 
 static struct option builtin_add_options[] = {
 	OPT__DRY_RUN(&show_only),
@@ -200,6 +200,7 @@ static struct option builtin_add_options[] = {
 	OPT_BOOLEAN('p', "patch", &patch_interactive, "interactive patching"),
 	OPT_BOOLEAN('f', "force", &ignored_too, "allow adding otherwise ignored files"),
 	OPT_BOOLEAN('u', "update", &take_worktree_changes, "update tracked files"),
+	OPT_BOOLEAN('A', "all", &addremove, "add all, noticing removal of tracked files"),
 	OPT_BOOLEAN( 0 , "refresh", &refresh_only, "don't add, only refresh the index"),
 	OPT_BOOLEAN( 0 , "ignore-errors", &ignore_add_errors, "just skip files which cannot be added because of errors"),
 	OPT_END(),
@@ -254,6 +255,14 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	git_config(add_config, NULL);
 
+	if (addremove && take_worktree_changes)
+		die("-A and -u are mutually incompatible");
+	if (addremove && !argc) {
+		static const char *here[2] = { ".", NULL };
+		argc = 1;
+		argv = here;
+	}
+
 	add_new_files = !take_worktree_changes && !refresh_only;
 	require_pathspec = !take_worktree_changes;
 
@@ -286,7 +295,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		goto finish;
 	}
 
-	if (take_worktree_changes)
+	if (take_worktree_changes || addremove)
 		exit_status |= add_files_to_cache(prefix, pathspec, flags);
 
 	if (add_new_files)
-- 
1.5.6.4.570.g052e6

[PATCH v2 3/4] git-add --all: tests

[Junio C Hamano]

And here is a small test script that makes sure that:

 - both modified and new files are included,
 - removed file is noticed, and
 - no ignored file is included.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This option has to be a superset of -u, so it must also notice removed
   files.  v2 adds that to the test.

 t/t2202-add-addremove.sh |   44 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 44 insertions(+), 0 deletions(-)
 create mode 100755 t/t2202-add-addremove.sh

diff --git a/t/t2202-add-addremove.sh b/t/t2202-add-addremove.sh
new file mode 100755
index 0000000..6a81510
--- /dev/null
+++ b/t/t2202-add-addremove.sh
@@ -0,0 +1,44 @@
+#!/bin/sh
+
+test_description='git add --all'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	(
+		echo .gitignore
+		echo will-remove
+	) >expect &&
+	(
+		echo actual
+		echo expect
+		echo ignored
+	) >.gitignore &&
+	>will-remove &&
+	git add --all &&
+	test_tick &&
+	git commit -m initial &&
+	git ls-files >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'git add --all' '
+	(
+		echo .gitignore
+		echo not-ignored
+		echo "M	.gitignore"
+		echo "A	not-ignored"
+		echo "D	will-remove"
+	) >expect &&
+	>ignored &&
+	>not-ignored &&
+	echo modification >>.gitignore &&
+	rm -f will-remove &&
+	git add --all &&
+	git update-index --refresh &&
+	git ls-files >actual &&
+	git diff-index --name-status --cached HEAD >>actual &&
+	test_cmp expect actual
+'
+
+test_done
-- 
1.5.6.4.570.g052e6

[PATCH v2 4/4] git-add --all: documentation

[Junio C Hamano]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-add.txt |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 3558905..2b6d6c8 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -9,7 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git add' [-n] [-v] [--force | -f] [--interactive | -i] [--patch | -p]
-	  [--update | -u] [--refresh] [--ignore-errors] [--]
+	  [--all | [--update | -u]] [--refresh] [--ignore-errors] [--]
 	  <filepattern>...
 
 DESCRIPTION
@@ -86,6 +86,12 @@ OPTIONS
 	command line. If no paths are specified, all tracked files in the
 	current directory and its subdirectories are updated.
 
+-A::
+--all::
+	Update files that git already knows about (same as '\--update')
+	and add all untracked files that are not ignored by '.gitignore'
+	mechanism.
+
 --refresh::
 	Don't add the file(s), but only refresh their stat()
 	information in the index.
-- 
1.5.6.4.570.g052e6

Re: [PATCH v2 1/4] builtin-add.c: restructure the code for maintainability

[Johannes Schindelin]

Hi,

On Sat, 19 Jul 2008, Junio C Hamano wrote:


> diff --git a/builtin-add.c b/builtin-add.c
> index bf13aa3..9b2ee8c 100644
> --- a/builtin-add.c
> +++ b/builtin-add.c
> [...]
> +	/*
> +	 * If we are adding new files, we need to scan the working
> +	 * tree to find the ones that match pathspecs; this needs
> +	 * to be done before we read the index.
> +	 */

This comment left me scratching my head.  While I do see a breakage when 
reading the index first, I had the impression that it should not.

I can only imagine that the other users of read_directory() depend on some 
funny interaction between the index and treat_directory().

Ciao,
Dscho

[PATCH/RFC] git add: do not add files from a submodule

[Johannes Schindelin]

It comes quite as a surprise to an unsuspecting Git user that calling
"git add submodule/file" (which is a mistake, alright) _removes_
the submodule in the index, and adds the file.  Instead, complain loudly.

While at it, be nice when the user said "git add submodule/" which is
most likely the consequence of tab-completion, and stage the submodule,
instead of trying to add the contents of that directory.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	This would need a companion patch for "git diff submodule/",
	obviously.  However, revision.c does not need a valid cache,
	usually.  So I am hesitant.

	Oh, and I am sure somebody will come up with a more elegant 
	solution to this problem.  I sure do not, having smashed my head 
	against the wall for a few hours.

 builtin-add.c              |   42 +++++++++++++++++++++++++++++++++++++++---
 t/t7400-submodule-basic.sh |   18 ++++++++++++++++++
 2 files changed, 57 insertions(+), 3 deletions(-)

diff --git a/builtin-add.c b/builtin-add.c
index 6f5672a..9453557 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -50,6 +50,33 @@ static void prune_directory(struct dir_struct *dir, const char **pathspec, int p
         free(seen);
 }
 
+static void treat_gitlinks(const char **pathspec, struct index_state *index)
+{
+	int i;
+
+	if (!pathspec || !*pathspec)
+		return;
+
+	for (i = 0; i < index->cache_nr; i++) {
+		struct cache_entry *ce = index->cache[i];
+		if (S_ISGITLINK(ce->ce_mode)) {
+			int len = ce_namelen(ce), j;
+			for (j = 0; pathspec[j]; j++) {
+				int len2 = strlen(pathspec[j]);
+				if (len2 <= len || pathspec[j][len] != '/' ||
+				    memcmp(ce->name, pathspec[j], len))
+					continue;
+				if (len2 == len + 1)
+					/* strip trailing slash */
+					pathspec[j] = xstrndup(ce->name, len);
+				else
+					die ("Path '%s' is in submodule '%.*s'",
+						pathspec[j], len, ce->name);
+			}
+		}
+	}
+}
+
 static void fill_directory(struct dir_struct *dir, const char **pathspec,
 		int ignored_too)
 {
@@ -245,6 +272,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	int flags;
 	int add_new_files;
 	int require_pathspec;
+	struct index_state index;
 
 	argc = parse_options(argc, argv, builtin_add_options,
 			  builtin_add_usage, 0);
@@ -283,12 +311,20 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	 * If we are adding new files, we need to scan the working
 	 * tree to find the ones that match pathspecs; this needs
 	 * to be done before we read the index.
+	 *
+	 * However, to avoid adding files from submodules, we have to
+	 * read the index first.  So read the index into a local
+	 * variable, and set the global index after fill_directory().
 	 */
-	if (add_new_files)
-		fill_directory(&dir, pathspec, ignored_too);
 
-	if (read_cache() < 0)
+	memset(&index, 0, sizeof(index));
+	if (read_index(&index) < 0)
 		die("index file corrupt");
+	treat_gitlinks(pathspec, &index);
+
+	if (add_new_files)
+		fill_directory(&dir, pathspec, ignored_too);
+	the_index = index;
 
 	if (refresh_only) {
 		refresh(verbose, pathspec);
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index cbc0c34..6da2545 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -209,4 +209,22 @@ test_expect_success 'update --init' '
 
 '
 
+test_expect_success 'do not add files from a submodule' '
+
+	git reset --hard &&
+	test_must_fail git add init/a
+
+'
+
+test_expect_success 'gracefully add submodule with a trailing slash' '
+
+	commit=$(cd init &&
+	 echo b > a &&
+	 git commit -m update a >/dev/null &&
+	 git rev-parse HEAD) &&
+	git add init/ &&
+	test_must_fail git diff --exit-code --cached init
+
+'
+
 test_done
-- 
1.5.6.2.516.g22071

Re: [PATCH v2 1/4] builtin-add.c: restructure the code for maintainability

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> This comment left me scratching my head.  While I do see a breakage when 
> reading the index first, I had the impression that it should not.

The directory traversal that originates from git-ls-files (not the
"show-index" mode which the command originally was about, but the "show
others" part of the feature that came much later) were primarily designed
for collecting paths that do not appear in the active_cache[].  Very early
version of git-add was about adding untracked paths (and update-index was
to stage changes to tracked files), and did have the fill_directory()
after we have read the index for this exact reason.

That changed late 2006 when Nico allowed git-add to stage already tracked
files as well.  We collect the paths in the work tree that match given
pathspec, and for the directory traverser to do that job, you would need
an empty index.

	Side note: 366bfcb (make 'git add' a first class user friendly
	interface to the index, 2006-12-04) is something to marvel at.  It
	describes the change with its documentation update fully, changes
	the semantics in a drastic way, with so little change.

        Documentation/git-add.txt  |   53 ++++++++++++-----------
        Documentation/tutorial.txt |   46 ++++++++++++++++++---
        builtin-add.c              |    6 +-
        wt-status.c                |    2 +-
        4 files changed, 72 insertions(+), 35 deletions(-)

Perhaps we can add a bit to the dir_struct we give to the traverser to
tell it to ignore the index even if we have already read one.  That would
be a much cleaner API enhancement than reading the index and setting aside
while calling read_directory() which feels like a you know what I would
call it.

Perhaps you can lose that comment like this.

 builtin-add.c |   12 ++++--------
 dir.c         |   12 +++++++++---
 dir.h         |    1 +
 3 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/builtin-add.c b/builtin-add.c
index fc3f96e..c6185c3 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -58,6 +58,7 @@ static void fill_directory(struct dir_struct *dir, const char **pathspec,
 
 	/* Set up the default git porcelain excludes */
 	memset(dir, 0, sizeof(*dir));
+	dir->ignore_index = 1;
 	if (!ignored_too) {
 		dir->collect_ignored = 1;
 		setup_standard_excludes(dir);
@@ -280,17 +281,12 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	}
 	pathspec = get_pathspec(prefix, argv);
 
-	/*
-	 * If we are adding new files, we need to scan the working
-	 * tree to find the ones that match pathspecs; this needs
-	 * to be done before we read the index.
-	 */
-	if (add_new_files)
-		fill_directory(&dir, pathspec, ignored_too);
-
 	if (read_cache() < 0)
 		die("index file corrupt");
 
+	if (add_new_files)
+		fill_directory(&dir, pathspec, ignored_too);
+
 	if (refresh_only) {
 		refresh(verbose, pathspec);
 		goto finish;
diff --git a/dir.c b/dir.c
index 29d1d5b..7447485 100644
--- a/dir.c
+++ b/dir.c
@@ -389,7 +389,7 @@ static struct dir_entry *dir_entry_new(const char *pathname, int len)
 
 struct dir_entry *dir_add_name(struct dir_struct *dir, const char *pathname, int len)
 {
-	if (cache_name_exists(pathname, len, ignore_case))
+	if (!dir->ignore_index && cache_name_exists(pathname, len, ignore_case))
 		return NULL;
 
 	ALLOC_GROW(dir->entries, dir->nr+1, dir->alloc);
@@ -483,8 +483,14 @@ static enum directory_treatment treat_directory(struct dir_struct *dir,
 	const char *dirname, int len,
 	const struct path_simplify *simplify)
 {
-	/* The "len-1" is to strip the final '/' */
-	switch (directory_exists_in_index(dirname, len-1)) {
+	enum exist_status in_index;
+
+	if (dir->ignore_index)
+		in_index = index_nonexistent;
+	else
+		/* The "len-1" is to strip the final '/' */
+		in_index = directory_exists_in_index(dirname, len-1);
+	switch (in_index) {
 	case index_directory:
 		return recurse_into_directory;
 
diff --git a/dir.h b/dir.h
index 2df15de..4ef1c99 100644
--- a/dir.h
+++ b/dir.h
@@ -38,6 +38,7 @@ struct dir_struct {
 		     show_other_directories:1,
 		     hide_empty_directories:1,
 		     no_gitlinks:1,
+		     ignore_index:1,
 		     collect_ignored:1;
 	struct dir_entry **entries;
 	struct dir_entry **ignored;

Re: [PATCH v2 1/4] builtin-add.c: restructure the code for maintainability

[Junio C Hamano]

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

> That changed late 2006 when Nico allowed git-add to stage already tracked
> files as well.  We collect the paths in the work tree that match given
> pathspec, and for the directory traverser to do that job, you would need
> an empty index.
>
> 	Side note: 366bfcb (make 'git add' a first class user friendly
> 	interface to the index, 2006-12-04) is something to marvel at.  It
> 	describes the change with its documentation update fully, changes
> 	the semantics in a drastic way, with so little change.
>
>         Documentation/git-add.txt  |   53 ++++++++++++-----------
>         Documentation/tutorial.txt |   46 ++++++++++++++++++---
>         builtin-add.c              |    6 +-
>         wt-status.c                |    2 +-
>         4 files changed, 72 insertions(+), 35 deletions(-)
>
> Perhaps we can add a bit to the dir_struct we give to the traverser to
> tell it to ignore the index even if we have already read one.  That would
> be a much cleaner API enhancement than reading the index and setting aside
> while calling read_directory() which feels like a you know what I would
> call it.

Thinking about this issue a bit more, I realize that the earlier "git add -A"
change was done in a quite inefficient way (i.e. it is as unefficient as
"git add -u && git add ." modulo one fork/exec and read/write index).  For
that matter, the original "git add ." could probably be more efficient
than it currently is.

The thing is, when the user asks "git add .", we do not have to examine
all paths we encounter and perform the excluded() and dir_add_name()
processing, both of which are slower code and use slower data structure by
git standard, especially when the index is already populated.

Instead, we should be able to implement "git add $pathspec..." as:

 - read the index;

 - read_directory() to process untracked, unignored files the current way,
   that is, recursively doing readdir(), filtering them by pathspec and
   excluded(), queueing them via dir_add_name() and finally do
   add_files(); and

 - iterate over the index, filtering them by pathspec, and update only the
   modified/type changed paths but not deleted ones.

And "git add -A" will become exactly the same as above, modulo:

 - missing $pathspec means "." instead of being an error; and 

 - "interate over the index" part handles deleted ones as well,
   i.e. exactly what the current update_callback() in builtin-add.c does.

It is likely that I am too tired to do this right tonight, so I'll go to
bed and expect to find a nicely done patch in my mailbox by somebody else
;-).

Re: [PATCH v2 1/4] builtin-add.c: restructure the code for maintainability

[Junio C Hamano]

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

> Thinking about this issue a bit more, I realize that the earlier "git add -A"
> change was done in a quite inefficient way (i.e. it is as unefficient as
> "git add -u && git add ." modulo one fork/exec and read/write index).  For
> that matter, the original "git add ." could probably be more efficient
> than it currently is.
> ...
> It is likely that I am too tired to do this right tonight, so I'll go to
> bed and expect to find a nicely done patch in my mailbox by somebody else
> ;-).

Well, I lied.  I couldn't sleep, so here is a preview that seems to pass
all the test at least.  It needs to be split into two, but I am too tired
to do that properly tonight.

The main part of the change is third hunk (preimage ll.258-) to the bottom
of builtin-add.c and follows the idea outlined in the message this is a
response to.

A private function add_files_to_cache() in builtin-add.c was borrowed by
checkout and commit re-implementors without getting properly refactored to
more library-ish place.  This does the refactoring, and most of the
changes you see in the diffstat are this; it should come before the main
part of the change.

---

 builtin-add.c |   78 ++++++--------------------------------------------------
 cache.h       |    1 +
 read-cache.c  |   61 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 71 insertions(+), 69 deletions(-)

diff --git a/builtin-add.c b/builtin-add.c
index fc3f96e..f9b25f2 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -8,10 +8,6 @@
 #include "dir.h"
 #include "exec_cmd.h"
 #include "cache-tree.h"
-#include "diff.h"
-#include "diffcore.h"
-#include "commit.h"
-#include "revision.h"
 #include "run-command.h"
 #include "parse-options.h"
 
@@ -79,59 +75,6 @@ static void fill_directory(struct dir_struct *dir, const char **pathspec,
 		prune_directory(dir, pathspec, baselen);
 }
 
-struct update_callback_data
-{
-	int flags;
-	int add_errors;
-};
-
-static void update_callback(struct diff_queue_struct *q,
-			    struct diff_options *opt, void *cbdata)
-{
-	int i;
-	struct update_callback_data *data = cbdata;
-
-	for (i = 0; i < q->nr; i++) {
-		struct diff_filepair *p = q->queue[i];
-		const char *path = p->one->path;
-		switch (p->status) {
-		default:
-			die("unexpected diff status %c", p->status);
-		case DIFF_STATUS_UNMERGED:
-		case DIFF_STATUS_MODIFIED:
-		case DIFF_STATUS_TYPE_CHANGED:
-			if (add_file_to_cache(path, data->flags)) {
-				if (!(data->flags & ADD_CACHE_IGNORE_ERRORS))
-					die("updating files failed");
-				data->add_errors++;
-			}
-			break;
-		case DIFF_STATUS_DELETED:
-			if (!(data->flags & ADD_CACHE_PRETEND))
-				remove_file_from_cache(path);
-			if (data->flags & (ADD_CACHE_PRETEND|ADD_CACHE_VERBOSE))
-				printf("remove '%s'\n", path);
-			break;
-		}
-	}
-}
-
-int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
-{
-	struct update_callback_data data;
-	struct rev_info rev;
-	init_revisions(&rev, prefix);
-	setup_revisions(0, NULL, &rev, NULL);
-	rev.prune_data = pathspec;
-	rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
-	rev.diffopt.format_callback = update_callback;
-	data.flags = flags;
-	data.add_errors = 0;
-	rev.diffopt.format_callback_data = &data;
-	run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
-	return !!data.add_errors;
-}
-
 static void refresh(int verbose, const char **pathspec)
 {
 	char *seen;
@@ -258,7 +201,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	if (addremove && take_worktree_changes)
 		die("-A and -u are mutually incompatible");
-	if (addremove && !argc) {
+	if ((addremove || take_worktree_changes) && !argc) {
 		static const char *here[2] = { ".", NULL };
 		argc = 1;
 		argv = here;
@@ -271,7 +214,9 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	flags = ((verbose ? ADD_CACHE_VERBOSE : 0) |
 		 (show_only ? ADD_CACHE_PRETEND : 0) |
-		 (ignore_add_errors ? ADD_CACHE_IGNORE_ERRORS : 0));
+		 (ignore_add_errors ? ADD_CACHE_IGNORE_ERRORS : 0) |
+		 (!(addremove || take_worktree_changes)
+		  ? ADD_CACHE_IGNORE_REMOVAL : 0));
 
 	if (require_pathspec && argc == 0) {
 		fprintf(stderr, "Nothing specified, nothing added.\n");
@@ -280,24 +225,19 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	}
 	pathspec = get_pathspec(prefix, argv);
 
-	/*
-	 * If we are adding new files, we need to scan the working
-	 * tree to find the ones that match pathspecs; this needs
-	 * to be done before we read the index.
-	 */
-	if (add_new_files)
-		fill_directory(&dir, pathspec, ignored_too);
-
 	if (read_cache() < 0)
 		die("index file corrupt");
 
+	if (add_new_files)
+		/* This picks up the paths that are not tracked */
+		fill_directory(&dir, pathspec, ignored_too);
+
 	if (refresh_only) {
 		refresh(verbose, pathspec);
 		goto finish;
 	}
 
-	if (take_worktree_changes || addremove)
-		exit_status |= add_files_to_cache(prefix, pathspec, flags);
+	exit_status |= add_files_to_cache(prefix, pathspec, flags);
 
 	if (add_new_files)
 		exit_status |= add_files(&dir, flags);
diff --git a/cache.h b/cache.h
index 38985aa..6f374ad 100644
--- a/cache.h
+++ b/cache.h
@@ -375,6 +375,7 @@ extern int remove_file_from_index(struct index_state *, const char *path);
 #define ADD_CACHE_VERBOSE 1
 #define ADD_CACHE_PRETEND 2
 #define ADD_CACHE_IGNORE_ERRORS	4
+#define ADD_CACHE_IGNORE_REMOVAL 8
 extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags);
 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);
diff --git a/read-cache.c b/read-cache.c
index a50a851..6833af6 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -8,6 +8,11 @@
 #include "cache-tree.h"
 #include "refs.h"
 #include "dir.h"
+#include "tree.h"
+#include "commit.h"
+#include "diff.h"
+#include "diffcore.h"
+#include "revision.h"
 
 /* Index extensions.
  *
@@ -1444,3 +1449,59 @@ int read_index_unmerged(struct index_state *istate)
 	istate->cache_nr = dst - istate->cache;
 	return !!last;
 }
+
+struct update_callback_data
+{
+	int flags;
+	int add_errors;
+};
+
+static void update_callback(struct diff_queue_struct *q,
+			    struct diff_options *opt, void *cbdata)
+{
+	int i;
+	struct update_callback_data *data = cbdata;
+
+	for (i = 0; i < q->nr; i++) {
+		struct diff_filepair *p = q->queue[i];
+		const char *path = p->one->path;
+		switch (p->status) {
+		default:
+			die("unexpected diff status %c", p->status);
+		case DIFF_STATUS_UNMERGED:
+		case DIFF_STATUS_MODIFIED:
+		case DIFF_STATUS_TYPE_CHANGED:
+			if (add_file_to_index(&the_index, path, data->flags)) {
+				if (!(data->flags & ADD_CACHE_IGNORE_ERRORS))
+					die("updating files failed");
+				data->add_errors++;
+			}
+			break;
+		case DIFF_STATUS_DELETED:
+			if (data->flags & ADD_CACHE_IGNORE_REMOVAL)
+				break;
+			if (!(data->flags & ADD_CACHE_PRETEND))
+				remove_file_from_index(&the_index, path);
+			if (data->flags & (ADD_CACHE_PRETEND|ADD_CACHE_VERBOSE))
+				printf("remove '%s'\n", path);
+			break;
+		}
+	}
+}
+
+int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
+{
+	struct update_callback_data data;
+	struct rev_info rev;
+	init_revisions(&rev, prefix);
+	setup_revisions(0, NULL, &rev, NULL);
+	rev.prune_data = pathspec;
+	rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
+	rev.diffopt.format_callback = update_callback;
+	data.flags = flags;
+	data.add_errors = 0;
+	rev.diffopt.format_callback_data = &data;
+	run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
+	return !!data.add_errors;
+}
+

Re: [PATCH/RFC] git add: do not add files from a submodule

[Johannes Schindelin]

Hi Junio,

On Mon, 21 Jul 2008, Johannes Schindelin wrote:

> It comes quite as a surprise to an unsuspecting Git user that calling
> "git add submodule/file" (which is a mistake, alright) _removes_
> the submodule in the index, and adds the file.  Instead, complain loudly.
> 
> While at it, be nice when the user said "git add submodule/" which is
> most likely the consequence of tab-completion, and stage the submodule,
> instead of trying to add the contents of that directory.

Do you plan to apply the split-up builtin-add enhancments you did a few 
nights ago, so that this patch can be elegant?  I think this fix should go 
into 1.6.0.

Ciao,
Dscho

Re: [PATCH/RFC] git add: do not add files from a submodule

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Do you plan to apply the split-up builtin-add enhancments you did a few 
> nights ago,...

I have a few updates to that one, I'll be sending them out shortly.

Switching branches between revs that have and do not have submodule at a
given path has always been broken.  It is not even a "known breakage",
which is a word used for something that has a sensible design already is
worked out but the implementation does not do so.

If we started the process of diagnosing and fixing these issues earlier,
and had plausible code to address the issue already in 'next' before the
current -rc cycle started, the topic would have been an obvious candidate
for the coming release and I'd further say it would be even worth delaying
the release for a few weeks if it takes more time.  But I have to say it
is too late for 1.6.0 now if we are just noticing and starting the
discussion.  This comment goes to the issue Pierre raised last night as
well.

Nobody prevents us from starting the process to discuss and prepare to put
something in 'next' for 1.6.1 cycle now, though.

Re: [PATCH/RFC] git add: do not add files from a submodule

[Pierre Habouzit]

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

On Wed, Jul 23, 2008 at 06:40:20AM +0000, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Do you plan to apply the split-up builtin-add enhancments you did a few 
> > nights ago,...
> 
> I have a few updates to that one, I'll be sending them out shortly.
> 
> Switching branches between revs that have and do not have submodule at a
> given path has always been broken.  It is not even a "known breakage",
> which is a word used for something that has a sensible design already is
> worked out but the implementation does not do so.
> 
> If we started the process of diagnosing and fixing these issues earlier,
> and had plausible code to address the issue already in 'next' before the
> current -rc cycle started, the topic would have been an obvious candidate
> for the coming release and I'd further say it would be even worth delaying
> the release for a few weeks if it takes more time.  But I have to say it
> is too late for 1.6.0 now if we are just noticing and starting the
> discussion.

  Well given that we now use submodules at work, and that git is
nowadays somewhere in the top 5 of my most consciously (as opposed to
the compiler that I rarely call by hand) used software suites (among my
editor, my MUA, my shell and my tiling WM), I'm very much interested in
tackling some things about what is (not) done with submodules yet.

  I sent a mail some time ago about those issues, in short words, those
concern git-checkout, git-reset, git-fetch and git-push for starters.
I'm not sure I can help with the code a lot, but I can for sure provide
a comprehensive list of things I would like the porcelain to do with
submodules if that helps.

> This comment goes to the issue Pierre raised last night as well.

  You mean the git checkout issue ? Because the issue is twofold, one
there is the missing warnings that users can shoot themselves in the
foot unnoticed, and there is the fact that "git checkout -- $foo" does
not what it should when "$foo" is a valid revision (and *that* is a
really really bad bug, that if I'm correct changing parse_options flags
to KEEP_DASHDASH fixes alright).

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH/RFC] git add: do not add files from a submodule

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> On Wed, Jul 23, 2008 at 06:40:20AM +0000, Junio C Hamano wrote:
> ...
>> If we started the process of diagnosing and fixing these issues earlier,
>> and had plausible code to address the issue already in 'next' before the
>> current -rc cycle started, the topic would have been an obvious candidate
>> for the coming release and I'd further say it would be even worth delaying
>> the release for a few weeks if it takes more time.  But I have to say it
>> is too late for 1.6.0 now if we are just noticing and starting the
>> discussion.
>
>   Well given that we now use submodules at work, and that git is
> nowadays somewhere in the top 5 of my most consciously (as opposed to
> the compiler that I rarely call by hand) used software suites (among my
> editor, my MUA, my shell and my tiling WM), I'm very much interested in
> tackling some things about what is (not) done with submodules yet.

Surely the effort is appreciated.

>> This comment goes to the issue Pierre raised last night as well.
>
>   You mean the git checkout issue?

Oh, no; that misuse of parse_opt() that forgot KEEP_DASHDASH one was not
what I had in mind.  I meant to say that your "switch branches between an
old pre-submodule rev and a new one that has a submodule at where a blob
or directory used to be" issue with a good explanation material was a good
starting point for submodule improvements for the next cycle.

I'd like the release schedule not too heavily based on "per feature", but
more time-based.

Re: [PATCH/RFC] git add: do not add files from a submodule

[Pierre Habouzit]

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

On Wed, Jul 23, 2008 at 06:31:16PM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > On Wed, Jul 23, 2008 at 06:40:20AM +0000, Junio C Hamano wrote:
> > ...
> >> If we started the process of diagnosing and fixing these issues earlier,
> >> and had plausible code to address the issue already in 'next' before the
> >> current -rc cycle started, the topic would have been an obvious candidate
> >> for the coming release and I'd further say it would be even worth delaying
> >> the release for a few weeks if it takes more time.  But I have to say it
> >> is too late for 1.6.0 now if we are just noticing and starting the
> >> discussion.
> >
> >   Well given that we now use submodules at work, and that git is
> > nowadays somewhere in the top 5 of my most consciously (as opposed to
> > the compiler that I rarely call by hand) used software suites (among my
> > editor, my MUA, my shell and my tiling WM), I'm very much interested in
> > tackling some things about what is (not) done with submodules yet.
> 
> Surely the effort is appreciated.

  okay I'll try to work on this on the git wiki so that collaboration is
possible.

> 
> >> This comment goes to the issue Pierre raised last night as well.
> >
> >   You mean the git checkout issue?
> 
> Oh, no; that misuse of parse_opt() that forgot KEEP_DASHDASH one was not
> what I had in mind.  I meant to say that your "switch branches between an
> old pre-submodule rev and a new one that has a submodule at where a blob
> or directory used to be" issue with a good explanation material was a good
> starting point for submodule improvements for the next cycle.

  ohh that :)

> I'd like the release schedule not too heavily based on "per feature", but
> more time-based.

  Yeah, we've seen in the past how it makes a release slip. Though it'd
be great to say e.g. that we won't do a 1.7.0 release[0] until we have
an UI for submodules we are prood of. It doesn't mean that we won't have
a 1.6.21 because it's slow to get into shape ;)

  IOW I'm all for time based releases, with some big milestones that
when completed bump the git version significantly. And hinting on what
are those milestones would probably be quite nice. I mean git
developpement is kind of a hit and run thing: people have an issue, come
with patches, and go back to their lives (except for a mere 20 regular
contributors with more than 50 patches). Maybe we should hint some
direction we would like to see a bit more. examples of such big
directions are:
  * submodules UI ;
  * sparse checkouts ;
  * ...


  [0] I don't really *mean* we must do it for 1.7.0, It's merely an
      example.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH/RFC] git add: do not add files from a submodule

[Johannes Schindelin]

Hi,

On Wed, 23 Jul 2008, Pierre Habouzit wrote:

> Though it'd be great to say e.g. that we won't do a 1.7.0 release[0] 
> until we have an UI for submodules we are prood of.

Or until we have everything ported over to parse_options?

Ciao,
Dscho

Re: [PATCH/RFC] git add: do not add files from a submodule

[Pierre Habouzit]

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

On Wed, Jul 23, 2008 at 07:10:09PM +0000, Johannes Schindelin wrote:
> Hi,
> 
> On Wed, 23 Jul 2008, Pierre Habouzit wrote:
> 
> > Though it'd be great to say e.g. that we won't do a 1.7.0 release[0] 
> > until we have an UI for submodules we are prood of.
> 
> Or until we have everything ported over to parse_options?

  hahah, okay, fair enough, I deserve this one ;)

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]