Re: [PATCH 3/3] builtin-add: fix exclude handling

From: Jeff King <peff@peff.net>
Date: Mar 9, 2010, 22:48 UTC
Message-ID: <20100309224815.GB25265@sigill.intra.peff.net>
In-Reply-To: <7vzl2s4fcy.fsf@alter.siamese.dyndns.org>

On Mon, Mar 01, 2010 at 12:26:37AM -0800, Junio C Hamano wrote:

Show 11 quoted lines

>  - "git add ignored-pattern.o", i.e. the pathspec exactly matches but is
>    ignored by the traversal.
> 
> For the former, the function immediately errored out.  The latter were
> queued in the dir structure and later used to give an error message with
> "use -f if you really mean it" advice.
> 
> e96980e (builtin-add: simplify (and increase accuracy of) exclude
> handling, 2007-06-12) somehow lost the latter.  This adds the logic back,
> but with a bit of twist, as the code after e96980e uses collect_ignored
> option that does half the necessary work during the traversal.

I'm not sure that logic is accurate. The "increase accuracy" part of e96980e was about the fact that COLLECT_IGNORED _knows_ something is ignored, but builtin-add's prune_directory has to just _guess_ about it.

And I think your new code may have similar problems. Look at the failing test in t0050 (the one you tweaked in patch 1/3). Before this patch, it silently fails to add. Which is a bug, of course, but not this one. But after this patch, we say "Oh, CamelCase was excluded, use -f, etc". But that's not true. We are just making a guess after the fact about why it wasn't included.

Now I am slightly hesitant to use that as an example, because it clearly was also broken _before_ your patch. So it may be that the behavior it exhibits (a pathspec not showing up as used, but the file exists and was _not_ ignored) shouldn't ever happen for other cases. I'm not sure, which makes me a little nervous about the change. But I would be willing to accept "it seems to work, so let's go with it for now and see if anybody complains" if you want to try that.

But also:

Show 13 quoted lines

> +		/* Existing file?  We must have ignored it */
> +		if (file_exists(match)) {
> +			int len = strlen(match);
> +			int i;
> +			for (i = 0; i < dir->ignored_nr; i++)
> +				if (dir->ignored[i]->len == len &&
> +				    !memcmp(dir->ignored[i]->name, match, len))
> +					break;
> +			if (dir->ignored_nr <= i)
> +				dir_add_ignored(dir, match, strlen(match));
> +			continue;
> +		}
> +		die("pathspec '%s' did not match any files", match);

Is that memcmp right? If I have something like:

  echo subdir >.gitignore
  git add 'sub*'

shouldn't it also say "Sorry, subdir [or sub*] was ignored"? But it doesn't actually get added to the exclude list, and we get "pathspec did not match any files", which is not quite true.

-Peff