Re: [PATCH v3] Add support for GIT_CEILING_DIRS

From: David Reiss <dreiss@facebook.com>
Date: May 15, 2008, 19:40 UTC
Message-ID: <482C91BD.5070504@facebook.com>
In-Reply-To: <alpine.DEB.1.00.0805151958180.30431@racer>

> 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