On Wed, Mar 30, 2016 at 3:31 PM, Fons Adriaensen <fons@xxxxxxxxxxxxxx> wrote:
On Tue, Mar 29, 2016 at 11:12:21AM -0400, Paul Davis wrote:
> (1) I explained to Fons in person and to everyon on the jack-devel mailing
> list what the issues with Fons' original patch. Robin Gareus has eloquently
> restated the basic point, but I'll do it again: the ability to REVIEW code
> changes is critical to long-lived projects, and when a change consists of
> 85% or more white-space/code style changes and only 15% actual functional
> differences, code review becomes all but impossible.
1. All the places where the actual code was changed were
clearly identified by comments.
That is of close to ZERO help to someone who is trying to read a large commit and understand what was changed.
I have no idea why you keep pushing this point. It is simply an utterly accepted feature of every open source project that involves distributed developers that you do not change the whitespace/indent/coding style, no matter how much you dislike it. I've worked on projects that would have just rejected your entire patch based on this aspect alone. I mentioned this to you in private.
Instead, I took time to investigate code formatters, identify which one would likely do the right thing, and come up with a strategy for creating a commit that only changed what actually mattered.
2. When I compare the commit (42393121) that applied my patch
with previous versions, there are *lots of* major coding
style and whitespace changes in files that I never touched.
This means the coding style wasn't consistent to start with.
Nobody is questioning this. That is why my process for dealing with your patch FIRST involved using uncrustify to canonicalize the coding style. Normally I would not have bothered to do this, since it created a huge commit that in theory is a 100% no-op, and wasn't addressing any actual reported problem or issue. But it was the only way I could see to convert your patch into an acceptable format. This particular step would have been unnecessary without all the extra changes in the original patch.
3. When I read that same commit (which I assume reflects the
preferred style) I see things like this:
It reflects the work of uncrustify. I have attached the config file that I used. I am entirely happy to consider revisions to the uncrustify config file and we can re-run the canonicalization step. I'm entirely open to the idea that I didn't get the uncrustify config correct, and that there are better options to be used in order to achieve the desired result. I'm happy to take suggestions on how to improve this.
Let me re-describe the scenario that we're discussing here:
I received a valuable but terribly formatted patch that changed large amounts of code for no reason at all. Rather than reject it, I mentioned this to its author, who basically shrugged and said "it is what is is, this is how i work", completely ignoring convention in distributed development. I spent time to come up with a strategy for generating a clean(er) version of the patch, which either because of errors in the process or errors in the original patch, caused a regression in server stability in the face of less-than-well-behaved clients. After some discussion on jack-devel, I reverted the commit.
At this point, the pushback I am getting is leading me towards simply rejecting the work out of hand and forgetting about it. The change/fix is an important one, and I want to see it included in jack1's codebase. But I will not put up with this BS attitude toward a problem that started when Fons chose to ignore conventional behaviour and believed that submitting a patch which was 80%+ reformatting changes was an acceptable strategy.
#
# uncrustify config file for paul davis
#
indent_with_tabs = 2 # 1=indent to level only, 2=indent with tabs
input_tab_size = 8 # original tab size
output_tab_size = 8 # new tab size
indent_columns = output_tab_size
indent_label = 1 # pos: absolute col, neg: relative column
#
# inter-symbol newlines
#
nl_enum_brace = remove # "enum {" vs "enum \n {"
nl_union_brace = remove # "union {" vs "union \n {"
nl_struct_brace = remove # "struct {" vs "struct \n {"
nl_do_brace = remove # "do {" vs "do \n {"
nl_if_brace = remove # "if () {" vs "if () \n {"
nl_for_brace = remove # "for () {" vs "for () \n {"
nl_else_brace = remove # "else {" vs "else \n {"
nl_while_brace = remove # "while () {" vs "while () \n {"
nl_switch_brace = remove # "switch () {" vs "switch () \n {"
nl_brace_while = remove # "} while" vs "} \n while" - cuddle while
nl_brace_else = remove # "} else" vs "} \n else" - cuddle else
nl_func_var_def_blk = 1
nl_fcall_brace = remove # "list_for_each() {" vs "list_for_each()\n{"
nl_fdef_brace = add # "int foo() {" vs "int foo()\n{"
# nl_after_return = TRUE;
# nl_before_case = 1
nl_assign_leave_one_liners = TRUE # don't split "foo_t f = { 1, 2 }"
#
# Source code modifications
#
mod_paren_on_return = remove # "return 1;" vs "return (1);"
mod_full_brace_if = add # "if (a) a--;" vs "if (a) { a--; }"
mod_full_brace_for = remove # "for () a--;" vs "for () { a--; }"
mod_full_brace_do = remove # "do a--; while ();" vs "do { a--; } while ();"
mod_full_brace_while = remove # "while (a) a--;" vs "while (a) { a--; }"
mod_full_brace_nl = 3 # don't remove if more than 3 newlines
#
# inter-character spacing options
#
sp_return_paren = force # "return (1);" vs "return(1);"
sp_sizeof_paren = remove # "sizeof (int)" vs "sizeof(int)"
sp_before_sparen = force # "if (" vs "if("
sp_after_sparen = force # "if () {" vs "if (){"
sp_after_cast = remove # "(int) a" vs "(int)a"
sp_inside_braces = add # "{ 1 }" vs "{1}"
sp_inside_braces_struct = add # "{ 1 }" vs "{1}"
sp_inside_braces_enum = add # "{ 1 }" vs "{1}"
sp_assign = add
sp_arith = add
sp_bool = add
sp_compare = add
sp_assign = add
sp_after_comma = add
sp_func_def_paren = force # "int foo (){" vs "int foo(){"
sp_func_call_paren = force # "foo (" vs "foo("
sp_func_proto_paren = remove # "int foo ();" vs "int foo();"
sp_else_brace = force # "if (condition) {" vs "if (condition){"
sp_brace_else = force # "} else" vs "}else"
#
# Aligning stuff
#
align_with_tabs = TRUE # use tabs to align
align_on_tabstop = TRUE # align on tabstops
# align_keep_tabs = true
align_enum_equ_span = 4 # '=' in enum definition
# align_nl_cont = TRUE
# align_var_def_span = 2
# align_var_def_inline = TRUE
# align_var_def_star = FALSE
# align_var_def_colon = TRUE
# align_assign_span = 1
align_struct_init_span = 3 # align stuff in a structure init '= { }'
align_right_cmt_span = 3
# align_pp_define_span = 8;
# align_pp_define_gap = 4;
# cmt_star_cont = FALSE
# indent_brace = 0
_______________________________________________ Linux-audio-user mailing list Linux-audio-user@xxxxxxxxxxxxxxxxxxxx http://lists.linuxaudio.org/listinfo/linux-audio-user
