Re: [PATCH] block: replace TABs with space

[Kevin Wolf]

Am 09.03.2023 um 19:46 hat Eric Blake geschrieben:
> On Fri, Mar 10, 2023 at 12:10:08AM +0800, Yeqi Fu wrote:
> > Bring the block files in line with the QEMU coding style, with spaces
> > for indentation.
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/371
> > 
> > Signed-off-by: Yeqi Fu <fufuyqqqqqq@gmail.com>
> > ---
> >  block/bochs.c       | 12 +++++------
> >  block/file-posix.c  | 52 ++++++++++++++++++++++-----------------------
> >  block/file-win32.c  | 38 ++++++++++++++++-----------------
> >  block/parallels.c   | 10 ++++-----
> >  block/qcow.c        | 10 ++++-----
> >  include/block/nbd.h |  2 +-
> >  6 files changed, 62 insertions(+), 62 deletions(-)
> > 
> > diff --git a/block/bochs.c b/block/bochs.c
> > index 2f5ae52c90..8ff38ac0d9 100644
> > --- a/block/bochs.c
> > +++ b/block/bochs.c
> > @@ -293,15 +293,15 @@ static void bochs_close(BlockDriverState *bs)
> >  }
> >  
> >  static BlockDriver bdrv_bochs = {
> > -    .format_name   = "bochs",
> > -    .instance_size = sizeof(BDRVBochsState),
> > -    .bdrv_probe            = bochs_probe,
> > -    .bdrv_open             = bochs_open,
> > +    .format_name    = "bochs",
> > +    .instance_size  = sizeof(BDRVBochsState),
> > +    .bdrv_probe     = bochs_probe,
> > +    .bdrv_open      = bochs_open,
> >      .bdrv_child_perm     = bdrv_default_perms,
> >      .bdrv_refresh_limits = bochs_refresh_limits,
> 
> Hmm. When shown in the diff, it looks like you are changing the
> alignment of the .bdrv_probe line; but in reality, the extra prefix
> from diff changes which tabstop the '=' goes out to, so you are
> preserving the original column.  Still, it looks really awkward that
> we have one alignment for .format_name through .bdrv_open, and another
> alignment for .bdrv_child_perm and .bdrv_refresh_limits.
> 
> My personal preference: '=' alignment is a lost cause.  It causes
> pointless churn if we later add a field with a longer name (all other
> lines have to reindent to match the new length).  I'd prefer exactly
> one space before '=' on all lines that you touch (and maybe touch a
> few more lines in the process).  But if we DO stick with alignment,
> I'd much rather that ALL '=' be in the same column, rather than the
> pre-existing half-and-half mix, even though your patch kept that
> visually undisturbed (other than when diff injects a prefix that
> rejiggers the tab stops).

I really appreciate the visual separation between field names and values
that helps me to recognise the functions much faster. Whenever I come
across a file that is formatted like you suggest, it takes me much
longer.

Now I can agree that when doing alignment, having the = in different
columns within a single BlockDriver definition might look a bit awkward,
but as long as it's still done in some kind of blocks, the visual
separation remains and I'm happy enough from a functional perspective.

So I think the existing mix is strictly better than no alignment at all.
In principle I wouldn't object to aligning everything to the same
column, though of course it means additional churn that will affect 'git
blame' etc. Which is why I'm generally not super enthusiastic about
patches changing only the formatting. But I guess if it's a one-time
thing in each file, it's not too bad. Let's just make sure to leave
enough space for the longest field even if a BlockDriver doesn't
currently implement it so that we don't start accumulating new
inconsistencies right afterwards.

Kevin