From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [tarantool-patches] Re: [PATCH] travis-ci: set right flags in release testing jobs References: <64db98703d236c2e6775601a48fb19c3bc8ba955.1556241512.git.alexander.turenko@tarantool.org> <20190428054028.vxdwsboswt5strjl@tkn_work_nb> <20190428213503.bnhfdzwiw4hnxkjw@tkn_work_nb> From: Vladislav Shpilevoy Message-ID: Date: Mon, 29 Apr 2019 13:07:58 +0300 MIME-Version: 1.0 In-Reply-To: <20190428213503.bnhfdzwiw4hnxkjw@tkn_work_nb> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit To: Alexander Turenko , Vladimir Davydov Cc: tarantool-patches@freelists.org List-ID: 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. >