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?
>
> --
> Brandon Williams