[Tarantool-patches] [PATCH 01/20] net.box: fix console connection breakage when request is discarded
Vladimir Davydov
vdavydov at tarantool.org
Thu Jul 29 13:40:00 MSK 2021
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 at . 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.
More information about the Tarantool-patches
mailing list