Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mon: fix coding-style on PG related Monitor files #6881

Merged
merged 2 commits into from Dec 19, 2015

Conversation

wido
Copy link
Member

@wido wido commented Dec 9, 2015

Using uncrustify the coding style of the files has been changed
to match what has been described in the CodingStyle document.

This process was fully automated using uncrustify and with the
configuration file in the 'share' directory this can be applied
to other files when needed.

Using uncrustify the coding style of the files has been changed
to match what has been described in the CodingStyle document.

This process was fully automated using uncrustify and with the
configuration file in the 'share' directory this can be applied
to other files when needed.
@ghost
Copy link

ghost commented Dec 9, 2015

@wido how is this going to work with backporting ?

@wido
Copy link
Member Author

wido commented Dec 10, 2015

@dachary So yes, that is the problem. I opened a discussion about this on the mailinglist since a lot of files don't have proper code style.

I see there is a small glitch, need to fix that.

@smithfarm
Copy link
Contributor

Looking at this patch, 95% of the changes appear to be whitespace.

While I am just as pedantic about whitespace as the next guy, wholesale whitespace changes like these do create spurious conflicts when cherry-picking and merging. As Loic implies, this makes backporting (already a tedious process) more difficult.

@wido
Copy link
Member Author

wido commented Dec 10, 2015

@smithfarm I started a discussion on the devel list about this: http://article.gmane.org/gmane.comp.file-systems.ceph.devel/28394

It was mainly when I started working on PGMonitor.cc I noticed that my editor was complaining about the use of tab, 2 spaces and trailing whitespace in the file.

That lead me to the CodingStyle document and me starting the discussion on ceph-devel.

It is mainly whitespace and indentation indeed. I can configure uncrustify more to also look at column widths for example.

@jecluis
Copy link
Member

jecluis commented Dec 10, 2015

is it not possible to use -Xignore-all-space when cherry-picking?

A quick git log -p --ignore-all-space returns just a few changes instead of a boat load of them.

@smithfarm
Copy link
Contributor

ok, I was not-so-blissfully unaware of -Xignore-all-space. I have updated the HOWTO accordingly: http://tracker.ceph.com/projects/ceph-releases/wiki/HOWTO_backport_commits

I retract my objection.

@jecluis
Copy link
Member

jecluis commented Dec 10, 2015

In case my comment was perceived as a factual solution to the problem you raised, please note that I have indeed no idea if it would work. I just meant that it looked like it could :)

@wido
Copy link
Member Author

wido commented Dec 10, 2015

@smithfarm Great!

@jecluis Can you comment on this in the file? I used the uncrustify tool, no manual intervention. It might be that this tool needs some tuning. It has a lot of configuration options!

@ghost
Copy link

ghost commented Dec 10, 2015

while -Xignore-all-space definitively helps, I'm not sure how effective it will be with regard to backports. A global re-indent will not only impact the context of a hunk (in which case -Xignore-all-space helps) but also the spaces in the lines being modified (in which case -Xignore-all-space won't help).

I guess the best way forward is to try and fix things, but do it in a way that if it turns out to be a bad idea we won't suffer too much from our mistakes. What about uncrustifying a frequently backported but not too big C++ file ? A file that is targeted for backporting at least once a month. This way we can gain first hand experience on the actual problems (or lack of problems ;-) this causes.

That or find someone with actual experience who can tell us "do that, it'll be fine" ;-)

/me conservative backporter

@smithfarm
Copy link
Contributor

There are two issues at work here. First is this particular PR, and second is whether merging this one PR will lead to more of the same. In the email thread, Sage agreed with the cleanup in this PR because the file in question is not heavily trafficked. Presumably if @wido were proposing blanket whitespace changes across the entire code base, the response would have been different.

@ghost
Copy link

ghost commented Dec 10, 2015

@smithfarm yes, I don't have an issue with this specific PR. @wido +1 on this one. It's the opportunity to discuss how to approach cosmetic refactoring, in general. thanks for clarifying this.

@wido
Copy link
Member Author

wido commented Dec 10, 2015

@smithfarm I am aware of what it might do if we do the entire code-base. The thread was mainly started out of curiosity since I started work there and I noticed it.

It was not my intention to do this for all files. We can do this on a per-file basis when we think we like it. That's why I also added the uncrustify configuration file.

@wido
Copy link
Member Author

wido commented Dec 18, 2015

How are we going forward with this?

liewegas added a commit that referenced this pull request Dec 19, 2015
mon: fix coding-style on PG related Monitor files

Reviewed-by: Sage Weil <sage@redhat.com>
@liewegas liewegas merged commit 2fe8180 into ceph:master Dec 19, 2015
@ghost ghost changed the title CodingStyle: Fix coding-style on PG related Monitor files mon: fix coding-style on PG related Monitor files Feb 10, 2016
@ghost ghost added the core label Feb 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants