Re: [PATCH v3 19/19] Add git-check-ignore sub-command
On Sat, Dec 29, 2012 at 01:23:52AM +, Adam Spiers wrote: > FYI, attached is the diff between check-ignore-v3 and my current > check-ignore, which is available at github: > > https://github.com/aspiers/git/commits/check-ignore [snipped] > diff --git a/pathspec.c b/pathspec.c > index 6724121..3789b14 100644 > --- a/pathspec.c > +++ b/pathspec.c [snipped] > -void validate_path(const char *path, const char *prefix) > +void die_if_path_beyond_symlink(const char *path, const char *prefix) [snipped] > diff --git a/pathspec.h b/pathspec.h > index c251441..1961b19 100644 > --- a/pathspec.h > +++ b/pathspec.h > @@ -1,6 +1,11 @@ > +#ifndef PATHSPEC_H > +#define PATHSPEC_H > + > extern char *find_used_pathspec(const char **pathspec); > extern void fill_pathspec_matches(const char **pathspec, char *seen, int > specs); > extern const char *treat_gitlink(const char *path); > extern void treat_gitlinks(const char **pathspec); > extern void validate_path(const char *path, const char *prefix); ^ I forgot to rename this one. Will be fixed in v4. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 19/19] Add git-check-ignore sub-command
On Fri, Dec 28, 2012 at 01:21:02PM -0800, Junio C Hamano wrote:
> Adam Spiers writes:
> > +static void output_exclude(const char *path, struct exclude *exclude)
> > +{
> > + char *bang = exclude->flags & EXC_FLAG_NEGATIVE ? "!" : "";
> > + char *dir = (exclude->flags & EXC_FLAG_MUSTBEDIR) ? "/" : "";
>
> That's inconsistent. Either s/bang/negated/ or s/dir/slash/ (but
> obviously not both).
OK. I'll make the use of parentheses there consistent too.
> > +static int check_ignore(const char *prefix, const char **pathspec)
> > +{
> > + struct dir_struct dir;
> > +...
> > + if (pathspec) {
> > +...
> > + } else {
> > + printf("no pathspec\n");
> > + }
>
> Is this an error message, an informative warning, or what? The
> command is presumably for script consumption downstream of a pipe.
> Does it make sense to emit this message to their standard input?
> Does it even have to be output? Especially what should happen when
> the user says "--quiet"?
>
> Perhaps
>
> if (!quiet)
> fprintf(stderr, "no pathspec given.\n");
>
> or something?
Hmm. I suspect this was an unfinished remnant of one of my early
prototypes, because this code path never gets hit. There's even a
test which checks that a fatal error is generated when no pathspecs
are given via argv.
However, the test for behaviour when no pathspecs are given via
--stdin *is* missing. In this case, I think the code you suggest
above makes sense for generating a warning, and the existing behaviour
of returning an exit code of 1 also seems correct. I have added your
code and a test to cover it.
> > +int cmd_check_ignore(int argc, const char **argv, const char *prefix)
> > +{
> > + int num_ignored = 0;
>
> I do not think you need to initialize this one.
Fixed.
> > + if (stdin_paths) {
> > + num_ignored = check_ignore_stdin_paths(prefix);
> > + } else {
> > + num_ignored = check_ignore(prefix, argv);
> > + maybe_flush_or_die(stdout, "ignore to stdout");
> > + }
> > +
> > + return num_ignored > 0 ? 0 : 1;
>
> Given that num_ignored will always be >=0, that is a funny way to
> say
>
> return !num_ignored;
Personally I find that harder to read, but I'm not a regular C
programmer so I'll defer to your sense of style.
> or if you plan to report a non-fatal error as negative return from
> the two check_ignore* functions,
I don't think that's necessary here, but I'll bear it in mind.
> > +stderr_contains () {
> > + regexp="$1"
> > + if grep -q "$regexp" "$HOME/stderr"
>
> Please do not use "grep -q"; the output from commands in test
> harness is normally hidden already. This only makes script more
> cluttered and robs debuggers of potentially useful output when the
> test script is run with "-v".
Fixed.
> > +test_check_ignore () {
> > + args="$1" expect_code="${2:-0}" global_args="$3"
> > +
> > + init_vars &&
> > + rm -f "$HOME/stdout" "$HOME/stderr" "$HOME/cmd" &&
> > + echo git $global_args check-ignore $quiet_opt $verbose_opt $args \
> > + >"$HOME/cmd" &&
>
> Does a debugger needs this "cmd" file when we already have "-v" option?
Yes, I think it's useful; for example:
$ ./t0008-ignores.sh -i -v
[... test fails ...]
$ cd trash\ directory.t0008-ignores
$ gdb --args ../../$( > +test_expect_success_multi () {
> > + prereq=
> > + if test $# -eq 4
> > + then
> > + prereq=$1
> > + shift
> > + fi
> > + testname="$1" expect_verbose="$2" code="$3"
> > +
> > + expect=$( echo "$expect_verbose" | sed -e 's/.* //' )
> > +
> > + test_expect_success $prereq "$testname" "
> > + expect '$expect' &&
> > + $code
> > + "
>
> This is brittle when $expect may have single quotes, isn't it?
Currently $expect will never have single quotes, but I grant this may
change in the future.
> Perhaps
>
> test_expect_success $prereq "$testname" '
> expect "$expect" && eval "$code"
> '
>
> may fix it,
Yes that works, thanks.
> but in general I hate to see each test script trying to
> invent their own mini-harness like this (and getting them wrong).
test_expect_success_multi is specific to check-ignore and avoids a
huge amount of code duplication. I can't think of a better approach
but if you can then I'm all ears.
I believe this only leaves two items from your review of v3 which are
yet to be resolved:
1. What should validate_path() should be renamed to in order to avoid
ambiguity with other path validation functions? Currently my
preferred choice is die_if_path_beyond_symlink() but I don't really
mind - I'll go with that unless I hear someone prefers another
name.
2. The now-public functions fill_pathspec_matches() and
find_used_pathspec(), need to be documented, and in particular need
to mention that they traverse the index not a tree or the working
directory. I'll work on improving my understanding of these
functions to the
Re: [PATCH v3 19/19] Add git-check-ignore sub-command
Adam Spiers writes:
> diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
> new file mode 100644
> index 000..c825736
> --- /dev/null
> +++ b/builtin/check-ignore.c
> @@ -0,0 +1,170 @@
> +#include "builtin.h"
> +#include "cache.h"
> +#include "dir.h"
> +#include "quote.h"
> +#include "pathspec.h"
> +#include "parse-options.h"
> +
> +static int quiet, verbose, stdin_paths;
> +static const char * const check_ignore_usage[] = {
> +"git check-ignore [options] pathname...",
> +"git check-ignore [options] --stdin < ",
> +NULL
> +};
> +
> +static int null_term_line;
> +
> +static const struct option check_ignore_options[] = {
> + OPT__QUIET(&quiet, N_("suppress progress reporting")),
> + OPT__VERBOSE(&verbose, N_("be verbose")),
> + OPT_GROUP(""),
> + OPT_BOOLEAN(0, "stdin", &stdin_paths,
> + N_("read file names from stdin")),
> + OPT_BOOLEAN('z', NULL, &null_term_line,
> + N_("input paths are terminated by a null character")),
> + OPT_END()
> +};
> +
> +static void output_exclude(const char *path, struct exclude *exclude)
> +{
> + char *bang = exclude->flags & EXC_FLAG_NEGATIVE ? "!" : "";
> + char *dir = (exclude->flags & EXC_FLAG_MUSTBEDIR) ? "/" : "";
That's inconsistent. Either s/bang/negated/ or s/dir/slash/ (but
obviously not both).
> +static int check_ignore(const char *prefix, const char **pathspec)
> +{
> + struct dir_struct dir;
> +...
> + if (pathspec) {
> +...
> + } else {
> + printf("no pathspec\n");
> + }
Is this an error message, an informative warning, or what? The
command is presumably for script consumption downstream of a pipe.
Does it make sense to emit this message to their standard input?
Does it even have to be output? Especially what should happen when
the user says "--quiet"?
Perhaps
if (!quiet)
fprintf(stderr, "no pathspec given.\n");
or something?
> +int cmd_check_ignore(int argc, const char **argv, const char *prefix)
> +{
> + int num_ignored = 0;
I do not think you need to initialize this one.
> + if (stdin_paths) {
> + num_ignored = check_ignore_stdin_paths(prefix);
> + } else {
> + num_ignored = check_ignore(prefix, argv);
> + maybe_flush_or_die(stdout, "ignore to stdout");
> + }
> +
> + return num_ignored > 0 ? 0 : 1;
Given that num_ignored will always be >=0, that is a funny way to
say
return !num_ignored;
or if you plan to report a non-fatal error as negative return from
the two check_ignore* functions, perhaps:
return num_ignored <= 0;
> diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
> new file mode 100755
> index 000..3937ca4
> --- /dev/null
> +++ b/t/t0008-ignores.sh
> @@ -0,0 +1,595 @@
> +#!/bin/sh
> +
> +test_description=check-ignore
> +
> +. ./test-lib.sh
> +
> +init_vars () {
> + global_excludes="$(pwd)/global-excludes"
> +}
> +
> +enable_global_excludes () {
> + init_vars
> + git config core.excludesfile "$global_excludes"
> +}
> +
> +expect_in () {
> + dest="$HOME/expected-$1" text="$2"
> + if test -z "$text"
> + then
> + >"$dest" # avoid newline
> + else
> + echo "$text" >"$dest"
> + fi
> +}
> +
> +expect () {
> + expect_in stdout "$1"
> +}
> +
> +expect_from_stdin () {
> + cat >"$HOME/expected-stdout"
> +}
> +
> +test_stderr () {
> + expected="$1"
> + expect_in stderr "$1" &&
> + test_cmp "$HOME/expected-stderr" "$HOME/stderr"
> +}
> +
> +stderr_contains () {
> + regexp="$1"
> + if grep -q "$regexp" "$HOME/stderr"
Please do not use "grep -q"; the output from commands in test
harness is normally hidden already. This only makes script more
cluttered and robs debuggers of potentially useful output when the
test script is run with "-v".
> +test_check_ignore () {
> + args="$1" expect_code="${2:-0}" global_args="$3"
> +
> + init_vars &&
> + rm -f "$HOME/stdout" "$HOME/stderr" "$HOME/cmd" &&
> + echo git $global_args check-ignore $quiet_opt $verbose_opt $args \
> + >"$HOME/cmd" &&
Does a debugger needs this "cmd" file when we already have "-v" option?
> +test_expect_success_multi () {
> + prereq=
> + if test $# -eq 4
> + then
> + prereq=$1
> + shift
> + fi
> + testname="$1" expect_verbose="$2" code="$3"
> +
> + expect=$( echo "$expect_verbose" | sed -e 's/.* //' )
> +
> + test_expect_success $prereq "$testname" "
> + expect '$expect' &&
> + $code
> + "
This is brittle when $expect may have single quotes, isn't it?
Perhaps
test_expect_success $prereq "$testname" '
expect "$expect" && eval "$code"
'
may fix it, but in general I hate to see each test script trying to
invent their own mini-harness like this (and getting them wrong).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message t
