Do "git add" as a builtin

Do "git add" as a builtin

[Linus Torvalds]

First try. Let's see how well this works.

In many ways, the hard parts of "git commit" are not so different from
this, and a builtin commit would share a lot of the code, I think.

Signed-off-by: Linus Torvalds <torvalds@osdl.org>
---

It passes the tests for me. I dropped the strange "--error-unmatch" test, 
because it was really ugly and I didn't see the point. It seems to do all 
the right things, but hey, a mistake here is obviously a bad thing.

I suspect that if I get around to "git commit", we'd be getting to the 
point where most of the core/easy/often-used/whatever commands would be 
all built-in.

diff --git a/Makefile b/Makefile
index f43ac63..e6f7794 100644
--- a/Makefile
+++ b/Makefile
@@ -218,7 +218,7 @@ LIB_OBJS = \
 
 BUILTIN_OBJS = \
 	builtin-log.o builtin-help.o builtin-count.o builtin-diff.o builtin-push.o \
-	builtin-grep.o
+	builtin-grep.o builtin-add.o
 
 GITLIBS = $(LIB_FILE) $(XDIFF_LIB)
 LIBS = $(GITLIBS) -lz
diff --git a/builtin-add.c b/builtin-add.c
new file mode 100644
index 0000000..e815b3d
--- /dev/null
+++ b/builtin-add.c
@@ -0,0 +1,228 @@
+/*
+ * "git add" builtin command
+ *
+ * Copyright (C) 2006 Linus Torvalds
+ */
+#include <fnmatch.h>
+
+#include "cache.h"
+#include "builtin.h"
+#include "dir.h"
+
+static const char builtin_add_usage[] =
+"git-add [-n] [-v] <filepattern>...";
+
+static int common_prefix(const char **pathspec)
+{
+	const char *path, *slash, *next;
+	int prefix;
+
+	if (!pathspec)
+		return 0;
+
+	path = *pathspec;
+	slash = strrchr(path, '/');
+	if (!slash)
+		return 0;
+
+	prefix = slash - path + 1;
+	while ((next = *++pathspec) != NULL) {
+		int len = strlen(next);
+		if (len >= prefix && !memcmp(path, next, len))
+			continue;
+		for (;;) {
+			if (!len)
+				return 0;
+			if (next[--len] != '/') 
+				continue;
+			if (memcmp(path, next, len+1))
+				continue;
+			prefix = len + 1;
+			break;
+		}
+	}
+	return prefix;
+}
+
+static int match(const char **pathspec, const char *name, int namelen, int prefix)
+{
+	const char *match;
+
+	name += prefix;
+	namelen -= prefix;
+
+	while ((match = *pathspec++) != NULL) {
+		int matchlen;
+
+		match += prefix;
+		matchlen = strlen(match);
+		if (!matchlen)
+			return 1;
+		if (!strncmp(match, name, matchlen)) {
+			if (match[matchlen-1] == '/')
+				return 1;
+			switch (name[matchlen]) {
+			case '/': case '\0':
+				return 1;
+			}
+		}
+		if (!fnmatch(match, name, 0))
+			return 1;
+	}
+	return 0;
+}
+
+static void prune_directory(struct dir_struct *dir, const char **pathspec, int prefix)
+{
+	int i;
+	struct dir_entry **src, **dst;
+
+	src = dst = dir->entries;
+	i = dir->nr;
+	while (--i >= 0) {
+		struct dir_entry *entry = *src++;
+		if (!match(pathspec, entry->name, entry->len, prefix)) {
+			free(entry);
+			continue;
+		}
+		*dst++ = entry;
+	}
+	dir->nr = dst - dir->entries;
+}
+
+static void fill_directory(struct dir_struct *dir, const char **pathspec)
+{
+	const char *path, *base;
+	int baselen;
+
+	/* Set up the default git porcelain excludes */
+	memset(dir, 0, sizeof(*dir));
+	dir->exclude_per_dir = ".gitignore";
+	path = git_path("info/exclude");
+	if (!access(path, R_OK))
+		add_excludes_from_file(dir, path);
+
+	/*
+	 * Calculate common prefix for the pathspec, and
+	 * use that to optimize the directory walk
+	 */
+	baselen = common_prefix(pathspec);
+	path = ".";
+	base = "";
+	if (baselen) {
+		char *common = xmalloc(baselen + 1);
+		common = xmalloc(baselen + 1);
+		memcpy(common, *pathspec, baselen);
+		common[baselen] = 0;
+		path = base = common;
+	}
+
+	/* Read the directory and prune it */
+	read_directory(dir, path, base, baselen);
+	if (pathspec)
+		prune_directory(dir, pathspec, baselen);
+}
+
+static int add_file_to_index(const char *path, int verbose)
+{
+	int size, namelen;
+	struct stat st;
+	struct cache_entry *ce;
+
+	if (lstat(path, &st))
+		die("%s: unable to stat (%s)", path, strerror(errno));
+
+	if (!S_ISREG(st.st_mode) && !S_ISLNK(st.st_mode))
+		die("%s: can only add regular files or symbolic links", path);
+
+	namelen = strlen(path);
+	size = cache_entry_size(namelen);
+	ce = xcalloc(1, size);
+	memcpy(ce->name, path, namelen);
+	ce->ce_flags = htons(namelen);
+	fill_stat_cache_info(ce, &st);
+
+	ce->ce_mode = create_ce_mode(st.st_mode);
+	if (!trust_executable_bit) {
+		/* If there is an existing entry, pick the mode bits
+		 * from it.
+		 */
+		int pos = cache_name_pos(path, namelen);
+		if (pos >= 0)
+			ce->ce_mode = active_cache[pos]->ce_mode;
+	}
+
+	if (index_path(ce->sha1, path, &st, 1))
+		die("unable to index file %s", path);
+	if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD))
+		die("unable to add %s to index",path);
+	if (verbose)
+		printf("add '%s'\n", path);
+	return 0;
+}
+
+static struct cache_file cache_file;
+
+int cmd_add(int argc, const char **argv, char **envp)
+{
+	int i, newfd;
+	int verbose = 0, show_only = 0;
+	const char *prefix = setup_git_directory();
+	const char **pathspec;
+	struct dir_struct dir;
+
+	git_config(git_default_config);
+
+	newfd = hold_index_file_for_update(&cache_file, get_index_file());
+	if (newfd < 0)
+		die("unable to create new cachefile");
+
+	if (read_cache() < 0)
+		die("index file corrupt");
+
+	for (i = 1; i < argc; i++) {
+		const char *arg = argv[i];
+
+		if (arg[0] != '-')
+			break;
+		if (!strcmp(arg, "--")) {
+			i++;
+			break;
+		}
+		if (!strcmp(arg, "-n")) {
+			show_only = 1;
+			continue;
+		}
+		if (!strcmp(arg, "-v")) {
+			verbose = 1;
+			continue;
+		}
+		die(builtin_add_usage);
+	}
+	git_config(git_default_config);
+	pathspec = get_pathspec(prefix, argv + i);
+
+	fill_directory(&dir, pathspec);
+
+	if (show_only) {
+		const char *sep = "", *eof = "";
+		for (i = 0; i < dir.nr; i++) {
+			printf("%s%s", sep, dir.entries[i]->name);
+			sep = " ";
+			eof = "\n";
+		}
+		fputs(eof, stdout);
+		return 0;
+	}
+
+	for (i = 0; i < dir.nr; i++)
+		add_file_to_index(dir.entries[i]->name, verbose);
+
+	if (active_cache_changed) {
+		if (write_cache(newfd, active_cache, active_nr) ||
+		    commit_index_file(&cache_file))
+			die("Unable to write new index file");
+	}
+
+	return 0;
+}
diff --git a/builtin.h b/builtin.h
index 7744f7d..1b77f4b 100644
--- a/builtin.h
+++ b/builtin.h
@@ -24,5 +24,6 @@ extern int cmd_count_objects(int argc, c
 
 extern int cmd_push(int argc, const char **argv, char **envp);
 extern int cmd_grep(int argc, const char **argv, char **envp);
+extern int cmd_add(int argc, const char **argv, char **envp);
 
 #endif
diff --git a/git.c b/git.c
index a94d9ee..fac46af 100644
--- a/git.c
+++ b/git.c
@@ -50,6 +50,7 @@ static void handle_internal_command(int 
 		{ "count-objects", cmd_count_objects },
 		{ "diff", cmd_diff },
 		{ "grep", cmd_grep },
+		{ "add", cmd_add },
 	};
 	int i;
 

Re: Do "git add" as a builtin

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> ... I dropped the strange "--error-unmatch" test, because it
> was really ugly and I didn't see the point.

By "not seeing the point", do you mean you do not agree with
what bba319b5 and 45e48120 tried to do to help users?

        $ git add no-such-path-in-the-tree
        $ git add 'this-pattern-matches-nobody*'

More realistically:

	$ git add Documentaiton		;# misspelled


[References]

(bba319b5)
When you say "git commit Documentaiton" to make partial commit
for the files only in that directory, we did not detect that as
a misspelled pathname and attempted to commit index without
change.  If nothing matched, there is no harm done, but if the
index gets modified otherwise by having another valid pathspec
or after an explicit update-index, a user will not notice
without paying attention to the "git status" preview.

This introduces --error-unmatch option to ls-files, and uses it
to detect this common user error.

(45e48120)
This is in the same spirit as an earlier patch for git-commit.
It does an extra ls-files to avoid complaining when a fully
tracked directory name is given on the command line (otherwise
--others restriction would say the pathspec does not match).

Re: Do "git add" as a builtin

[Linus Torvalds]

On Wed, 17 May 2006, Junio C Hamano wrote:
> 
> By "not seeing the point", do you mean you do not agree with
> what bba319b5 and 45e48120 tried to do to help users?

Naah, I just didn't see why, and didn't bother to go exploring.

How about this patch on top of the previous one?

		Linus

----
diff --git a/builtin-add.c b/builtin-add.c
index e815b3d..82e8f44 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -44,50 +44,89 @@ static int common_prefix(const char **pa
 	return prefix;
 }
 
-static int match(const char **pathspec, const char *name, int namelen, int prefix)
+static int match_one(const char *match, const char *name, int namelen)
 {
+	int matchlen;
+
+	/* If the match was just the prefix, we matched */
+	matchlen = strlen(match);
+	if (!matchlen)
+		return 1;
+
+	/*
+	 * If we don't match the matchstring exactly,
+	 * we need to match by fnmatch
+	 */
+	if (strncmp(match, name, matchlen))
+		return !fnmatch(match, name, 0);
+
+	/*
+	 * If we did match the string exactly, we still
+	 * need to make sure that it happened on a path
+	 * component boundary (ie either the last character
+	 * of the match was '/', or the next character of
+	 * the name was '/' or the terminating NUL.
+	 */
+	return	match[matchlen-1] == '/' ||
+		name[matchlen] == '/' ||
+		!name[matchlen];
+}
+
+static int match(const char **pathspec, const char *name, int namelen, int prefix, char *seen)
+{
+	int retval;
 	const char *match;
 
 	name += prefix;
 	namelen -= prefix;
 
-	while ((match = *pathspec++) != NULL) {
-		int matchlen;
-
+	for (retval = 0; (match = *pathspec++) != NULL; seen++) {
+		if (retval & *seen)
+			continue;
 		match += prefix;
-		matchlen = strlen(match);
-		if (!matchlen)
-			return 1;
-		if (!strncmp(match, name, matchlen)) {
-			if (match[matchlen-1] == '/')
-				return 1;
-			switch (name[matchlen]) {
-			case '/': case '\0':
-				return 1;
-			}
+		if (match_one(match, name, namelen)) {
+			retval = 1;
+			*seen = 1;
 		}
-		if (!fnmatch(match, name, 0))
-			return 1;
 	}
-	return 0;
+	return retval;
 }
 
 static void prune_directory(struct dir_struct *dir, const char **pathspec, int prefix)
 {
-	int i;
+	char *seen;
+	int i, specs;
 	struct dir_entry **src, **dst;
 
+	for (specs = 0; pathspec[specs];  specs++)
+		/* nothing */;
+	seen = xmalloc(specs);
+	memset(seen, 0, specs);
+
 	src = dst = dir->entries;
 	i = dir->nr;
 	while (--i >= 0) {
 		struct dir_entry *entry = *src++;
-		if (!match(pathspec, entry->name, entry->len, prefix)) {
+		if (!match(pathspec, entry->name, entry->len, prefix, seen)) {
 			free(entry);
 			continue;
 		}
 		*dst++ = entry;
 	}
 	dir->nr = dst - dir->entries;
+
+	for (i = 0; i < specs; i++) {
+		struct stat st;
+		const char *match;
+		if (seen[i])
+			continue;
+
+		/* Existing file? We must have ignored it */
+		match = pathspec[i];
+		if (!lstat(match, &st))
+			continue;
+		die("pathspec '%s' did not match any files", match);
+	}
 }
 
 static void fill_directory(struct dir_struct *dir, const char **pathspec)

Re: Do "git add" as a builtin

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> On Wed, 17 May 2006, Junio C Hamano wrote:
>> 
>> By "not seeing the point", do you mean you do not agree with
>> what bba319b5 and 45e48120 tried to do to help users?
>
> Naah, I just didn't see why, and didn't bother to go exploring.
>
> How about this patch on top of the previous one?

Well, not good as-is.  This makes it barf on this sequence:

	$ rm -f junk
        $ cd junk
        $ git init-db
        $ date >frotz
        $ mkdir nitfol
        $ date >nitfol/rezrov
	$ git add .		;# OK up to this point - added everything.

	$ git add .		;# This is bogus because...
        fatal: pathspec '' did not match any files
	$ git add nitfol	;# ...this does not barf.

I admit I did not spot it when I read the code, but this part
gets an empty string for 'match' when pathspec is '.'.

> +		/* Existing file? We must have ignored it */
> +		match = pathspec[i];
> +		if (!lstat(match, &st))
> +			continue;
> +		die("pathspec '%s' did not match any files", match);

That's why '.' barfs but nitfol doesn't.

Re: Do "git add" as a builtin

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> Linus Torvalds <torvalds@osdl.org> writes:
>
>> On Wed, 17 May 2006, Junio C Hamano wrote:
>>> 
>>> By "not seeing the point", do you mean you do not agree with
>>> what bba319b5 and 45e48120 tried to do to help users?
>>
>> Naah, I just didn't see why, and didn't bother to go exploring.
>>
>> How about this patch on top of the previous one?
>
> Well, not good as-is.  This makes it barf on this sequence:
>...

Ouch, things are worse than I thought...

	$ mkdir foo
        $ date >bar
        $ git-add foo/../bar
	$ git ls-files
        foo/../bar

Huh?

[WARNING] Please be careful when using "git add" from "next" branch

[Junio C Hamano]

There is still a small breakage in the built-in "git add" on the
"next" branch that ends up creating nonsense tree objects.

        $ mkdir foo
        $ date >bar
        $ git-add foo/../bar
        $ git ls-files
        foo/../bar
        $ git ls-tree -r -t $(git-write-tree)
        040000 tree ef5562cd3a9bf66d41a8d4f42f159e8c694ce7e3	foo
        040000 tree 6e1612248e8da43fc5f91592e559da1ad5f9a852	foo/..
        100644 blob 53ab446c3f4e42ce9bb728a0ccb283a101be4979	foo/../bar

If you do not do funky things like foo/../bar, I do not think
you have to worry, but scripted use might break.  It used to
warn and ignore such bogus input, but now it happily accepts it
and produces bogus index which results in bogus trees.

"git update-index --add" is not affected by this breakage.

Re: Do "git add" as a builtin

[Linus Torvalds]

On Thu, 18 May 2006, Junio C Hamano wrote:
> 
> Well, not good as-is.  This makes it barf on this sequence:
> 
> 	$ rm -f junk
>         $ cd junk
>         $ git init-db
>         $ date >frotz
>         $ mkdir nitfol
>         $ date >nitfol/rezrov
> 	$ git add .		;# OK up to this point - added everything.
> 
> 	$ git add .		;# This is bogus because...
>         fatal: pathspec '' did not match any files

Ahh. The empty pathspec is special.

It's special for a totally stupid reason: it's not a valid pathname, but 
we obviously _turn_ it into a valid pathname when we read the directory 
tree (ie from a filesystem standpoint, it means ".").

So then, when we do the "lstat()", we really _should_ have done the same.

If course, since the lstat() is there just to test for existence, and 
since "." always exists, it's easier to just pass an empty match entirely 
than to turn it into "." and then do an lstat that we know is going to 
succeed.

So the patch would be something trivial like this..

		Linus

---
diff --git a/builtin-add.c b/builtin-add.c
index 7083820..ac9ed2d 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -124,7 +124,7 @@ static void prune_directory(struct dir_s
 
 		/* Existing file? We must have ignored it */
 		match = pathspec[i];
-		if (!lstat(match, &st))
+		if (!*match || !lstat(match, &st))
 			continue;
 		die("pathspec '%s' did not match any files", match);
 	}

Re: Do "git add" as a builtin

[Linus Torvalds]

On Thu, 18 May 2006, Junio C Hamano wrote:
> 
> Ouch, things are worse than I thought...
> 
> 	$ mkdir foo
>         $ date >bar
>         $ git-add foo/../bar
> 	$ git ls-files
>         foo/../bar
> 
> Huh?

"git-update-index" does a "verify_path()" which I missed, so the new 
builtin add doesn't do it.

We could do it either in the "add_cache_entry()" function (which would be 
safest, because then by _definition_ you couldn't make this mistake 
again), or we could do it in the caller.

This patch does it in the caller, just to match the old "git add" (which 
would just say "Ignoring path ..." and not do anything).

But if people are ok with changing it from a "print a warning and ignore" 
into an _error_, we could just move it into "add_cache_entry()".

The real _meat_ of this patch is really just that first hunk to 
"builtin-add.c" - all the rest is just making that "verify_path()" 
function available in the git library.

		Linus
---
diff --git a/builtin-add.c b/builtin-add.c
index 7083820..0346bb5 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -172,6 +172,11 @@ static int add_file_to_index(const char 
 	if (lstat(path, &st))
 		die("%s: unable to stat (%s)", path, strerror(errno));
 
+	if (!verify_path(path)) {
+		fprintf(stderr, "Ignoring path %s\n", path);
+		return -1;
+	}
+		
 	if (!S_ISREG(st.st_mode) && !S_ISLNK(st.st_mode))
 		die("%s: can only add regular files or symbolic links", path);
 
diff --git a/cache.h b/cache.h
index b1300cd..f9b7049 100644
--- a/cache.h
+++ b/cache.h
@@ -143,6 +143,7 @@ #define alloc_nr(x) (((x)+16)*3/2)
 /* Initialize and use the cache information */
 extern int read_cache(void);
 extern int write_cache(int newfd, struct cache_entry **cache, int entries);
+extern int verify_path(const char *path);
 extern int cache_name_pos(const char *name, int namelen);
 #define ADD_CACHE_OK_TO_ADD 1		/* Ok to add */
 #define ADD_CACHE_OK_TO_REPLACE 2	/* Ok to replace file/directory */
diff --git a/read-cache.c b/read-cache.c
index ed0da38..6b323dd 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -347,6 +347,70 @@ int ce_path_match(const struct cache_ent
 }
 
 /*
+ * We fundamentally don't like some paths: we don't want
+ * dot or dot-dot anywhere, and for obvious reasons don't
+ * want to recurse into ".git" either.
+ *
+ * Also, we don't want double slashes or slashes at the
+ * end that can make pathnames ambiguous.
+ */
+static int verify_dotfile(const char *rest)
+{
+	/*
+	 * The first character was '.', but that
+	 * has already been discarded, we now test
+	 * the rest.
+	 */
+	switch (*rest) {
+	/* "." is not allowed */
+	case '\0': case '/':
+		return 0;
+
+	/*
+	 * ".git" followed by  NUL or slash is bad. This
+	 * shares the path end test with the ".." case.
+	 */
+	case 'g':
+		if (rest[1] != 'i')
+			break;
+		if (rest[2] != 't')
+			break;
+		rest += 2;
+	/* fallthrough */
+	case '.':
+		if (rest[1] == '\0' || rest[1] == '/')
+			return 0;
+	}
+	return 1;
+}
+
+int verify_path(const char *path)
+{
+	char c;
+
+	goto inside;
+	for (;;) {
+		if (!c)
+			return 1;
+		if (c == '/') {
+inside:
+			c = *path++;
+			switch (c) {
+			default:
+				continue;
+			case '/': case '\0':
+				break;
+			case '.':
+				if (verify_dotfile(path))
+					continue;
+			}
+			return 0;
+		}
+		c = *path++;
+	}
+}
+
+/*
  * Do we have another file that has the beginning components being a
  * proper superset of the name we're trying to add?
  */
diff --git a/update-index.c b/update-index.c
index f6b09a4..69b9a71 100644
--- a/update-index.c
+++ b/update-index.c
@@ -8,6 +8,7 @@ #include "strbuf.h"
 #include "quote.h"
 #include "cache-tree.h"
 #include "tree-walk.h"
+#include "dir.h"
 
 /*
  * Default to not allowing changes to the list of files. The
@@ -245,70 +246,6 @@ static int refresh_cache(int really)
 	return has_errors;
 }
 
-/*
- * We fundamentally don't like some paths: we don't want
- * dot or dot-dot anywhere, and for obvious reasons don't
- * want to recurse into ".git" either.
- *
- * Also, we don't want double slashes or slashes at the
- * end that can make pathnames ambiguous.
- */
-static int verify_dotfile(const char *rest)
-{
-	/*
-	 * The first character was '.', but that
-	 * has already been discarded, we now test
-	 * the rest.
-	 */
-	switch (*rest) {
-	/* "." is not allowed */
-	case '\0': case '/':
-		return 0;
-
-	/*
-	 * ".git" followed by  NUL or slash is bad. This
-	 * shares the path end test with the ".." case.
-	 */
-	case 'g':
-		if (rest[1] != 'i')
-			break;
-		if (rest[2] != 't')
-			break;
-		rest += 2;
-	/* fallthrough */
-	case '.':
-		if (rest[1] == '\0' || rest[1] == '/')
-			return 0;
-	}
-	return 1;
-}
-
-static int verify_path(const char *path)
-{
-	char c;
-
-	goto inside;
-	for (;;) {
-		if (!c)
-			return 1;
-		if (c == '/') {
-inside:
-			c = *path++;
-			switch (c) {
-			default:
-				continue;
-			case '/': case '\0':
-				break;
-			case '.':
-				if (verify_dotfile(path))
-					continue;
-			}
-			return 0;
-		}
-		c = *path++;
-	}
-}
-
 static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,
 			 const char *path, int stage)
 {

Re: Do "git add" as a builtin

[Linus Torvalds]

On Thu, 18 May 2006, Linus Torvalds wrote:
> 
> But if people are ok with changing it from a "print a warning and ignore" 
> into an _error_, we could just move it into "add_cache_entry()".

This is the incremental patch to do that instead, if you prefer it.

Putting it into add_cache_entry() definitely has advantages, in that it 
should make it much harder for this bug to happen again - all users will 
now be verified.

With this one, it's now a fatal error to try to add a pathname that cannot 
be added, ie

	[torvalds@g5 git]$ git add .git/config 
	fatal: unable to add .git/config to index

and

	[torvalds@g5 git]$ git add foo/../bar 
	fatal: unable to add foo/../bar to index

instead of the old "Ignoring path xyz" warning that would end up silently 
succeeding on any other paths.

		Linus

---
diff --git a/builtin-add.c b/builtin-add.c
index 0346bb5..ac9ed2d 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -172,11 +172,6 @@ static int add_file_to_index(const char 
 	if (lstat(path, &st))
 		die("%s: unable to stat (%s)", path, strerror(errno));
 
-	if (!verify_path(path)) {
-		fprintf(stderr, "Ignoring path %s\n", path);
-		return -1;
-	}
-		
 	if (!S_ISREG(st.st_mode) && !S_ISLNK(st.st_mode))
 		die("%s: can only add regular files or symbolic links", path);
 
diff --git a/read-cache.c b/read-cache.c
index 6b323dd..9a272f8 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -551,6 +551,8 @@ int add_cache_entry(struct cache_entry *
 
 	if (!ok_to_add)
 		return -1;
+	if (!verify_path(ce->name))
+		return -1;
 
 	if (!skip_df_check &&
 	    check_file_directory_conflict(ce, pos, ok_to_replace)) {

Re: Do "git add" as a builtin

[Linus Torvalds]

On Thu, 18 May 2006, Linus Torvalds wrote:
> > 
> > But if people are ok with changing it from a "print a warning and ignore" 
> > into an _error_, we could just move it into "add_cache_entry()".
> 
> This is the incremental patch to do that instead, if you prefer it.

Thinking some more about it, I think I personally much prefer this.

Especially as a quick look-through seems to say that there's actually a 
path through git-update-index too that allows a unverified filename to get 
into the index (the new "--unresolve" thing also misses the need to verify 
the path).

Making it a fatal error makes it more obvious that the user did something 
fundamentally wrong. And the safety in doing it in add_cache_entry() just 
makes be happier, particularly in light of the above problem with 
--unresolve.

That still leaves the question of whether we should also drop all the 
"verify_path()" calls in update-index.c, and make it fatal there too. I 
think we probably should.

(At that point we could turn verify_path() back into a static function, 
this time local entirely to read-cache.c, rather than update-index.c)

			Linus

Re: Do "git add" as a builtin

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> Thinking some more about it, I think I personally much prefer this.

I like this much better than the old (pre-builtin) behaviour.

> Especially as a quick look-through seems to say that there's actually a 
> path through git-update-index too that allows a unverified filename to get 
> into the index (the new "--unresolve" thing also misses the need to verify 
> the path).

Perhaps, but unresolve splits an entry that is available from
the heads being merged, so it would use unverified filename to
try finding the ents from trees, but get_tree_entry() would not
find one, so I think we are safe already.  Nevertheless, I think
your change makes things more strict and safer.

I doubt this would break people's scripts.  If they were relying
on the old behaviour, that means they threw real paths and
garbage at update-index and relied on it to sift them apart,
which indicates they were buggy to begin with anyway.

Re: Do "git add" as a builtin

[Linus Torvalds]

On Thu, 18 May 2006, Junio C Hamano wrote:
> 
> Perhaps, but unresolve splits an entry that is available from
> the heads being merged, so it would use unverified filename to
> try finding the ents from trees, but get_tree_entry() would not
> find one, so I think we are safe already.

Yes, I agree, that looks safe.

> I doubt this would break people's scripts.  If they were relying
> on the old behaviour, that means they threw real paths and
> garbage at update-index and relied on it to sift them apart,
> which indicates they were buggy to begin with anyway.

Agreed. I don't think the semantic change can really break anything, and 
if anything will probably help expose any really strange/broken stuff.

		Linus

FIXED: [WARNING] Please be careful when using "git add" from "next" branch

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> There is still a small breakage in the built-in "git add" on the
> "next" branch that ends up creating nonsense tree objects.

This was fixed this morning, in case I scared people.