Re: [PATCHv2] Makefile: add missing phony target

[Matthieu Moy]

Elia Pinto  writes:

> This is the second version of this patch.
> Added the corrections suggested by Matthieu Moy ($gmane/282221)

Sorry, but my main concern was that the patch could not be reviewed in
good conditions as-is, and I think it still cannot be. It's very hard to
spot which .PHONY rules you're adding and which are just code movement.
You should really split this into one "code movement" patch and one
"actual bugfix" patch. Or someone with better eyes than me should review
the patch ;-).

> @@ -2215,6 +2221,7 @@ test-svn-fe$X: vcs-svn/lib.a
>  test-%$X: test-%.o GIT-LDFLAGS $(GITLIBS)
>   $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) 
> $(filter %.a,$^) $(LIBS)
>  
> +.PHONY: check_sha1 $(SP_OBJ) sparse check check-sha1
>  check-sha1:: test-sha1$X
>   ./test-sha1.sh
>  
> @@ -2224,7 +2231,6 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
>   $(QUIET_SP)cgcc -no-compile $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) \
>   $(SPARSE_FLAGS) $<
>  
> -.PHONY: sparse $(SP_OBJ)
>  sparse: $(SP_OBJ)

This "sparse" movement looks again contradictory with the goal announced
in the commit message.

> @@ -2237,6 +2243,7 @@ check: common-cmds.h
>   exit 1; \
>   fi
>  
> +
>  ### Installation rules

Useless hunk.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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

Re: [PATCHv2] Makefile: add missing phony target

[Elia Pinto]

2015-12-11 15:40 GMT+01:00 Matthieu Moy :
> Elia Pinto  writes:
>
>> This is the second version of this patch.
>> Added the corrections suggested by Matthieu Moy ($gmane/282221)
>
> Sorry, but my main concern was that the patch could not be reviewed in
> good conditions as-is, and I think it still cannot be. It's very hard to
> spot which .PHONY rules you're adding and which are just code movement.
> You should really split this into one "code movement" patch and one
> "actual bugfix" patch. Or someone with better eyes than me should review
> the patch ;-).

Ok. No problem. I thought there was no need for a patch so simple. But ok.

Thank you. I will reroll.
>
>> @@ -2215,6 +2221,7 @@ test-svn-fe$X: vcs-svn/lib.a
>>  test-%$X: test-%.o GIT-LDFLAGS $(GITLIBS)
>>   $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) 
>> $(filter %.a,$^) $(LIBS)
>>
>> +.PHONY: check_sha1 $(SP_OBJ) sparse check check-sha1
>>  check-sha1:: test-sha1$X
>>   ./test-sha1.sh
>>
>> @@ -2224,7 +2231,6 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
>>   $(QUIET_SP)cgcc -no-compile $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) \
>>   $(SPARSE_FLAGS) $<
>>
>> -.PHONY: sparse $(SP_OBJ)
>>  sparse: $(SP_OBJ)
>
> This "sparse" movement looks again contradictory with the goal announced
> in the commit message.
The idea was to group all the phony before all the target, not to put
the phony necessarily before the closest target. but ok
>
>> @@ -2237,6 +2243,7 @@ check: common-cmds.h
>>   exit 1; \
>>   fi
>>
>> +
My bad :=)
>>  ### Installation rules
>
> Useless hunk.
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/
--
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

Re: [PATCHv2] Makefile: add missing phony target

[Matthieu Moy]

Elia Pinto  writes:

> 2015-12-11 15:40 GMT+01:00 Matthieu Moy :
>> Elia Pinto  writes:
>>
>>> This is the second version of this patch.
>>> Added the corrections suggested by Matthieu Moy ($gmane/282221)
>>
>> Sorry, but my main concern was that the patch could not be reviewed in
>> good conditions as-is, and I think it still cannot be. It's very hard to
>> spot which .PHONY rules you're adding and which are just code movement.
>> You should really split this into one "code movement" patch and one
>> "actual bugfix" patch. Or someone with better eyes than me should review
>> the patch ;-).
>
> Ok. No problem. I thought there was no need for a patch so simple. But ok.

The point is: once a tricky bug was found in a patch (and I did on v1),
you cannot claim anymore that it is "so simple". If it was that simple,
you would have cought it before sending.

>>> @@ -2215,6 +2221,7 @@ test-svn-fe$X: vcs-svn/lib.a
>>>  test-%$X: test-%.o GIT-LDFLAGS $(GITLIBS)
>>>   $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter 
>>> %.o,$^) $(filter %.a,$^) $(LIBS)
>>>
>>> +.PHONY: check_sha1 $(SP_OBJ) sparse check check-sha1
>>>  check-sha1:: test-sha1$X
>>>   ./test-sha1.sh
>>>
>>> @@ -2224,7 +2231,6 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
>>>   $(QUIET_SP)cgcc -no-compile $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) \
>>>   $(SPARSE_FLAGS) $<
>>>
>>> -.PHONY: sparse $(SP_OBJ)
>>>  sparse: $(SP_OBJ)
>>
>> This "sparse" movement looks again contradictory with the goal announced
>> in the commit message.
> The idea was to group all the phony before all the target, not to put
> the phony necessarily before the closest target. but ok

I personally prefer the old way. I have no strong objection to changing,
but currently your commit message says "Also put the .PHONY declaration
immediately before the target declaration", which is clearly not a
justification to do this change.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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