Re: Fwd: Runaway "git remote" if group definition contains a remote by the same name

From: Jeff King <peff@peff.net>
Date: Dec 31, 2013, 08:06 UTC
Message-ID: <20131231080647.GA25838@sigill.intra.peff.net>
In-Reply-To: <xmqqlhz2rw7s.fsf@gitster.dls.corp.google.com>

On Mon, Dec 30, 2013 at 11:10:31AM -0800, Junio C Hamano wrote:

Show 12 quoted lines

> > So I think the sanest thing is probably:
> >
> >   1. Teach "fetch" to expand recursively in a single process, and then
> >      tell sub-processes (via a new command-line option) not to expand
> >      any further.
> >
> >   2. Teach "fetch" to detect cycles (probably just by a simple depth
> >      counter).
> 
> I suspect that the expansion code will just accumulate remotes found
> into a string-list (as part of 4. below), so deduping would be
> fairly easily done without a depth counter.

I don't think that will work (at least not naively). The end-product of step 1, and the string_list that is de-duped in step 4, is a list of the concrete remotes. The cycles occur between groups, which are not mentioned in the final list.

You can keep a separate list of the groups we visit, of course, but we do not otherwise need it.

One thing that does make such a list easier is that we do not need to care about order. E.g., in config like this:

  [remotes]
  a = c
  b = c
  c = d e

you can mark "c" as seen after visiting it via "a". It is not technically a cycle, but since we would want to suppress duplicates anyway, we can be overly broad.

Show 7 quoted lines

> >   3. Teach the group-reading code to detect groups more robustly, so
> >      that a single-item group like "remotes.foo=bar" correctly recurses
> >      to "bar".
> 
> A single-item remote group is somewhat annoying, but expanding it
> only at some places while ignoring it at other places is even more
> annoying, so this sounds like a right thing to do.

The only configuration that I think would be negatively affected is something like:

  [remote]
  foo = foo
  [remote "foo"]
  url = ...

that silently works now, but would become broken (because we would complain about the cycle). I think that's OK; that config is clearly stupid and broken. If it were "remote.foo = foo bar", trying to expand the concrete "foo" and "bar", that might make some sense, but then it is already broken in the current code (that is the example that started the discussion).

-Peff