diff mbox series

[1/4] clone: add tests for --template and some disallowed option pairs

Message ID 4cdcedff313751da8c91d701c095f1051e759ce2.1599848727.git.gitgitgadget@gmail.com
State Superseded
Headers show
Series clone: allow configurable default for -o/--origin | expand

Commit Message

Elijah Newren via GitGitGadget Sept. 11, 2020, 6:25 p.m. UTC
From: Sean Barag <sean@barag.org>

Some combinations of command-line options to `git clone` are invalid,
but there were previously no tests ensuring those combinations reported
errors.  Similarly, `git clone --template` didn't appear to have any
tests.

Signed-off-by: Sean Barag <sean@barag.org>
---
 t/t5606-clone-options.sh | 44 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

Comments

Derrick Stolee Sept. 11, 2020, 6:57 p.m. UTC | #1
On 9/11/2020 2:25 PM, Sean Barag via GitGitGadget wrote:
> From: Sean Barag <sean@barag.org>
> 
> Some combinations of command-line options to `git clone` are invalid,
> but there were previously no tests ensuring those combinations reported
> errors.  Similarly, `git clone --template` didn't appear to have any
> tests.
> 
> Signed-off-by: Sean Barag <sean@barag.org>
> ---
>  t/t5606-clone-options.sh | 44 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
> index e69427f881..d20a78f84b 100755
> --- a/t/t5606-clone-options.sh
> +++ b/t/t5606-clone-options.sh
> @@ -19,6 +19,50 @@ test_expect_success 'clone -o' '
>  
>  '
>  
> +test_expect_success 'disallows --bare with --origin' '
> +
> +	test_expect_code 128 git clone -o foo --bare parent clone-bare-o 2>err &&
> +	test_debug "cat err" &&
> +	test_i18ngrep "\-\-bare and --origin foo options are incompatible" err
> +
> +'

It seems that all of your tests have an extraneous newline
at the end.

Thanks,
-Stolee
Jeff King Sept. 11, 2020, 7:56 p.m. UTC | #2
On Fri, Sep 11, 2020 at 02:57:11PM -0400, Derrick Stolee wrote:

> > diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
> > index e69427f881..d20a78f84b 100755
> > --- a/t/t5606-clone-options.sh
> > +++ b/t/t5606-clone-options.sh
> > @@ -19,6 +19,50 @@ test_expect_success 'clone -o' '
> >  
> >  '
> >  
> > +test_expect_success 'disallows --bare with --origin' '
> > +
> > +	test_expect_code 128 git clone -o foo --bare parent clone-bare-o 2>err &&
> > +	test_debug "cat err" &&
> > +	test_i18ngrep "\-\-bare and --origin foo options are incompatible" err
> > +
> > +'
> 
> It seems that all of your tests have an extraneous newline
> at the end.

And the beginning. :)

It's matching the surrounding test, which are written in an older
inconsistent style. I think it's OK to match them, but cleaning them
would be welcome, too.

While I'm looking at this hunk, two other things:

 - do we really care about code 128, or just failure? test_must_fail
   might be a better choice

 - I didn't even know we had test_debug. ;) The last time somebody added
   a call to it was in 2012. I think it's being used as intended here,
   but I'm not sure that the clutter to the test is worth it (we have
   other tools like "-i" to stop at the right spot and let you inspect
   the broken state).

 - the backslash escapes confused me for a moment. I guess they are
   trying to hide the dashes from grep's option parser. That's OK,
   though I'd have probably just started with "bare" since we're
   matching a substring anyway. I think you could also use "-e" with
   test_i18ngrep.

-Peff
Eric Sunshine Sept. 11, 2020, 8:07 p.m. UTC | #3
On Fri, Sep 11, 2020 at 3:56 PM Jeff King <peff@peff.net> wrote:
> And the beginning. :)
>
> It's matching the surrounding test, which are written in an older
> inconsistent style. I think it's OK to match them, but cleaning them
> would be welcome, too.
>
> While I'm looking at this hunk, two other things:
>
>  - do we really care about code 128, or just failure? test_must_fail
>    might be a better choice
>
>  - I didn't even know we had test_debug. ;) The last time somebody added
>    a call to it was in 2012. I think it's being used as intended here,
>    but I'm not sure that the clutter to the test is worth it (we have
>    other tools like "-i" to stop at the right spot and let you inspect
>    the broken state).
>
>  - the backslash escapes confused me for a moment. I guess they are
>    trying to hide the dashes from grep's option parser. That's OK,
>    though I'd have probably just started with "bare" since we're
>    matching a substring anyway. I think you could also use "-e" with
>    test_i18ngrep.

Thanks, you said everything I was going to say about this, thus saving
me some typing.

A couple more observations related to a few of the subsequent tests. This:

    template="$TRASH_DIRECTORY/template-with-config" &&

probably doesn't need $TRASH_DIRECTORY since that happens to be the
current working directory anyhow, so:

    template=template-with-config &&

should suffice (unless you had a problem doing it that way). Or you
could drop the variable altogether and use the literal name where you
need it.

Also, instead of:

    (cd clone-template-config && test "$(git config --local foo.bar)"
= "from_template")

would probably be written these days as:

    echo from_template >expect &&
    git -C clone-template-config config --local foo.bar >actual &&
    test_cmp expect actual
Junio C Hamano Sept. 11, 2020, 9:02 p.m. UTC | #4
Derrick Stolee <stolee@gmail.com> writes:

>> +test_expect_success 'disallows --bare with --origin' '
>> +
>> +	test_expect_code 128 git clone -o foo --bare parent clone-bare-o 2>err &&
>> +	test_debug "cat err" &&
>> +	test_i18ngrep "\-\-bare and --origin foo options are incompatible" err
>> +
>> +'
>
> It seems that all of your tests have an extraneous newline
> at the end.

That matches the older style to make the test body stand out by
having blank lines around it.  All existing tests from the era (not
limited to this script) used to do so.

It is OK to update them to modern style, but let's not do so in the
middle of the series.

Thanks.
Derrick Stolee Sept. 12, 2020, 12:41 a.m. UTC | #5
On 9/11/2020 5:02 PM, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
> 
>>> +test_expect_success 'disallows --bare with --origin' '
>>> +
>>> +	test_expect_code 128 git clone -o foo --bare parent clone-bare-o 2>err &&
>>> +	test_debug "cat err" &&
>>> +	test_i18ngrep "\-\-bare and --origin foo options are incompatible" err
>>> +
>>> +'
>>
>> It seems that all of your tests have an extraneous newline
>> at the end.
> 
> That matches the older style to make the test body stand out by
> having blank lines around it.  All existing tests from the era (not
> limited to this script) used to do so.
> 
> It is OK to update them to modern style, but let's not do so in the
> middle of the series.

Thanks for correcting me. (and Peff for doing the same).

I should have looked at the context better, because that
is frequently the reason for "inconsistent" style. My
apologies.

-Stolee
Taylor Blau Sept. 12, 2020, 3:17 a.m. UTC | #6
On Fri, Sep 11, 2020 at 03:56:22PM -0400, Jeff King wrote:
>  - I didn't even know we had test_debug. ;) The last time somebody added
>    a call to it was in 2012. I think it's being used as intended here,
>    but I'm not sure that the clutter to the test is worth it (we have
>    other tools like "-i" to stop at the right spot and let you inspect
>    the broken state).

Me either :). I think what you said about using -i to inspect whatever
is broken applies not only to this test, but also to all other tests in
the suite.

I'd be pretty happy to see 'test_debug' go away. There are only 104 uses
of it in the tree, and I think most of them could simply be removed
without needing to touch anything else.

I've never found it useful in debugging a problem on my end, but perhaps
others have. Food for thought.

> -Peff

Thanks,
Taylor
Sean Barag Sept. 15, 2020, 4:09 p.m. UTC | #7
On Fri, 11 Sep 2020 15:56:22 -0400, Jeff King wrote:
 > do we really care about code 128, or just failure? test_must_fail
   might be a better choice

Good point - `test_must_fail` is probably fine here.  I went with an
explicit error code so this test wouldn't pass in the event of an
outright crash, but I'm happy to adjust for v2.

> I didn't even know we had test_debug. ;) The last time somebody added
  a call to it was in 2012. I think it's being used as intended here,
  but I'm not sure that the clutter to the test is worth it (we have
  other tools like "-i" to stop at the right spot and let you inspect
  the broken state).

Frankly I'd forgotten I'd included it!  It's definitely not necessary.
Will remove for v2 as well.

> the backslash escapes confused me for a moment. I guess they are
  trying to hide the dashes from grep's option parser. That's OK,
  though I'd have probably just started with "bare" since we're
  matching a substring anyway. I think you could also use "-e" with
  test_i18ngrep.

Adding `-e` would solve this handily.  Thanks for the suggestion!

Sean Barag
Sean Barag Sept. 16, 2020, 3:15 a.m. UTC | #8
> A couple more observations related to a few of the subsequent tests.
> This:
>
>     template="$TRASH_DIRECTORY/template-with-config" &&
>
> probably doesn't need $TRASH_DIRECTORY since that happens to be the
> current working directory anyhow, so:
>
>     template=template-with-config &&
>
> should suffice (unless you had a problem doing it that way). Or you
> could drop the variable altogether and use the literal name where you
> need it.

I hadn't realized $TRASH_DIRECTORY was the pwd, but it makes perfect
sense.  Will update for v2.

> Also, instead of:
>
>     (cd clone-template-config && test "$(git config --local foo.bar)"
> = "from_template")
>
> would probably be written these days as:
>
>     echo from_template >expect &&
>     git -C clone-template-config config --local foo.bar >actual &&
>     test_cmp expect actual

Oooh `test_cmp` looks much better.  Thanks for the tip!
Jeff King Sept. 16, 2020, 4:36 p.m. UTC | #9
On Tue, Sep 15, 2020 at 09:09:44AM -0700, Sean Barag wrote:

> On Fri, 11 Sep 2020 15:56:22 -0400, Jeff King wrote:
>  > do we really care about code 128, or just failure? test_must_fail
>    might be a better choice
> 
> Good point - `test_must_fail` is probably fine here.  I went with an
> explicit error code so this test wouldn't pass in the event of an
> outright crash, but I'm happy to adjust for v2.

That's good thinking, but test_must_fail already has you covered; it
will complain about any death-by-signal. It wouldn't distinguish
between, say exit codes 128 and 1, but 128 is the code used by our die()
function, so expecting it isn't very specific anyway. :)

-Peff
diff mbox series

Patch

diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index e69427f881..d20a78f84b 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -19,6 +19,50 @@  test_expect_success 'clone -o' '
 
 '
 
+test_expect_success 'disallows --bare with --origin' '
+
+	test_expect_code 128 git clone -o foo --bare parent clone-bare-o 2>err &&
+	test_debug "cat err" &&
+	test_i18ngrep "\-\-bare and --origin foo options are incompatible" err
+
+'
+
+test_expect_success 'disallows --bare with --separate-git-dir' '
+
+	test_expect_code 128 git clone --bare --separate-git-dir dot-git-destiation parent clone-bare-sgd 2>err &&
+	test_debug "cat err" &&
+	test_i18ngrep "\-\-bare and --separate-git-dir are incompatible" err
+
+'
+
+test_expect_success 'uses "origin" for default remote name' '
+
+	git clone parent clone-default-origin &&
+	(cd clone-default-origin && git rev-parse --verify refs/remotes/origin/master)
+
+'
+
+test_expect_success 'prefers --template config over normal config' '
+
+	template="$TRASH_DIRECTORY/template-with-config" &&
+	mkdir "$template" &&
+	git config --file "$template/config" foo.bar from_template &&
+	test_config_global foo.bar from_global &&
+	git clone "--template=$template" parent clone-template-config &&
+	(cd clone-template-config && test "$(git config --local foo.bar)" = "from_template")
+
+'
+
+test_expect_success 'prefers -c config over --template config' '
+
+	template="$TRASH_DIRECTORY/template-with-ignored-config" &&
+	mkdir "$template" &&
+	git config --file "$template/config" foo.bar from_template &&
+	git clone "--template=$template" -c foo.bar=inline parent clone-template-inline-config &&
+	(cd clone-template-inline-config && test "$(git config --local foo.bar)" = "inline")
+
+'
+
 test_expect_success 'redirected clone does not show progress' '
 
 	git clone "file://$(pwd)/parent" clone-redirected >out 2>err &&