RFC patchgit-add: Don't exclude explicitly-specified tracked files

Currently, 'git add' will complain about excluded files, even if they are already tracked:

$ mkdir dir && touch dir/file && cat > .gitignore <<< dir
$ git add -f dir/file
$ git status
  ...
  new file:   dir/file
  ...
$ git add dir/file
The following paths are ignored by one of your .gitignore files:
dir
Use -f if you really want to add them.
fatal: no files added

This commit changes 'git add' to disregard excludes for tracked files whose paths are explicitly specified on the command-line. So in the above example, 'git add dir/file' no longer requires a '-f'. However, 'git add dir' does.

Signed-off-by: Greg Brockman <gdb@mit.edu>
---
 builtin/add.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

What do people think of this behavior? My motivation in writing this patch is that I sometimes track files in an ignored directory, and it can be cumbersome to remember to pass '-f' when adding them. Related commands such as 'git add -p' and 'git commit -a' do not require a '-f' in this case, so it feels natural to me not to require extra user confirmation when an explicit path has been provided.

As always, thanks in advance for your comments.

On Wed, Aug 11, 2010 at 07:03, Greg Brockman <gdb@mit.edu> wrote:

I like it. I keep a /etc in git with .gitignore "*". This would help a lot for use cases like that. Explicitly specifying a full path should override gitignore IMO.

I think with some tests this should be ready to go.

Are we talking about two different issues here?

Am 11.08.2010 14:24, schrieb Ævar Arnfjörð Bjarmason:

I'm all for not complaining when adding an ignored file that is already tracked, as the user already told us he wants to track this file despite .gitignore.

I'm not so sure if we should silently add ignored files just because they appear on the command line. For me having to force the first time I do a "git add" for an otherwise ignored file looks like a feature.

Correct me if I'm wrong, but I read Ævar's use to be exactly the one I described. In particular, he presumably has a few interesting files in /etc that he tracks but wants to ignore the rest. So as far as I can tell we are all talking about the same issue.

To be clear, I certainly agree that having to force the first time you run 'git add' is a feature, and my patch explicitly does not change this functionality.

Anyway, modulo further discussion, I will add some tests and send the revised version to the list. Thanks all for looking this over.

Greg Brockman <gdb@MIT.EDU> writes:

Thanks.

I think it is reasonble if we don't complain in this particular case.

What should happen if the user did these instead, after adding "dir" as an ignored entry, and adding dir/file but not dir/untracked to the index?

 (1)    git add dir/file dir/untracked    ; explicitly named
 (2)    git add dir/*			  ; have shell glob--same as (1)
 (3)    git add "dir/*"                   ; have git glob
 (4)    git add dir                       ; have git recurse

What does your code do?

In all four cases, the output I get is: """ The following paths are ignored by one of your .gitignore files: dir Use -f if you really want to add them. fatal: no files added """

Note that this is also the output if you run

(5) rm dir/untracked && git add "dir/*"

Greg

Greg Brockman <gdb@MIT.EDU> writes:

Here is the one that troubles me the most:

 (6) git add dir/f*

This _looks_ like very explicitly named from git's point of view, but from the end user's point of view it is not. Depending on presense (or absense) of another file whose name begins with 'f' in the directory, the add will be either prevented or silently accepted.

I am not sure what the best solution would be; I tend to think the current behaviour is slightly saner in the face of shell globbing.

I agree that case seems rather gray. But in what circumstances would you expect it to be a problem? I think only if the user has a tracked file with local changes that he or she does not wish to be committed. How common is it that a user will do this and expect gitignores to protect that file's changes I don't believe I've ever tried that in practice, but I also wouldn't trust gitignores to protect me from myself, as a careless 'git commit -a' would still commit my changes.

Perhaps one could add some syntax to .gitignore files that configures this behavior. I can come up with a proposal for something along those lines if you believe it would be useful. However, I don't have great use case for it, unless we also make 'git commit -a' and friends respect this configuration. (As an example, perhaps a line like +dir would mean that file should be excluded even if the user runs 'git add dir/file' while a line like dir would mean that 'git add dir/file' should succeed if dir/file is already tracked.)

Greg Brockman <gdb@MIT.EDU> writes:

This is not exactly true:

$ echo '*.pdf' > .gitignore; touch foo.pdf; git add -f foo.pdf $ echo content >> foo.pdf; git add foo.pdf

Here, the second "git add" didn't need the -f flag.

So, your problem is not about already-tracked exclude files, but it is about already-tracked files in an excluded directory.

I don't think you need this to solve the problem, and as Junio said, that would make "git add dir/*" add all the ignored files, which would make -f essentially useless.

After a quick look at the code, the issue seems close to (dir.c):

struct dir_entry *dir_add_ignored(struct dir_struct *dir, const char *pathname, int len)
{
	if (!cache_name_is_other(pathname, len))
		return NULL;

	ALLOC_GROW(dir->ignored, dir->ignored_nr+1, dir->ignored_alloc);
	return dir->ignored[dir->ignored_nr++] = dir_entry_new(pathname, len);
}

I guess the "if (!cache_name_is_other(pathname, len))" test is the one allowing the behavior I got above, but here, in the case of "git add dir/file" with "dir" being ignored, "pathname" is just "dir", not "dir/file", hence your problem.

Good catch.  My commit message is definitely wrong.

I respectfully disagree with this assessment, however.

First of all, as you point out 'git add foo.pdf' works where foo.pdf has been explicitly ignored, while in contrast 'git add dir/file' fails when file has only been indirectly ignored because it is in an ignored directory. In the former case, the user explicitly specified a policy for that file. In the later case, the policy is only indirectly expressed because that file happens to be in an ignored directory; in my and Ævar's use cases what we mean is "I only care about a few files in a big directory and don't feel like writing out a .gitignore entry for each one". So it doesn't make sense to me to allow the user to add the file in the first place (whether by a 'git add foo.pdf' or a 'git add f*') but not the second. Perhaps the answer is to swing in the opposite direction of this patch series and make 'git add foo.pdf' fail as well, but I must ask what we'd be defending against... it seems the only reason is to allow the user to maintain local changes to a tracked file, but as I've stated many other tools don't seem to similarly respect the .gitignore.

Secondly, I don't think this makes '-f' useless. '-f' would still be used to initially add an untracked file to the index. So this would maintain an invariant that no ignored files are tracked unless the user has specified a '-f' for it in the past.

Incidentally, I noticed that 'git add dir/file' for ignored dir worked fine in an older version of git. 'git bisect' reveals that the behavior I would like to change was introduced in 29209cb. From the commit message, I get the sense that this particular behavior was not actually intentional (someone please correct me if I'm missing something).

Thanks,

Greg

Greg Brockman <gdb@MIT.EDU> writes:

This remains (see below) ...

... but this is actually wrong, yes. Your commit message states

| This commit changes 'git add' to disregard excludes for tracked
| files whose paths are explicitly specified on the command-line.

I had missed the "tracked files whose ..." part, and focused on the "path is explicitely specified on the command-line".

And actually, all you need is to see whether the file is tracked or not, and not whether it's been given from the command-line. With your patch, I get:

$ git init git Initialized empty Git repository in /tmp/git/.git/ $ cd git $ mkdir dir $ touch dir/file $ echo dir > .gitignore $ git add -f dir/file $ echo content >> dir/file $ git add dir/file $ git add dir/f* # <--- shell globing

Up to now, everything OK. But:

$ git add dir/f\* # <--- Git globing. The following paths are ignored by one of your .gitignore files: dir Use -f if you really want to add them. fatal: no files added

I think Git should not apply any .gitignore rule to already-tracked files, whether they are given from the command-line explicitely or through globbing.

One case which can be discussed:

$ git add dir The following paths are ignored by one of your .gitignore files: dir Use -f if you really want to add them. fatal: no files added

I don't think I should need a -f flag here either, since dir/ contains only tracked files. But I don't care much here.

My understanding is that the goal was to reject the first "git add subdir/file", but not subsequent ones.

I'd suggest that you write a first patch introducing new tests, possibly marked as test_expect_failure, so that people can at least agree on the desired behavior, and then an implementation could follow.

Greg Brockman <gdb@MIT.EDU> writes:

Sorry, but I don't get this argument. When the user says "everything in this directory is ignored", why does it make it less direct than "this particular file is ignored"?

In general, I view the presence of a dir entry in a .gitignore as the user setting a default policy for files in that directory, but the user might actually mean for there to be some exceptions to that policy.

For example, in my personal usage, when I ignore a directory but track some files within it, this is because I don't want to specify an ignore for every single other file in that directory. Also note that negated .gitignore entries don't seem to work in this case, i.e. a .gitignore with contents dir !dir/file won't actually let file be addable again.

In contrast, when I add dir/file to a .gitignore, there is no doubt that I want to ignore that particular file.

Does that make more sense?

Greg

Greg Brockman wrote:

Aside: yeah, this is something I have run into before.  The workaround
I used was to use a dir/.gitignore instead, like this:

 *
 !/file

which is ugly.

Maybe git should get internal support for something like

 dir/**

(meaning “all files under dir”) and “!dir/file” could be internally munged to

 !/dir
 dir/**
 !dir/file

when it appears after “dir”.

On Thu, Aug 12, 2010 at 15:54, Greg Brockman <gdb@mit.edu> wrote:

I initially misread what this series was about, and I was about to withdraw my support for it. But this seems completely reasonable, and actually I think Git's current behavior here is clearly a bug.

To elabore with examples this behavior here is fine, and I think everyone agrees with that:

    aoeu tmp (160M) $ git init meh
    Initialized empty Git repository in /tmp/meh/.git/
    aoeu tmp (160M) $ cd !$
    cd meh
    aoeu meh (master) $ echo '*' > .gitignore
    aoeu meh (master) $ mkdir ignore-dir
    aoeu meh (master) $ echo ignore > ignore-dir/file
    aoeu meh (master) $ echo ignore > file
    aoeu meh (master) $ git add file ignore-dir
    The following paths are ignored by one of your .gitignore files:
    file
    ignore-dir
    Use -f if you really want to add them.
    fatal: no files added

Here I have * in .gitignore but I'm adding files with an explicit path. Making this not ask for -f would be pretty bad, e.g. for the glob reasons Junio cited.

So I have to -f it:

    aoeu meh (master) $ git add -f file ignore-dir
    aoeu meh (master) $ git commit -m"commiting ignored stuff"
    [master (root-commit) 6cae514] commiting ignored stuff
     2 files changed, 2 insertions(+), 0 deletions(-)
     create mode 100644 file
     create mode 100644 ignore-dir/file

However this part I think is a bug:

    aoeu meh (master) $ echo whee >> ignore-dir/file
    aoeu meh (master) $ echo whee >> file
    aoeu meh (master) $ git status --short
     M file
     M ignore-dir/file
    aoeu meh (master) $ git add file
    aoeu meh (master) $ git add ignore-dir/file
    The following paths are ignored by one of your .gitignore files:
    ignore-dir
    Use -f if you really want to add them.
    fatal: no files added
    $ git status --short
    M  file
     M ignore-dir/file

Here "file" is already tracked by Git and it doesn't complain when I "git add" a update to it, but it complains about "ignore-dir/file" just because it's in a subdirectory.

I hadn't noticed this before because I usually use "git add -u", which doesn't complain about the ignore and happily updates the file in the index:

    aoeu meh (master) $ git add -u
    aoeu meh (master) $ git status --short
    M  file
    M  ignore-dir/file

I think "git add ignore-dir/file" above should act exactly like "git add file", and not force me to add a "-f" to "git add".

I agree. That seems very much like the correct behavior. FWIW, prior to sending the patch I hadn't noticed that 'git add file' already works without a '-f', which is why my commit message doesn't mention it.

Anyway, I'm not sure if there's consensus on this being the right behavior. Thus I will take Matthieu's advice and propose a test-case for the desired behavior. I'll send that under separate cover in a few moments.

Thanks everyone for contributing your thoughts thus far.

Greg

This test case attempts to match the behavior of 'git add ignore-file'
with 'git add ignore-dir/file' when .gitignore contains entries for
ignore-file and ignore-dir.
---
 t/t3700-add.sh |   30 ++++++++++++++++++++++++++++++
 1 files changed, 30 insertions(+), 0 deletions(-)

This patch is a follow-up to the thread '[RFC/PATCH] git-add: Don't exclude explicitly-specified tracked files' at

  http://thread.gmane.org/gmane.comp.version-control.git/153194

In this patch, I propose the desired behavior of 'git add' for files contained in ignored directories. I have attempted to mirror the behavior of 'git add' for ignored files, but I may have gotten that wrong or that might not actually be what we want here. Comments on whether this is the desired behavior and whether these test-cases accurate capture that behavior would be appreciated.

Thank you,

Greg

Whoops, forgot the sign-off line:

Signed-off-by: Greg Brockman <gdb@mit.edu>

...

Greg Brockman <gdb@MIT.EDU> writes:

Good :-).

In the final version, you can make the tests test_expect_failure in the first patch, and turn them back into test_expect_success in the second (which fixes the issue). This makes it clear what your change to the code do, and makes sure the test suite passes for each commit.

I think >ignored-dir/file is more portable than touch, and is recommanded in the testsuite. But a quick grep shows that touch is already used.

(so, this is the first thing you're fixing, shouldn't be controversial)

Currently, "git add 'dir/*'" will add the files under dir/ if dir/ isn't ignored, and require -f if dir is ignored.

I don't think you want to complain with "did not match any files" here.

Just making sure I'm reading correctly: this is the second thing that should be fixed, and that your earlier patch didn't.

You're not testing the case

  git add ignored-dir/

which gives another case where Git tries to add files not explicitely given on the command-line. But the correct behavior of this case may be more controversial, so maybe it's indeed better to focus on the other cases.

Ok, sure.  I'll switch over to >ignored-dir/file.

Hopefully not :).

Well, I copied the behavior of 'git add "*"' here, where every file in . is ignored. E.g. """ $ echo >ignore-file $ echo .gitignore >>.gitignore $ echo ignore-file >>.gitignore $ git add '*' fatal: pathspec '*' did not match any files """ One could reasonably choose to instead say "The following paths are ignored by one of your .gitignore files: ...". When I chose the "did not match any files", I had been assuming that globbing works roughly analogously to shell globbing, meaning the error one gets from a '*' should be the same as that one gets from a 'dir/*' or '*/*'. I realized today that git globbing seems to act differently depending on where the wildcard appears. E.g.: """ $ mkdir a && echo >a/b $ git add '*/*' # complains, doesn't stage any files fatal: pathspec '*/*' did not match any files $ git add 'a/*' $ echo change >a/b $ git add '*/*' # doesn't complain, but doesn't stage changes $ git ls-files -m a/b """ Is this a bug? I looked for some docs on the globbing functionality, but I didn't come across anything that specified it in detail. Anyway, either way I still think consistency is ideal, and hence would still vote for "did not match any files". Thoughts?

Yes, that's correct. (Again, just trying to match functionality with ignored files.)

A fair point. I would have thought the behavior here should be unchanged, namely that 'git add ignored-dir/' should spit out a "The following paths are ignored by one of your .gitignore files: ..." error, regardless of the directory's contents. Does anyone believe this should be different/would it be useful for me to draw up a test case for it now? In any case, I'll certainly put a test case for this into the final patch.

Greg

Greg Brockman <gdb@mit.edu> writes:

OK, that makes sense (you can add comments to your tests or commit message to justify this is case someone wonders later).

There are many known inconsistancies with Git globing, yes. See for example:

http://thread.gmane.org/gmane.comp.version-control.git/128672/focus=128759

It makes sense to make "git add dir/" equivalent to "git add dir/*", but I don't really care either way.

On Wed, Aug 18, 2010 at 09:29, Greg Brockman <gdb@mit.edu> wrote:

Please make this a test, per this bit in t/README:

 - Put all code inside test_expect_success and other assertions.

   Even code that isn't a test per se, but merely some setup code
   should be inside a test assertion.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Not sure what is the gain by doing so, and the vast majority of tests already there use the style of Greg's patch ...

On Wed, Aug 18, 2010 at 13:47, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

Most of those cases were already discussed in <1278082789-19872-8-git-send-email-avarab@gmail.com> and surrounding mails, I'll point to those instead of repeating things, unless there's something else that needs clarifying (and perhaps elaborated in t/README).

They do because we haven't converted them yet. New tests following the recommendations in t/README helps with that conversion effort.

Matthieu Moy wrote:

Right. The usual rule when contributing to an existing project is "imitate the surruounding code", but there is often a tension between global style guidelines and the local conventions.

In this case I have to agree with Matthieu: the test script is easier to read if it follows a single, consistent style. The cleanup can happen another day.

On Thu, Aug 19, 2010 at 00:00, Jonathan Nieder <jrnieder@gmail.com> wrote:

Sure, I don't feel in any way strongly about it. I just try to (hopefully mostly helpfully) to point out common things that contradict the docs we have, especially with the tests, since I'm getting pretty familiar with them :)

Ævar Arnfjörð Bjarmason wrote:

Thanks for doing so, by the way. One of the best ways to make sure the docs are sane is to use them.

When a guideline is unclear, probably I should have been updating the docs instead of dispensing advice.

How about something like this?

On Wed, Aug 25, 2010 at 03:13, Jonathan Nieder <jrnieder@gmail.com> wrote:

That looks good.