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

From: Adam Spiers <git@adamspiers.org>
Date: Sep 10, 2012, 11:09 UTC
Message-ID: <20120910110928.GA12974@atlantic.linksys.moosehall>
In-Reply-To: <CACsJy8A2-C9xSz2LXt9Ptjxhe++i2vcBSMY-cxJLWUiutajZUQ@mail.gmail.com>

On Tue, Sep 04, 2012 at 08:06:12PM +0700, Nguyen Thai Ngoc Duy wrote:

Show 13 quoted lines

> On Sun, Sep 2, 2012 at 7:12 AM, Adam Spiers <git@adamspiers.org> wrote:
> > --- a/builtin/add.c
> > +++ b/builtin/add.c
> > @@ -273,7 +273,7 @@ static int add_files(struct dir_struct *dir, int flags)
> >                 fprintf(stderr, _(ignore_error));
> >                 for (i = 0; i < dir->ignored_nr; i++)
> >                         fprintf(stderr, "%s\n", dir->ignored[i]->name);
> > -               fprintf(stderr, _("Use -f if you really want to add them.\n"));
> > +               fprintf(stderr, _("Use -f if you really want to add them, or git check-ignore to see\nwhy they're ignored.\n"));
> >                 die(_("no files added"));
> >         }
> 
> String too long (> 80 chars).

You mean the line of code is too long, or the argument to _(), or both? I didn't like this either, but I saw that builtin/checkout.c already did something similar twice, and I wasn't sure how else to do it. Suggestions gratefully received.

Show 17 quoted lines

> > +static const char * const check_ignore_usage[] = {
> > +"git check-ignore pathname...",
> > +"git check-ignore --stdin [-z] < <list-of-paths>",
> > +NULL
> > +};
> > +
> > +static const struct option check_ignore_options[] = {
> > +       OPT_BOOLEAN(0 , "stdin", &stdin_paths, "read file names from stdin"),
> > +       OPT_BOOLEAN('z', NULL, &null_term_line,
> > +               "input paths are terminated by a null character"),
> > +       OPT_END()
> > +};
> 
> You may want to mark help strings ("read file names from stdin" and
> "input paths... null character") and check_ignore_usage[] for
> translation. Just wrap those strings with N_() and you'll be fine. For
> similar changes, check out nd/i18n-parseopt-help on branch 'pu'.

Thanks, I'll do that.

[snipped discussion of "include" / "exclude" which already continued elsewhere]

Show 16 quoted lines

> > +static void check_ignore(const char *prefix, const char **pathspec)
> > +{
> > +       struct dir_struct dir;
> > +       const char *path;
> > +       char *seen = NULL;
> > +
> > +       /* read_cache() is only necessary so we can watch out for submodules. */
> > +       if (read_cache() < 0)
> > +               die(_("index file corrupt"));
> > +
> > +       memset(&dir, 0, sizeof(dir));
> > +       dir.flags |= DIR_COLLECT_IGNORED;
> > +       setup_standard_excludes(&dir);
> 
> You should support ignore rules from files and command line arguments
> too, like ls-files. For quick testing.

You mean --exclude, --exclude-from, and --exclude-per-directory? Sure, although I have limited time right now, so maybe these could be added in a later iteration?

Show 9 quoted lines

> > +static NORETURN void error_with_usage(const char *msg)
> > +{
> > +       error("%s", msg);
> > +       usage_with_options(check_ignore_usage, check_ignore_options);
> > +}
> 
> Interesting. We have usage_msg_opt() in parse-options.c, but it's more
> verbose. Perhaps this function should be moved to parse-options.c
> because it may be useful to other commands as well?

I'll look into it.