[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149

Shyamsundar  changed:

   What|Removed |Added

   Fixed In Version|glusterfs-5.0   |glusterfs-6.0



--- Comment #45 from Shyamsundar  ---
This bug is getting closed because a release has been made available that
should address the reported issue. In case the problem is still not fixed with
glusterfs-6.0, please open a new bug report.

glusterfs-6.0 has been announced on the Gluster mailinglists [1], packages for
several distributions should become available in the near future. Keep an eye
on the Gluster Users mailinglist [2] and the update infrastructure for your
distribution.

[1] https://lists.gluster.org/pipermail/announce/2019-March/000120.html
[2] https://www.gluster.org/pipermail/gluster-users/

-- 
You are receiving this mail because:
You are on the CC list for the bug.
___
Gluster-infra mailing list
Gluster-infra@gluster.org
https://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149

Shyamsundar  changed:

   What|Removed |Added

 Status|POST|CLOSED
   Fixed In Version||glusterfs-5.0
 Resolution|--- |CURRENTRELEASE
Last Closed||2018-10-23 11:06:59



--- Comment #44 from Shyamsundar  ---
This bug is getting closed because a release has been made available that
should address the reported issue. In case the problem is still not fixed with
glusterfs-5.0, please open a new bug report.

glusterfs-5.0 has been announced on the Gluster mailinglists [1], packages for
several distributions should become available in the near future. Keep an eye
on the Gluster Users mailinglist [2] and the update infrastructure for your
distribution.

[1] https://lists.gluster.org/pipermail/announce/2018-October/000115.html
[2] https://www.gluster.org/pipermail/gluster-users/

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=EnfXxOC6pu=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
https://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #43 from Worker Ant  ---
REVIEW: https://review.gluster.org/21193 (xlators/experimental: move template
files to '.c.in' type) posted (#5) for review on master by Xavi Hernandez

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=srBiAHHbSB=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
https://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #42 from Worker Ant  ---
REVIEW: https://review.gluster.org/21351 (rfc.sh: fix the error in case
clang-format is not present) posted (#1) for review on master by Amar Tumballi

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=fU3tlrgzek=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
https://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149

Nithya Balachandran  changed:

   What|Removed |Added

 CC||nbala...@redhat.com



--- Comment #41 from Nithya Balachandran  ---
Is there a clang-format package for RHEL?

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=Kzwc4Kd7m2=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
https://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149

Worker Ant  changed:

   What|Removed |Added

 Status|ASSIGNED|POST



--- Comment #40 from Worker Ant  ---
REVIEW: https://review.gluster.org/21193 (xlators/experimental: move template
files to '.c.in' type) posted (#1) for review on master by Amar Tumballi

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=jniqbuy7xw=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
https://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149

Amar Tumballi  changed:

   What|Removed |Added

 Status|POST|ASSIGNED



--- Comment #39 from Amar Tumballi  ---
This still needs work:

1. Need to identify and rename the 'template' files, which are not directly
compiled by gcc. It shouldn't have `.c` as the type. (or `.h` for that matter).

2. clang-format job should still validate only files which are ! contrib.

3. ./rfc.sh changes makes the change on top most patch locally, but doesn't
commit it if there are any changes after clang-format. Need to make it commit
it automatically before submit.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=xuSD9dJTq0=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
https://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #38 from Worker Ant  ---
COMMIT: https://review.gluster.org/21164 committed in master by "Amar Tumballi"
 with a commit message- template files: revert clang

Change-Id: If3925191d23afe83cbbdbc3cf0554c0a9c76d043
updates: bz#1564149
Signed-off-by: Amar Tumballi 

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=nwd1DSxZgw=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
https://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #37 from Worker Ant  ---
REVIEW: https://review.gluster.org/21164 (template files: revert clang) posted
(#1) for review on master by Amar Tumballi

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=hKbKvlGrZ4=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
https://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #36 from Worker Ant  ---
COMMIT: https://review.gluster.org/20892 committed in master by "Nigel Babu"
 with a commit message- clang-format: add the config file

Also update the required documents and scripts to enable clang-format

Change-Id: I73aae6db06c2f732a1779d59a73bc05e28beafba
updates: bz#1564149
Signed-off-by: Amar Tumballi 

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=pZDAWlFywq=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
https://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149

Worker Ant  changed:

   What|Removed |Added

 Status|NEW |POST



--- Comment #35 from Worker Ant  ---
REVIEW: https://review.gluster.org/20892 (clang-format: add the config file)
posted (#1) for review on master by Amar Tumballi

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=4REaz6rDQS=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
https://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #34 from Nigel Babu  ---
Jenkins job is ready: https://review.gluster.org/#/c/20418/

Cannot merge this until we have a config file as it's a voting job.

Yaniv, just to be clear, these machines are still VMs :) We only have 4
physical machines. We spin up VMs on them.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=8egmcddSNX=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #33 from Yaniv Kaul  ---
(In reply to Nigel Babu from comment #32)
> We were blocked on Fedora builders. We procured them 2 weeks ago and they've
> had some time to settle in. Infra-wise, we're nearly ready to go. I have a
> job half complete. I'll try to get the Jenkins job complete this week. Then
> it's waiting on us agreeing on the exact config file on
> https://github.com/nigelbabu/clang-format-sample that needs to be then
> checked in and used in the Jenkins jobs.

Static code analysis can run on VMs even, but I'm glad to see where are good
with resources.

As for agreeing - let's go with small, incremental wins - we can get in
whatever everyone (or majority) seem to agree with, and incrementally add more
checks in steps.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=ES3lk6zY7M=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149

Nigel Babu  changed:

   What|Removed |Added

  Flags|needinfo?(nig...@redhat.com |
   |)   |



--- Comment #32 from Nigel Babu  ---
We were blocked on Fedora builders. We procured them 2 weeks ago and they've
had some time to settle in. Infra-wise, we're nearly ready to go. I have a job
half complete. I'll try to get the Jenkins job complete this week. Then it's
waiting on us agreeing on the exact config file on
https://github.com/nigelbabu/clang-format-sample that needs to be then checked
in and used in the Jenkins jobs.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=FMrP3rHErm=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149

Yaniv Kaul  changed:

   What|Removed |Added

  Flags||needinfo?(nig...@redhat.com
   ||)



--- Comment #31 from Yaniv Kaul  ---
(In reply to Nigel Babu from comment #29)
> Um, so I've been accepting the pull requests that come in more or less
> mostly so we can see what the output looks like. I'm going to wait for 24h
> for each of the remaining pull requests for comments.

Nigel, what's the latest on this?

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=SxcJgvo0BW=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #30 from Xavi Hernandez  ---
(In reply to Jeff Darcy from comment #24)
> Personally I'm willing to go with the flow on those. I've laid out my
> reasoning, other people seem less inclined to compromise, I think we all
> have better things to do than turn it into a pitched battle.

Besides IndentCaseLabels and SpaceBeforeParens, do you agree with the other
changes ? If you are ok with them, I'll update the pull request without the
conflicting ones and, if necessary, push them in a future change.

We also need to decide if we prefer right pointer alignment or declarations
alignment, since they seem to be incompatible.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=6InTjKahRI=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #29 from Nigel Babu  ---
Um, so I've been accepting the pull requests that come in more or less mostly
so we can see what the output looks like. I'm going to wait for 24h for each of
the remaining pull requests for comments.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=un1MZubORN=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #28 from Xavi Hernandez  ---
(In reply to Aravinda VK from comment #27)
> I prefer Right(`AlignEscapedNewlines=Right`), since Patch will have minimum
> number of lines change on modification. For example,
> 
> #define gf_log(dom, levl, fmt...) do {  \
> FMT_WARN (fmt); \
> _gf_log (dom, __FILE__, __FUNCTION__, __LINE__, \
>  levl, ##fmt);  \
> } while (0)
> 
> If longest line is changed then patch diff shows all of them as changed
> lines in case of `AlignEscapedNewLines=Left`

That makes sense. It's ok for me.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=CsBYbRgu0h=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #27 from Aravinda VK  ---
(In reply to Xavi Hernandez from comment #19)
> I made a mistake on one of the proposed values:
> 
> AlignEscapedNewLinesLeft=false
> 
> What I wanted to propose was to set it to 'true' (which matches the
> explanation I gave).

I prefer Right(`AlignEscapedNewlines=Right`), since Patch will have minimum
number of lines change on modification. For example,

#define gf_log(dom, levl, fmt...) do {  \
FMT_WARN (fmt); \
_gf_log (dom, __FILE__, __FUNCTION__, __LINE__, \
 levl, ##fmt);  \
} while (0)

If longest line is changed then patch diff shows all of them as changed lines
in case of `AlignEscapedNewLines=Left`

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=eZWnKhPOYv=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #25 from Xavi Hernandez  ---
I don't want to start any battle. It was requested to create pull requests with
the proposed changes and that's what I've done. I've not hidden that some of
the changes are debatable so that everyone can give his point of view. If they
are not accepted it's ok for me.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=UQALtNHVM8=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #23 from Xavi Hernandez  ---
The change will also include some of the controversial options:

IndentCaseLabels=true
SpaceBeforeParens=ControlStatements

What should we do with them ?

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=vxXuKAwVH5=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #22 from Xavi Hernandez  ---
Some issues I've seen:

* Some initializations are formatted like this:

char  gfid_local[GF_UUID_BUF_SIZE] = {
0,
};

I've tried to change AllowShortBlocksOnASingleLine=true, but no change here.

* Braces after functions are not put into a new line. It seems that
BreakBeforeBraces=Attach has precedence to BraceWrapping.AfterFunction=true.

* AlignConsecutiveAssignments=true aligns some assignments inside a function
(not only variables initialization at the beginning). I think this is not
necessary.

* Some assignments are rendered like this:

meta_dst->size =
hton64(ntoh64(meta_dst->size) + ntoh64(meta_src->size));
meta_dst->file_count =
hton64(ntoh64(meta_dst->file_count) + ntoh64(meta_src->file_count));

I think we should increase PenaltyBreakAssignment to prevent this. I've chosen
a random value (200) and this is the result:

meta_dst->size = hton64(ntoh64(meta_dst->size) +
ntoh64(meta_src->size));
meta_dst->file_count = hton64(ntoh64(meta_dst->file_count) +
  ntoh64(meta_src->file_count));

I'll create a new pull request with these changes.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=1tnUHvZJiN=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #21 from Jeff Darcy  ---
The feedback so far has been strongly positive, but one issue has been raised.
As Xavi mentioned, the PointerAlignment doesn't seem to work properly on lines
affected by AlignConsecutive*. AFAICT it's trying to center the asterisk,
perhaps using PointerAlignment to decide left/right bias in case of a tie, but
still leaving undesirable spaces between the asterisk and the variable name.
Awkward. If these options are incompatible, which do *we* believe should take
precedence?

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=gJdtESgCqG=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #19 from Xavi Hernandez  ---
I made a mistake on one of the proposed values:

AlignEscapedNewLinesLeft=false

What I wanted to propose was to set it to 'true' (which matches the explanation
I gave).

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=Bc6Pl04N8e=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #18 from Jeff Darcy  ---
Monday morning EST would be fine.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=jlSePmYcq9=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #17 from Amar Tumballi  ---
Thanks for these pointers Jeff, makes the case even more critical to work
towards.

Shyam kindly agreed to make this a blocker for 4.1 branching out, which means,
we have to come to agreement soon.

While many standards followed by gluster was due to having a look at some of
the projects which were active then, I am fine with options which makes the
project get closer to more generally adopted standards, while keeping some of
our developer's concerns (if anyone shows any). Till now, I see Xavi and you
showing interest in this.

Can we plan to have a meeting/call on Monday morning EST/Evening IST? And come
to agreement?

@Nigel, meantime we need a job to sanitize the patch with the same file.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=0BdPYj351F=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #16 from Jeff Darcy  ---
FWIW, since this came up in the maintainers' meeting...

The inconsistency of style has had a measurable negative impact on ramp-up time
for new developers at Facebook. I'm sure the same has been true of new
developers at Red Hat and elsewhere. This impact manifests in several ways:

(1) Every developer has to spend time adjusting editor settings to have code
not seem randomly indented in one set of files, then again for another set of
files which violate our existing standard in a different way. Readability
matters. Also, when they're *writing* code, each developer has to tweak
settings some more because our standards are so unlike any other.

(2) Many aspects of our old standard (to which most code still conforms) are
simply alien to someone who learned C in the last ten years. Again, readability
matters. It might not seem like a big deal to read code that's formatted
"strangely" and indeed it's a good skill for all developers to have, but on top
of everything else it slows down new contributors.

(3) Time is wasted addressing these issues in code reviews, especially when a
patch is submitted to code that was already in violation of all standards and
common sense.

Lastly, I'll add that the combination of eight-column indents, 80-column limit,
and gigantic functions nested ten levels deep has led to a lot of code wedged
up against the right margin. Relaxing the 80-column limit would be ill advised
in light of real human-factors studies showing that anything more than 80-100
columns (results vary) makes it harder to scan from one line to the next. Of
the other two factors, the indent width can be changed with a reformat. The
gigantic functions and deep nesting are terrible habits, but those habits
aren't likely to change any time soon. Even if the only change was to reduce
indentation and spurious line breaks, I think a reformat would be justified.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=eLTCNJSFpj=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #15 from Xavi Hernandez  ---
(In reply to Jeff Darcy from comment #13)
> IndentCaseLabels: personally I don't like indenting case labels because they
> create a gap with no close brace - unless people put braces around cases,
> which is a bad habit for other reasons - but I know I'm probably outvoted on
> that one.

The reason I proposed it is because if we don't indent labels, we have a brace
after 'switch' which is not followed by indentation, as it normally happens,
but we'll still have indentation of the code inside the case label without
braces (I also don't like the braces inside cases):

switch (select) {
case VALUE1:
some_code();
break;
}

(Visually, the closing brace seems to come from 'case' and not from 'switch')

I think it's visually simpler to indent the cases to make it clear they belong
to the switch case. This way it's easy to see where the switch starts and ends.

switch (select) {
case VALUE1:
some_code();
break;
}

Note that with the reduction of tabs from 8 to 4, the code is at the same
indentation, so the gap with the closing brace is not big. The only change is
the case label itself.

> I almost don't have an opinion on the alignment options. For much of my
> career I would group the * with the type rather than the variable (i.e.
> PAS_Left) because it made more intuitive sense. On one hand, it seems like
> most of the world now agrees. On the other, it's not the way things are in
> Gluster now and hence requires a change in habits.

I don't have a strong preference here either. I've never used the PAS_Right
alignment before working on gluster, so I can adapt to whatever alignment is
preferred. I only wanted to point out that if we want to go with PAS_Right, it
currently has an issue in combination with AlignConsecutiveDeclarations.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=rIzAzGvuih=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149

Amar Tumballi  changed:

   What|Removed |Added

   Priority|unspecified |high



--- Comment #14 from Amar Tumballi  ---
> On the other, it's not the way things are in Gluster now and hence requires a 
> change in habits.

The whole point of this exercise is that we don't need to change habit, tool
does it. Well, you have to get used to the change while reading code.

All the different formats, with 4 space indent is at
https://github.com/nigelbabu/clang-format-sample 

I will request Nigel to run a sample with above argument before maintainers
meeting today to have them compared to the same file.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=TQSClkEIUm=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #13 from Jeff Darcy  ---
IndentCaseLabels: personally I don't like indenting case labels because they
create a gap with no close brace - unless people put braces around cases, which
is a bad habit for other reasons - but I know I'm probably outvoted on that
one.

SpaceBeforeParens: I like the space between function name and arguments. The
fact that none of the "modern" styles is not very compelling by itself (appeal
to popularity) but I'm probably outvoted there too.

AlwaysBreakAfterReturnType: Yes. That's the option I was looking for. For
people who use IDEs it probably doesn't matter, but having the function name
start at the beginning of the line is very handy for those using non-IDE
editors like vi(m). It also leaves more space for parameters, making long
parameter lists - which we tend to have - more readable.

I almost don't have an opinion on the alignment options. For much of my career
I would group the * with the type rather than the variable (i.e. PAS_Left)
because it made more intuitive sense. On one hand, it seems like most of the
world now agrees. On the other, it's not the way things are in Gluster now and
hence requires a change in habits.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=zHAKTcfHCj=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #12 from Xavi Hernandez  ---
Using Chromium as base, basically the changes you propose seem ok. I would like
to propose these additional changes:

* I prefer to use 4 spaces for indent. 2 seems too few to me.

* IndentCaseLabels = true. If we use 4 spaces for indent, we could consider
indenting case labels to make them more readable and similar to other code
blocks enclosed between braces.

* SpaceBeforeParens = ControlStatements. I think the additional space before
parens on function calls doesn't provide any advantage (in fact none of the
default configurations uses it).

* BinPackParameters = true.

* AlwaysBreakAfterReturnType = true. I think that separating return type from
function name is useful in some places that use long function names or argument
types.

* AlignEscapedNewLinesLeft=false. Otherwise it fills files with a lot of
unnecessary spaces.

* AlignConsecutiveDeclarations=true. Using this option has the side effect that
pointers are not correctly aligned as we currently do. Pointers will be
rendered like this:

int32_t var1 = 1;
void *  var2 = NULL;

* AlignConsecutiveAssignments=true.

I'm not sure if the last two options provide any interesting advantage. So, if
the pointer alignment is more important, we can disable them.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=NWRhQJ1EHB=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #11 from Jeff Darcy  ---
Whatever we pick is going to require some tweaking. Some of the things I
notice:

 * All use two-space indents, which I personally find atrocious.

 * All put a function's type on the same line as its name.

 * All put the opening brace for a function on the last line of the arguments,
instead of at the beginning of a separate line.

 * Mozilla, Webkit, and LLVM indent 'case' from 'switch'

 * None enforce a space between a function name and opening paren in calls.

 * Mozilla, LLVM, and Google all express pointers as "type* name" instead of
the more familiar (in our code) "type *name"

So Chromium seems closest, but none are particularly close. Looking at the
clang-format pages, here are some options I'd suggest for compatibility with
our existing standard:

 !AllowShortIfStatementsOnASingleLine
 !AllowShortLoopsOnASingleLine
 BraceWrapping(!AfterControlStatement)
 BraceWrapping(AfterFunction)
 BraceWrapping(!BeforeElse)
 ColumnLimit(80)
 !IndentCaseLabels
 IndentWidth(8) or IndentWidth(4) depending on outcome of that conversation
 PointerAlignment(PAS_Right)
 SpaceBeforeParens(SBPO_Always)
 TabWidth(8)
 UseTab(UT_Never)

This fixes most of the warts, though I don't see an option to control function
type and name on the same line and I didn't expect one to enforce braces around
single statements (which would be nice).

BTW, yes, I know that our coding style is rather archaic. The fact that all of
Clang's defined styles are so different than ours is testament to that. There
are some who would argue that any "big bang" reformatting should modernize as
many aspects as possible, including even function opening braces and
(*shudder*) two-wide indents. I'm not necessarily opposed to such changes, but
I think they need to be considered *individually* and not just adopted by
accident. We already did that once, when we imported a check script from the
Linux kernel without anything resembling proper review and it turned out to be
wildly incompatible with our existing code. Let's not repeat that mistake.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=zW0VzdcJDh=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #10 from Amar Tumballi  ---
(Update from Nigel)

Which format should we start with as base? Google/LLVM/Mozilla/Webkit/Chromium
   *
[Chromium](https://gist.github.com/nigelbabu/667f3b626779bb14858aa4b520e67c18#file-chromium-c)
 
*
[Mozilla](https://gist.github.com/nigelbabu/667f3b626779bb14858aa4b520e67c18#file-mozilla-c)
*
[LLVM](https://gist.github.com/nigelbabu/667f3b626779bb14858aa4b520e67c18#file-llvm-c)
*
[Webkit](https://gist.github.com/nigelbabu/667f3b626779bb14858aa4b520e67c18#file-webkit-c)
*
[Google](https://gist.github.com/nigelbabu/667f3b626779bb14858aa4b520e67c18#file-google-c)

When do we want to make this change? Before the 4.1 branching seems like a good
time to make vast changes.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=Dee27RsksB=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #9 from Amar Tumballi  ---
(In reply to Nigel Babu from comment #7)
> Alright. So we have agreement that this is a good idea. How do we want to
> implement this? As a check or as a pre-commit hook?

This is a multi-step process!

0. Team agrees on a style and a config file representing the style.
1. Commit the coding style guide to codebase and make changes in rfc.sh to use
it.
2. 'gluster-ant' commits a single large patch for whole codebase with a
standard clang-format style. (This should be only changes which happened due to
clang-format, and no other changes should be in the patch. This can crash
gerrit if we send it to review. 
  -> NOTE: This change can be as big as moving gluster repo from tla to git, as
we have now 2 repositories, 'historic' and 'glusterfs' to understand the actual
source of a line, if one needs 'git blame'.

3. Have the job ready to check the patch with the config file, on the server
side (along with a pre-check in rfc.sh to warn people), this should be a Voting
job in smoke.

4. We all live happily ever after.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=rNXvC1NMQK=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #8 from Amar Tumballi  ---
(In reply to Shreyas Siravara from comment #6)
> Whoever came up with this idea is a genius, and I totally ack it :)

All credits for this to Shreyas Siravara! As the original idea was proposed
while we were walking for dinner on Boston's streets.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=dndCr6e7EV=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149

Nigel Babu  changed:

   What|Removed |Added

 CC||nig...@redhat.com



--- Comment #7 from Nigel Babu  ---
Alright. So we have agreement that this is a good idea. How do we want to
implement this? As a check or as a pre-commit hook?

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=DAhAqpl7XZ=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149

Shreyas Siravara  changed:

   What|Removed |Added

 CC||sshre...@fb.com



--- Comment #6 from Shreyas Siravara  ---
Whoever came up with this idea is a genius, and I totally ack it :)

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=2quGLHK1M0=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #5 from Jeff Darcy  ---
I think we're all in agreement on spaces vs. tabs. Tabs are eight spaces wide,
full stop, end of story. It's kind of survivable with eight-space indents, but
any other indent width becomes a total nightmare if tabs are involved.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=DdID1I6Srl=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #4 from Shyamsundar  ---
Overall ack to the proposal.

Nice call-out by Jeff on any other change requiring a big-bang addressing to be
a part of the same.

I can live with 4 spaces :), but no tabs (way too much discrepancy across
editors as pointed out).

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=PlhQBRYYxh=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149

Xavi Hernandez  changed:

   What|Removed |Added

 CC||jaher...@redhat.com



--- Comment #3 from Xavi Hernandez  ---
I think it's ok to use 4 spaces for indentation. It helps prevent ugly wraps on
some function calls with long names and/or arguments when they are indented.

Regarding the use of tabs, I would prefer to still use spaces instead of tabs.
The traditional tab size is 8 spaces and this is used in (almost ?) all
editors, so to prevent the code from being unreadable if someone uses an
unconfigured editor, we should still use spaces, specially if we change to a 4
spaces indentation.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=zuf3rRa054=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra