[RFC/PATCH] git-add: Don't exclude explicitly-specified tracked files

[RFC/PATCH] git-add: Don't exclude explicitly-specified tracked files

[Greg Brockman]

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.

diff --git a/builtin/add.c b/builtin/add.c
index 56a4e0a..46b1fdb 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -423,8 +423,27 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		/* Set up the default git porcelain excludes */
 		memset(&dir, 0, sizeof(dir));
 		if (!ignored_too) {
+			const char **tracked = xmalloc(sizeof(char *) * (argc + 1));
+			const char **p;
+			int tidx = 0;
+			int pidx = 0;
+
 			dir.flags |= DIR_COLLECT_IGNORED;
 			setup_standard_excludes(&dir);
+
+			for (p = pathspec; *p; p++) {
+				if ((*p)[0] && cache_name_exists(*p, strlen(*p), 0))
+					tracked[tidx++] = *p;
+				else
+					pathspec[pidx++] = *p;
+			}
+
+			tracked[tidx] = NULL;
+			pathspec[pidx] = NULL;
+			exit_status |= add_files_to_cache(prefix, tracked, flags);
+			/* All files were tracked */
+			if (pidx == 0)
+				goto finish;
 		}
 
 		/* This picks up the paths that are not tracked */
-- 
1.7.0.4

Re: [RFC/PATCH] git-add: Don't exclude explicitly-specified tracked files

[Ævar Arnfjörð Bjarmason]

On Wed, Aug 11, 2010 at 07:03, Greg Brockman <gdb@mit.edu> wrote:
> 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.

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.

Re: [RFC/PATCH] git-add: Don't exclude explicitly-specified tracked files

[Junio C Hamano]

Greg Brockman <gdb@MIT.EDU> writes:

> 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

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?

Re: [RFC/PATCH] git-add: Don't exclude explicitly-specified tracked files

[Greg Brockman]

> 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

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

Re: [RFC/PATCH] git-add: Don't exclude explicitly-specified tracked files

[Jens Lehmann]

Are we talking about two different issues here?

Am 11.08.2010 14:24, schrieb Ævar Arnfjörð Bjarmason:
> On Wed, Aug 11, 2010 at 07:03, Greg Brockman <gdb@mit.edu> wrote:
>> Currently, 'git add' will complain about excluded files, even if they
>> are already tracked:

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.

>> ... so it feels natural to me not
>> to require extra user confirmation when an explicit path has been provided.
> 
> 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'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.

Re: [RFC/PATCH] git-add: Don't exclude explicitly-specified tracked files

[Greg Brockman]

> Are we talking about two different issues here?
>
> Am 11.08.2010 14:24, schrieb Ævar Arnfjörð Bjarmason:
>> On Wed, Aug 11, 2010 at 07:03, Greg Brockman <gdb@mit.edu> wrote:
>>> Currently, 'git add' will complain about excluded files, even if they
>>> are already tracked:
>
> 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.
>
>>> ... so it feels natural to me not
>>> to require extra user confirmation when an explicit path has been provided.
>>
>> 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'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.

Re: [RFC/PATCH] git-add: Don't exclude explicitly-specified tracked files

[Junio C Hamano]

Greg Brockman <gdb@MIT.EDU> writes:

>> 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
>
> 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/*"

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.

Re: [RFC/PATCH] git-add: Don't exclude explicitly-specified tracked files

[Greg Brockman]

> 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.)

Re: [RFC/PATCH] git-add: Don't exclude explicitly-specified tracked files

[Matthieu Moy]

Greg Brockman <gdb@MIT.EDU> writes:

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

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.

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

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.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC/PATCH] git-add: Don't exclude explicitly-specified tracked files

[Greg Brockman]

>> Currently, 'git add' will complain about excluded files, even if they
>> are already tracked:
>
> 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.
Good catch.  My commit message is definitely wrong.

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

Re: [RFC/PATCH] git-add: Don't exclude explicitly-specified tracked files

[Matthieu Moy]

Greg Brockman <gdb@MIT.EDU> writes:

>>> This commit changes 'git add' to disregard excludes for tracked files
>>> whose paths are explicitly specified on the command-line.
>>
>> I don't think you need this to solve the problem,

This remains (see below) ...

>> and as Junio said, that would make "git add dir/*" add all the
>> ignored files, which would make -f essentially useless.

... 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.

> 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).

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.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC/PATCH] git-add: Don't exclude explicitly-specified tracked files

[Junio C Hamano]

Greg Brockman <gdb@MIT.EDU> writes:

> 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.

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"?

Re: [RFC/PATCH] git-add: Don't exclude explicitly-specified tracked files

[Greg Brockman]

>> 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.
>
> 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

Re: [RFC/PATCH] git-add: Don't exclude explicitly-specified tracked files

[Ævar Arnfjörð Bjarmason]

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

> 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.

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".

Re: [RFC/PATCH] git-add: Don't exclude explicitly-specified tracked files

[Jonathan Nieder]

Greg Brockman wrote:

> 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.

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”.

Re: [RFC/PATCH] git-add: Don't exclude explicitly-specified tracked files

[Greg Brockman]

> 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

[RFC/PATCH] Add test case for dealing with a tracked file in an ignored directory

[Greg Brockman]

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

diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 7d7140d..97ba9e9 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -288,4 +288,34 @@ test_expect_success 'git add --dry-run --ignore-missing of non-existing file' '
 	test_cmp expect.err actual.err
 '
 
+cat >expect <<EOF
+The following paths are ignored by one of your .gitignore files:
+ignored-dir
+Use -f if you really want to add them.
+fatal: no files added
+EOF
+
+test_expect_success 'git add with file in ignored directory' '
+	mkdir ignored-dir &&
+	echo ignored-dir >> .gitignore &&
+	touch ignored-dir/file &&
+	test_must_fail git add ignored-dir/file >actual 2>&1 &&
+	test_cmp actual expect &&
+	git add -f ignored-dir/file &&
+	git add ignored-dir/file &&
+	echo change > ignored-dir/file &&
+	git add ignored-dir/file &&
+	! ( git ls-files -m ignored-dir/file | grep ignored-dir/file )
+'
+
+test_expect_success 'git add with ignored directory using git globs' "
+	mkdir ignored-dir2 && echo ignored-dir2 >> .gitignore && touch ignored-dir2/file &&
+	git add 'ignored-dir2/*' >actual 2>&1 &&
+	echo \"fatal: pathspec 'ignored-dir2/*' did not match any files\" | test_cmp - actual
+	git add -f ignored-dir2/file && echo change > ignored-dir2/file &&
+	git add 'ignored-dir2/*' >actual 2>&1 &&
+	echo '' | test_cmp - actual &&
+	git ls-files -m ignored-dir2/file | grep ignored-dir2/file
+"
+
 test_done
-- 
1.7.2.1.68.g1ba78

Re: [RFC/PATCH] Add test case for dealing with a tracked file in an ignored directory

[Greg Brockman]

Whoops, forgot the sign-off line:

> 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.

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

> ---
>  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
...

Re: [RFC/PATCH] Add test case for dealing with a tracked file in an ignored directory

[Matthieu Moy]

Greg Brockman <gdb@MIT.EDU> writes:

> 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.

Good :-).

> +test_expect_success 'git add with file in ignored directory' '

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.

> +	mkdir ignored-dir &&
> +	echo ignored-dir >> .gitignore &&
> +	touch ignored-dir/file &&

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.

> +	test_must_fail git add ignored-dir/file >actual 2>&1 &&
> +	test_cmp actual expect &&
> +	git add -f ignored-dir/file &&
> +	git add ignored-dir/file &&

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

> +test_expect_success 'git add with ignored directory using git globs' "
> +	mkdir ignored-dir2 && echo ignored-dir2 >> .gitignore && touch ignored-dir2/file &&
> +	git add 'ignored-dir2/*' >actual 2>&1 &&
> +	echo \"fatal: pathspec 'ignored-dir2/*' did not match any files\" | test_cmp - actual

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.

> +	git add -f ignored-dir2/file && echo change > ignored-dir2/file &&
> +	git add 'ignored-dir2/*' >actual 2>&1 &&

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.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC/PATCH] Add test case for dealing with a tracked file in an ignored directory

[Ævar Arnfjörð Bjarmason]

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

> +cat >expect <<EOF
> +The following paths are ignored by one of your .gitignore files:
> +ignored-dir
> +Use -f if you really want to add them.
> +fatal: no files added
> +EOF

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.

Re: [RFC/PATCH] Add test case for dealing with a tracked file in an ignored directory

[Matthieu Moy]

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

> On Wed, Aug 18, 2010 at 09:29, Greg Brockman <gdb@mit.edu> wrote:
>
>> +cat >expect <<EOF
>> +The following paths are ignored by one of your .gitignore files:
>> +ignored-dir
>> +Use -f if you really want to add them.
>> +fatal: no files added
>> +EOF
>
> 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.

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

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC/PATCH] Add test case for dealing with a tracked file in an ignored directory

[Ævar Arnfjörð Bjarmason]

On Wed, Aug 18, 2010 at 13:47, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Not sure what is the gain by doing so

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).

> and the vast majority of tests already there use the style of Greg's
> patch ...

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

Re: [RFC/PATCH] Add test case for dealing with a tracked file in an ignored directory

[Jonathan Nieder]

Matthieu Moy wrote:

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

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.

Re: [RFC/PATCH] Add test case for dealing with a tracked file in an ignored directory

[Ævar Arnfjörð Bjarmason]

On Thu, Aug 19, 2010 at 00:00, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Matthieu Moy wrote:
>
>> Not sure what is the gain by doing so, and the vast majority of tests
>> already there use the style of Greg's patch ...
>
> 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.

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 :)

Re: [RFC/PATCH] Add test case for dealing with a tracked file in an ignored directory

[Greg Brockman]

>> +     mkdir ignored-dir &&
>> +     echo ignored-dir >> .gitignore &&
>> +     touch ignored-dir/file &&
>
> 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.
Ok, sure.  I'll switch over to >ignored-dir/file.

>> +     test_must_fail git add ignored-dir/file >actual 2>&1 &&
>> +     test_cmp actual expect &&
>> +     git add -f ignored-dir/file &&
>> +     git add ignored-dir/file &&
>
> (so, this is the first thing you're fixing, shouldn't be
> controversial)
Hopefully not :).

>> +test_expect_success 'git add with ignored directory using git globs' "
>> +     mkdir ignored-dir2 && echo ignored-dir2 >> .gitignore && touch ignored-dir2/file &&
>> +     git add 'ignored-dir2/*' >actual 2>&1 &&
>> +     echo \"fatal: pathspec 'ignored-dir2/*' did not match any files\" | test_cmp - actual
>
> 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.
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?

>> +     git add -f ignored-dir2/file && echo change > ignored-dir2/file &&
>> +     git add 'ignored-dir2/*' >actual 2>&1 &&
>
> Just making sure I'm reading correctly: this is the second thing that
> should be fixed, and that your earlier patch didn't.
Yes, that's correct.  (Again, just trying to match functionality with
ignored files.)

> 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.
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

Re: [RFC/PATCH] Add test case for dealing with a tracked file in an ignored directory

[Matthieu Moy]

Greg Brockman <gdb@mit.edu> writes:

>> I don't think you want to complain with "did not match any files"
>> here.
> 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
> """

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

> realized today that git globbing seems to act differently depending on
> where the wildcard appears.  E.g.:

> Is this a bug?

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

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

>> 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.
> 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.

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

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC/PATCH] Add test case for dealing with a tracked file in an ignored directory

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:

> I just try to
> (hopefully mostly helpfully) to point out common things that
> contradict the docs we have

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?

diff --git a/t/README b/t/README
index 410499a..f347de7 100644
--- a/t/README
+++ b/t/README
@@ -235,7 +235,9 @@ Do's, don'ts & things to keep in mind
 -------------------------------------
 
 Here are a few examples of things you probably should and shouldn't do
-when writing tests.
+when writing tests.  If you are editing an existing test script and have
+the time for it, consider updating the script to follow these guidelines
+in a separate patch before adding your new test.
 
 Do:
 
-- 

Re: [RFC/PATCH] Add test case for dealing with a tracked file in an ignored directory

[Ævar Arnfjörð Bjarmason]

On Wed, Aug 25, 2010 at 03:13, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ævar Arnfjörð Bjarmason wrote:
>
>> I just try to
>> (hopefully mostly helpfully) to point out common things that
>> contradict the docs we have
>
> 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?
>
> diff --git a/t/README b/t/README
> index 410499a..f347de7 100644
> --- a/t/README
> +++ b/t/README
> @@ -235,7 +235,9 @@ Do's, don'ts & things to keep in mind
>  -------------------------------------
>
>  Here are a few examples of things you probably should and shouldn't do
> -when writing tests.
> +when writing tests.  If you are editing an existing test script and have
> +the time for it, consider updating the script to follow these guidelines
> +in a separate patch before adding your new test.

That looks good.