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

[Jeff King <peff@peff.net>]

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

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

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