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

Unify code indentation #188

Closed
johnlinp opened this issue Mar 20, 2018 · 16 comments

Comments

@johnlinp
Copy link

commented Mar 20, 2018

The indentation in the code is not consistent. Tabs and spaces are mixed. Some code blocks inside for or if are not indent properly. The problems increase the difficulty to understand the code.

I would like to submit a pull request to clean up the indentation. Any suggestions are welcome.

@kdudka

This comment has been minimized.

Copy link
Member

commented Apr 27, 2018

There seems to be no discussion any more. There were just few comments on #189. To put it simple, does anybody object against unifying the indentation of logrotate source code as such?

We will of course do our best to keep the changes minimal and preserve majority of the current coding style.

@johnlinp

This comment has been minimized.

Copy link
Author

commented May 14, 2018

With the "4-space" vim config:

set expandtab
set tabstop=4
set softtabstop=4
set shiftwidth=4

I got:

 config.c    | 3098 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------------------------------------------
 log.c       |  116 +++---
 log.h       |    8 +-
 logrotate.c | 3730 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------------------------------------------------------------------------
 logrotate.h |    4 +-
 queue.h     |  480 ++++++++++++-------------
 6 files changed, 3718 insertions(+), 3718 deletions(-)

Then with the "tab" vim config:

set noexpandtab

I got:

 config.c    | 2418 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------------------------------------------------
 log.c       |   22 +-
 log.h       |   10 +-
 logrotate.c | 3088 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------------------------------------------------------------------------
 logrotate.h |   78 ++---
 queue.h     |  218 +++++++-------
 6 files changed, 2917 insertions(+), 2917 deletions(-)

It seems that using tabs will make less changes than using 4 spaces. I will update #189 based on tabs.

By the way, I am working on the latest commit: 3bb59ca.

johnlinp added a commit to johnlinp/logrotate that referenced this issue May 14, 2018

@kdudka

This comment has been minimized.

Copy link
Member

commented May 14, 2018

There might be some misunderstanding on my part but I fail to see how the above is better than:

 config.c    | 2494 +++++++++++++++++++++++++++++------------------------------
 log.c       |  108 +--
 log.h       |    8 +-
 logrotate.c | 2012 +++++++++++++++++++++++------------------------
 logrotate.h |    6 +-
 5 files changed, 2314 insertions(+), 2314 deletions(-)

... which was the result of my 10 minutes effort described at #189 (comment)

I though that by further tweaks we would end up with an even smaller diffstat.

@johnlinp

This comment has been minimized.

Copy link
Author

commented May 14, 2018

@kdudka I found that your vim config in the comment not very useful to unify the indentation. With the vim config:

set noet
set sw=4
set ts=8

Lines with 1 level indentation would be 4 spaces, and lines with 2 level indentation would be 1 tab. I don't think it's desired.

@kdudka

This comment has been minimized.

Copy link
Member

commented May 14, 2018

Those options were not set arbitrarily. They were chosen because they seemed to reflect the majority of the current coding style. I believe that the resulting diffstat confirms it. Am I missing anything?

@kdudka

This comment has been minimized.

Copy link
Member

commented May 14, 2018

Also, when I checkout r3-0 dated back to 1999, this seems to be the only coding style used over the whole code base. The coding style has diverged during the development of the subsequent 3.x.x releases because contributors simply did not care about the original coding style.

@johnlinp

This comment has been minimized.

Copy link
Author

commented May 14, 2018

...contributors simply did not care about the original coding style.

Maybe it's because of the confusion of mixing spaces and tabs? If we continue using this mixed indentation style, I believe that the future contributors will continue ignore the coding style.

I suggest that we should stick to one style, either spaces or tabs.

@kdudka

This comment has been minimized.

Copy link
Member

commented May 14, 2018

Not really. vim: noet sw=4 ts=8 is one style. We just need to make sure that the coding style will not diverge again by adding vim modeline and perhaps some Travis CI hook. Otherwise, these troubles may reoccur, no matter which style you choose.

To be clear, vim: noet sw=4 ts=8 is not my personal preference and I have never used it for my own projects. It is just the original indentation style of logrotate and still used by majority of its current code base.

@johnlinp

This comment has been minimized.

Copy link
Author

commented May 15, 2018

When I use vim: noet sw=4 ts=8 against the latest commit (3bb59ca), the result is:

 config.c    | 2496 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------------------------------------------------------------------------
 log.c       |  108 ++++-----
 log.h       |    8 +-
 logrotate.c | 2010 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------------------------------------------------
 logrotate.h |    4 +-
 queue.h     |  442 +++++++++++++++++-----------------
 6 files changed, 2534 insertions(+), 2534 deletions(-)

So basically we have 2 options:

  1. 4-space style indentation: change 2917 lines.
  2. mixed style (spaces + tabs) indentation: change 2534 lines.

I would say that I will vote for the 4-space style.

@kdudka

This comment has been minimized.

Copy link
Member

commented Aug 10, 2018

I have prepared two competing pull requests for comparison: #212 and #213

Could you please all have a look and comment on them?

@johnlinp

This comment has been minimized.

Copy link
Author

commented Aug 10, 2018

+1 for #213 since I prefer all-space or all-tab style.

@kdudka

This comment has been minimized.

Copy link
Member

commented Aug 15, 2018

So far 2 people have voted for #213 and nobody for #212. Nobody is proposing anything else...

@glensc

This comment has been minimized.

Copy link
Collaborator

commented Aug 15, 2018

i see 1 and 0 ;)

@kdudka

This comment has been minimized.

Copy link
Member

commented Aug 15, 2018

@glensc I counted your "like" on #213 as a vote.

@kdudka

This comment has been minimized.

Copy link
Member

commented Aug 29, 2018

Nobody has commented on this in the last two weeks. I am going to merge #213 on Friday September 14th unless someone objects till then.

kdudka added a commit that referenced this issue Aug 31, 2018

kdudka added a commit that referenced this issue Aug 31, 2018

kdudka added a commit that referenced this issue Aug 31, 2018

kdudka added a commit that referenced this issue Aug 31, 2018

@kdudka kdudka closed this in 3467326 Sep 14, 2018

@johnlinp

This comment has been minimized.

Copy link
Author

commented Sep 17, 2018

@kdudka Thank you very much for resolving this issue. I really appreciate this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.