git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Adam Spiers <git@adamspiers.org>
To: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Cc: git list <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>, Jeff King <peff@peff.net>
Subject: Re: [PATCH 9/9] Add git-check-ignores
Date: Sun, 2 Sep 2012 15:50:29 +0100	[thread overview]
Message-ID: <CAOkDyE9wPUOwJpeKQ5wSCoufqyqE9zwRuBuNvDGEZ-z8452DwA@mail.gmail.com> (raw)
In-Reply-To: <CACsJy8CLzTddPOCzGhNkOGYjTvEtgWFoD+6DLhAew4Rf1aw5WA@mail.gmail.com>

Hi there,

Firstly, thanks for the quick feedback!

On Sun, Sep 2, 2012 at 11:41 AM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> On Sun, Sep 2, 2012 at 7:12 AM, Adam Spiers <git@adamspiers.org> wrote:
>> This works in a similar manner to git-check-attr.  Some code
>> was reused from add.c by refactoring out into pathspec.c.
>
> Thanks, comments from a quick glance. First of all, can we make it
> work (or share code) with .gitattributes? We may need to debug
> .gitattributes as well as .gitignore. A common command would be nice.

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

>> +SYNOPSIS
>> +--------
>> +[verse]
>> +'git check-ignore' pathname...
>> +'git check-ignore' --stdin [-z] < <list-of-paths>
>
> 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.

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

>  - Going to the next level, we could print path and the the location
> of the final exclude/include rule (file and line number).

That's the current behaviour, and I believe it covers the most common
use case.

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

>> @@ -338,6 +338,7 @@ static void handle_internal_command(int argc, const char **argv)
>>                 { "bundle", cmd_bundle, RUN_SETUP_GENTLY },
>>                 { "cat-file", cmd_cat_file, RUN_SETUP },
>>                 { "check-attr", cmd_check_attr, RUN_SETUP },
>> +               { "check-ignore", cmd_check_ignore, RUN_SETUP | NEED_WORK_TREE },
>>                 { "check-ref-format", cmd_check_ref_format },
>>                 { "checkout", cmd_checkout, RUN_SETUP | NEED_WORK_TREE },
>>                 { "checkout-index", cmd_checkout_index,
>
> 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 ;-)

Thanks again,
Adam

  reply	other threads:[~2012-09-02 14:50 UTC|newest]

Thread overview: 90+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-09-02  0:12 [PATCH 0/9] new git check-ignore sub-command Adam Spiers
2012-09-02  0:12 ` [PATCH 1/9] Update directory listing API doc to match code Adam Spiers
2012-09-02  0:12 ` [PATCH 2/9] Improve documentation and comments regarding directory traversal API Adam Spiers
2012-09-02  0:12 ` [PATCH 3/9] Rename cryptic 'which' variable to more consistent name Adam Spiers
2012-09-02 19:56   ` Junio C Hamano
2012-09-02  0:12 ` [PATCH 4/9] Refactor excluded_from_list Adam Spiers
2012-09-04 12:32   ` Nguyen Thai Ngoc Duy
2012-09-02  0:12 ` [PATCH 5/9] Refactor excluded and path_excluded Adam Spiers
2012-09-04 12:40   ` Nguyen Thai Ngoc Duy
2012-09-04 17:23     ` Junio C Hamano
2012-09-05 10:28       ` Nguyen Thai Ngoc Duy
2012-09-06  3:21         ` Junio C Hamano
2012-09-06 12:13           ` Nguyen Thai Ngoc Duy
2012-09-06 14:59             ` Thiago Farina
2012-09-06 15:05               ` Nguyen Thai Ngoc Duy
2012-09-06 17:42                 ` Adam Spiers
2012-09-06 21:07                 ` Junio C Hamano
2012-09-02  0:12 ` [PATCH 6/9] For each exclude pattern, store information about where it came from Adam Spiers
2012-09-02 17:00   ` Philip Oakley
2012-09-02 19:02     ` Junio C Hamano
2012-09-02 22:36       ` Philip Oakley
2012-09-06 17:56         ` Adam Spiers
2012-09-02  0:12 ` [PATCH 7/9] Extract some useful pathspec handling code from builtin/add.c into a library Adam Spiers
2012-09-02  0:12 ` [PATCH 8/9] Provide free_directory() for reclaiming dir_struct memory Adam Spiers
2012-09-02  0:12 ` [PATCH 9/9] Add git-check-ignores Adam Spiers
2012-09-02 10:41   ` Nguyen Thai Ngoc Duy
2012-09-02 14:50     ` Adam Spiers [this message]
2012-09-02 20:38       ` Junio C Hamano
2012-09-03  4:14       ` Nguyen Thai Ngoc Duy
2012-09-02 20:41   ` Junio C Hamano
2012-09-03  1:47     ` Junio C Hamano
2012-09-20 19:46     ` [PATCH v2 00/14] new git check-ignore sub-command Adam Spiers
2012-09-20 19:46       ` [PATCH v2 01/14] Update directory listing API doc to match code Adam Spiers
2012-09-20 19:46       ` [PATCH v2 02/14] Improve documentation and comments regarding directory traversal API Adam Spiers
2012-09-20 19:46       ` [PATCH v2 03/14] Rename cryptic 'which' variable to more consistent name Adam Spiers
2012-09-20 19:46       ` [PATCH v2 04/14] Rename path_excluded() to is_path_excluded() Adam Spiers
2012-09-20 19:46       ` [PATCH v2 05/14] Rename excluded_from_list() to is_excluded_from_list() Adam Spiers
2012-09-20 19:46       ` [PATCH v2 06/14] Rename excluded() to is_excluded() Adam Spiers
2012-09-20 19:46       ` [PATCH v2 07/14] Refactor is_excluded_from_list() Adam Spiers
2012-09-20 19:46       ` [PATCH v2 08/14] Refactor is_excluded() Adam Spiers
2012-09-20 19:46       ` [PATCH v2 09/14] Refactor is_path_excluded() Adam Spiers
2012-09-20 19:46       ` [PATCH v2 10/14] For each exclude pattern, store information about where it came from Adam Spiers
2012-09-20 21:31         ` Junio C Hamano
2012-12-26 15:46           ` Adam Spiers
2012-09-20 19:46       ` [PATCH v2 11/14] Refactor treat_gitlinks() Adam Spiers
2012-09-20 19:46       ` [PATCH v2 12/14] Extract some useful pathspec handling code from builtin/add.c into a library Adam Spiers
2012-09-21  7:54         ` Michael Haggerty
2012-09-20 19:46       ` [PATCH v2 13/14] Provide free_directory() for reclaiming dir_struct memory Adam Spiers
2012-09-21  8:03         ` Michael Haggerty
2012-09-21 16:11           ` Junio C Hamano
2012-09-20 19:46       ` [PATCH v2 14/14] Add git-check-ignore sub-command Adam Spiers
2012-09-21  5:44         ` Johannes Sixt
2012-09-25 23:25           ` Junio C Hamano
2012-09-26  5:49             ` Johannes Sixt
2012-09-26 14:03               ` Junio C Hamano
2012-09-21  7:23         ` Michael Haggerty
2012-09-21 16:27           ` Junio C Hamano
2012-09-21 19:42         ` Junio C Hamano
2012-09-20 21:26       ` [PATCH v2 00/14] new git check-ignore sub-command Junio C Hamano
2012-09-20 21:43         ` Junio C Hamano
2012-09-20 23:45           ` Adam Spiers
2012-09-21  4:34             ` Junio C Hamano
2012-12-16 19:35               ` [PATCH 0/3] Help newbie git developers avoid obvious pitfalls Adam Spiers
2012-12-16 19:35                 ` [PATCH 1/3] SubmittingPatches: add convention of prefixing commit messages Adam Spiers
2012-12-16 23:15                   ` Junio C Hamano
2012-12-16 19:36                 ` [PATCH 2/3] Documentation: move support for old compilers to CodingGuidelines Adam Spiers
2012-12-16 19:36                 ` [PATCH 3/3] Makefile: use -Wdeclaration-after-statement if supported Adam Spiers
2012-12-17  1:52                   ` Junio C Hamano
2012-12-17  2:15                     ` Adam Spiers
2012-12-17  4:18                       ` Junio C Hamano
2012-12-22 12:25                         ` Adam Spiers
2012-12-22 18:39                           ` Junio C Hamano
2012-12-26 15:44           ` [PATCH v2 00/14] new git check-ignore sub-command Adam Spiers
2012-09-21 19:00       ` Junio C Hamano
2012-12-16 23:04         ` compiler checks Adam Spiers
2012-09-24 22:31       ` [PATCH v2 00/14] new git check-ignore sub-command Junio C Hamano
2012-09-04 13:06   ` [PATCH 9/9] Add git-check-ignores Nguyen Thai Ngoc Duy
2012-09-04 17:26     ` Junio C Hamano
2012-09-05 10:25       ` Nguyen Thai Ngoc Duy
2012-09-10 11:15         ` Adam Spiers
2012-09-10 11:09     ` Adam Spiers
2012-09-10 12:25       ` Nguyen Thai Ngoc Duy
2012-09-10 16:30       ` Junio C Hamano
2012-09-02 20:35 ` [PATCH 0/9] new git check-ignore sub-command Junio C Hamano
2012-09-06 17:44   ` Adam Spiers
2012-09-07 10:03   ` Adam Spiers
2012-09-07 16:45     ` Junio C Hamano
2012-09-19 19:00       ` [PATCH] Document conventions on static initialization and else cuddling Adam Spiers
2012-09-19 20:43         ` Junio C Hamano
2012-09-19 21:14           ` Adam Spiers

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAOkDyE9wPUOwJpeKQ5wSCoufqyqE9zwRuBuNvDGEZ-z8452DwA@mail.gmail.com \
    --to=git@adamspiers.org \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=pclouds@gmail.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).