patch, 3 partsclang-format: fix rules to make the CI job cleaner

> The current value for the column limit is set to 80. While this is as
> expected, we often prefer readability over this strict limit. This means
> it is common to find code which extends over 80 characters. So let's
> change the column limit to be 0 instead. This ensures that the formatter
> doesn't complain about code strictly not following the column limit.

> On 24/10/09 05:55AM, Karthik Nayak wrote:
>> The current value for the column limit is set to 80. While this is as
>> expected, we often prefer readability over this strict limit. This means
>> it is common to find code which extends over 80 characters. So let's
>> change the column limit to be 0 instead. This ensures that the formatter
>> doesn't complain about code strictly not following the column limit.
>
> The column limit does lead to quite a few false positives. At the same
> time though, in some ways having a tool point out all the instances it
> occurs does make it easier to review if any should be addressed.
>
> If the goal is to have a CI job that we generally expect to pass, then
> it makes sense to remove it. I don't feel super strongly either way.

> Justin Tobler <jltobler@gmail.com> writes:
>
>> On 24/10/09 05:55AM, Karthik Nayak wrote:
>>> The current value for the column limit is set to 80. While this is as
>>> expected, we often prefer readability over this strict limit. This means
>>> it is common to find code which extends over 80 characters. So let's
>>> change the column limit to be 0 instead. This ensures that the formatter
>>> doesn't complain about code strictly not following the column limit.
>>
>> The column limit does lead to quite a few false positives. At the same
>> time though, in some ways having a tool point out all the instances it
>> occurs does make it easier to review if any should be addressed.
>>
>> If the goal is to have a CI job that we generally expect to pass, then
>> it makes sense to remove it. I don't feel super strongly either way.
> Is it possible for gatekeeper jobs to complain only on newly added
> violations?

> This avoids weird formatting like:
>
>    static const struct strbuf *
>           a_really_really_large_function_name(struct strbuf resolved,
>           const char *path, int flags)
>
> or
>
>    static const struct strbuf *a_really_really_large_function_name(
>    	    struct strbuf resolved, const char *path, int flags)
>
> to instead have something more readable like:
>
>    static const struct strbuf *a_really_really_large_function_name(struct strbuf resolved,
>           const char *path, int flags)
>
> This is done by bumping the values of 'PenaltyReturnTypeOnItsOwnLine'
> and 'PenaltyBreakOpenParenthesis' to 300. This is so that we can allow a
> few characters above the 80 column limit to make code more readable.

> Karthik Nayak <karthik.188@gmail.com> writes:
>
> [snip]
>
> > This avoids weird formatting like:
> >
> >    static const struct strbuf *
> >           a_really_really_large_function_name(struct strbuf resolved,
> >           const char *path, int flags)
> >
> > or
> >
> >    static const struct strbuf *a_really_really_large_function_name(
> >    	    struct strbuf resolved, const char *path, int flags)
> >
> > to instead have something more readable like:
> >
> >    static const struct strbuf *a_really_really_large_function_name(struct strbuf resolved,
> >           const char *path, int flags)
> >
> > This is done by bumping the values of 'PenaltyReturnTypeOnItsOwnLine'
> > and 'PenaltyBreakOpenParenthesis' to 300. This is so that we can allow a
> > few characters above the 80 column limit to make code more readable.
>
> I'm really liking the idea of penalties, but I feel we're relying too
> much on guestimation of these values. What do you think about adding
> example files to our codebase? Having concrete examples at hand will
> allow us to tweak the values in the future, while preserving behavior
> for existing cases. Or when we decide to change them, we understand
> what and when.

> On Mon, Oct 14, 2024 at 11:08:29AM +0200, Toon Claes wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>> [snip]
>>
>> > This avoids weird formatting like:
>> >
>> >    static const struct strbuf *
>> >           a_really_really_large_function_name(struct strbuf resolved,
>> >           const char *path, int flags)
>> >
>> > or
>> >
>> >    static const struct strbuf *a_really_really_large_function_name(
>> >    	    struct strbuf resolved, const char *path, int flags)
>> >
>> > to instead have something more readable like:
>> >
>> >    static const struct strbuf *a_really_really_large_function_name(struct strbuf resolved,
>> >           const char *path, int flags)
>> >
>> > This is done by bumping the values of 'PenaltyReturnTypeOnItsOwnLine'
>> > and 'PenaltyBreakOpenParenthesis' to 300. This is so that we can allow a
>> > few characters above the 80 column limit to make code more readable.
>>
>> I'm really liking the idea of penalties, but I feel we're relying too
>> much on guestimation of these values. What do you think about adding
>> example files to our codebase? Having concrete examples at hand will
>> allow us to tweak the values in the future, while preserving behavior
>> for existing cases. Or when we decide to change them, we understand
>> what and when.
>
> I am not sure I see it the same way.
>
> I might just be ill-informed or not experienced with these clang-format
> rules, but having these penalties be defined as such makes it difficult
> to reason about what lines will and won't be re-wrapped as a result of
> running the formatter.
>
> What is the purpose of these penalties?
>

> Karthik Nayak <karthik.188@gmail.com> writes:
>
> [snip]
>
>> This avoids weird formatting like:
>>
>>    static const struct strbuf *
>>           a_really_really_large_function_name(struct strbuf resolved,
>>           const char *path, int flags)
>>
>> or
>>
>>    static const struct strbuf *a_really_really_large_function_name(
>>    	    struct strbuf resolved, const char *path, int flags)
>>
>> to instead have something more readable like:
>>
>>    static const struct strbuf *a_really_really_large_function_name(struct strbuf resolved,
>>           const char *path, int flags)
>>
>> This is done by bumping the values of 'PenaltyReturnTypeOnItsOwnLine'
>> and 'PenaltyBreakOpenParenthesis' to 300. This is so that we can allow a
>> few characters above the 80 column limit to make code more readable.
>
> I'm really liking the idea of penalties, but I feel we're relying too
> much on guestimation of these values. What do you think about adding

> example files to our codebase? Having concrete examples at hand will
> allow us to tweak the values in the future, while preserving behavior
> for existing cases. Or when we decide to change them, we understand
> what and when.
>
> Now, I'm not sure where to put such files. I think I would suggest
> something like t/style-lint or t/clang-format. Anyway, for our tooling
> it doesn't seem to matter, because both `make style` and
> `ci/run-style-check.sh` pick up all .c and .h files anywhere in the
> source tree. Adding a README to that directory will help people
> understand why the files are there.
>

>
> In 42efde4c29 (clang-format: adjust line break penalties, 2017-09-29) we
> adjusted the line break penalties to really fine tune what we care about
> while doing line breaks. Modify some of those to be more inline with
> what we care about in the Git project now.
>
> We need to understand that the values set to penalties in
> '.clang-format' are relative to each other and do not hold any absolute
> value. The penalty arguments take an 'Unsigned' value, so we have some
> liberty over the values we can set.
>
> First, in that commit, we decided, that under no circumstances do we
> want to exceed 80 characters. This seems a bit too strict. We do
> overshoot this limit from time to time to prioritize readability.

> So
> let's reduce the value for 'PenaltyExcessCharacter' to 10. This means we
> that we add a penalty of 10 for each character that exceeds the column
> limit. By itself this is enough to restrict to column limit. Tuning
> other penalties in relation to this is what is important.
>
> The penalty `PenaltyBreakAssignment` talks about the penalty for
> breaking an assignment operator on to the next line. In our project, we
> are okay with this, so giving a value of 5, which is below the value for
> 'PenaltyExcessCharacter' ensures that in the end, even 1 character over
> the column limit is not worth keeping an assignment on the same line.
>
> Similarly set the penalty for breaking before the first call parameter
> 'PenaltyBreakBeforeFirstCallParameter' and the penalty for breaking
> comments 'PenaltyBreakComment' and the penalty for breaking string
> literals 'PenaltyBreakString' also to 5.
>
> Finally, we really care about not breaking the return type into its own
> line and we really care about not breaking before an open parenthesis.
> This avoids weird formatting like:
>
>    static const struct strbuf *
>           a_really_really_large_function_name(struct strbuf resolved,
>           const char *path, int flags)

>
> or
>
>    static const struct strbuf *a_really_really_large_function_name(
>             struct strbuf resolved, const char *path, int flags)

>
> to instead have something more readable like:
>
>    static const struct strbuf *a_really_really_large_function_name(struct strbuf resolved,
>           const char *path, int flags)
>
> This is done by bumping the values of 'PenaltyReturnTypeOnItsOwnLine'
> and 'PenaltyBreakOpenParenthesis' to 300. This is so that we can allow a
> few characters above the 80 column limit to make code more readable.

>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  .clang-format | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/.clang-format b/.clang-format
> index 41969eca4b..66a2360ae5 100644
> --- a/.clang-format
> +++ b/.clang-format
> @@ -209,13 +209,14 @@ KeepEmptyLinesAtTheStartOfBlocks: false
>
>  # Penalties
>  # This decides what order things should be done if a line is too long
> -PenaltyBreakAssignment: 10
> -PenaltyBreakBeforeFirstCallParameter: 30
> -PenaltyBreakComment: 10
> +PenaltyBreakAssignment: 5
> +PenaltyBreakBeforeFirstCallParameter: 5
> +PenaltyBreakComment: 5
>  PenaltyBreakFirstLessLess: 0
> -PenaltyBreakString: 10
> -PenaltyExcessCharacter: 100
> -PenaltyReturnTypeOnItsOwnLine: 60
> +PenaltyBreakOpenParenthesis: 300

> +PenaltyBreakString: 5
> +PenaltyExcessCharacter: 10
> +PenaltyReturnTypeOnItsOwnLine: 300
>
>  # Don't sort #include's
>  SortIncludes: false
> --
> 2.47.0
>

> On Fri, Oct 11, 2024 at 6:50 PM Karthik Nayak <karthik.188@gmail.com> wrote:
>>
>> In 42efde4c29 (clang-format: adjust line break penalties, 2017-09-29) we
>> adjusted the line break penalties to really fine tune what we care about
>> while doing line breaks. Modify some of those to be more inline with
>> what we care about in the Git project now.
>>
>> We need to understand that the values set to penalties in
>> '.clang-format' are relative to each other and do not hold any absolute
>> value. The penalty arguments take an 'Unsigned' value, so we have some
>> liberty over the values we can set.
>>
>> First, in that commit, we decided, that under no circumstances do we
>> want to exceed 80 characters. This seems a bit too strict. We do
>> overshoot this limit from time to time to prioritize readability.
>
> I think that attempting to get the weights right so as to avoid cases
> where there was an intentional affordance for readability is going to
> be essentially impossible. Areas where there's an intentional
> disregard for the clang-format-generated formatting should disable the
> formatter for that line/region, instead of trying to find a way to
> adjust the rules to produce something that's going to end up being
> context dependent.
>

> Example: In ref-filter.c, there's 13 lines when initializing the
> `valid_atom` array that are >80 characters, and 20 lines that are >80
> columns (when using 8-space tabs). Line breaking in that block of code
> may be undesirable, so just disable clang-format there. I don't think
> there's a consistent set of penalties you could establish that would
> handle that well without mishandling some other section of code.

> It's also not clear what the reason for the overshoot is in many cases.
> - difference between "80 characters" and "80 columns"?
>     - (1394 >80char lines in *.{h,c}, 4849 >80col lines in the same files)
> - intentional for readability?
> - refactorings pushed originally compliant lines out of compliance?
> - no one caught it and it was just added without any intentional decision?
>

>> So
>> let's reduce the value for 'PenaltyExcessCharacter' to 10. This means we
>> that we add a penalty of 10 for each character that exceeds the column
>> limit. By itself this is enough to restrict to column limit. Tuning
>> other penalties in relation to this is what is important.
>>
>> The penalty `PenaltyBreakAssignment` talks about the penalty for
>> breaking an assignment operator on to the next line. In our project, we
>> are okay with this, so giving a value of 5, which is below the value for
>> 'PenaltyExcessCharacter' ensures that in the end, even 1 character over
>> the column limit is not worth keeping an assignment on the same line.
>>
>> Similarly set the penalty for breaking before the first call parameter
>> 'PenaltyBreakBeforeFirstCallParameter' and the penalty for breaking
>> comments 'PenaltyBreakComment' and the penalty for breaking string
>> literals 'PenaltyBreakString' also to 5.
>>
>> Finally, we really care about not breaking the return type into its own
>> line and we really care about not breaking before an open parenthesis.
>> This avoids weird formatting like:
>>
>>    static const struct strbuf *
>>           a_really_really_large_function_name(struct strbuf resolved,
>>           const char *path, int flags)
>
> Is this how it'd be indented without the penalties, or would it do
> this, with the function name indented the same amount as the return
> type (which is, in C, probably going to be the 0th column most times):
>
> static const struct strbuf *
> a_really_really_large_function_name(struct strbuf resolved,
>         const char *path, int flags)
>

>>
>> or
>>
>>    static const struct strbuf *a_really_really_large_function_name(
>>             struct strbuf resolved, const char *path, int flags)
>
> Personal opinion: I prefer this over the version that has a single
> argument on the first line. My preference for reading functions is:
>
> return_type func_name(arg1, arg2,
>                       arg3, arg4,
>                       arg5, arg6, ...);
>
> Or
>
> return_type func_name(
>         arg1, arg2, arg3, arg4,
>         arg5, arg6, ...);
>

> or, in some cases, putting every argument on their own line (typically
> when the majority of the arguments are already on their own line, not
> having one "hiding" somewhere is preferable, but at this point if
> that's not what my formatter does, I don't fight it).
>
> For functions that accept an obvious first parameter, such as
> `strbuf_add`, maybe having the first parameter on the first line is
> acceptable/desirable, since it's "obvious" what it is/does. But for
> many functions that's not the case, and needing to read the end of the
> first line, potentially beyond the 80th column, feels weird.
>
>>
>> to instead have something more readable like:
>>
>>    static const struct strbuf *a_really_really_large_function_name(struct strbuf resolved,
>>           const char *path, int flags)
>>
>> This is done by bumping the values of 'PenaltyReturnTypeOnItsOwnLine'
>> and 'PenaltyBreakOpenParenthesis' to 300. This is so that we can allow a
>> few characters above the 80 column limit to make code more readable.
>
> A few examples, such as by formatting the code using the current rules
> (since much of the codebase does not currently comply), and then
> changing the penalties and seeing what changes, might be nice?
>

>
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>>  .clang-format | 13 +++++++------
>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/.clang-format b/.clang-format
>> index 41969eca4b..66a2360ae5 100644
>> --- a/.clang-format
>> +++ b/.clang-format
>> @@ -209,13 +209,14 @@ KeepEmptyLinesAtTheStartOfBlocks: false
>>
>>  # Penalties
>>  # This decides what order things should be done if a line is too long
>> -PenaltyBreakAssignment: 10
>> -PenaltyBreakBeforeFirstCallParameter: 30
>> -PenaltyBreakComment: 10
>> +PenaltyBreakAssignment: 5
>> +PenaltyBreakBeforeFirstCallParameter: 5
>> +PenaltyBreakComment: 5
>>  PenaltyBreakFirstLessLess: 0
>> -PenaltyBreakString: 10
>> -PenaltyExcessCharacter: 100
>> -PenaltyReturnTypeOnItsOwnLine: 60
>> +PenaltyBreakOpenParenthesis: 300
>
> How does this interact with PenaltyBreakBeforeFirstCallParameter? Does
> one override the other?
>

>> +PenaltyBreakString: 5
>> +PenaltyExcessCharacter: 10
>> +PenaltyReturnTypeOnItsOwnLine: 300
>>
>>  # Don't sort #include's
>>  SortIncludes: false
>> --
>> 2.47.0
>>

>
> Kyle Lippincott <spectral@google.com> writes:
>
> > On Fri, Oct 11, 2024 at 6:50 PM Karthik Nayak <karthik.188@gmail.com> wrote:
> >>
> >> In 42efde4c29 (clang-format: adjust line break penalties, 2017-09-29) we
> >> adjusted the line break penalties to really fine tune what we care about
> >> while doing line breaks. Modify some of those to be more inline with
> >> what we care about in the Git project now.
> >>
> >> We need to understand that the values set to penalties in
> >> '.clang-format' are relative to each other and do not hold any absolute
> >> value. The penalty arguments take an 'Unsigned' value, so we have some
> >> liberty over the values we can set.
> >>
> >> First, in that commit, we decided, that under no circumstances do we
> >> want to exceed 80 characters. This seems a bit too strict. We do
> >> overshoot this limit from time to time to prioritize readability.
> >
> > I think that attempting to get the weights right so as to avoid cases
> > where there was an intentional affordance for readability is going to
> > be essentially impossible. Areas where there's an intentional
> > disregard for the clang-format-generated formatting should disable the
> > formatter for that line/region, instead of trying to find a way to
> > adjust the rules to produce something that's going to end up being
> > context dependent.
> >
>
> To some extent I agree. But the issue is that clang-format is still not
> enforced within the code base. So expecting users to add:
>     // clang-format off
> will not hold, at least for _now_.
>
> So the next best thing we can do is to get the format rules as close as
> we can to the current styling, so the actual errors thrown by the CI job
> is something we can look at without containing too many false positives.
>
> > Example: In ref-filter.c, there's 13 lines when initializing the
> > `valid_atom` array that are >80 characters, and 20 lines that are >80
> > columns (when using 8-space tabs). Line breaking in that block of code
> > may be undesirable, so just disable clang-format there. I don't think
> > there's a consistent set of penalties you could establish that would
> > handle that well without mishandling some other section of code.
>
> While true, we can quantify if it is better or not:
>
>       ❯ ci/run-style-check.sh @~50 | wc -l
>       4718 (master)
>
>       ❯ ci/run-style-check.sh @~53 | wc -l
>       4475 (with these patches)
>
> And looking through the other changes, those look like violations which
> should have been fixed.
>
> > It's also not clear what the reason for the overshoot is in many cases.
> > - difference between "80 characters" and "80 columns"?
> >     - (1394 >80char lines in *.{h,c}, 4849 >80col lines in the same files)
> > - intentional for readability?
> > - refactorings pushed originally compliant lines out of compliance?
> > - no one caught it and it was just added without any intentional decision?
> >
>
> I agree with your inference here, but I'm not sure there is a smooth way
> to have this information. Either we go full in and say we enable the
> formatting and every patch must conform to it, or we simply keep the
> clang-format as a warning system. Currently we do neither. I'd say we
> should be in a state to reach the latter and then we can gradually think
> of how to move to the former.
>
> >> So
> >> let's reduce the value for 'PenaltyExcessCharacter' to 10. This means we
> >> that we add a penalty of 10 for each character that exceeds the column
> >> limit. By itself this is enough to restrict to column limit. Tuning
> >> other penalties in relation to this is what is important.
> >>
> >> The penalty `PenaltyBreakAssignment` talks about the penalty for
> >> breaking an assignment operator on to the next line. In our project, we
> >> are okay with this, so giving a value of 5, which is below the value for
> >> 'PenaltyExcessCharacter' ensures that in the end, even 1 character over
> >> the column limit is not worth keeping an assignment on the same line.
> >>
> >> Similarly set the penalty for breaking before the first call parameter
> >> 'PenaltyBreakBeforeFirstCallParameter' and the penalty for breaking
> >> comments 'PenaltyBreakComment' and the penalty for breaking string
> >> literals 'PenaltyBreakString' also to 5.
> >>
> >> Finally, we really care about not breaking the return type into its own
> >> line and we really care about not breaking before an open parenthesis.
> >> This avoids weird formatting like:
> >>
> >>    static const struct strbuf *
> >>           a_really_really_large_function_name(struct strbuf resolved,
> >>           const char *path, int flags)
> >
> > Is this how it'd be indented without the penalties, or would it do
> > this, with the function name indented the same amount as the return
> > type (which is, in C, probably going to be the 0th column most times):
> >
> > static const struct strbuf *
> > a_really_really_large_function_name(struct strbuf resolved,
> >         const char *path, int flags)
> >
>
> It will be indented, so not the 0th column.

>
> >>
> >> or
> >>
> >>    static const struct strbuf *a_really_really_large_function_name(
> >>             struct strbuf resolved, const char *path, int flags)
> >
> > Personal opinion: I prefer this over the version that has a single
> > argument on the first line. My preference for reading functions is:
> >
> > return_type func_name(arg1, arg2,
> >                       arg3, arg4,
> >                       arg5, arg6, ...);
> >
> > Or
> >
> > return_type func_name(
> >         arg1, arg2, arg3, arg4,
> >         arg5, arg6, ...);
> >
>
> I'm mostly basing my changes on the current state of the 'clang-format'
> and our code base. I'm happy to change it if everyone agrees on this :)

>
> > or, in some cases, putting every argument on their own line (typically
> > when the majority of the arguments are already on their own line, not
> > having one "hiding" somewhere is preferable, but at this point if
> > that's not what my formatter does, I don't fight it).
> >
> > For functions that accept an obvious first parameter, such as
> > `strbuf_add`, maybe having the first parameter on the first line is
> > acceptable/desirable, since it's "obvious" what it is/does. But for
> > many functions that's not the case, and needing to read the end of the
> > first line, potentially beyond the 80th column, feels weird.
> >
> >>
> >> to instead have something more readable like:
> >>
> >>    static const struct strbuf *a_really_really_large_function_name(struct strbuf resolved,
> >>           const char *path, int flags)

> >>
> >> This is done by bumping the values of 'PenaltyReturnTypeOnItsOwnLine'
> >> and 'PenaltyBreakOpenParenthesis' to 300. This is so that we can allow a
> >> few characters above the 80 column limit to make code more readable.
> >
> > A few examples, such as by formatting the code using the current rules
> > (since much of the codebase does not currently comply), and then
> > changing the penalties and seeing what changes, might be nice?
> >
>
> You mean apart from the example above?

>
> >
> >>
> >> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> >> ---
> >>  .clang-format | 13 +++++++------
> >>  1 file changed, 7 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/.clang-format b/.clang-format
> >> index 41969eca4b..66a2360ae5 100644
> >> --- a/.clang-format
> >> +++ b/.clang-format
> >> @@ -209,13 +209,14 @@ KeepEmptyLinesAtTheStartOfBlocks: false
> >>
> >>  # Penalties
> >>  # This decides what order things should be done if a line is too long
> >> -PenaltyBreakAssignment: 10
> >> -PenaltyBreakBeforeFirstCallParameter: 30
> >> -PenaltyBreakComment: 10
> >> +PenaltyBreakAssignment: 5
> >> +PenaltyBreakBeforeFirstCallParameter: 5
> >> +PenaltyBreakComment: 5
> >>  PenaltyBreakFirstLessLess: 0
> >> -PenaltyBreakString: 10
> >> -PenaltyExcessCharacter: 100
> >> -PenaltyReturnTypeOnItsOwnLine: 60
> >> +PenaltyBreakOpenParenthesis: 300
> >
> > How does this interact with PenaltyBreakBeforeFirstCallParameter? Does
> > one override the other?
> >
>
> From my understanding PenaltyBreakOpenParenthesis seems to apply more
> generally and is the more preferred value.
>
> [1]: https://github.com/llvm/llvm-project/blob/a4367d2d136420f562f64e7731b9393fb609f3fc/clang/lib/Format/TokenAnnotator.cpp#L4322
>
> >> +PenaltyBreakString: 5
> >> +PenaltyExcessCharacter: 10
> >> +PenaltyReturnTypeOnItsOwnLine: 300
> >>
> >>  # Don't sort #include's
> >>  SortIncludes: false
> >> --
> >> 2.47.0
> >>

> On Tue, Oct 15, 2024 at 5:37 AM karthik nayak <karthik.188@gmail.com> wrote:
>>
>> Kyle Lippincott <spectral@google.com> writes:
>>
>> > On Fri, Oct 11, 2024 at 6:50 PM Karthik Nayak <karthik.188@gmail.com> wrote:
>> >>
>> >> In 42efde4c29 (clang-format: adjust line break penalties, 2017-09-29) we
>> >> adjusted the line break penalties to really fine tune what we care about
>> >> while doing line breaks. Modify some of those to be more inline with
>> >> what we care about in the Git project now.
>> >>
>> >> We need to understand that the values set to penalties in
>> >> '.clang-format' are relative to each other and do not hold any absolute
>> >> value. The penalty arguments take an 'Unsigned' value, so we have some
>> >> liberty over the values we can set.
>> >>
>> >> First, in that commit, we decided, that under no circumstances do we
>> >> want to exceed 80 characters. This seems a bit too strict. We do
>> >> overshoot this limit from time to time to prioritize readability.
>> >
>> > I think that attempting to get the weights right so as to avoid cases
>> > where there was an intentional affordance for readability is going to
>> > be essentially impossible. Areas where there's an intentional
>> > disregard for the clang-format-generated formatting should disable the
>> > formatter for that line/region, instead of trying to find a way to
>> > adjust the rules to produce something that's going to end up being
>> > context dependent.
>> >
>>
>> To some extent I agree. But the issue is that clang-format is still not
>> enforced within the code base. So expecting users to add:
>>     // clang-format off
>> will not hold, at least for _now_.
>>
>> So the next best thing we can do is to get the format rules as close as
>> we can to the current styling, so the actual errors thrown by the CI job
>> is something we can look at without containing too many false positives.
>>
>> > Example: In ref-filter.c, there's 13 lines when initializing the
>> > `valid_atom` array that are >80 characters, and 20 lines that are >80
>> > columns (when using 8-space tabs). Line breaking in that block of code
>> > may be undesirable, so just disable clang-format there. I don't think
>> > there's a consistent set of penalties you could establish that would
>> > handle that well without mishandling some other section of code.
>>
>> While true, we can quantify if it is better or not:
>>
>>       ❯ ci/run-style-check.sh @~50 | wc -l
>>       4718 (master)
>>
>>       ❯ ci/run-style-check.sh @~53 | wc -l
>>       4475 (with these patches)
>>
>> And looking through the other changes, those look like violations which
>> should have been fixed.
>>
>> > It's also not clear what the reason for the overshoot is in many cases.
>> > - difference between "80 characters" and "80 columns"?
>> >     - (1394 >80char lines in *.{h,c}, 4849 >80col lines in the same files)
>> > - intentional for readability?
>> > - refactorings pushed originally compliant lines out of compliance?
>> > - no one caught it and it was just added without any intentional decision?
>> >
>>
>> I agree with your inference here, but I'm not sure there is a smooth way
>> to have this information. Either we go full in and say we enable the
>> formatting and every patch must conform to it, or we simply keep the
>> clang-format as a warning system. Currently we do neither. I'd say we
>> should be in a state to reach the latter and then we can gradually think
>> of how to move to the former.
>>
>> >> So
>> >> let's reduce the value for 'PenaltyExcessCharacter' to 10. This means we
>> >> that we add a penalty of 10 for each character that exceeds the column
>> >> limit. By itself this is enough to restrict to column limit. Tuning
>> >> other penalties in relation to this is what is important.
>> >>
>> >> The penalty `PenaltyBreakAssignment` talks about the penalty for
>> >> breaking an assignment operator on to the next line. In our project, we
>> >> are okay with this, so giving a value of 5, which is below the value for
>> >> 'PenaltyExcessCharacter' ensures that in the end, even 1 character over
>> >> the column limit is not worth keeping an assignment on the same line.
>> >>
>> >> Similarly set the penalty for breaking before the first call parameter
>> >> 'PenaltyBreakBeforeFirstCallParameter' and the penalty for breaking
>> >> comments 'PenaltyBreakComment' and the penalty for breaking string
>> >> literals 'PenaltyBreakString' also to 5.
>> >>
>> >> Finally, we really care about not breaking the return type into its own
>> >> line and we really care about not breaking before an open parenthesis.
>> >> This avoids weird formatting like:
>> >>
>> >>    static const struct strbuf *
>> >>           a_really_really_large_function_name(struct strbuf resolved,
>> >>           const char *path, int flags)
>> >
>> > Is this how it'd be indented without the penalties, or would it do
>> > this, with the function name indented the same amount as the return
>> > type (which is, in C, probably going to be the 0th column most times):
>> >
>> > static const struct strbuf *
>> > a_really_really_large_function_name(struct strbuf resolved,
>> >         const char *path, int flags)
>> >
>>
>> It will be indented, so not the 0th column.
>
> I'm not getting that behavior when I try it. Is this only indented
> with your updated penalties?
>
> Currently on ef8ce8f3d4344fd3af049c17eeba5cd20d98b69f, git status is
> clean, added this line (no line breaks, just in case my email client
> makes a mess of this) to top of path.h (chosen arbitrarily):
>
> static const struct strbuf *a_really_really_large_function_name(struct
> strbuf resolved, const char *path, int flags);
>
> and ran `clang-format path.h | head -n20` and got this (where `int
> flags` is indented to align with the opening `(`, but tabs cause
> problems yet again):
>
> static const struct strbuf *
> a_really_really_large_function_name(struct strbuf resolved, const char *path,
>     int flags);
>
> `clang-format --version` shows it's a google-internal build, but it
> still respects the .clang-format file, so this shouldn't matter? I'm
> assuming it's a relatively recent (within the past 1 month) commit
> that it's based off of.
>

>>
>> >>
>> >> or
>> >>
>> >>    static const struct strbuf *a_really_really_large_function_name(
>> >>             struct strbuf resolved, const char *path, int flags)
>> >
>> > Personal opinion: I prefer this over the version that has a single
>> > argument on the first line. My preference for reading functions is:
>> >
>> > return_type func_name(arg1, arg2,
>> >                       arg3, arg4,
>> >                       arg5, arg6, ...);
>> >
>> > Or
>> >
>> > return_type func_name(
>> >         arg1, arg2, arg3, arg4,
>> >         arg5, arg6, ...);
>> >
>>
>> I'm mostly basing my changes on the current state of the 'clang-format'
>> and our code base. I'm happy to change it if everyone agrees on this :)
>
> I'm wondering if tabs have caused some confusion here... [thought
> continued below]
>
>>
>> > or, in some cases, putting every argument on their own line (typically
>> > when the majority of the arguments are already on their own line, not
>> > having one "hiding" somewhere is preferable, but at this point if
>> > that's not what my formatter does, I don't fight it).
>> >
>> > For functions that accept an obvious first parameter, such as
>> > `strbuf_add`, maybe having the first parameter on the first line is
>> > acceptable/desirable, since it's "obvious" what it is/does. But for
>> > many functions that's not the case, and needing to read the end of the
>> > first line, potentially beyond the 80th column, feels weird.
>> >
>> >>
>> >> to instead have something more readable like:
>> >>
>> >>    static const struct strbuf *a_really_really_large_function_name(struct strbuf resolved,
>> >>           const char *path, int flags)
>
> Does this have `const char *path` aligned with the opening `(`? if so,
> then that matches my first example and I'm fine with it.

> If this is
> truly "first argument immediately after (, other arguments indented
> one indentation level", then my original comment stands: I don't find
> this readable at all, and I don't see evidence of this being an
> acceptable format according to our CodingGuidelines document.
>
> I also don't understand how the penalties would produce t
>

>> >>
>> >> This is done by bumping the values of 'PenaltyReturnTypeOnItsOwnLine'
>> >> and 'PenaltyBreakOpenParenthesis' to 300. This is so that we can allow a
>> >> few characters above the 80 column limit to make code more readable.
>> >
>> > A few examples, such as by formatting the code using the current rules
>> > (since much of the codebase does not currently comply), and then
>> > changing the penalties and seeing what changes, might be nice?
>> >
>>
>> You mean apart from the example above?
>
> The example above is hypothetical, but I think I was thinking about
> this incorrectly. Our existing codebase isn't formatted with
> clang-format, and formatting it first and then adjusting the penalties
> doesn't really provide much useful information.
>
> Setting the penalties as you have them in this patch, and running it
> on a copy of the line you have there, produces this for me:
>
> static const struct strbuf *
> a_really_really_large_function_name(struct strbuf resolved, const char *path,
>                                     int flags);
>
> The 80 column limit is still _strictly_ adhered to, it won't even go
> over by 1 character:
>
> static const struct strbuf *
> a_really_really_large_function_named_Ed(struct strbuf resolved,
>                                         const char *path, int flags);
>
> (note: switched tabs to spaces because tabs are difficult to use
> during discussions like this)
>
> Specifically:
> - 80 column hard limit applied
> - return type on its own line
> - continuation arguments are aligned on the next line (which is
> expected since we have AlignAfterOpenBracket set).
>

>
> We generally align consecutive macro definitions for better readability:
>
>   #define OUTPUT_ANNOTATE_COMPAT      (1U<<0)
>   #define OUTPUT_LONG_OBJECT_NAME     (1U<<1)
>   #define OUTPUT_RAW_TIMESTAMP        (1U<<2)
>   #define OUTPUT_PORCELAIN            (1U<<3)

>
> over
>
>   #define OUTPUT_ANNOTATE_COMPAT (1U<<0)
>   #define OUTPUT_LONG_OBJECT_NAME (1U<<1)
>   #define OUTPUT_RAW_TIMESTAMP (1U<<2)
>   #define OUTPUT_PORCELAIN (1U<<3)
>
> So let's add the rule in clang-format to follow this.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  .clang-format | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/.clang-format b/.clang-format
> index 66a2360ae5..9547fe1b77 100644
> --- a/.clang-format
> +++ b/.clang-format
> @@ -32,6 +32,9 @@ AlignConsecutiveAssignments: false
>  # double b = 3.14;
>  AlignConsecutiveDeclarations: false
>
> +# Align consecutive macro definitions.
> +AlignConsecutiveMacros: true
> +
>  # Align escaped newlines as far left as possible
>  # #define A   \
>  #   int aaaa; \
> --
> 2.47.0
>

> On Fri, Oct 11, 2024 at 6:50 PM Karthik Nayak <karthik.188@gmail.com> wrote:
>>
>> We generally align consecutive macro definitions for better readability:
>>
>>   #define OUTPUT_ANNOTATE_COMPAT      (1U<<0)
>>   #define OUTPUT_LONG_OBJECT_NAME     (1U<<1)
>>   #define OUTPUT_RAW_TIMESTAMP        (1U<<2)
>>   #define OUTPUT_PORCELAIN            (1U<<3)
>
> I like this change, thanks. Is there a way of apply clang-format for
> *only* one rule/aspect? i.e. can we apply *only* this, and preserve
> every other line? At first glance, I don't see a way of doing it. If
> there was, I might recommend a whole series just to applying these
> changes, but with how out of compliance much of the codebase is today,
> that's not going to be feasible; we'd need to format it in a way that
> we might not want (the current style), and then fix it, and that seems
> counterproductive.
>

>
> We enforce alignment of expressions after linebreaks. Which means for
> code such as
>
>     return a || b;
>
> it will expect:
>
>    return a ||
>           b;
>
> we instead want 'b' to be indent with tabs, which is already done by the
> 'ContinuationIndentWidth' variable.

> So let's explicitly set
> 'AlignOperands' to false.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  .clang-format | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/.clang-format b/.clang-format
> index 9547fe1b77..b48e7813e4 100644
> --- a/.clang-format
> +++ b/.clang-format
> @@ -42,10 +42,9 @@ AlignConsecutiveMacros: true
>  #   int cccccccc;
>  AlignEscapedNewlines: Left
>
> -# Align operands of binary and ternary expressions
> -# int aaa = bbbbbbbbbbb +
> -#           cccccc;
> -AlignOperands: true
> +# Don't enforce alignment after linebreaks and instead
> +# rely on the ContinuationIndentWidth value.
> +AlignOperands: false
>
>  # Don't align trailing comments
>  # int a; // Comment a
> --
> 2.47.0
>

> On Fri, Oct 11, 2024 at 6:50 PM Karthik Nayak <karthik.188@gmail.com> wrote:
>>
>> We enforce alignment of expressions after linebreaks. Which means for
>> code such as
>>
>>     return a || b;
>>
>> it will expect:
>>
>>    return a ||
>>           b;
>>
>> we instead want 'b' to be indent with tabs, which is already done by the
>> 'ContinuationIndentWidth' variable.
>
> Why do we want `b` to be indented by 8 columns instead of aligned? I
> think this is harder to read:
>
> int some_int_variable = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +
>         bbbbbbbbbbbbbbbbbbbbbbbbbbbbbb;
>

> Of course, this is even better, if it fits in 80 cols:
>
> int some_int_variable =
>         aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbb;
>

> On 24/10/18 10:46AM, Karthik Nayak wrote:
>> In 42efde4c29 (clang-format: adjust line break penalties, 2017-09-29) we
>> adjusted the line break penalties to really fine tune what we care about
>> while doing line breaks. Modify some of those to be more inline with
>> what we care about in the Git project now.
>
> From my understanding, the original motivation for these changes was to
> cut down on the noise from the clang-format CI job. These changes seem
> reasonable for that purpose, but affect the also formatter in general.
>

> Out of curiousity, would it be possible to just configured clang-format
> for the CI job to behave in this manner? Ultimately, I'm not sure that
> would be good idea though because having a diverged set of rules may
> just cause more noise.
>

> Karthik Nayak (2):
>   clang-format: re-adjust line break penalties
>   clang-format: align consecutive macro definitions
>
>  .clang-format | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)

> On Fri, Oct 18, 2024 at 10:46:44AM +0200, Karthik Nayak wrote:
>> Karthik Nayak (2):
>>   clang-format: re-adjust line break penalties
>>   clang-format: align consecutive macro definitions
>>
>>  .clang-format | 16 ++++++++++------
>>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> Thanks, will queue. Are we ready to start merging this one down?
>

> Taylor Blau <me@ttaylorr.com> writes:
>
> > On Fri, Oct 18, 2024 at 10:46:44AM +0200, Karthik Nayak wrote:
> >> Karthik Nayak (2):
> >>   clang-format: re-adjust line break penalties
> >>   clang-format: align consecutive macro definitions
> >>
> >>  .clang-format | 16 ++++++++++------
> >>  1 file changed, 10 insertions(+), 6 deletions(-)
> >
> > Thanks, will queue. Are we ready to start merging this one down?
>
> I'd wait for some reviews :)

> On Sun, Oct 20, 2024 at 06:17:58AM -0500, karthik nayak wrote:
>> Taylor Blau <me@ttaylorr.com> writes:
>>
>> > On Fri, Oct 18, 2024 at 10:46:44AM +0200, Karthik Nayak wrote:
>> >> Karthik Nayak (2):
>> >>   clang-format: re-adjust line break penalties
>> >>   clang-format: align consecutive macro definitions
>> >>
>> >>  .clang-format | 16 ++++++++++------
>> >>  1 file changed, 10 insertions(+), 6 deletions(-)
>> >
>> > Thanks, will queue. Are we ready to start merging this one down?
>>
>> I'd wait for some reviews :)
>
> OK. My impression was that the dust had more or less settled from
> earlier rounds. But let's wait.
>
> Thanks,
> Taylor

> Taylor Blau <me@ttaylorr.com> writes:
>
> > On Sun, Oct 20, 2024 at 06:17:58AM -0500, karthik nayak wrote:
> >> Taylor Blau <me@ttaylorr.com> writes:
> >>
> >> > On Fri, Oct 18, 2024 at 10:46:44AM +0200, Karthik Nayak wrote:
> >> >> Karthik Nayak (2):
> >> >>   clang-format: re-adjust line break penalties
> >> >>   clang-format: align consecutive macro definitions
> >> >>
> >> >>  .clang-format | 16 ++++++++++------
> >> >>  1 file changed, 10 insertions(+), 6 deletions(-)
> >> >
> >> > Thanks, will queue. Are we ready to start merging this one down?
> >>
> >> I'd wait for some reviews :)
> >
> > OK. My impression was that the dust had more or less settled from
> > earlier rounds. But let's wait.
> >
> > Thanks,
> > Taylor
>
> I'd be happy if it merged down, I'll see if someone from GitLab can help
> with a review.