From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp3.mail.ru (smtp3.mail.ru [94.100.179.58]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id C770C45C305 for ; Sun, 6 Dec 2020 04:52:06 +0300 (MSK) Date: Sun, 6 Dec 2020 04:52:09 +0300 From: Alexander Turenko Message-ID: <20201206015208.42nc3o3dmg3df6c6@tkn_work_nb> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH v2] Move .tarantoolctl to test-run tool submodule List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Alexander V. Tikhonov" Cc: tarantool-patches@dev.tarantool.org On Tue, Dec 01, 2020 at 05:58:36PM +0300, Alexander V. Tikhonov wrote: > Moved .tarantoolctl configuration file to test-run tool submodule > repository as: > > /test-run/.tarantoolctl > > Also set backward compability to use old path location: > > /test/.tarantoolctl > > as its primary place. > > Updated tests with replication_sync_timeout check value. Set it to > hidden value due to it could be set the other than default in options > at test-run run command. > > Found that no need to copy tarantoolctl configuration file to binary > path any more, after it was moved to test-run repository, so reverting > changes from: > > aa609de2d1afeb8f99ae1630f9274e8f18a6cfea ('cmake for tests updated: copy ctl config in builddir') > > Needed for #5504 > --- > > Github: https://github.com/tarantool/tarantool/tree/avtikhon/move_tarantoolctl_config > Issue: https://github.com/tarantool/tarantool/issues/5504 The patch is okay. I made the following tweaks: 1. Added 'test' prefix. 2. Carried the long line. 3. Changed the first part of the message to clarify that the file was placed into test-run by the previous commit and just removed here. 4. Clarified how removal of the .tarantoolctl affects the behaviour. The rest of the commit message is also dubious. For example, 'Updated tests with replication_sync_timeout check value'. With what? What is the 'replication_sync_timeout check value'? I left it as is, but, please, take care to grammar and how others may understand you. Your task is to explain non-trivial things that were changed. It is not something that should be in a reader responsibility: don't understand -- go ahead. No. See any Vlad's commit message for inspiration. I don't know almost anything about Raft and its implementation details, but I'm able to understand an idea of a commit. Pushed to master, 2.6, 2.5, 1.10.