On Tue, Feb 19, 2013 at 5:54 PM, Junio C Hamano <gits...@pobox.com> wrote: > Adam Spiers <g...@adamspiers.org> writes: > >> Fix a corner case where check-ignore would segfault when run with the >> '.' argument from the top level of a repository, due to prefix_path() >> converting '.' into the empty string. It doesn't make much sense to >> call check-ignore from the top level with '.' as a parameter, since >> the top-level directory would never typically be ignored, but of >> course it should not segfault in this case. >> >> Signed-off-by: Adam Spiers <g...@adamspiers.org> >> --- > > Please step back a bit and explain why the original had check for > path in the first place?
I can't remember to be honest. > If the answer is "the code wanted to special case the question 'is > the top-level excluded?', Yes, I think that's the most likely explanation. Maybe it got missed in a variable renaming refactoring. > but used a wrong variable to implement the > check, and this patch is a fix to that", then the proposed commit > log message looks incomplete. The cause of the segv is not that > prefix_path() returns an empty string, but because the function > called inside the "if" block was written without expecting to be fed > the path that refers to the top-level of the working tree, no? > > While this change certainly will prevent the "check the top-level" > request to last-exclude-matching-path, I have to wonder if it is a > good idea to force the caller of the l-e-m-p function to even care. > > In other words, would it be a cleaner approach to fix the l-e-m-p > function so that the caller can ask "check the top-level" and give a > sensible answer (perhaps the answer may be "nothing matches"), and > remove the "&& path" (or "&& full_path") special case from > this call site? Yes, that did cross my mind. I also wondered whether hash_name() should do stricter input validation, but I guess that could have an impact on performance. > The last sentence "It doesn't make much sense..." in the proposed > log message would become a good justification for such a special > case at the beginning of l-e-m-p function, I would think. Fair enough. I'll reply to this with a new version.  I wish there was a clean way to include the new version inline, but as I've noted before, there doesn't seem to be: http://article.gmane.org/gmane.comp.version-control.git/146110 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html