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

From: Jeff King <peff@peff.net>
Date: Apr 8, 2010, 06:34 UTC
Message-ID: <20100408063422.GD30473@coredump.intra.peff.net>
In-Reply-To: <l2xbe6fef0d1004072307ma8dff5c2ic5dce170b28e5957@mail.gmail.com>

On Thu, Apr 08, 2010 at 02:07:11PM +0800, Tay Ray Chuan wrote:

Show 11 quoted lines

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

The commands I meant were push, fetch, and pull. I couldn't think of any others.

Show 7 quoted lines

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

Perhaps. In general that is not a helpful message to show to the user, but it is very unlikely to be shown at all. You would have to have no configured remote for your current branch, _and_ you would have had to delete the config for the "origin" remote.

Fetch's message is equally hard to trigger, I think, which is perhaps why nobody has complained about it yet.

Show 6 quoted lines

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

No, I meant $foo as a variable, as you interpreted.

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

I think documenting that in the commit message would be fine, but I don't have a strong opinion. It seems like the existing "git ls-remote $foo" tests would cover this by correctly interpreting $foo as a remote.

If you do keep it, it also should use test_must_fail.

Also, I note that you are testing for:

  fatal: 'refs*master' does not appear to be a git repository
  fatal: The remote end hung up unexpectedly

which is coming from two separate processes, which means the output may have a race condition. I suspect we're OK because the second message is triggered by closing the fd which must happen after the first message is printed.

-Peff