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

[Swen Schillig]

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 ?

Cheers Swen