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

[Nguyen Thai Ngoc Duy <pclouds@gmail.com>]

On Sun, Sep 2, 2012 at 9:50 PM, Adam Spiers <git@adamspiers.org> 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..

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

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

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

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