Re: [PATCH 9/9] Add git-check-ignores

On Sun, Sep 2, 2012 at 9:50 PM, Adam Spiers [off-list ref] wrote:

I'm no expert on .gitattributes and check-attr, but AFAICS, all the
opportunities to share code in the plumbing and front-end seem to be
taken already, e.g. the directory traversal and path handling.  The
CLI argument parsing is necessarily different because check-attr
requires a list of attributes as well as a list of files, and of
course the output routines have to be different too.

The only opportunity for code reuse which I saw but /didn't/ take was
around the --stdin line parsing code which is duplicated between:

    check_attr_stdin_paths
    check_ignore_stdin_paths
    cmd_checkout_index
    cmd_update_index
    hash_stdin_paths

I attempted to refactor these, but quickly realised that due to the
lack of proper closures in C, the overheads and complexity incurred by
performing such a refactoring probably outweighed the benefits, so I
gave up on the idea.

Having said that, I'm totally open to suggestions if you can spot
other places where code could be reused :)

Yeah. That was my impression too. I just hoped a new set of eyes might
discover something ;) At lease we could prepare the output format that
can be reused (maybe with little changes) for check-attr debugging if
it comes later. Or make this command part of check-attr..

quoted

Also --quiet option, where check-ignore returns 0 if the given path is
ignored, 1 otherwise?

I considered that, but couldn't think of appropriate behaviour when
multiple paths are given, so in the end I decided to remain consistent
with check-attr, which always returns 0.  But I'm happy to change it
if you can think of a more useful behaviour.  For example we could
have a --count option which produces no output but has an exit status
corresponding to the number of ignored files.

We could take this opportunity to kill "add --ignore-missing", which
is basically .gitignore checking and it accepts multiple paths, I
think.

quoted

 - If many paths are given, then perhaps we could print ignored paths
(no extra info).

How is this different to git ls-files -i -o ?

I think ls-files requires real files on working directory, but
check-ignore can deal with just non-existing paths.

quoted

 - For debugging, given one path, we print all the rules that are
applied to it, which may help understand how/why it goes wrong.

That would be nice, but I'm not sure it's a tremendously common use
case.  Could you think of a scenario in which it would be useful?  I
guess it could be done by adding a new DIR_DEBUG_IGNORED flag to
dir_struct which would make the exclude matcher functions collect all
matching patterns, rather than just returning the first one.  This in
turn would require another field for collecting all matched patterns.

Mixing include/exclude ignore rules multiple times could be hard to
figure out what goes wrong. But as we haven't seen an actual use case
yet, just leave it out.

quoted

I don't think we really need NEED_WORK_TREE here. .gitignore can be
read from index only.

I thought about that, but in the end I decided it probably didn't make
sense, because none of the exclude matching routines match against the
index - they all match against the working tree and core.excludesfile.
This would also require changing the matching logic to honor the index,
but I didn't see the benefit in doing that, since all operations which
involve excludes (add, status, etc.) relate to a work tree.

But as with all of the above, please don't hesitate to point out if
I've missed something.  You guys are the experts, not me ;-)

Again I was thinking that check-ignore could work with imaginary paths
and could be used by scripts. If a script just wants to check certain
paths are excluded, it should not need to move to working directory
(though it probably is in working directory most of the time).
-- 
Duy