Re: [PATCH v3 19/19] Add git-check-ignore sub-command

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 28, 2012, 21:21 UTC
Message-ID: <7vtxr5ho01.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1356575558-2674-20-git-send-email-git@adamspiers.org>

Adam Spiers <git@adamspiers.org> writes:

Show 37 quoted lines

> diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
> new file mode 100644
> index 0000000..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 < <list-of-paths>",
> +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).

Show 9 quoted lines

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

Show 8 quoted lines

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

Show 48 quoted lines

> diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
> new file mode 100755
> index 0000000..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".

Show 7 quoted lines

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

Show 15 quoted lines

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