Tarantool development patches archive
 help / color / mirror / Atom feed
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.

  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