From: Alexander Turenko <alexander.turenko@tarantool.org> To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Cc: tarantool-patches@freelists.org Subject: [tarantool-patches] Re: [PATCH] travis-ci: set right flags in release testing jobs Date: Mon, 29 Apr 2019 00:35:04 +0300 [thread overview] Message-ID: <20190428213503.bnhfdzwiw4hnxkjw@tkn_work_nb> (raw) In-Reply-To: <dbf16df6-f9e6-7452-2fc7-96c88f71b4d6@tarantool.org> > >>> 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.
next prev parent reply other threads:[~2019-04-28 21:35 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-04-26 1:29 [tarantool-patches] " Alexander Turenko 2019-04-26 10:18 ` [tarantool-patches] " Vladislav Shpilevoy 2019-04-28 5:40 ` Alexander Turenko 2019-04-28 8:54 ` Vladislav Shpilevoy 2019-04-28 9:11 ` Vladislav Shpilevoy 2019-04-28 21:38 ` Alexander Turenko 2019-04-28 21:35 ` Alexander Turenko [this message] 2019-04-29 10:07 ` Vladislav Shpilevoy 2019-04-29 12:57 ` [PATCH v2] " Alexander Turenko 2019-04-29 17:09 ` Vladimir Davydov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190428213503.bnhfdzwiw4hnxkjw@tkn_work_nb \ --to=alexander.turenko@tarantool.org \ --cc=tarantool-patches@freelists.org \ --cc=v.shpilevoy@tarantool.org \ --subject='[tarantool-patches] Re: [PATCH] travis-ci: set right flags in release testing jobs' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox