Adam Spiers <git@xxxxxxxxxxxxxx> writes:
>> Remove a sweep-the-issue-under-the-rug conditional in check-ignore
>> that avoided to pass an empty string to the callchain while at it.
>> It is a valid question to ask for check-ignore if the top-level is
>> set to be ignored by default, even though the answer is most likely
>> no, if only because there is currently no way to specify such an
>
> Hmm, I see very little difference between the use of "most likely" and
> the use of the words "much" and "typically" which you previously
> considered "a sure sign that the design of the fix is iffy".
Your patch were "The reason why feeding empty string upsets
hash_name() were not investigated; by punting the '.' as input, and
ignoring the possibility that such a question might make sense, I
can work around the segfault. I do not even question if hash_name()
that misbehaves on an empty string is a bug. Just make sure we do
not tickle the function with a problematic input".
The patch you are responding to declares that hash_name() should
work sensibly on an empty string, and that is the _only_ necessary
change for the fix. We could keep "&& path[0]", but even without
it, by fixing the hash_name(), we will no longer segfault.
My "most likely" is about "the special case '&& path[0]' produces
correct result, and it is likely to stay so in the near future until
we update .gitignore format to allow users to say 'ignore the top by
default', which is not likely to happen soon". It is not about the
nature of the fix at all.
Still do not see the difference?
The removal of the "&& path[0]" is about allowing such a question
whose likeliness may be remote. In the current .gitignore format,
you may not be able to say "ignore the whole thing by default", so
in that sense, the answer to the question this part of the code is
asking when given "." may always be "no". Keeping the "&& path[0]"
will optimize for that case.
And "unusual thing to ask" below is to judge if answering such a
question is worth optimizing for (the verdict is "no, it is not a
common thing to do").
>> entry in the .gitignore file. But it is an unusual thing to ask and
>> it is not worth optimizing for it by special casing at the top level
>> of the call chain.
>
> Although I agree with your proposed patch's sentiment of avoiding
> sweeping this corner case under the rug, 'check-ignore .' still
> wouldn't match anything if for example './' was a supported mechanism
> for ignoring the top level.
It indicates that there may be more bugs (that may not result in
segv) somewhere in check-ignore codepath, if (1)
echo ./ >.gitignore
were to say "ignore everything in the tree by default.", and (2) the
real ignore check does work that way, but (3)
git check-ignore .
says "we do not ignore that one". Such a bug may come from some
code that is not prepared to see an empty pathname that refers to
the top-level in the codepath, which was why I originally asked
Does the callchain that goes down to this function have other places
that assume they will never see an empty string, like this function
does, which I _think_ is the real issue?
in one of the previous messages.
But is
echo ./ >.gitignore
a way to say "ignore everything in the tree by default" in the first
place? I think historically that has never been the case, I recall
that the list had discussions on that topic in the past (it may be
before you appeared here), and I do not recall we reached a concensus
that we should make it mean that nor applied a patch to do so.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html