Re: [Outreachy kernel] Re: [PATCH] Correct indentation in Hal8723BReg.h

["Fabio M. De Francesco" <fmdefrancesco@gmail.com>]

On Tuesday, April 6, 2021 10:01:55 AM CEST Julia Lawall wrote:
> 
> On Tue, 6 Apr 2021, FMDF wrote:
> 
> > On Tue, Apr 6, 2021 at 9:27 AM Julia Lawall <julia.lawall@inria.fr> wrote:
> > >
> > >
> > >
> > > On Tue, 6 Apr 2021, Fabio M. De Francesco wrote:
> > >
> > > > On Tuesday, April 6, 2021 8:40:37 AM CEST Julia Lawall wrote:
> > > > >
> > > > > On Tue, 6 Apr 2021, Fabio M. De Francesco wrote:
> > > > >
> > > > > > On Tuesday, April 6, 2021 7:52:52 AM CEST Fabio M. De Francesco wrote:
> > > > > > > On Tuesday, April 6, 2021 7:12:20 AM CEST Greg KH wrote:
> > > > > > > > On Tue, Apr 06, 2021 at 04:08:15AM +0200, Fabio M. De Francesco wrote:
> > > > > > > > > Correct indentation issues of many #define and comments in Hal8723BReg.h
> > > > > > > > >
> > > > > > > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > > > > > > > ---
> > > > > > > > >  drivers/staging/rtl8723bs/hal/Hal8723BReg.h | 604 ++++++++++----------
> > > > > > > > >  1 file changed, 302 insertions(+), 302 deletions(-)
> > > > > > > >
> > > > > > > > You forgot to cc: all of the needed mailing lists.  And your subject
> > > > > > > > line needs some work.  But the biggest issue is that I do not understand
> > > > > > > > what this patch does at all.  What is wrong with the original code here
> > > > > > > > that required you to "correct" it?
> > > > > > > >
> > > > > > > > confused,
> > > > > > > >
> > > > > > > > greg k-h
> > > > > > > >
> > > > > > > I'm sorry that I forgot to cc linux-staging and linux-kernel mailing lists
> > > > > > > and for the subject I had already sent a v2 patch that had
> > > > > > > all the required tags (staging: rtl8723: hal:).
> > > > > > >
> > > > > > > I think that what was wrong with the original code was that
> > > > > > > the #define values were far from the symbols with random indentation.
> > > > > > >
> > > > > > > For example, I changed something like:
> > > > > > > #define SYMB1           0123
> > > > > > > #define SYMB2 5678
> > > > > > > #define SYMB3            9ABC
> > > > > > >
> > > > > > > to:
> > > > > > > #define SYMB1   0123
> > > > > > > #define SYMB2   5678
> > > > > > > #define SYMB3                   9ABC
> > > > > >
> > > > > > the preceding defined values are in a single column. I don't understand why KMail show them in different levels of indentation. However, I hope that my argument in support of the patch was still clear.
> > > > >
> > > > > There is surely a difference with the use of spaces and tabs.  If you
> > > > > change something about the defines and this problem shows up, you can make
> > > > > another patch in the series to make the spacing more uniform.
> > > > >
> > > > > julia
> > > > >
> > >
> > > > I have not this problem in the file in my git/kernels/staging directory,
> > >
> > > Are you just looking at the file in your editor, or have you actually
> > > counted the spaces and tabs?
> > >
> > > julia
> >
> > I'm just looking at the file using Vim and all seems perfectly aligned,
> > but now
> 
> Some code may be
> 
> #define NAMEXXXXXXXX27
> #define NAMET27
> 
> These may look the same in vim.  That is 8 spaces = 1 tab (I realize that
> this is a massive oversimplification :).  If you start on the left and
> move one space over at a time you will see the difference, because it will
> move over the entire tab in one step.
> 
> When you turn the code into a patch, one space (or - or +) gets added to
> the front of every line.  If a tab is used in the middle of the line, this
> will often have no effect on most of the line.  If spaces are used, the
> whole line will move over by one.  Thus the code can look more unevenly
> indented in the patch than in the original file.

Ok, I understand that I have to be very careful when working with spaces and tabs because 
they have different side effects on formatting patches, but...
How can developers check that the patches they are going to send with git are 
properly indented? I mean, that the patches exactly represent what one sees in vim or in less? 

Apart the patch we are talking about in this thread, there is more: checkpatch.pl emits 
warnings when certain lines are not aligned with an open parenthesis of a previous 
line. How to do proper alignment? Using +/- spaces to align can lead to strange results 
(if I have understood what you wrote). How to make a good alignment without incurring
in undesired side effects that propagate in the resulting patch?

Thanks,

Fabio  

> 
> But working with code is not about just looking at it, but also moving
> from place to place in it to change things.  One can move across tabs more
> efficiently, so it could be nice that if the developer already started
> using tabs sometime, then to use them all the time,.
> 
> julia
> 
> >
> > I am not sure to understand what you mean about "counting the spaces and tabs".
> >
> > However, what I did when I found lines with random indentation was:
> > 1) to delete all the spaces and all the tabs that were placed between
> > the #define
> > directive and the defined value;
> > 2) add a single space in order to separate
> > the value from the directive and two (8 character standard) tabs for align the
> > current value under the one of the previous line;
> >
> > I'm preparing a v3 patch to explain what is wrong with the existing code
> > and why [I think] my change makes it better. Probably you've already seen
> > that Greg has responded asking just this.
> >
> > Thanks a lot,
> >
> > Fabio
> >
> > >
> > > > it just happen when I see that patch in KMail. When I open it with Vim,
> > > > all the defines are correctly aligned in a single column (that is, the
> > > > correction I made to the indentation levels of the defines is there). As
> > > > a consequense, I suppose that the patch itself is correctly formatted.
> > > > Doesn't it?
> > > >
> > > > However, what is really important is what Greg K-H thinks of the patch. I'm not sure I was able to explain to him what I've made and why it improves the readibility of the whole file (may you please take a look at the original in Greg's tree? You would see that random levels of indentation I'm talking about). As you can read above, I asked him if I should make a v3 of that with a clearer explanation about the work I've made on those random levels of indentation but I haven't yet had his response.
> > > >
> > > > What do you personally think about producing a v3 patch with a better explanation of the work I've made?
> > > >
> > > > Thanks for your time and patience,
> > > >
> > > > Fabio
> > > >
> > > >
> > > >
> > > >
> > > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/CAPj211vtzZDmwYomg9BBcZwPWxiiwD55%3DyEpcAXL2WgzGyvUDQ%40mail.gmail.com.
> >
>