# Adding empty directory gives bogus error message 16 messages from 2007-06-10 to 2007-06-12. Participants: Jonas Fonseca, Jeff King, Johannes Schindelin, Junio C Hamano. Thread: https://gitlist.dev/t/8542 ## Jonas Fonseca, 2007-06-10 20:46 Subject: Adding empty directory gives bogus error message Message-ID: <20070610204648.GA32214@diku.dk> URL: https://gitlist.dev/e/20070610204648.GA32214%40diku.dk ``` Hello, During a talk with madduck on #git today, we stumbled upon this confusing error message: $ mkdir repo $ cd repo/ $ git init Initialized empty Git repository in .git/ $ mkdir empty $ git add empty/ The following paths are ignored by one of your .gitignore files: empty/ (directory) Use -f if you really want to add them. $ git add -f empty/ fatal: unable to index file empty/ First, it misleads the user by assuming that something was added to .gitignore or another exclude file, and the second error message is not very helpful. Looking at the code, I am not sure how this can be fixed. Simply printing a warning when finding "some" empty directory might do, but it is not the best way to help somebody new to git. Refusing to add anything when finding an empty directory deep in some hierarchy is not a good option either. Git 1.4 silently did nothing, so 1.5 at least tries to be more clueful. Anyway, I thought I would mention it and see what happens. ;) -- Jonas Fonseca ``` ## Jeff King, 2007-06-11 12:30 Subject: Re: Adding empty directory gives bogus error message Message-ID: <20070611123045.GA28814@coredump.intra.peff.net> URL: https://gitlist.dev/e/20070611123045.GA28814%40coredump.intra.peff.net In-Reply-To: <20070610204648.GA32214@diku.dk> ``` On Sun, Jun 10, 2007 at 10:46:48PM +0200, Jonas Fonseca wrote: > During a talk with madduck on #git today, we stumbled upon this > confusing error message: > > $ mkdir repo > $ cd repo/ > $ git init > Initialized empty Git repository in .git/ > $ mkdir empty > $ git add empty/ > The following paths are ignored by one of your .gitignore files: > empty/ (directory) > Use -f if you really want to add them. > $ git add -f empty/ > fatal: unable to index file empty/ Urgh, that's ugly. The problem is that git-add sticks assumed-to-be-ignored stuff back into the list of files found by read_directory, but with some special ignored flags (which aren't used anywhere else!). When the assumption is wrong (because the path is _actually_ just empty), you get the bogus message, and when you try to force it, you get an error from elsewhere in the code. Patch series will be out momentarily. -Peff ``` ## Jeff King, 2007-06-11 13:39 Subject: [PATCH 1/3] refactor dir_add_name Message-ID: <20070611133944.GA7008@coredump.intra.peff.net> URL: https://gitlist.dev/e/20070611133944.GA7008%40coredump.intra.peff.net In-Reply-To: <20070611123045.GA28814@coredump.intra.peff.net> ``` This is in preparation for keeping two entry lists in the dir object. This patch adds and uses the alloc_grow macro, which implements the commonly used idiom of growing a dynamic array using the alloc_nr function (not just in dir.c, but everywhere). We also move creation of a dir_entry to dir_entry_new. Signed-off-by: Jeff King --- If we like the alloc_grow approach, there are a lot of places where we can drop a 3-5 line conditional into a single line. I find it much more readable, but others may disagree. cache.h | 6 ++++++ dir.c | 23 +++++++++++------------ 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/cache.h b/cache.h index 5e7381e..f771519 100644 --- a/cache.h +++ b/cache.h @@ -224,6 +224,12 @@ extern void verify_filename(const char *prefix, const char *name); extern void verify_non_filename(const char *prefix, const char *name); #define alloc_nr(x) (((x)+16)*3/2) +#define alloc_grow(x, nr, alloc) do { \ + if(nr >= alloc) { \ + alloc = alloc_nr(alloc); \ + x = xrealloc((x), alloc * sizeof(*(x))); \ + } \ +} while(0) /* Initialize and use the cache information */ extern int read_index(struct index_state *); diff --git a/dir.c b/dir.c index f543f50..e810258 100644 --- a/dir.c +++ b/dir.c @@ -271,27 +271,26 @@ int excluded(struct dir_struct *dir, const char *pathname) return 0; } -struct dir_entry *dir_add_name(struct dir_struct *dir, const char *pathname, int len) -{ +static +struct dir_entry *dir_entry_new(const char *pathname, int len) { struct dir_entry *ent; - - if (cache_name_pos(pathname, len) >= 0) - return NULL; - - if (dir->nr == dir->alloc) { - int alloc = alloc_nr(dir->alloc); - dir->alloc = alloc; - dir->entries = xrealloc(dir->entries, alloc*sizeof(ent)); - } ent = xmalloc(sizeof(*ent) + len + 1); ent->ignored = ent->ignored_dir = 0; ent->len = len; memcpy(ent->name, pathname, len); ent->name[len] = 0; - dir->entries[dir->nr++] = ent; return ent; } +struct dir_entry *dir_add_name(struct dir_struct *dir, const char *pathname, int len) +{ + if (cache_name_pos(pathname, len) >= 0) + return NULL; + + alloc_grow(dir->entries, dir->nr, dir->alloc); + return dir->entries[dir->nr++] = dir_entry_new(pathname, len); +} + enum exist_status { index_nonexistent = 0, index_directory, -- 1.5.2.1.958.gbaa74-dirty ``` ## Jeff King, 2007-06-11 13:39 Subject: [PATCH 2/3] dir_struct: add collect_ignored option Message-ID: <20070611133950.GB7008@coredump.intra.peff.net> URL: https://gitlist.dev/e/20070611133950.GB7008%40coredump.intra.peff.net In-Reply-To: <20070611123045.GA28814@coredump.intra.peff.net> ``` When set, this option will cause read_directory to keep track of which entries were ignored. While this shouldn't effect functionality in most cases, it can make warning messages to the user much more useful. Signed-off-by: Jeff King --- dir.c | 12 ++++++++++++ dir.h | 5 ++++- 2 files changed, 16 insertions(+), 1 deletions(-) diff --git a/dir.c b/dir.c index e810258..1ffc1e5 100644 --- a/dir.c +++ b/dir.c @@ -291,6 +291,15 @@ struct dir_entry *dir_add_name(struct dir_struct *dir, const char *pathname, int return dir->entries[dir->nr++] = dir_entry_new(pathname, len); } +struct dir_entry *dir_add_ignored(struct dir_struct *dir, const char *pathname, int len) +{ + if(cache_name_pos(pathname, len) >= 0) + return NULL; + + alloc_grow(dir->ignored, dir->ignored_nr, dir->ignored_alloc); + return dir->ignored[dir->ignored_nr++] = dir_entry_new(pathname, len); +} + enum exist_status { index_nonexistent = 0, index_directory, @@ -463,6 +472,8 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co continue; exclude = excluded(dir, fullname); + if (exclude && dir->collect_ignored) + dir_add_ignored(dir, fullname, baselen + len); if (exclude != dir->show_ignored) { if (!dir->show_ignored || DTYPE(de) != DT_DIR) { continue; @@ -609,6 +620,7 @@ int read_directory(struct dir_struct *dir, const char *path, const char *base, i read_directory_recursive(dir, path, base, baselen, 0, simplify); free_simplify(simplify); qsort(dir->entries, dir->nr, sizeof(struct dir_entry *), cmp_name); + qsort(dir->ignored, dir->ignored_nr, sizeof(*dir->ignored), cmp_name); return dir->nr; } diff --git a/dir.h b/dir.h index 172147f..c94f3cb 100644 --- a/dir.h +++ b/dir.h @@ -31,11 +31,14 @@ struct exclude_list { struct dir_struct { int nr, alloc; + int ignored_nr, ignored_alloc; unsigned int show_ignored:1, show_other_directories:1, hide_empty_directories:1, - no_gitlinks:1; + no_gitlinks:1, + collect_ignored:1; struct dir_entry **entries; + struct dir_entry **ignored; /* Exclude info */ const char *exclude_per_dir; -- 1.5.2.1.958.gbaa74-dirty ``` ## Jeff King, 2007-06-11 13:39 Subject: [PATCH 3/3] builtin-add: simplify (and increase accuracy of) exclude handling Message-ID: <20070611133956.GC7008@coredump.intra.peff.net> URL: https://gitlist.dev/e/20070611133956.GC7008%40coredump.intra.peff.net In-Reply-To: <20070611123045.GA28814@coredump.intra.peff.net> ``` Previously, the code would always set up the excludes, and then manually pick through the pathspec we were given, assuming that non-added but existing paths were just ignored. This was mostly correct, but would erroneously mark a totally empty directory as 'ignored'. Instead, we now use the collect_ignored option of dir_struct, which unambiguously tells us whether a path was ignored. This simplifies the code, and means empty directories are now just not mentioned at all. Furthermore, we now conditionally ask dir_struct to respect excludes, depending on whether the '-f' flag has been set. This means we don't have to pick through the result, checking for an 'ignored' flag; ignored entries were either added or not in the first place. We can safely get rid of the special 'ignored' flags to dir_entry, which were not used anywhere else. Signed-off-by: Jeff King --- builtin-add.c | 89 ++++++++++++++++++++++++++++++-------------------------- dir.c | 1 - dir.h | 4 +-- 3 files changed, 49 insertions(+), 45 deletions(-) diff --git a/builtin-add.c b/builtin-add.c index 1591171..8988930 100644 --- a/builtin-add.c +++ b/builtin-add.c @@ -19,6 +19,23 @@ static const char builtin_add_usage[] = static int take_worktree_changes; static const char *excludes_file; +static int in_pathspec(const char *k, const char **pathspec) { + while(*pathspec) { + if (!strcmp(k, *pathspec)) + return 1; + pathspec++; + } + return 0; +} + +static void prune_ignored(struct dir_struct *dir, const char **pathspec) { + int i; + for (i = 0; i < dir->ignored_nr; i++) { + if (!in_pathspec(dir->ignored[i]->name, pathspec)) + dir->ignored[i] = NULL; + } +} + static void prune_directory(struct dir_struct *dir, const char **pathspec, int prefix) { char *seen; @@ -40,42 +57,29 @@ static void prune_directory(struct dir_struct *dir, const char **pathspec, int p dir->nr = dst - dir->entries; for (i = 0; i < specs; i++) { - struct stat st; - const char *match; - if (seen[i]) - continue; - - match = pathspec[i]; - if (!match[0]) - continue; - - /* Existing file? We must have ignored it */ - if (!lstat(match, &st)) { - struct dir_entry *ent; - - ent = dir_add_name(dir, match, strlen(match)); - ent->ignored = 1; - if (S_ISDIR(st.st_mode)) - ent->ignored_dir = 1; - continue; - } - die("pathspec '%s' did not match any files", match); + if(!seen[i] && !file_exists(pathspec[i])) + die("pathspec '%s' did not match any files", + pathspec[i]); } } -static void fill_directory(struct dir_struct *dir, const char **pathspec) +static void fill_directory(struct dir_struct *dir, const char **pathspec, + int ignored_too) { 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); - if (!access(excludes_file, R_OK)) - add_excludes_from_file(dir, excludes_file); + if (!ignored_too) { + dir->collect_ignored = 1; + dir->exclude_per_dir = ".gitignore"; + path = git_path("info/exclude"); + if (!access(path, R_OK)) + add_excludes_from_file(dir, path); + if (!access(excludes_file, R_OK)) + add_excludes_from_file(dir, excludes_file); + } /* * Calculate common prefix for the pathspec, and @@ -93,8 +97,10 @@ static void fill_directory(struct dir_struct *dir, const char **pathspec) /* Read the directory and prune it */ read_directory(dir, path, base, baselen, pathspec); - if (pathspec) + if (pathspec) { + prune_ignored(dir, pathspec); prune_directory(dir, pathspec, baselen); + } } static void update_callback(struct diff_queue_struct *q, @@ -160,6 +166,7 @@ int cmd_add(int argc, const char **argv, const char *prefix) const char **pathspec; struct dir_struct dir; int add_interactive = 0; + int has_ignored; for (i = 1; i < argc; i++) { if (!strcmp("--interactive", argv[i]) || @@ -219,13 +226,11 @@ int cmd_add(int argc, const char **argv, const char *prefix) } pathspec = get_pathspec(prefix, argv + i); - fill_directory(&dir, pathspec); + fill_directory(&dir, pathspec, ignored_too); if (show_only) { const char *sep = "", *eof = ""; for (i = 0; i < dir.nr; i++) { - if (!ignored_too && dir.entries[i]->ignored) - continue; printf("%s%s", sep, dir.entries[i]->name); sep = " "; eof = "\n"; @@ -237,25 +242,19 @@ int cmd_add(int argc, const char **argv, const char *prefix) if (read_cache() < 0) die("index file corrupt"); - if (!ignored_too) { - int has_ignored = 0; - for (i = 0; i < dir.nr; i++) - if (dir.entries[i]->ignored) - has_ignored = 1; - if (has_ignored) { - fprintf(stderr, ignore_warning); - for (i = 0; i < dir.nr; i++) { - if (!dir.entries[i]->ignored) - continue; - fprintf(stderr, "%s", dir.entries[i]->name); - if (dir.entries[i]->ignored_dir) - fprintf(stderr, " (directory)"); - fputc('\n', stderr); - } - fprintf(stderr, - "Use -f if you really want to add them.\n"); - exit(1); + has_ignored = 0; + for (i = 0; i < dir.ignored_nr; i++) { + if (dir.ignored[i]) + has_ignored = 1; + } + if (has_ignored) { + fprintf(stderr, ignore_warning); + for (i = 0; i < dir.ignored_nr; i++) { + if (dir.ignored[i]) + fprintf(stderr, "%s\n", dir.ignored[i]->name); } + fprintf(stderr, "Use -f if you really want to add them.\n"); + exit(1); } for (i = 0; i < dir.nr; i++) diff --git a/dir.c b/dir.c index 1ffc1e5..d903a49 100644 --- a/dir.c +++ b/dir.c @@ -275,7 +275,6 @@ static struct dir_entry *dir_entry_new(const char *pathname, int len) { struct dir_entry *ent; ent = xmalloc(sizeof(*ent) + len + 1); - ent->ignored = ent->ignored_dir = 0; ent->len = len; memcpy(ent->name, pathname, len); ent->name[len] = 0; diff --git a/dir.h b/dir.h index c94f3cb..ec0e8ab 100644 --- a/dir.h +++ b/dir.h @@ -13,9 +13,7 @@ struct dir_entry { - unsigned int ignored : 1; - unsigned int ignored_dir : 1; - unsigned int len : 30; + unsigned int len; char name[FLEX_ARRAY]; /* more */ }; ``` ## Johannes Schindelin, 2007-06-11 14:13 Subject: Re: [PATCH 1/3] refactor dir_add_name Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0706111512580.4059%40racer.site In-Reply-To: <20070611133944.GA7008@coredump.intra.peff.net> ``` Hi, On Mon, 11 Jun 2007, Jeff King wrote: > If we like the alloc_grow approach, there are a lot of places where we > can drop a 3-5 line conditional into a single line. I find it much more > readable, but others may disagree. I like it! Ciao, Dscho ``` ## Jonas Fonseca, 2007-06-11 15:01 Subject: Re: [PATCH 3/3] builtin-add: simplify (and increase accuracy of) exclude handling Message-ID: <20070611150122.GA11020@diku.dk> URL: https://gitlist.dev/e/20070611150122.GA11020%40diku.dk In-Reply-To: <20070611133956.GC7008@coredump.intra.peff.net> ``` Hej Jeff, Thanks for looking into this and making these patches. :) Jeff King wrote Mon, Jun 11, 2007: > builtin-add.c | 89 ++++++++++++++++++++++++++++++-------------------------- > dir.c | 1 - > dir.h | 4 +-- > 3 files changed, 49 insertions(+), 45 deletions(-) > > diff --git a/builtin-add.c b/builtin-add.c > index 1591171..8988930 100644 > --- a/builtin-add.c > +++ b/builtin-add.c > @@ -160,6 +166,7 @@ int cmd_add(int argc, const char **argv, const char *prefix) > const char **pathspec; > struct dir_struct dir; > int add_interactive = 0; > + int has_ignored; > > for (i = 1; i < argc; i++) { > if (!strcmp("--interactive", argv[i]) || > @@ -237,25 +242,19 @@ int cmd_add(int argc, const char **argv, const char *prefix) > if (read_cache() < 0) > die("index file corrupt"); > > - if (!ignored_too) { > - int has_ignored = 0; > - for (i = 0; i < dir.nr; i++) > - if (dir.entries[i]->ignored) > - has_ignored = 1; > - if (has_ignored) { > - fprintf(stderr, ignore_warning); > - for (i = 0; i < dir.nr; i++) { > - if (!dir.entries[i]->ignored) > - continue; > - fprintf(stderr, "%s", dir.entries[i]->name); > - if (dir.entries[i]->ignored_dir) > - fprintf(stderr, " (directory)"); > - fputc('\n', stderr); > - } > - fprintf(stderr, > - "Use -f if you really want to add them.\n"); > - exit(1); > + has_ignored = 0; > + for (i = 0; i < dir.ignored_nr; i++) { > + if (dir.ignored[i]) > + has_ignored = 1; > + } > + if (has_ignored) { > + fprintf(stderr, ignore_warning); > + for (i = 0; i < dir.ignored_nr; i++) { > + if (dir.ignored[i]) > + fprintf(stderr, "%s\n", dir.ignored[i]->name); > } > + fprintf(stderr, "Use -f if you really want to add them.\n"); > + exit(1); > } > > for (i = 0; i < dir.nr; i++) I think you could even get rid of has_ignored with something like this. diff --git a/builtin-add.c b/builtin-add.c index 8988930..da6ab11 100644 --- a/builtin-add.c +++ b/builtin-add.c @@ -166,7 +166,6 @@ int cmd_add(int argc, const char **argv, const char *prefix) const char **pathspec; struct dir_struct dir; int add_interactive = 0; - int has_ignored; for (i = 1; i < argc; i++) { if (!strcmp("--interactive", argv[i]) || @@ -242,12 +241,7 @@ int cmd_add(int argc, const char **argv, const char *prefix) if (read_cache() < 0) die("index file corrupt"); - has_ignored = 0; - for (i = 0; i < dir.ignored_nr; i++) { - if (dir.ignored[i]) - has_ignored = 1; - } - if (has_ignored) { + if (dir.ignored_nr) { fprintf(stderr, ignore_warning); for (i = 0; i < dir.ignored_nr; i++) { if (dir.ignored[i]) -- Jonas Fonseca ``` ## Jeff King, 2007-06-11 15:54 Subject: Re: [PATCH 3/3] builtin-add: simplify (and increase accuracy of) exclude handling Message-ID: <20070611155425.GA9316@coredump.intra.peff.net> URL: https://gitlist.dev/e/20070611155425.GA9316%40coredump.intra.peff.net In-Reply-To: <20070611150122.GA11020@diku.dk> ``` On Mon, Jun 11, 2007 at 05:01:23PM +0200, Jonas Fonseca wrote: > Thanks for looking into this and making these patches. :) No problem. I had run it across it before and it was on my "look into this" list...thank you for providing a test case. :) > I think you could even get rid of has_ignored with something like this. Nope, I had originally wanted to do that, but the dir_struct.ignored list contains _all_ ignored items, not just those that were originally in the pathspec. The prune_ignored call sets uninteresting ones to NULL. That function could compact the list and re-set ignored_nr, but it doesn't currently do so. An even more elegant solution would be for read_directory to mark whether an ignored file comes from a pathspec, or was found through recursion. That would be more efficient, and it would remove the prune_ignored thing, which is IMHO a little hack-ish. I don't have time to work on it now, but I might look at it more tonight or tomorrow (but please, if you are interested, take a crack at it). -Peff ``` ## Junio C Hamano, 2007-06-11 16:23 Subject: Re: [PATCH 1/3] refactor dir_add_name Message-ID: <7vk5uaqx3q.fsf@assigned-by-dhcp.pobox.com> URL: https://gitlist.dev/e/7vk5uaqx3q.fsf%40assigned-by-dhcp.pobox.com In-Reply-To: <20070611133944.GA7008@coredump.intra.peff.net> ``` Jeff King writes: > If we like the alloc_grow approach, there are a lot of places where we > can drop a 3-5 line conditional into a single line. I find it much more > readable, but others may disagree. I like the readability, but > cache.h | 6 ++++++ > dir.c | 23 +++++++++++------------ > 2 files changed, 17 insertions(+), 12 deletions(-) > > diff --git a/cache.h b/cache.h > index 5e7381e..f771519 100644 > --- a/cache.h > +++ b/cache.h > @@ -224,6 +224,12 @@ extern void verify_filename(const char *prefix, const char *name); > extern void verify_non_filename(const char *prefix, const char *name); > > #define alloc_nr(x) (((x)+16)*3/2) > +#define alloc_grow(x, nr, alloc) do { \ > + if(nr >= alloc) { \ > + alloc = alloc_nr(alloc); \ > + x = xrealloc((x), alloc * sizeof(*(x))); \ > + } \ > +} while(0) worry a bit about macro safety. I think the presence of an assignment to alloc and x would make sure we would catch an error to pass non lvalue as 'alloc' and 'x', so it may be Ok as is. A comment before the macro, and a space between 'if' and opening parenthesis, would be good things to have. ``` ## Jonas Fonseca, 2007-06-11 19:15 Subject: Re: [PATCH 3/3] builtin-add: simplify (and increase accuracy of) exclude handling Message-ID: <20070611191554.GA32151@diku.dk> URL: https://gitlist.dev/e/20070611191554.GA32151%40diku.dk In-Reply-To: <20070611155425.GA9316@coredump.intra.peff.net> ``` Jeff King wrote Mon, Jun 11, 2007: > On Mon, Jun 11, 2007 at 05:01:23PM +0200, Jonas Fonseca wrote: > > I think you could even get rid of has_ignored with something like this. > > Nope, I had originally wanted to do that, but the dir_struct.ignored > list contains _all_ ignored items, not just those that were originally > in the pathspec. The prune_ignored call sets uninteresting ones to > NULL. That function could compact the list and re-set ignored_nr, but > it doesn't currently do so. Ah, I see. > An even more elegant solution would be for read_directory to mark > whether an ignored file comes from a pathspec, or was found through > recursion. That would be more efficient, and it would remove the > prune_ignored thing, which is IMHO a little hack-ish. > > I don't have time to work on it now, but I might look at it more tonight > or tomorrow (but please, if you are interested, take a crack at it). Yes, I think it might be nice for me to do if you don't mind. I would like some more experience with the git code. Maybe even redo the whole patch series to also fix the concerns about the alloc_grow macro. -- Jonas Fonseca ``` ## Jeff King, 2007-06-11 19:46 Subject: Re: [PATCH 1/3] refactor dir_add_name Message-ID: <20070611194651.GA15309@coredump.intra.peff.net> URL: https://gitlist.dev/e/20070611194651.GA15309%40coredump.intra.peff.net In-Reply-To: <7vk5uaqx3q.fsf@assigned-by-dhcp.pobox.com> ``` On Mon, Jun 11, 2007 at 09:23:05AM -0700, Junio C Hamano wrote: > > +#define alloc_grow(x, nr, alloc) do { \ > > + if(nr >= alloc) { \ > > + alloc = alloc_nr(alloc); \ > > + x = xrealloc((x), alloc * sizeof(*(x))); \ > > + } \ > > +} while(0) > > worry a bit about macro safety. I think the presence of an Agreed. The multiple evaluation of 'x' means that growing foo[i++] will cause quite a confusing bug. I would prefer it as an inline, but at least the sizeof requires macro magic. Not to mention the ugliness of moving all those lvalues as pointers. But we could do something like (totally untested): #define alloc_grow(x, nr, alloc) \ alloc_grow_helper(&(x), nr, &(alloc), sizeof(*(x))) inline void alloc_grow_helper(void **x, int nr, int *alloc, int size) { if (nr >= *alloc) { *alloc = alloc_nr(*alloc); *x = xrealloc(*x, *alloc * size); } } Horribly ugly (I'm seeing stars!) but probably a bit safer in the long run, and nobody needs to look at it most of the time. :) What do you think? > assignment to alloc and x would make sure we would catch an > error to pass non lvalue as 'alloc' and 'x', so it may be Ok as Even lvalues can have side effects, so multiple evaluation is still a problem. > is. A comment before the macro, and a space between 'if' and > opening parenthesis, would be good things to have. Yes, sorry, my fingers are always forgetting the git whitespace conventions. I (or Jonas) will submit a better commented version, but do let us know which version you like. -Peff ``` ## Jeff King, 2007-06-11 19:48 Subject: Re: [PATCH 3/3] builtin-add: simplify (and increase accuracy of) exclude handling Message-ID: <20070611194810.GB15309@coredump.intra.peff.net> URL: https://gitlist.dev/e/20070611194810.GB15309%40coredump.intra.peff.net In-Reply-To: <20070611191554.GA32151@diku.dk> ``` On Mon, Jun 11, 2007 at 09:15:54PM +0200, Jonas Fonseca wrote: > > I don't have time to work on it now, but I might look at it more tonight > > or tomorrow (but please, if you are interested, take a crack at it). > > Yes, I think it might be nice for me to do if you don't mind. I would > like some more experience with the git code. Maybe even redo the whole > patch series to also fix the concerns about the alloc_grow macro. Great, feel free to rework the series as you see fit. My "tonight" timeframe was a bit optimistic anyway, as we have a new baby here. :) -Peff ``` ## Junio C Hamano, 2007-06-12 07:13 Subject: Re: [PATCH 1/3] refactor dir_add_name Message-ID: <7v7iq9lk76.fsf@assigned-by-dhcp.pobox.com> URL: https://gitlist.dev/e/7v7iq9lk76.fsf%40assigned-by-dhcp.pobox.com In-Reply-To: <20070611194651.GA15309@coredump.intra.peff.net> ``` Jeff King writes: > ... But we could do something like > (totally untested): > > #define alloc_grow(x, nr, alloc) \ > alloc_grow_helper(&(x), nr, &(alloc), sizeof(*(x))) > > inline > void alloc_grow_helper(void **x, int nr, int *alloc, int size) > { > if (nr >= *alloc) { > *alloc = alloc_nr(*alloc); > *x = xrealloc(*x, *alloc * size); > } > } > > Horribly ugly (I'm seeing stars!) but probably a bit safer in the long > run, and nobody needs to look at it most of the time. :) > > What do you think? That looks ugly and also I am curious what the generated assembly would look like. Hopefully the compiler is clever enough to generate the same code, but I dunno. Unless somebody else more versed with C preprocessor tricks comes along and offers a better advice, I would go with the earlier simpler one with a big fat warning. I however would prefer all caps name for a magic macro like this, whose sole point is a huge side effect. Anyway, it appears that Jonas picked up your patch to polish up, so I won't touch this series until that resurfaces. ``` ## Jeff King, 2007-06-12 12:19 Subject: Re: [PATCH 1/3] refactor dir_add_name Message-ID: <20070612121939.GB25145@coredump.intra.peff.net> URL: https://gitlist.dev/e/20070612121939.GB25145%40coredump.intra.peff.net In-Reply-To: <7v7iq9lk76.fsf@assigned-by-dhcp.pobox.com> ``` On Tue, Jun 12, 2007 at 12:13:01AM -0700, Junio C Hamano wrote: > That looks ugly and also I am curious what the generated > assembly would look like. Hopefully the compiler is clever > enough to generate the same code, but I dunno. I was curious, too...the assembly generated by gcc -O2 is identical for both versions. > Unless somebody else more versed with C preprocessor tricks > comes along and offers a better advice, I would go with the > earlier simpler one with a big fat warning. I however would > prefer all caps name for a magic macro like this, whose sole > point is a huge side effect. Agreed on the all-caps (in either case) because of the side effects. The more I think about it, I think the inline'd version is better. Multiple evaluation pre-processor bugs are _nasty_ to find, and while the implementation is ugly, it's better to contain the ugliness to one spot than to introduce a dangerous interface that will be used all over. I'm a bit rusty on my preprocessor tricks, but ISTR that there really isn't a good way to portably avoid the problems. gcc has typeof, which we could use to make temporary copies (which gcc would presumably optimize out), but I imagine we don't want to be gcc-specific. We could conditionally use that construct, but maybe at that point we're getting as ugly as the inline). -Peff ``` ## Jonas Fonseca, 2007-06-12 21:42 Subject: Re: [PATCH 3/3] builtin-add: simplify (and increase accuracy of) exclude handling Message-ID: <20070612214214.GA21858@diku.dk> URL: https://gitlist.dev/e/20070612214214.GA21858%40diku.dk In-Reply-To: <20070611155425.GA9316@coredump.intra.peff.net> ``` From: Jeff King Previously, the code would always set up the excludes, and then manually pick through the pathspec we were given, assuming that non-added but existing paths were just ignored. This was mostly correct, but would erroneously mark a totally empty directory as 'ignored'. Instead, we now use the collect_ignored option of dir_struct, which unambiguously tells us whether a path was ignored. This simplifies the code, and means empty directories are now just not mentioned at all. Furthermore, we now conditionally ask dir_struct to respect excludes, depending on whether the '-f' flag has been set. This means we don't have to pick through the result, checking for an 'ignored' flag; ignored entries were either added or not in the first place. We can safely get rid of the special 'ignored' flags to dir_entry, which were not used anywhere else. Signed-off-by: Jeff King Signed-off-by: Jonas Fonseca --- builtin-add.c | 69 +++++++++++++++++--------------------------------------- dir.c | 16 +++++++++++- dir.h | 4 +-- 3 files changed, 36 insertions(+), 53 deletions(-) Jeff King wrote Mon, Jun 11, 2007: > On Mon, Jun 11, 2007 at 05:01:23PM +0200, Jonas Fonseca wrote: > > I think you could even get rid of has_ignored with something like this. > > Nope, I had originally wanted to do that, but the dir_struct.ignored > list contains _all_ ignored items, not just those that were originally > in the pathspec. The prune_ignored call sets uninteresting ones to > NULL. That function could compact the list and re-set ignored_nr, but > it doesn't currently do so. > > An even more elegant solution would be for read_directory to mark > whether an ignored file comes from a pathspec, or was found through > recursion. That would be more efficient, and it would remove the > prune_ignored thing, which is IMHO a little hack-ish. OK, I tried to do this, however, I got a bit confused with the intended behavior. Anyway, it passes the test suite, is silent for empty directories and will only show exact matches of the pathspec (this was the confusing part) as ignored items. diff --git a/builtin-add.c b/builtin-add.c index 1591171..ad6aca8 100644 --- a/builtin-add.c +++ b/builtin-add.c @@ -40,42 +40,29 @@ static void prune_directory(struct dir_struct *dir, const char **pathspec, int p dir->nr = dst - dir->entries; for (i = 0; i < specs; i++) { - struct stat st; - const char *match; - if (seen[i]) - continue; - - match = pathspec[i]; - if (!match[0]) - continue; - - /* Existing file? We must have ignored it */ - if (!lstat(match, &st)) { - struct dir_entry *ent; - - ent = dir_add_name(dir, match, strlen(match)); - ent->ignored = 1; - if (S_ISDIR(st.st_mode)) - ent->ignored_dir = 1; - continue; - } - die("pathspec '%s' did not match any files", match); + if(!seen[i] && !file_exists(pathspec[i])) + die("pathspec '%s' did not match any files", + pathspec[i]); } } -static void fill_directory(struct dir_struct *dir, const char **pathspec) +static void fill_directory(struct dir_struct *dir, const char **pathspec, + int ignored_too) { 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); - if (!access(excludes_file, R_OK)) - add_excludes_from_file(dir, excludes_file); + if (!ignored_too) { + dir->collect_ignored = 1; + dir->exclude_per_dir = ".gitignore"; + path = git_path("info/exclude"); + if (!access(path, R_OK)) + add_excludes_from_file(dir, path); + if (!access(excludes_file, R_OK)) + add_excludes_from_file(dir, excludes_file); + } /* * Calculate common prefix for the pathspec, and @@ -219,13 +206,11 @@ int cmd_add(int argc, const char **argv, const char *prefix) } pathspec = get_pathspec(prefix, argv + i); - fill_directory(&dir, pathspec); + fill_directory(&dir, pathspec, ignored_too); if (show_only) { const char *sep = "", *eof = ""; for (i = 0; i < dir.nr; i++) { - if (!ignored_too && dir.entries[i]->ignored) - continue; printf("%s%s", sep, dir.entries[i]->name); sep = " "; eof = "\n"; @@ -237,25 +222,13 @@ int cmd_add(int argc, const char **argv, const char *prefix) if (read_cache() < 0) die("index file corrupt"); - if (!ignored_too) { - int has_ignored = 0; - for (i = 0; i < dir.nr; i++) - if (dir.entries[i]->ignored) - has_ignored = 1; - if (has_ignored) { - fprintf(stderr, ignore_warning); - for (i = 0; i < dir.nr; i++) { - if (!dir.entries[i]->ignored) - continue; - fprintf(stderr, "%s", dir.entries[i]->name); - if (dir.entries[i]->ignored_dir) - fprintf(stderr, " (directory)"); - fputc('\n', stderr); - } - fprintf(stderr, - "Use -f if you really want to add them.\n"); - exit(1); + if (dir.ignored_nr) { + fprintf(stderr, ignore_warning); + 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"); + exit(1); } for (i = 0; i < dir.nr; i++) diff --git a/dir.c b/dir.c index 1ffc1e5..f3a6757 100644 --- a/dir.c +++ b/dir.c @@ -275,7 +275,6 @@ static struct dir_entry *dir_entry_new(const char *pathname, int len) { struct dir_entry *ent; ent = xmalloc(sizeof(*ent) + len + 1); - ent->ignored = ent->ignored_dir = 0; ent->len = len; memcpy(ent->name, pathname, len); ent->name[len] = 0; @@ -432,6 +431,18 @@ static int simplify_away(const char *path, int pathlen, const struct path_simpli return 0; } +static int in_pathspec(const char *path, int len, const struct path_simplify *simplify) +{ + if (simplify) { + for (; simplify->path; simplify++) { + if (len == simplify->len + && !memcmp(path, simplify->path, len)) + return 1; + } + } + return 0; +} + /* * Read a directory tree. We currently ignore anything but * directories, regular files and symlinks. That's because git @@ -472,7 +483,8 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co continue; exclude = excluded(dir, fullname); - if (exclude && dir->collect_ignored) + if (exclude && dir->collect_ignored + && in_pathspec(fullname, baselen + len, simplify)) dir_add_ignored(dir, fullname, baselen + len); if (exclude != dir->show_ignored) { if (!dir->show_ignored || DTYPE(de) != DT_DIR) { diff --git a/dir.h b/dir.h index c94f3cb..ec0e8ab 100644 --- a/dir.h +++ b/dir.h @@ -13,9 +13,7 @@ struct dir_entry { - unsigned int ignored : 1; - unsigned int ignored_dir : 1; - unsigned int len : 30; + unsigned int len; char name[FLEX_ARRAY]; /* more */ }; -- 1.5.2.1.958.g264c-dirty -- Jonas Fonseca ``` ## Jonas Fonseca, 2007-06-12 21:51 Subject: Re: [PATCH 1/3] refactor dir_add_name Message-ID: <20070612215104.GB21858@diku.dk> URL: https://gitlist.dev/e/20070612215104.GB21858%40diku.dk In-Reply-To: <7v7iq9lk76.fsf@assigned-by-dhcp.pobox.com> ``` Junio C Hamano wrote Tue, Jun 12, 2007: > Anyway, it appears that Jonas picked up your patch to polish up, > so I won't touch this series until that resurfaces. My idea was to put the nr, alloc, and entries members into a separate struct dir_entry_list, which should allow the dir_add_* functions to be "united" and thus somewhat remove the need for alloc_grow. However, after spending some time on it, changing dir.nr to dir.entry_list.nr etc. reduced readability so much that I ended up trying to make other cleanups and the simplicity of the series was lost. Anyway, I don't think I can come up with a more polished version without adding a few cleanup patches along the way. So I think apart from the amended version of 3/3 I will take back my intension to polish it up. -- Jonas Fonseca ```