| Message ID | 1468501843-14927-1-git-send-email-caoj.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On 07/14/2016 07:10 AM, Cao jin wrote: > replace tab with spaces > > Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> > --- > async.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Whitespace-only changes are best done as part of a series that is already touching nearby code for other reasons (depending on the size of the whitespace changes and on the rest of your patch, it may be okay to squash the whitespace change in place, or better to split into separate patches to make review of both patches easier). Otherwise, it just makes 'git blame' output dirtier. > > diff --git a/async.c b/async.c > index 1f9754b..8589017 100644 > --- a/async.c > +++ b/async.c > @@ -217,7 +217,7 @@ aio_ctx_check(GSource *source) > for (bh = ctx->first_bh; bh; bh = bh->next) { > if (!bh->deleted && bh->scheduled) { > return true; > - } > + } Nothing wrong with the patch itself, and I won't oppose it going in, but it's generally not worth the effort if nothing else in this file needs fixing.
On 07/14/2016 10:08 PM, Eric Blake wrote: > On 07/14/2016 07:10 AM, Cao jin wrote: >> replace tab with spaces >> >> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> >> --- >> async.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > Whitespace-only changes are best done as part of a series that is > already touching nearby code for other reasons (depending on the size of > the whitespace changes and on the rest of your patch, it may be okay to > squash the whitespace change in place, or better to split into separate > patches to make review of both patches easier). Otherwise, it just > makes 'git blame' output dirtier. I see. Since async.c & aio-posix.c are belong to the same maintaiers, so, Fam & Stefan, is it ok to squash this into that "remove useless parameter" patch? If not, we can just forget this one.
On Fri, Jul 15, 2016 at 09:48:50AM +0800, Cao jin wrote: > On 07/14/2016 10:08 PM, Eric Blake wrote: > > On 07/14/2016 07:10 AM, Cao jin wrote: > > > replace tab with spaces > > > > > > Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> > > > --- > > > async.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > Whitespace-only changes are best done as part of a series that is > > already touching nearby code for other reasons (depending on the size of > > the whitespace changes and on the rest of your patch, it may be okay to > > squash the whitespace change in place, or better to split into separate > > patches to make review of both patches easier). Otherwise, it just > > makes 'git blame' output dirtier. > > I see. > Since async.c & aio-posix.c are belong to the same maintaiers, so, Fam & > Stefan, is it ok to squash this into that "remove useless parameter" patch? > If not, we can just forget this one. The "remove useless parameter" patch doesn't touch the function you are modifying here. Please don't squash the patches. Since you have already posted this patch I will merge it. In the future please don't submit whitespace changes, tiny coding style cleanups, etc in by themselves. Thanks for all your contributions. I do not want to discourage you but my view is that code changes should only be made if they fix a bug, improve performance measurably, add a feature, or significantly improve the code. Every patch has a cost in terms of code review, merging/testing, backporting, bisecting, documentation, etc. We could discuss each of these in detail but basically a code change creates work or takes time from one or more people in these areas. In a perfect world with unlimited resources all patches would be equally welcome. Due to limited resources it's best to submit the types of patches I mentioned above where the cost/benefit ratio is favorable. Thanks, Stefan
On Thu, Jul 14, 2016 at 09:10:43PM +0800, Cao jin wrote: > replace tab with spaces > > Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> > --- > async.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Thanks, applied to my block tree: https://github.com/stefanha/qemu/commits/block Stefan
On 07/15/2016 06:40 PM, Stefan Hajnoczi wrote: > On Fri, Jul 15, 2016 at 09:48:50AM +0800, Cao jin wrote: >> On 07/14/2016 10:08 PM, Eric Blake wrote: >>> On 07/14/2016 07:10 AM, Cao jin wrote: >>>> replace tab with spaces >>>> >>>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> >>>> --- >>>> async.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> Whitespace-only changes are best done as part of a series that is >>> already touching nearby code for other reasons (depending on the size of >>> the whitespace changes and on the rest of your patch, it may be okay to >>> squash the whitespace change in place, or better to split into separate >>> patches to make review of both patches easier). Otherwise, it just >>> makes 'git blame' output dirtier. >> >> I see. >> Since async.c & aio-posix.c are belong to the same maintaiers, so, Fam & >> Stefan, is it ok to squash this into that "remove useless parameter" patch? >> If not, we can just forget this one. > > The "remove useless parameter" patch doesn't touch the function you are > modifying here. Please don't squash the patches. > > Since you have already posted this patch I will merge it. In the future > please don't submit whitespace changes, tiny coding style cleanups, etc > in by themselves. > > Thanks for all your contributions. I do not want to discourage you but > my view is that code changes should only be made if they fix a bug, > improve performance measurably, add a feature, or significantly improve > the code. > > Every patch has a cost in terms of code review, merging/testing, backporting, > bisecting, documentation, etc. We could discuss each of these in detail > but basically a code change creates work or takes time from one or more > people in these areas. > > In a perfect world with unlimited resources all patches would be equally > welcome. Due to limited resources it's best to submit the types of > patches I mentioned above where the cost/benefit ratio is favorable. > > Thanks, > Stefan > Thanks Stefan, and sorry for the inconvenience brought to you. I thought this kind of tiny things would be very simple for maintainers, now I understand
diff --git a/async.c b/async.c index 1f9754b..8589017 100644 --- a/async.c +++ b/async.c @@ -217,7 +217,7 @@ aio_ctx_check(GSource *source) for (bh = ctx->first_bh; bh; bh = bh->next) { if (!bh->deleted && bh->scheduled) { return true; - } + } } return aio_pending(ctx) || (timerlistgroup_deadline_ns(&ctx->tlg) == 0); }
replace tab with spaces Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> --- async.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)