| Message ID | 4cdcedff313751da8c91d701c095f1051e759ce2.1599848727.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | clone: allow configurable default for -o/--origin | expand |
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
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
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
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.
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
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
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
> 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!
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 --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 &&