Re: What's cooking in git.git (Sep 2018, #01; Tue, 4)

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Sun, Nov 25 2018, Torsten Bögershausen wrote:

> On Sun, Nov 25, 2018 at 05:28:35AM +0100, Torsten Bögershausen wrote:
>> On Sat, Nov 24, 2018 at 08:33:37PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> >
>> > On Wed, Sep 05 2018, Ævar Arnfjörð Bjarmason wrote:
>> >
>> > > On Wed, Sep 05 2018, Eric Sunshine wrote:
>>
>> []
>>
>> > > SunCC used to be ahead of GCC & Clang when it came to certain classes of
>> > > warnings, but e.g. now everything it complains about is because it
>> > > doesn't understand C as well, e.g. we have quite a few compile warnings
>> > > due to code like this, which it claims is unreachable (but isn't):
>> > > https://github.com/git/git/blob/v2.19.0-rc2/read-cache.c#L950-L955
>> >
>>
>> Wait a second - even if the compiler claims something (wrong)...
>> there a still 1+1/2 questions from my side:
>>
>>
>> int verify_path(const char *path, unsigned mode)
>> {
>> 	char c;
>> 	     ^
>> 	/* Q1: should  "c" be initialized like this: */
>> 	char c = *path;
>>
>> 	if (has_dos_drive_prefix(path))
>> 		return 0;
>>
>> 	goto inside;
>> 	^^^^^^^^^^^^ /* Q2: and why do we need the "goto" here ? */
>> 	for (;;) {
>> 		if (!c)
>> 			return 1;
>> 		if (is_dir_sep(c)) {
>> inside:
>
> After some re-reading,
> I think that the "goto inside" was just hard to read....
>
> Out of interest:
> would the following make the compiler happy ?
>
>
> diff --git a/read-cache.c b/read-cache.c
> index 49add63fe1..d574d58b9d 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -951,17 +951,15 @@ static int verify_dotfile(const char *rest, unsigned mode)
>
>  int verify_path(const char *path, unsigned mode)
>  {
> -	char c;
> +	char c = *path ? '/' : '\0';
>
>  	if (has_dos_drive_prefix(path))
>  		return 0;
>
> -	goto inside;
>  	for (;;) {
>  		if (!c)
>  			return 1;
>  		if (is_dir_sep(c)) {
> -inside:
>  			if (protect_hfs) {
>  				if (is_hfs_dotgit(path))
>  					return 0;

I haven't tested (it's tedious) but yes, I can tell you SunCC won't
whine about this. I've only seen its unreachability detector get
confused about goto inside a loop, not the the sort of code you've
replaced this with.

We should not be appeasing these old compiler warnings in cases where
they're wrong. You can check out the CI output I linked to to see 10-20
cases in the codebase where SunCC is wrong about unreliability.

Whether we should just fix this for its own sake is another question.