[PATCH] Have the smbcli_session record the OS and Native LANMAN of the remote server

[Sam Liddicott]

* Gerald Carter wrote, On 29/04/09 12:41:
> Hey Sam,
>   
>> The above disguises the intention in a tedium of 
>> small operations, taking 8 lines, only two of
>> which represent common execution behaviour.
>>
>> There's too little meaning, too little going on 
>> to spread over eight lines.
>>     
>
> I'm on the outside of this one, but I'll chime in anyways.
>   
you're welcome
> Coding style debates are a waste of time in my opinion.
> Developers should optimize for the collective and not the
> individual.  
I appreciate your comments Jerry. I think perhaps we would have begun a
debate on coding style; I took Stefan's question as an enquiry and not
requirement.

I think you make some good points, I hope you don't mind me commenting
on them. I don't intend to start a fruitless debate, only to illustrate
what I'm thinking.

> It is imperative for someone else to be able
> to pick up your code and immediately understand it.
>   

quite so, it is this on which the merits of conventions are often debated.

> Diverging from the established conventions of the large
> body of code is just a distraction.
>   

I realise that you and Metze think that is what I've done, but I think
you've mostly been looking at Samba code which happens to follow your
preferred style (perhaps you wrote most of what you look at).

However I've not used styles which aren't widely used in the samba4
codebase, as I show in answer to a point below.

> If it were me, I would rather not worry about the cosmetics
> and my own personal style.  I would want the functional
> change accepted into upstream.   

I'm more worried about how much I have to re-work patches to meet other
personal styles, and then I have to hack up my wip tree afterwards to
contain the new "approved" patch, sometimes after our own tree has
undergone internal testing.

> Currently there's an
> established coding convention that has obviously not been
> followed.
>   
There's more than one established convention widely present in the code
base.

A rough audit of the samba code (less than an hour to run):

$find . -type f -name '*.[ch]' | xargs -L 1 git blame | grep -v '^0000' \
         | grep 'if[ (][^"]*[^=!><|&+-'\'']=[^=].*)' | tee /tmp/style.who

Picks over 400 examples of an if condition testing an embedded
assignment, (which I think is what the main objection is - but I'm
guessing) - but certainly many are of a similar form to my patch.
(A different audit suggests that there were 10 times as many, but I'm
not sure about that)

Some found by the above command are:

if (!(ctrl[i] = talloc(ctrl, struct ldb_control))) {

if(r && (res = mp_int_copy(rout, r)) != MP_OK) goto CLEANUP;

if (!(cli = smbcli_state_init(NULL)) ||
            !smbcli_socket_connect(cli, server_name, destports,
                                   ev_ctx, resolve_ctx, options,
                                   iconv_convenience,
                   socket_options)) {

And I like it, I find it very expressive. I don't mind if you and Metze
don't like it, but the code base is full of it.
The last two are particularly like mine.

I note that metze prefers:
    if(v & 1) {
      if((res = mp_int_mul(c, &t, c)) != MP_OK)
        goto CLEANUP;
    }

But A Bartlett prefers a 1 liner:
  if(q && (res = mp_int_copy(qout, q)) != MP_OK) goto CLEANUP;

To see who uses this disputed style, with a line count:
$< /tmp/style.who sed -e "s/^[^(]*(//;s/ *200.*//;" | sort -n | uniq -c
| sort -nr
    107 Jelmer Vernooij
    107 Andrew Bartlett
     60 Andrew Tridgell
     32 Stefan Metzmacher
     27 Volker Lendecke
     27 Simo Sorce
     20 Sam Liddicott
     19 Heimdal Import User
     11 James Peach
      7 Tom Parkin
      3 Kai Blin
      3 Derrell Lipman
      1 Michael Adam
      1 Jeremy Allison

So in conclusion I think it's I case of objectors saying "I wouldn't
have written it like that" - (a valid view) but it's OK because my name
will be on the commit, not theirs.

I've fallen before between conflicting requirements of Samba team
members, unable to satisfy both.

In perspective, this patch works, and it takes into account functional
feedback from Volker..

It's not this patch that's the problem, but the over 150 that we've got
queued up for our next code drop.

> But the final decision is not mine, so take this for what
> it's worth.
>   

Well coming from you it's worth a lot, and it stimulated me to
investigate how far out I was, and I'm happy that I'm not far out at
all. Whether or not this patch is accepted, I'm still worried about the
other 150.

Sam