Re: [PATCH] ls-remote: default to 'origin' when no remote specified

From: Tay Ray Chuan <rctay89@gmail.com>
Date: Apr 8, 2010, 06:07 UTC
Message-ID: <l2xbe6fef0d1004072307ma8dff5c2ic5dce170b28e5957@mail.gmail.com>
In-Reply-To: <20100408044552.GA30473@coredump.intra.peff.net>

Hi,

On Thu, Apr 8, 2010 at 12:45 PM, Jeff King <peff@peff.net> wrote:

Show 15 quoted lines

> On Thu, Apr 08, 2010 at 11:58:03AM +0800, Tay Ray Chuan wrote:
>
>> Instead of breaking execution when no remote (as specified in the
>> variable dest) is specified when git-ls-remote is invoked, continue on
>> and let remote_get() handle it.
>>
>> That way, we are able to use the default remote (by default, "origin"),
>> as git-fetch, git-push, and others, do.
>>
>> While we're at it, die with a more interesting message ("Where do you
>> want to..."), as git-fetch does, instead of the plain usage help.
>
> I don't really see a problem with this. The current behavior produces an
> error, so it is not as if we are breaking somebody's workflow, and the
> only sensible default is the same one used by the other commands.

I'm trying to make it behave like the other commands that deal with remotes, such as git-fetch and git-push; when they are run without any arguments, they default to "origin" or branch.<name>.remote.

Assuming that you and I are talking about the same "other commands", than the default isn't an issue; the rules used to determine the default remote is done in remote_get(), so they are similar.

>> +     if (!remote)
>> +             die("Where do you want to list from today?");
>
> Heh.

Do you think this is too light-hearted for ls-remote's role? If so, I'll just revert back exiting with the usage text.

Show 6 quoted lines

>> +test_expect_success 'dies with message when no remote specified and no default remote found' '
>> +
>> +     !(git ls-remote >actual 2>&1) &&
>> +     test_cmp exp actual
>
> Use test_must_fail?

Noted.

Show 22 quoted lines

>> +cat >exp <<EOF
>> +fatal: 'refs*master' does not appear to be a git repository
>> +fatal: The remote end hung up unexpectedly
>> +EOF
>> +test_expect_success 'confuses pattern as remote when no remote specified' '
>> +     #
>> +     # Although ugly, this behaviour is akin to the confusion of refspecs for
>> +     # remotes by git-fetch and git-push, eg:
>> +     #
>> +     #   $ git fetch branch
>> +     #
>> +
>> +     # We could just as easily have used "master"; the "*" emphasizes its
>> +     # role as a pattern.
>> +     !(git ls-remote refs*master >actual 2>&1) &&
>> +     test_cmp exp actual
>> +
>> +'
>
> This seems like a very odd thing to be testing. Should you not instead
> test that "git ls-remote $foo" still treats $foo as a remote and lists
> it, which is what we really care about?

There are already existing tests to test "git ls-remote $foo" (unless you mean '$' to have a special significance, like '*' does).

The test was to let current and future git hackers/users know that the usage of <pattern> as the remote by git-ls-remote ("<pattern> does not appear to be a git repository") is indeed expected behaviour.

-- 
Cheers,
Ray Chuan