Mailing List Archive: [PATCH 1/2] Indent code before next channge

[PATCH 1/2] Indent code before next channge

Jun 29, 2021, 1:12 PM

Post #1 of 7 (594 views)

From: John Ericson <John.Ericson@Obsidian.Systems>

This hopefully makes for a better diff.
---
configure.ac | 136 +++++++++++++++++++++++++--------------------------
1 file changed, 68 insertions(+), 68 deletions(-)

diff --git a/configure.ac b/configure.ac
index 3a14c2a7..b0a6d7fe 100644
--- a/configure.ac
+++ b/configure.ac
@@ -4580,82 +4580,82 @@ AC_ARG_WITH([kerberos5],
AC_DEFINE([KRB5], [1], [Define if you want Kerberos 5 support])
KRB5_MSG="yes"

- AC_PATH_TOOL([KRB5CONF], [krb5-config],
- [$KRB5ROOT/bin/krb5-config],
- [$KRB5ROOT/bin:$PATH])
- if test -x $KRB5CONF ; then
- K5CFLAGS="`$KRB5CONF --cflags`"
- K5LIBS="`$KRB5CONF --libs`"
- CPPFLAGS="$CPPFLAGS $K5CFLAGS"
-
- AC_MSG_CHECKING([for gssapi support])
- if $KRB5CONF | grep gssapi >/dev/null ; then
- AC_MSG_RESULT([yes])
- AC_DEFINE([GSSAPI], [1],
- [.Define this if you want GSSAPI
- support in the version 2 protocol])
- GSSCFLAGS="`$KRB5CONF --cflags gssapi`"
- GSSLIBS="`$KRB5CONF --libs gssapi`"
- CPPFLAGS="$CPPFLAGS $GSSCFLAGS"
- else
- AC_MSG_RESULT([no])
- fi
- AC_MSG_CHECKING([whether we are using Heimdal])
- AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[ #include <krb5.h>
- ]], [[ char *tmp = heimdal_version; ]])],
- [ AC_MSG_RESULT([yes])
- AC_DEFINE([HEIMDAL], [1],
- [.Define this if you are using the Heimdal
- version of Kerberos V5]) ],
- [AC_MSG_RESULT([no])
- ])
- else
- CPPFLAGS="$CPPFLAGS -I${KRB5ROOT}/include"
- LDFLAGS="$LDFLAGS -L${KRB5ROOT}/lib"
- AC_MSG_CHECKING([whether we are using Heimdal])
- AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[ #include <krb5.h>
- ]], [[ char *tmp = heimdal_version; ]])],
+ AC_PATH_TOOL([KRB5CONF], [krb5-config],
+ [$KRB5ROOT/bin/krb5-config],
+ [$KRB5ROOT/bin:$PATH])
+ if test -x $KRB5CONF ; then
+ K5CFLAGS="`$KRB5CONF --cflags`"
+ K5LIBS="`$KRB5CONF --libs`"
+ CPPFLAGS="$CPPFLAGS $K5CFLAGS"
+
+ AC_MSG_CHECKING([for gssapi support])
+ if $KRB5CONF | grep gssapi >/dev/null ; then
+ AC_MSG_RESULT([yes])
+ AC_DEFINE([GSSAPI], [1],
+ [.Define this if you want GSSAPI
+ support in the version 2 protocol])
+ GSSCFLAGS="`$KRB5CONF --cflags gssapi`"
+ GSSLIBS="`$KRB5CONF --libs gssapi`"
+ CPPFLAGS="$CPPFLAGS $GSSCFLAGS"
+ else
+ AC_MSG_RESULT([no])
+ fi
+ AC_MSG_CHECKING([whether we are using Heimdal])
+ AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[ #include <krb5.h>
+ ]], [[ char *tmp = heimdal_version; ]])],
[ AC_MSG_RESULT([yes])
- AC_DEFINE([HEIMDAL])
- K5LIBS="-lkrb5"
- K5LIBS="$K5LIBS -lcom_err -lasn1"
- AC_CHECK_LIB([roken], [net_write],
- [K5LIBS="$K5LIBS -lroken"])
- AC_CHECK_LIB([des], [des_cbc_encrypt],
- [K5LIBS="$K5LIBS -ldes"])
- ], [ AC_MSG_RESULT([no])
- K5LIBS="-lkrb5 -lk5crypto -lcom_err"
- ])
- AC_SEARCH_LIBS([dn_expand], [resolv])
+ AC_DEFINE([HEIMDAL], [1],
+ [.Define this if you are using the Heimdal
+ version of Kerberos V5]) ],
+ [AC_MSG_RESULT([no])
+ ])
+ else
+ CPPFLAGS="$CPPFLAGS -I${KRB5ROOT}/include"
+ LDFLAGS="$LDFLAGS -L${KRB5ROOT}/lib"
+ AC_MSG_CHECKING([whether we are using Heimdal])
+ AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[ #include <krb5.h>
+ ]], [[ char *tmp = heimdal_version; ]])],
+ [ AC_MSG_RESULT([yes])
+ AC_DEFINE([HEIMDAL])
+ K5LIBS="-lkrb5"
+ K5LIBS="$K5LIBS -lcom_err -lasn1"
+ AC_CHECK_LIB([roken], [net_write],
+ [K5LIBS="$K5LIBS -lroken"])
+ AC_CHECK_LIB([des], [des_cbc_encrypt],
+ [K5LIBS="$K5LIBS -ldes"])
+ ], [ AC_MSG_RESULT([no])
+ K5LIBS="-lkrb5 -lk5crypto -lcom_err"
+ ])
+ AC_SEARCH_LIBS([dn_expand], [resolv])

- AC_CHECK_LIB([gssapi_krb5], [gss_init_sec_context],
- [ AC_DEFINE([GSSAPI])
- GSSLIBS="-lgssapi_krb5" ],
- [ AC_CHECK_LIB([gssapi], [gss_init_sec_context],
+ AC_CHECK_LIB([gssapi_krb5], [gss_init_sec_context],
[ AC_DEFINE([GSSAPI])
- GSSLIBS="-lgssapi" ],
- [ AC_CHECK_LIB([gss], [gss_init_sec_context],
+ GSSLIBS="-lgssapi_krb5" ],
+ [ AC_CHECK_LIB([gssapi], [gss_init_sec_context],
[ AC_DEFINE([GSSAPI])
- GSSLIBS="-lgss" ],
- AC_MSG_WARN([Cannot find any suitable gss-api library - build may fail]))
+ GSSLIBS="-lgssapi" ],
+ [ AC_CHECK_LIB([gss], [gss_init_sec_context],
+ [ AC_DEFINE([GSSAPI])
+ GSSLIBS="-lgss" ],
+ AC_MSG_WARN([Cannot find any suitable gss-api library - build may fail]))
+ ])
])
- ])

- AC_CHECK_HEADER([gssapi.h], ,
- [ unset ac_cv_header_gssapi_h
- CPPFLAGS="$CPPFLAGS -I${KRB5ROOT}/include/gssapi"
- AC_CHECK_HEADERS([gssapi.h], ,
- AC_MSG_WARN([Cannot find any suitable gss-api header - build may fail])
- )
- ]
- )
+ AC_CHECK_HEADER([gssapi.h], ,
+ [ unset ac_cv_header_gssapi_h
+ CPPFLAGS="$CPPFLAGS -I${KRB5ROOT}/include/gssapi"
+ AC_CHECK_HEADERS([gssapi.h], ,
+ AC_MSG_WARN([Cannot find any suitable gss-api header - build may fail])
+ )
+ ]
+ )

- oldCPP="$CPPFLAGS"
- CPPFLAGS="$CPPFLAGS -I${KRB5ROOT}/include/gssapi"
- AC_CHECK_HEADER([gssapi_krb5.h], ,
- [ CPPFLAGS="$oldCPP" ])
+ oldCPP="$CPPFLAGS"
+ CPPFLAGS="$CPPFLAGS -I${KRB5ROOT}/include/gssapi"
+ AC_CHECK_HEADER([gssapi_krb5.h], ,
+ [ CPPFLAGS="$oldCPP" ])

- fi
+ fi
if test -n "${rpath_opt}" ; then
LDFLAGS="$LDFLAGS ${rpath_opt}${KRB5ROOT}/lib"
fi
--
2.31.1

_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Re: [PATCH 1/2] Indent code before next channge [ In reply to ]

openssh at davidnewall

Jun 29, 2021, 10:12 PM

Post #2 of 7 (594 views)

Permalink

On 30/6/21 5:42 am, list@johnericson.me wrote:
> From: John Ericson <John.Ericson@Obsidian.Systems>
>
> This hopefully makes for a better diff.
> ---
> configure.ac | 136 +++++++++++++++++++++++++--------------------------
> 1 file changed, 68 insertions(+), 68 deletions(-)

I rely on diff to see what is different between two versions, and not
necessarily
consecutive versions. It is an immensely effective technique for
finding where
something became broken.

Diffs like that which John proposes add noise, dramatically increasing
the effort
required to see real changes. Did he simply change indent or did he
also change
text? I can't tell at a glance.

I request that his diff be rejected.
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Re: [PATCH 1/2] Indent code before next channge [ In reply to ]

list at johnericson

Jun 29, 2021, 10:29 PM

Post #3 of 7 (594 views)

Permalink

tl;dr I am happy to redo the patches.

On 6/30/21 1:12 AM, David Newall wrote:

> I rely on diff to see what is different between two versions, and not
> necessarily
> consecutive versions. It is an immensely effective technique for
> finding where
> something became broken.

I also rely on that. I agree completely it is immensely effective.

I split the change in two commits precisely because I also care about
legible diffs. But I recognize there are many opinions on how best to
achieve that. I am happy to try it another way.

> Diffs like that which John proposes add noise, dramatically increasing
> the effort
> required to see real changes. Did he simply change indent or did he
> also change
> text? I can't tell at a glance.

The nature of the change is putting a decent chunk of existing autoconf
m4 within a newly-introduced if-then-else. That means either the
indention will become wrong, or there will be some churn/noise
reindenting the old code.

I chose to reindent first, making on odd 2x indent, and then add the new
code and if-then-else, also fixing the indent.

Another option would be to switch the order of the two patches: making
the meaningful change without touching the old code, and then indenting
the old code another level. A third option would be to simply do the
first of those to patches, making the minimal change and then leaving
the indentation incorrect.

I am happy to resubmit the patches broken down according to either of
those 2 alternatives, or any other alternative.

John

_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Re: [PATCH 1/2] Indent code before next channge [ In reply to ]

openssh at davidnewall

Jun 30, 2021, 3:14 AM

Post #4 of 7 (594 views)

Permalink

Hi John,

On 30/6/21 2:57 pm, John Cotton Ericson wrote:
> The nature of the change is putting a decent chunk of existing
> autoconf m4 within
> a newly-introduced if-then-else. That means either the indention will
> become wrong,
> or there will be some churn/noise reindenting the old code.
>
> I chose to reindent first, making on odd 2x indent, and then add the
> new code and
> if-then-else, also fixing the indent.

The problem is that a diff for a version from before your patch
comparing with a version
after will be noisy. That's what I want you to avoid.

> A third option would be to simply do the first of those to patches,
> making the minimal
> change and then leaving the indentation incorrect.

That's what I think you should do. Adding a comment at the if-then-else
saying why the
indent is wrong might help avoid flammage about wrongly indented code.

Regards,

David
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Re: [PATCH 1/2] Indent code before next channge [ In reply to ]

deengert at gmail

Jun 30, 2021, 5:37 AM

Post #5 of 7 (594 views)

Permalink

Three other options:
With diff use one of theses options: -E --ignore-tab-expansion, -Z --ignore-trailing-space,
-b --ignore-space-change, -w --ignore-all-space, and/or -B --ignore-blank-lines

With git-blame use the -w option.

With git do all format changes in separate commits, then add commit(s) to .ignoreRevsFile
so these format only commits do not show up in git blame. Best used when doing mass formatting
changes to the source, then inforce formatting standards when doing new commits.

On 6/30/2021 5:14 AM, David Newall wrote:
> Hi John,
>
> On 30/6/21 2:57 pm, John Cotton Ericson wrote:
>> The nature of the change is putting a decent chunk of existing autoconf m4 within
>> a newly-introduced if-then-else. That means either the indention will become wrong,
>> or there will be some churn/noise reindenting the old code.
>>
>> I chose to reindent first, making on odd 2x indent, and then add the new code and
>> if-then-else, also fixing the indent.
>
> The problem is that a diff for a version from before your patch comparing with a version
> after will be noisy. That's what I want you to avoid.
>
>> A third option would be to simply do the first of those to patches, making the minimal
>> change and then leaving the indentation incorrect.
>
> That's what I think you should do. Adding a comment at the if-then-else saying why the
> indent is wrong might help avoid flammage about wrongly indented code.
>
> Regards,
>
> David
> _______________________________________________
> openssh-unix-dev mailing list
> openssh-unix-dev@mindrot.org
> https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

--

Douglas E. Engert <DEEngert@gmail.com>

_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Re: [PATCH 1/2] Indent code before next channge [ In reply to ]

list at johnericson

Jun 30, 2021, 9:58 AM

Post #6 of 7 (594 views)

Permalink

Douglas,

Thanks. Those are good hidden gems with git, for sure!

Correct me if I am wrong, but since I am using git-send-email/git-format-patch the diff ones don't apply to me, right? Those would be for e.g. David and reviewers in general, not patch submitters?

Thus, the additional options for *creating* the patches you are proposing are:
* Combine patch, rely on reviewer to use the provided diff options
* Split patch, create a file like https://github.com/llvm/llvm-project/blob/main/.git-blame-ignore-revs for reviewers to use with git blame.
?

John

On Wed, Jun 30, 2021, at 8:37 AM, Douglas E Engert wrote:
> Three other options:
> With diff use one of theses options: -E --ignore-tab-expansion, -Z --ignore-trailing-space,
> -b --ignore-space-change, -w --ignore-all-space, and/or -B --ignore-blank-lines
>
> With git-blame use the -w option.
>
> With git do all format changes in separate commits, then add commit(s) to .ignoreRevsFile
> so these format only commits do not show up in git blame. Best used when doing mass formatting
> changes to the source, then inforce formatting standards when doing new commits.
>
>
> On 6/30/2021 5:14 AM, David Newall wrote:
> > Hi John,
> >
> > On 30/6/21 2:57 pm, John Cotton Ericson wrote:
> >> The nature of the change is putting a decent chunk of existing autoconf m4 within
> >> a newly-introduced if-then-else. That means either the indention will become wrong,
> >> or there will be some churn/noise reindenting the old code.
> >>
> >> I chose to reindent first, making on odd 2x indent, and then add the new code and
> >> if-then-else, also fixing the indent.
> >
> > The problem is that a diff for a version from before your patch comparing with a version
> > after will be noisy. That's what I want you to avoid.
> >
> >> A third option would be to simply do the first of those to patches, making the minimal
> >> change and then leaving the indentation incorrect.
> >
> > That's what I think you should do. Adding a comment at the if-then-else saying why the
> > indent is wrong might help avoid flammage about wrongly indented code.
> >
> > Regards,
> >
> > David
> > _______________________________________________
> > openssh-unix-dev mailing list
> > openssh-unix-dev@mindrot.org
> > https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
>
> --
>
> Douglas E. Engert <DEEngert@gmail.com>
>
> _______________________________________________
> openssh-unix-dev mailing list
> openssh-unix-dev@mindrot.org
> https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
>
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Re: [PATCH 1/2] Indent code before next channge [ In reply to ]

deengert at gmail

Jun 30, 2021, 11:23 AM

Post #7 of 7 (594 views)

Permalink

On 6/30/2021 11:58 AM, John Ericson wrote:
> Douglas,
>
> Thanks. Those are good hidden gems with git, for sure!
>
> Correct me if I am wrong, but since I am using git-send-email/git-format-patch the diff ones don't apply to me, right? Those would be for e.g. David and reviewers in general, not patch submitters?

Yes, David Newall <openssh@davidnewall.com> was complaining he could not tell if the
changes were just indenting or other formatting changes. So the use of diff options
would help him a lot.

>
> Thus, the additional options for /creating/ the patches you are proposing are:
>
> * Combine patch, rely on reviewer to use the provided diff options
> * Split patch, create a file like https://github.com/llvm/llvm-project/blob/main/.git-blame-ignore-revs <https://github.com/llvm/llvm-project/blob/main/.git-blame-ignore-revs> for reviewers to use
> with git blame.

I have not submitted any OpenSSH patches in a long time. I am saying there are ways
for the OpenSSH maintainers to address indentation changes.

But I am not sure why you had to change the indentations in your submission the first place.

>
> ?
>
> John
>
> On Wed, Jun 30, 2021, at 8:37 AM, Douglas E Engert wrote:
>> Three other options:
>> With diff use one of theses options: -E --ignore-tab-expansion, -Z --ignore-trailing-space,
>> -b --ignore-space-change, -w --ignore-all-space, and/or -B --ignore-blank-lines
>>
>> With git-blame use the -w option.
>>
>> With git do all format changes in separate commits, then add commit(s) to .ignoreRevsFile
>> so these format only commits do not show up in git blame. Best used when doing mass formatting
>> changes to the source, then inforce formatting standards when doing new commits.
>>
>>
>> On 6/30/2021 5:14 AM, David Newall wrote:
>> > Hi John,
>> >
>> > On 30/6/21 2:57 pm, John Cotton Ericson wrote:
>> >> The nature of the change is putting a decent chunk of existing autoconf m4 within
>> >> a newly-introduced if-then-else. That means either the indention will become wrong,
>> >> or there will be some churn/noise reindenting the old code.
>> >>
>> >> I chose to reindent first, making on odd 2x indent, and then add the new code and
>> >> if-then-else, also fixing the indent.
>> >
>> > The problem is that a diff for a version from before your patch comparing with a version
>> > after will be noisy. That's what I want you to avoid.
>> >
>> >> A third option would be to simply do the first of those to patches, making the minimal
>> >> change and then leaving the indentation incorrect.
>> >
>> > That's what I think you should do. Adding a comment at the if-then-else saying why the
>> > indent is wrong might help avoid flammage about wrongly indented code.
>> >
>> > Regards,
>> >
>> > David
>> > _______________________________________________
>> > openssh-unix-dev mailing list
>> > openssh-unix-dev@mindrot.org <mailto:openssh-unix-dev@mindrot.org>
>> > https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev <https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev>
>>
>> --
>>
>> Douglas E. Engert <DEEngert@gmail.com <mailto:DEEngert@gmail.com>>
>>
>> _______________________________________________
>> openssh-unix-dev mailing list
>> openssh-unix-dev@mindrot.org <mailto:openssh-unix-dev@mindrot.org>
>> https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev <https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev>
>>
>

--

Douglas E. Engert <DEEngert@gmail.com>

_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev