Re: [PATCH v4 0/5] road to reentrant real_path

On Wed, Jan 4, 2017 at 10:22 AM, Stefan Beller [off-list ref] wrote:

On Wed, Jan 4, 2017 at 10:13 AM, Brandon Williams [off-list ref] wrote:

quoted

On 01/04, Jeff King wrote:

quoted

On Wed, Jan 04, 2017 at 07:56:02AM +0100, Torsten Bögershausen wrote:

quoted

On 04.01.17 01:48, Jeff King wrote:

quoted

On Tue, Jan 03, 2017 at 11:09:18AM -0800, Brandon Williams wrote:

quoted

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.

quoted

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

quoted

--
Brandon Williams