Re: [Tarantool-patches] [PATCH 01/20] net.box: fix console connection breakage when request is discarded

[Vladimir Davydov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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.