From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id BBF4C6EC55; Thu, 29 Jul 2021 13:40:03 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org BBF4C6EC55 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1627555203; bh=RBDqnzSU/KX1cg12wfOZ8TD5VaM9zKv3TOORnTL1CGA=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=A4hj3bPp8Fnzxx/4BznJFUo9psaTu5w4gdZoiq6bfB1S04iktcf3/EwiAxNon0QBW CvW2bqr/rWVc2VJrK4aEA2j8+K4CLG2W5hQ4/837GIrPxQH8OTys5TP2MBkwn4PGzz mreOo5Y1p/nAGhzUvyglMpOlZZ7zc3VI43eAak8Y= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [94.100.181.251]) (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 6F7656EC55 for ; Thu, 29 Jul 2021 13:40:02 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 6F7656EC55 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1m93SL-0001BB-Ix; Thu, 29 Jul 2021 13:40:02 +0300 Date: Thu, 29 Jul 2021 13:40:00 +0300 To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Message-ID: <20210729104000.pnvqf6u3xnjadsvv@esperanza> References: <3180fd58-9239-6007-e8ce-2d779defb17d@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3180fd58-9239-6007-e8ce-2d779defb17d@tarantool.org> X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD941C43E597735A9C33D83595CA30D6DC5179D1C9A908C47E5182A05F538085040591BB65205A398A9879EAF100A09B6C3D7B812FDA9FB9554F77F684A120C607A X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7DECE8D0A5E25C0FCEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637AA51C5C0FFD146988638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D89BD26BC27F1C4A27AFE91479AA3961FE117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC8C7ADC89C2F0B2A5A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F44604297287769387670735201E561CDFBCA1751FCB629EEF1311BF91D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6753C3A5E0A5AB5B7089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A527A7DFD05704E2251DC0BC53FCD78726C49BF2326A70D820D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA753530422897FB34C3410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3472E5ECC12A9739C1549839F3F1E15B92AE0B4C27BFC2C99652C9DC938A0F08E3993FAA058C0304541D7E09C32AA3244CF8020F9CC1A6D218EC10EDF4709EE9BBF26BFA4C8A6946B8FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojPp/mPgZxawGZ+UwhZHAFmw== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5DE1C5D9AC277C160D64E598EB717E51CE274CEFED1673C562683ABF942079399BFB559BB5D741EB966A65DFF43FF7BE03240331F90058701C67EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH 01/20] net.box: fix console connection breakage when request is discarded X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Vladimir Davydov via Tarantool-patches Reply-To: Vladimir Davydov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" On Thu, Jul 29, 2021 at 12:49:17AM +0200, Vladislav Shpilevoy wrote: > Thanks for the patch! > > The commit is different from what I see on the branch. Please, > try to keep the email thread up to date. Or do you prefer the > github PRs now? I just added changelogs and references to tickets so didn't bother to resend. Sorry about that. The PR is for alyapunov@. FWIW I don't like PRs, I don't like emails, either, but I think there should be one and only one way to submit a patch for review... Some thoughts: - Divergence between a branch and patches sent via email may be dangerous, because one may forget to push a branch or resend emails. Ideally, if patches are sent via email, they should be applied with git-am. - git-am is not particularly user-friendly. It's easier to pull patches from a branch. This is a huge plus for PRs. - Unfortunately, PRs are not very convenient when it comes to reviewing patch series because of the nature of git, which was initially designed for reviewing patches by email. Since there's no versioning of the same commit (commit sha changes on each amend), PR comments to a patch cannot be linked to the same commit after amending the patch. AFAIU PRs require pushing fix-commits instead of amending and squashing before push, which simply doesn't work for huge patch sets. In general, emails are more flexible. - This is unrelated to reviews, but I hate seeing tons of comments in a GitHub issue mentioning that someone pushed his private branch mentioning the issue to the Tarantool repository. I think we should prohibit pushing branches to the main repository and make everyone fork it instead. > > Here and in other emails I will use the commits I see on the > branch. > > > diff --git a/test/box/net.box_discard_console_request.result b/test/box/net.box_discard_console_request.result > > new file mode 100644 > > index 000000000..e8da50a2f > > --- /dev/null > > +++ b/test/box/net.box_discard_console_request.result > > For bug tests we now use name format `gh-####-your-description`. > The other names different from this format are outdated and should > not be used as an example. > https://github.com/tarantool/tarantool/wiki/Code-review-procedure#testing All net.box tests have prefix net_box_ and add gh-#### as a suffix. So I renamed the test to net.box_discard_console_request_gh-6249. > > The rest of the commit LGTM. Thanks. Pushed to the master branch, 1.10, 2.7, and 2.8.