On Wed, Jan 4, 2017 at 10:22 AM, Stefan Beller <sbeller@xxxxxxxxxx> wrote:
> On Wed, Jan 4, 2017 at 10:13 AM, Brandon Williams <bmwill@xxxxxxxxxx> wrote:
>> On 01/04, Jeff King wrote:
>>> On Wed, Jan 04, 2017 at 07:56:02AM +0100, Torsten Bögershausen wrote:
>>>
>>> > On 04.01.17 01:48, Jeff King wrote:
>>> > > On Tue, Jan 03, 2017 at 11:09:18AM -0800, Brandon Williams wrote:
>>> > >
>>> > >> Only change with v4 is in [1/5] renaming the #define MAXSYMLINKS back to
>>> > >> MAXDEPTH due to a naming conflict brought up by Lars Schneider.
>>> > >
>>> > > Hmm. Isn't MAXSYMLINKS basically what you want here, though? It what's
>>> > > what all other similar functions will be using.
>>> > >
>>> > > The only problem was that we were redefining the macro. So maybe:
>>> > >
>>> > > #ifndef MAXSYMLINKS
>>> > > #define MAXSYMLINKS 5
>>> > > #endif
>>> > >
>>> > > would be a good solution?
>>> > Why 5 ? (looking at the 20..30 below)
>>> > And why 5 on one system and e.g. on my Mac OS
>>> > #define MAXSYMLINKS 32
>>>
>>> I mentioned "5" because that is the current value of MAXDEPTH. I do
>>> think it would be reasonable to bump it to something higher.
>>>
>>> > Would the same value value for all Git installations on all platforms make sense?
>>> > #define GITMAXSYMLINKS 20
>>>
>>> I think it's probably more important to match the rest of the OS, so
>>> that open("foo") and realpath("foo") behave similarly on the same
>>> system. Though I think even that isn't always possible, as the limit is
>>> dynamic on some systems.
>>>
>>> I think the idea of the 20-30 range is that it's small enough to catch
>>> an infinite loop quickly, and large enough that nobody will ever hit it
>>> in practice. :)
>>
>> I agree that we should have similar guarantees as the OS provides,
>> especially if the OS already has MAXSYMLINKS defined. What then, should
>> the fall back value be if the OS doesn't have this defined? 5 like we
>> have done historically, or something around the 20-30 range as Torsten
>> suggests?
>
> As a fallback I'd rather go for a larger number than too small.
> The reason for the existence is just to break an infinite loop
> (and report an error? which the current code doesn't quite do,
> but your series actually does).
>
> If the number is too large, then it takes a bit longer to generate the error
> message, but the error path is no big deal w.r.t. performance, so it's fine
> for it taking a bit longer.
>
> If the number is too low, then we may hinder people from getting actual
> work done, (i.e. they have to figure out what the problem is and recompile
> git), so I'd think a larger number is not harmful. So 32?
>
I think I agree as well.
Thanks,
Jake
>>
>> --
>> Brandon Williams