Re: [PATCH v3] Add support for GIT_CEILING_DIRS

[David Reiss <dreiss@facebook.com>]

> By now, I strongly believe that these changes are too large.  I am
> convinced that what you desire can be expressed much simpler, and thus
> less error-prone.
Most of the code is in the one function to parse out the colon-separated
environment variable value and compute the longest directory prefix.
I'm not convinced this can be made much simpler.  (Using strtok_r could
help, but would require an allocation.)  Most of the rest of the changes
are test code and indentation.
 
> Also, I think that your test cases are too extensive.  While it is usually
> good to have exhaustive tests, running them takes time.  And if it takes
> so much time that hardly anybody bothers with running the test suite,
> there are _too_ many tests.
I am more than happy to remove most of them, leaving only basic sanity
checks.  However, I would prefer to leave them in but comment them out,
so that if I or someone else wants to modify this code later, they would
be able to run a more extensive test suite.  I'll submit a modified
patch with this change.

> You know, I wonder why I even bothered writing those responses, if you
> just ignore them.
I must say that I am very confused.  I thought I followed all of your
responses to the letter.  Could you please be more specific about the
ones I missed?  I'm happy to make further changes.

> This has _no_ place in the Git source code.  Have you looked around, and
> found similar dead code?  No?  That's because Git's source code is not a
> graveyard of useless code bits, but it is a collection of elegant code. 
> Mostly, at least.
As I stated in "PATCH v2", I was unsure what the convention was for unit
tests like this.  Most of the git code is (obviously) functional tests,
but it is impossible to test how this code would behave with a git
directory under "/" using a functional test, unless it was run as root.
Someone just pointed out to me that there are some C-based tests (like
test-sha1) that are run from "make test".  I guess I can move the test
function to a new one of those, but it will require making
longest_ancestor_length extern.

> Instead of wasting my time further, I will try to come up with a better
> implementation, as is the way of Open Source.
I am sorry if this has wasted your time.  I really have been trying to
incorporate your feedback into my patch, and the code has definitely
improved as a result.  However, my main goal is simply to get this
feature working (I have already patched it into my own git build, and it
has saved me a lot of time), so if you come up with a better
implementation, that would be great!

--David