[LAU] Ardour: exporting woes

[Paul Davis]

On Wed, Mar 30, 2016 at 3:31 PM, Fons Adriaensen <fons at linuxaudio.org>
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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linuxaudio.org/pipermail/linux-audio-user/attachments/20160330/680adb63/attachment.html>
-------------- next part --------------
#
# 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