Re: [PATCH] stripspace: allow -s/-c outside git repository

From: Martin Ågren <martin.agren@gmail.com>
Date: Dec 18, 2018, 06:09 UTC
Message-ID: <CAN0heSoekZ_vjR6DRjSAfw74hn_NU3-5TFf6-Hn9z2D_zE-mNg@mail.gmail.com>
In-Reply-To: <20181217165957.GA60293@google.com>

On Mon, 17 Dec 2018 at 22:56, Jonathan Nieder <jrnieder@gmail.com> wrote:

> That makes experimenting with the stripspace command unnecessarily
> fussy.  Fix it by discovering the git directory gently, as intended
> all along.

Show 5 quoted lines

>         if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {
> -               setup_git_directory_gently(NULL);
> +               setup_git_directory_gently(&nongit);
>                 git_config(git_default_config, NULL);
>         }

Makes me wonder if `setup_git_directory_gently()` should BUG if it receives NULL. That would require some reshuffling in setup.c, since `setup_git_directory()` calls out to it with NULL... Just thinking out loud. In any case, and more importantly, this is the last user providing NULL except for the one in `setup_git_directory()`.

Show 16 quoted lines

> diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
> index 5ce47e8af5..0c24a0f9a3 100755
> --- a/t/t0030-stripspace.sh
> +++ b/t/t0030-stripspace.sh
> @@ -430,9 +430,15 @@ test_expect_success '-c with changed comment char' '
>  test_expect_success '-c with comment char defined in .git/config' '
>         test_config core.commentchar = &&
>         printf "= foo\n" >expect &&
> -       printf "foo" | (
> -               mkdir sub && cd sub && git stripspace -c
> -       ) >actual &&
> +       rm -fr sub &&
> +       mkdir sub &&
> +       printf "foo" | git -C sub stripspace -c >actual &&
> +       test_cmp expect actual
> +'

A small while-at-it conversion from subshell (with a funny pipe into it) to `-C sub`. The `rm -fr` invocation is not in the original, so shouldn't be needed. This one looks safe enough, though it makes me wonder why you need it. `mkdir -p sub`? Probably not worth a v2.

Show 6 quoted lines

> +
> +test_expect_success '-c outside git repository' '
> +       printf "# foo\n" >expect &&
> +       printf "foo" | nongit git stripspace -c >actual &&
>         test_cmp expect actual
>  '

Nice.

Martin