[tarantool-patches] Re: [PATCH] travis-ci: set right flags in release testing jobs

[Vladislav Shpilevoy]

LGTM. Vova, please, do a second review.

On 29/04/2019 00:35, Alexander Turenko wrote:
>>>>> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
>>>>> index 2dfc4fc9d..14177aaab 100644
>>>>> --- a/src/box/lua/net_box.c
>>>>> +++ b/src/box/lua/net_box.c
>>>>> @@ -689,12 +689,12 @@ netbox_decode_sql_info(struct lua_State *L, const char **data)
>>>>>  	if (map_size == 2) {
>>>>>  		key = mp_decode_uint(data);
>>>>>  		assert(key == SQL_INFO_AUTOINCREMENT_IDS);
>>>>> -		(void)key;
>>>>> +		(void) key;
>>>>
>>>> 1. Unnecessary diff. Please, drop.
>>>
>>> I don't think so. It is okay to fix a code block that you touch in a
>>> commit to fix it according to a code style.
>>
>> 1. You did not touch this block. This concrete line was not an error nor
>> warning. In this commit you did not touch 'key' variable at all. So
>> this change is unrelated. Please, drop. You just took an arbitrary variable
>> and decided to add a whitespace - this is what is called an unrelated change.
>>
>>>
>>> This would be applicable if I would change some unrelated block of code:
>>
>> This is what you did - 'key' variable is unrelated to other changes and
>> to the commit message and title.
>>
>>> here I'm agree with you.
>>>
> 
> We discussed it voicely. In short: my position was that when you touch a
> code block (or even a function) it is okay to fix the code style within.
> Vlad adds another criteria here: it is okay if changes are big enough.
> 
> This is more or less format rule I can agree with. So I dropped this
> change.
> 
>>>>>  		return -1;
>>>>>  	}
>>>>>  	return 0;
>>>>> @@ -1071,12 +1079,16 @@ xrow_decode_ballot(struct xrow_header *row, struct ballot *ballot)
>>>>>  	ballot->is_ro = false;
>>>>>  	vclock_create(&ballot->vclock);
>>>>>  
>>>>> +	const char *start = NULL;
>>>>> +	const char *end = NULL;
>>>>> +
>>>>>  	if (row->bodycnt == 0)
>>>>>  		goto err;
>>>>> +
>>>>
>>>> 7. Excessive empty line. Please, drop.
>>>
>>> Don't know which empty line you considered as excessive.
>>
>> 5. We both know, that it is the line I've commented above - right
>> below 'goto err'.
>>
>>> If you can link
>>> a guide clause about that or describe a formal rule you want to make
>>> mandatory, then put it here.
>>
>> There is a rule about not making unrelated changes. This is a sense of
>> atomic commits. A change is unrelated, when it stands away from other
>> changes and can be dropped without consequences. Here this empty line
>> 1) stands away from other changes, 2) can be dropped. So it is unrelated.
>>
>> In addition to formal definition of 'commit' we have something even
>> better - Vladimir D. responses in freelists.
>>
>> Grep by 'unrelated':
>> https://www.freelists.org/post/tarantool-patches/PATCH-22-libcorefiber-Allow-to-extend-default-stack-size,1
>> https://www.freelists.org/post/tarantool-patches/PATCH-v2-34-sql-use-space-by-name-in-SQL,1
>>
>>> In the latter case it would be good to
>>> strengthen it with examples from our code. (However I propose to don't
>>> spend time on that: it is counter-productive.)
>>
>> It *is* productive. Because it teaches how to make diffs more atomic
>> and simple. It makes reviewer's life easier and simplifies search for
>> errors - the smaller is diff, the easier it is to find all errors.
>>
>> Talking of 'code examples' - it is not about code. It is about
>> commits and their diff. First version of this commit looked like a
>> scattered number of '+' and '-' many of which were like:
>>
>> ---
>> -	(void)key;
>> +	(void) key;
>> ---
>>  	if (row->bodycnt == 0)
>>  		goto err;
>> +
>>  	assert(row->bodycnt == 1);
>> ---
>>
>> And some of the others were just unnecessary.
>>
>> Obviously, it does not make review process easier. I needed to somehow
>> determine which of these changes were about the bug, which of them were
>> just about your personal taste of adding empty lines in the code you've
>> seen alongside. It is not ok. If you want to fix code style, then send
>> a separate commit. This commit is declared as
>> 'set right flags in release testing jobs' - how do these new empty lines
>> and new whitespaces in the random code relate to the commit title and to
>> the given changes?
>>
>> Together with the changes proposed by me in this mail, we've dropped
>> from your original commit already 20 lines of pure diff, both of '+'
>> and '-'.
> 
> I think it is okay to push some amount of code style changes together
> with significant changes. The question is about a balance between
> readability for reviewers and readers and converging a code quality to
> an ideal across a time. Both ends (lack of a balance) is not good.
> 
> In such questions I usually lean on intuitive understanding of
> maintainers and valuable contributors where the balance should be.
> 
> In this case I made xrow_decode_dml() and xrow_decode_ballot() a bit
> more similar in the sense how blocks are separated. This is mostly a
> thing of preference, so I don't mind to remove this empty line.
> 
> So I changed it as you suggested.
>