[Fwd: Re: [PATCH] s3: Fix max indentation and max column]

[Jeremy Allison]

On Wed, Mar 07, 2018 at 08:53:39AM +0100, Swen Schillig via samba-technical wrote:
> On Tue, 2018-03-06 at 15:22 -0800, Jeremy Allison wrote:
> > On Tue, Mar 06, 2018 at 01:15:57PM +0100, Swen Schillig via samba-technical wrote:
> > > From 3d418cd2213a3721bd4cc6185ad5d515fcbd724b Mon Sep 17 00:00:00
> > > 2001
> > > From: Swen Schillig <swen at vnet.ibm.com>
> > > Date: Mon, 5 Mar 2018 12:55:23 +0100
> > > Subject: [PATCH] s3: Fix max indentation and max column
> > > 
> > > Minor cleanup reducing the max indentation level and max column length.
> > > 
> > > Signed-off-by: Swen Schillig <swen at vnet.ibm.com>
> > > ---
> > >  source3/smbd/dir.c | 43 +++++++++++++++++++++++--------------------
> > >  1 file changed, 23 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/source3/smbd/dir.c b/source3/smbd/dir.c
> > > index 6621b4ee387..4fad65c90d7 100644
> > > --- a/source3/smbd/dir.c
> > > +++ b/source3/smbd/dir.c
> > > @@ -159,29 +159,32 @@ static struct dptr_struct *dptr_get(struct smbd_server_connection *sconn,
> > >  				    int key, bool forclose)
> > >  {
> > >  	struct dptr_struct *dptr;
> > > +	const int dirhandles_open = sconn->searches.dirhandles_open;
> > >  
> > > -	for(dptr = sconn->searches.dirptrs; dptr; dptr = dptr->next) {
> > > -		if(dptr->dnum == key) {
> > > -			if (!forclose && !dptr->dir_hnd) {
> > > -				if (sconn->searches.dirhandles_open >= MAX_OPEN_DIRECTORIES)
> > > -					dptr_idleoldest(sconn);
> > > -				DEBUG(4,("dptr_get: Reopening dptr key %d\n",key));
> > > -
> > > -				if (!(dptr->dir_hnd = OpenDir(NULL,
> > > -							dptr->conn,
> > > -							dptr->smb_dname,
> > > -							dptr->wcard,
> > > -							dptr->attr))) {
> > > -					DEBUG(4,("dptr_get: Failed to "
> > > -						"open %s (%s)\n",
> > > -						dptr->smb_dname->base_name,
> > > -						strerror(errno)));
> > > -					return NULL;
> > > -				}
> > > +	for (dptr = sconn->searches.dirptrs; dptr; dptr = dptr->next) {
> > > +		if(dptr->dnum != key) {
> > > +			continue;
> > > +		}
> > > +
> > > +		if (!forclose && !dptr->dir_hnd) {
> > > +			if (dirhandles_open >= MAX_OPEN_DIRECTORIES) {
> > > +				dptr_idleoldest(sconn);
> > > +			}
> > > +			DEBUG(4,("dptr_get: Reopening dptr key %d\n",key));
> > > +
> > > +			dptr->dir_hnd = OpenDir(NULL, dptr->conn,
> > > +						dptr->smb_dname,
> > > +						dptr->wcard, dptr->attr);
> > 
> > As a formatting comment. Please don't change:
> > 
> > > -				if (!(dptr->dir_hnd = OpenDir(NULL,
> > > -							dptr->conn,
> > > -							dptr->smb_dname,
> > > -							dptr->wcard,
> > > -							dptr->attr))) {
> > 
> > to
> > 
> > > +			dptr->dir_hnd = OpenDir(NULL, dptr->conn,
> > > +						dptr->smb_dname,
> > > +						dptr->wcard, dptr->attr);
> > 
> > when reformatting. When calling a function one paramter per line is *much* easier to
> > read and modify than multiple paramerers per-line.
> > 
> > This is because if you need to change one parameter later, it makes it extremely
> > clear what it is you are changing in a future patchset.
> > 
> > I know it seems overly picky, but I'd like to NAK until I see a version
> > that looks like:
> > 
> > > +			dptr->dir_hnd = OpenDir(NULL,
> > > +						dptr->conn,
> > > +						dptr->smb_dname,
> > > +						dptr->wcard,
> > > +						dptr->attr);
> > 
> > instead. It's very close to being right, I want to make the
> > patch be beautiful as well :-) :-).
> 
> Well, you know how it is, beauty depends on the viewer :-)
> but here I'm afraid I would code against the SAMBA Coding rules if I'd
> do it the way you want.
> Since this is an invocation and no declaration or definition of a
> function, I have to use the full length.
> 
> So, I guess I'm stuck, I won't get a RB+ from you without the re-do but
> I might get a NACK from another team-member if I do.
> 
> Any suggestions ?

Well what you're doing here is also reformatting the original
code (which curently matches my preferred pattern). So IMHO when
removing a layer of indentation you can keep the existing formatting
of the parameter list without violating the Samba coding rules.

Plus right now you have a NAK with the patten change, but
only a *potential* NAK without the pattern change.

So the way forward is to re-submit this without the pattern
change and I'll +1 and ask for a second Team member +1. If
we get another NAK due to the patten change then we'll cross
that bridge when we come to it.

In the meantime I'll work on getting README.Coding changed
so the "correct" way of parameter passing is preferred :-).

Cheers,

	Jeremy.