Questionable "cosmetic changes" commits

Questionable "cosmetic changes" commits

[Mark H Weaver]

[-- Attachment #1: Type: text/plain, Size: 3090 bytes --]

Hello fellow Guix,

In recent months there have been several "cosmetic changes" commits that
I find questionable.  These commits reorder package fields and reindent
code that was already ordered and indented according to our conventions,
apparently in order to match the author's personal preferences.

These commits also sometimes remove useful comments, although this is
not mentioned in the commit logs and not easily noticed in the diffs
amongst the noise of reordering and reindentation.

Here are some recent examples:

https://git.sv.gnu.org/cgit/guix.git/commit/?id=c3264f9e100ad6aefe5216002b68f3bfdcf6be95
https://git.sv.gnu.org/cgit/guix.git/commit/?id=416b1b9f56b514677660b56992cea1c78e00f519
https://git.sv.gnu.org/cgit/guix.git/commit/?id=2394b76bd223f5903e325f1f0e6d6b6fe69d00ed
https://git.sv.gnu.org/cgit/guix.git/commit/?id=d829c348f8a3c4de7e0dedeb4f96913357ae5294
https://git.sv.gnu.org/cgit/guix.git/commit/?id=7c63d0e29f3b33719a2e581d07125a9d03a0ec88
https://git.sv.gnu.org/cgit/guix.git/commit/?id=51e7e72bca9622560cde27db785b2d3e3fe058ae
https://git.sv.gnu.org/cgit/guix.git/commit/?id=0bb718c1b2c8df29ec85a81f002c54061c05ef65
https://git.sv.gnu.org/cgit/guix.git/commit/?id=b168f2ba53b938e1b322c79e5bfa47fcc506b803
https://git.sv.gnu.org/cgit/guix.git/commit/?id=d257697dc1ab4d2a278415d75b057c072f4660ec
https://git.sv.gnu.org/cgit/guix.git/commit/?id=b96961c9d25dd4efeaecf33813f9025282b25869
https://git.sv.gnu.org/cgit/guix.git/commit/?id=98f1951bb93524ed7bd212d884ed17ef21d4653d

I've included below one example for illustration.  The following commit
removes two comments (one about licensing details, and another
explaining why 'libffi' is needed in propagated-inputs), moves the
'home-page' field from its conventional place above the 'synopsis' to
below the 'description' (a common feature among this recent batch of
commits), swaps the order of the 'inputs' and 'native-inputs' fields,
and reindents several fields to be more vertically oriented, as
illustrated by the following change:

___ (native-search-paths
____ (list (search-path-specification
___________ (variable "GI_TYPELIB_PATH")
___________ (files '("lib/girepository-1.0")))))

was changed to:

___ (native-search-paths
____ (list
_____ (search-path-specification
______ (variable "GI_TYPELIB_PATH")
______ (files '("lib/girepository-1.0")))))

I think that commits like this are best avoided for several reasons.
Most importantly, they make merges between branches more error prone.

We all have our own personal preferences of how best to indent scheme
code, but if more of us adopted the habit of needlessly reordering
fields and reindenting code of every package we touch, as one of us
seems to have done, it could get out of hand quickly.  For example, my
personal preference would be to reverse the indentation change of the
'native-search-paths' field shown above, and to move the 'home-page'
field back above the 'synopsis' to match our usual conventions.  Should
I change those things back the next time I update that package?

What do other people think?

      Regards,
        Mark


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: [PATCH] gnu: gobject-introspection: Make some cosmetic changes --]
[-- Type: text/x-patch, Size: 3750 bytes --]

From c3264f9e100ad6aefe5216002b68f3bfdcf6be95 Mon Sep 17 00:00:00 2001
From: Raghav Gururajan <raghavgururajan@disroot.org>
Date: Thu, 24 Sep 2020 08:57:59 -0400
Subject: [PATCH] gnu: gobject-introspection: Make some cosmetic changes.

* gnu/packages/glib.scm (gobject-introspection): Make some cosmetic changes.

Signed-off-by: Danny Milosavljevic <dannym@scratchpost.org>
---
 gnu/packages/glib.scm | 45 ++++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/gnu/packages/glib.scm b/gnu/packages/glib.scm
index 43523e516d..a4fa6faefb 100644
--- a/gnu/packages/glib.scm
+++ b/gnu/packages/glib.scm
@@ -429,17 +429,20 @@ dynamic loading, and an object system.")
   (package
     (name "gobject-introspection")
     (version "1.62.0")
-    (source (origin
-             (method url-fetch)
-             (uri (string-append "mirror://gnome/sources/"
-                   "gobject-introspection/" (version-major+minor version)
-                   "/gobject-introspection-" version ".tar.xz"))
-             (sha256
-              (base32 "18lhglg9v6y83lhqzyifc1z0wrlawzrhzzxx0a3h1g7xaz97xvmi"))
-             (patches (search-patches
-                       "gobject-introspection-cc.patch"
-                       "gobject-introspection-girepository.patch"
-                       "gobject-introspection-absolute-shlib-path.patch"))))
+    (source
+     (origin
+       (method url-fetch)
+       (uri
+        (string-append "mirror://gnome/sources/"
+                       "gobject-introspection/" (version-major+minor version)
+                       "/gobject-introspection-" version ".tar.xz"))
+       (sha256
+        (base32 "18lhglg9v6y83lhqzyifc1z0wrlawzrhzzxx0a3h1g7xaz97xvmi"))
+       (patches
+        (search-patches
+         "gobject-introspection-cc.patch"
+         "gobject-introspection-girepository.patch"
+         "gobject-introspection-absolute-shlib-path.patch"))))
     (build-system meson-build-system)
     (arguments
      `(#:phases
@@ -450,25 +453,23 @@ dynamic loading, and an object system.")
                (("#!@PYTHON_CMD@")
                 (string-append "#!" (which "python3"))))
              #t)))))
+    (native-inputs
+     `(("glib" ,glib "bin")
+       ("pkg-config" ,pkg-config)))
     (inputs
      `(("bison" ,bison)
        ("flex" ,flex)
        ("glib" ,glib)
        ("python" ,python-wrapper)
        ("zlib" ,zlib)))
-    (native-inputs
-     `(("glib" ,glib "bin")
-       ("pkg-config" ,pkg-config)))
     (propagated-inputs
-     `(;; In practice, GIR users will need libffi when using
-       ;; gobject-introspection.
-       ("libffi" ,libffi)))
+     `(("libffi" ,libffi)))
     (native-search-paths
-     (list (search-path-specification
-            (variable "GI_TYPELIB_PATH")
-            (files '("lib/girepository-1.0")))))
+     (list
+      (search-path-specification
+       (variable "GI_TYPELIB_PATH")
+       (files '("lib/girepository-1.0")))))
     (search-paths native-search-paths)
-    (home-page "https://wiki.gnome.org/GObjectIntrospection")
     (synopsis "Generate interface introspection data for GObject libraries")
     (description
      "GObject introspection is a middleware layer between C libraries (using
@@ -476,7 +477,7 @@ GObject) and language bindings.  The C library can be scanned at compile time
 and generate a metadata file, in addition to the actual native C library.  Then
 at runtime, language bindings can read this metadata and automatically provide
 bindings to call into the C library.")
-    ; Some bits are distributed under the LGPL2+, others under the GPL2+
+    (home-page "https://wiki.gnome.org/GObjectIntrospection")
     (license license:gpl2+)))
 
 (define intltool
-- 
2.29.2

Re: Questionable "cosmetic changes" commits

[Ryan Prior]

[-- Attachment #1: Type: text/plain, Size: 805 bytes --]

Hi Mark!

On December 2, 2020, Mark H Weaver <mhw@netris.org> wrote:
> We all have our own personal preferences of how best to indent scheme
> code, but if more of us adopted the habit of needlessly reordering
> fields and reindenting code of every package we touch, as one of us
> seems to have done, it could get out of hand quickly.

I don't particularly hold any opinion about stylistic commits except
that I prefer tools like gofmt, Python's Black and standard.js which
enforce uniform code style, and would use such a tool for my Guile code
if it exists.

I do think it's important to acknowledge that the commits written by
Raghav were part of his internship and advised by his mentors who signed
off on the commits, so it's not like these changes were unsolicited and
materialized out of nowhere.

[-- Attachment #2: Type: text/html, Size: 3129 bytes --]

Re: Questionable "cosmetic changes" commits

[Tobias Geerinckx-Rice]

[-- Attachment #1: Type: text/plain, Size: 445 bytes --]

Ryan Prior 写道：
>
> I don't particularly hold any opinion about stylistic commits 
> except
> that I prefer tools like gofmt, Python's Black and standard.js 
> which
> enforce uniform code style, and would use such a tool for my 
> Guile code
> if it exists.

Guix already has a uniform code formatter -- GNU Emacs :-)  (After 
applying .dir-locals.el, and possibly invoked with 
etc/indent-code.el.)

Kind regards,

T G-R

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 247 bytes --]

Re: Questionable "cosmetic changes" commits

[Hartmut Goebel]

Hi Martin,

Am 02.12.20 um 19:55 schrieb Mark H Weaver:
> I think that commits like this are best avoided for several reasons.
[…]
> Should I change those things back the next time I update that package?

My main project (PyInstaller) has the policy to not accept any
white-space changes and reformatting, except this is done due to
functional change. I'm very picky about this, since digging through a
pile of cosmetic changes to find the cause of a functional change is
cumbersome.

For guix packages, finding the cause of a functional change is less of
an issue. Anyhow, each change contributes to the load of data to be
transferred. This not only is an issue for people with low bandwidths,
but also contributes to climate change. The later is especially true,
since "guix pull" pulls all changes on all machines using guix - so each
single change will be downloaded many thousand time for next 10 years.

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

Re: Questionable "cosmetic changes" commits

[Mark H Weaver]

Hi Ryan,

Ryan Prior <ryanprior@hey.com> writes:
> I do think it's important to acknowledge that the commits written by
> Raghav were part of his internship and advised by his mentors who signed
> off on the commits, so it's not like these changes were unsolicited and
> materialized out of nowhere.

If those cosmetic changes were solicited by his mentors, then please
consider my comments to be directed at them.  Incidentally, my comments
were not meant to shame anyone.  I very much appreciate Raghav's
contributions to Guix.  I'm merely pointing out a practice that I think
is suboptimal and should be discouraged.

     Thanks,
       Mark

Re: Questionable "cosmetic changes" commits

[Bengt Richter]

Hi Ryan, Mark, et al,

On +2020-12-02 20:13:56 +0000, Ryan Prior wrote:
> Hi Mark!
> 
> On December 2, 2020, Mark H Weaver <mhw@netris.org> wrote:
> > We all have our own personal preferences of how best to indent scheme
> > code, but if more of us adopted the habit of needlessly reordering
> > fields and reindenting code of every package we touch, as one of us
> > seems to have done, it could get out of hand quickly.
> 
> I don't particularly hold any opinion about stylistic commits except
> that I prefer tools like gofmt, Python's Black and standard.js which
> enforce uniform code style, and would use such a tool for my Guile code
> if it exists.

Agree.
As Tobias points out, it exists inside emacs. But I like small independent
tools for specific things. Especially if they compose well.

> 
> I do think it's important to acknowledge that the commits written by
> Raghav were part of his internship and advised by his mentors who signed
> off on the commits, so it's not like these changes were unsolicited and
> materialized out of nowhere.

I find myself with schizophrenic sympathies here :)

On the one hand:
    1. Don't make unnecessary work for me.
    2. If it ain't broke, don't fix it.
    3. Mother appreciates help in the kitchen, but
       don't touch the fine crystal! (Mother: "That's precious,
       from your great grandmother, only for special occasions.
       You know what cut glass crystal like that is worth?"
       Smarty Kid: "Buying or selling?" :)
    4: At work, sign for janitorial services: DO NOT MOVE
       ANYTHING ON MY DESK!! (arrangement of untidy mess encodes
       part of my workflow state).
On the other hand:
    1. I habitually use emacs scheme-mode indenting as well as shell-script-mode,
       and find it a useful lens to reveal the meaning of the code, and my errors.
    2. I prefer to read pretty-printed code, and wouldn't mind if all
       submitted code automatically were canonicalized in a well-defined way.
       I use emacs indenting because it is right there when I am editing.
    3. NOT to canonicalize can obscure dumb errors, and that can be a smokescreen for worse.
    4. For code, it is the abstract syntax tree that matters (in telling the machine
       what to do), not cosmetics, but cosmetics matter in making info human-sensible.
In conclusion:
    1. I want the best of all worlds, so I always like to pick a la carte,
       unless the Chef is three-flowers-plus, or I am budget-constrained.
    2. I lean towards wanting a standard pretty-print canonicalization, if it doesn't make work for me ;)
    3. diff has options to ignore white-space differences, etc., so I wonder what tools need what options
       to ease Mark's pain, (until everything is canonicalized, when that pain should disappear anyway).
    4. Canonicalization should actually help make reviewing easier, and more effective, IWT.
    5. I would like better factoring of functionality, so that e.g. I wouldn't have to start emacs
       (I know, some never leave :) to canonicalize a file the way scheme-mode does. Unix philosophy?
       (E.g., don't give cat a new built-in option like -nA, but realize how easily it could compose with a factored-out
       scheme-source canonicalizer. Some of you could probably write a bash script to use emacs as a filter,
       but is that a sensible way to go? Probably, if you are a fluent hacker and you need a tool in a hurry,
       but not for deciding separate and separable distribution components.
       (I think I got my nostalgic ideas of components from Meccano kit from the '40s ;)
tl;DR:
   1. I vote for running all code to be submitted through a standard pretty-printing canonicalizer.
      It could even inject TODO stubs for missing synopses, and doc strings (as an option :)
-- 
Regards,
Bengt Richter

Re: Questionable "cosmetic changes" commits

[Raghav Gururajan]

Hello Mark and Others!

Thank you for your concern.

I can tell you that those cosmetic changes I made were 100% irrational, useless and noisy.

I have clinical OCD [1] and ADHD [2], for which I regularly take Fluoxetine and Methylphenidate to keep things under control. Due to this, if the packages I am editing is not it in a specific way, I am unable to focus properly. That too, if I am working on multiple package definitions and if each pack-defs are arranged in different way, it is very very hard for me. I can tell you that these cosmetic changes are merely the product of my mental-illness and not of my mental-faculties.

Moving forward, I will try hard not to do this. But can I ask you all to allow these cosmetic commits for my existing projects (i.e. commits from wip-desktop and pidgin patch-set) please?

I understand that these commits were a burden to everyone and my sincere apologies for that.

[1] https://www.nimh.nih.gov/health/topics/obsessive-compulsive-disorder-ocd/index.shtml
[2] https://www.nimh.nih.gov/health/topics/attention-deficit-hyperactivity-disorder-adhd/index.shtml

Regards,
RG.

Re: Questionable "cosmetic changes" commits

[Ryan Prior]

[-- Attachment #1: Type: text/plain, Size: 3114 bytes --]

On December 4, 2020, Raghav Gururajan <raghavgururajan@disroot.org>
wrote:
> I can tell you that those cosmetic changes I made were 100%
> irrational, useless and noisy.

That's certainly a way to frame it, but I'd like to hold some space for
the idea that the things we neuroatypical people do to manage and
satisfy our own unusual perspectives are not irrational or useless if
they make sense to us and serve a purpose for us.

> Due to [clinical conditions], if the packages I am editing is not it
> in a specific way, I am unable to focus properly. That too, if I am
> working on multiple package definitions and if each pack-defs are
> arranged in different way, it is very very hard for me.

That is an interesting use-case I hadn't considered, but a valid one,
and it gives me ideas. I'm going to experiment with some tools to make
this feasible.

Consider the tooling for Unison[1] which reduces code churn in a number
of unique ways.[2]  It can store programs as abstract syntax trees
rather than plain text files, and it's content-addressed, referring to
functions by their hashes rather than their fixed names. As a programmer
that gives you the freedom to choose names and language syntax that suit
you without imposing on others to follow suit.

Due to the functional paradigm and technical structure of Guix, I think
we can build some of the same capabilities in our tooling. Like Unison
functions, our package definitions are reduced to a declarative content-
addressed form, from which a textual definition could be generated again
using a reverse process. By such a process, you & I could edit package
definitions in whatever textual form suits us individually without
having to agree on anything, not even the names of symbols. Then we can
use a tool to automatically canonicalize our code into the form most
widely acceptable to the community when it's time to submit a patch.

Taking this idea to its logical conclusion & building on Guile's multi-
language-syntax paradigm, I can picture using a futuristic tool to edit
package definitions in Emacs lisp or JavaScript, again without requiring
that anybody upstream adopt those languages or even recognize that I'm
using them.

I'll see what I can hack together for a naïve implementation of this
concept and present it for review.

> Moving forward, I will try hard not to do this. But can I ask you all
> to allow these cosmetic commits for my existing projects (i.e. commits
> from wip-desktop and pidgin patch-set) please?

It doesn't bug me, but I know it does bug other people and imagine we
want to avoid creating unnecessary review work for people who follow the
commit stream.

> I understand that these commits were a burden to everyone and my
> sincere apologies for that.

I appreciate the grace and consideration you brought to your response &
all your contributions to Guix!


[1] https://www.unisonweb.org
[2] https://www.unisonweb.org/2020/04/10/reducing-churn/#definitions-
getting-renamed-or-moved

[-- Attachment #2: Type: text/html, Size: 5465 bytes --]

Re: Questionable "cosmetic changes" commits

[Raghav Gururajan]

Hi Ryan!

>> I can tell you that those cosmetic changes I made were 100% irrational, useless and noisy.
> 
> That's certainly a way to frame it, but I'd like to hold some space for the idea that the things we
> neuroatypical people do to manage and satisfy our own unusual perspectives are not irrational or
> useless if they make sense to us and serve a purpose for us.

+1

I meant to say that, the actions were irrational by itself. But when contrasted with right reasons, we can see why sometimes an irrational act can be a rational act, when it benefits the agent's overall outcome.

>> Due to [clinical conditions], if the packages I am editing is not it in a specific way, I am unable
>> to focus properly. That too, if I am working on multiple package definitions and if each pack-defs
>> are arranged in different way, it is very very hard for me.
> 
> That is an interesting use-case I hadn't considered, but a valid one, and it gives me ideas. I'm
> going to experiment with some tools to make this feasible.

Thanks Ryan!

Yeah, my brain laterally connects fields of different package definitions. Like a spread-sheet, where each columns are different package definitions and each row is a fields of a package's definition.

For example, if column 1 is glib and column 2 is gtk+, my brain spots pattern or errors when [arguments] field of both the packages are in the same row. Let's say [arguments] field of glib pack-def (column) is in 4th place (row); and; if the 4th place (row) of gtk+ pack-def (column) is [propagated-inputs]; my brain goes haywire. So I first do the cosmetic change of rearranging the fields of gtk+ pack-def to match with pack-def of gtk+. This is just one example.

> Consider the tooling for Unison[1] which reduces code churn in a number of unique ways.[2] It can
> store programs as abstract syntax trees rather than plain text files, and it's content-addressed,
> referring to functions by their hashes rather than their fixed names. As a programmer that gives
> you the freedom to choose names and language syntax that suit you without imposing on others to
> follow suit.

That's very interesting. I'll have a look.

> Due to the functional paradigm and technical structure of Guix, I think we can build some of the
> same capabilities in our tooling. Like Unison functions, our package definitions are reduced to a
> declarative content-addressed form, from which a textual definition could be generated again using
> a reverse process. By such a process, you & I could edit package definitions in whatever textual
> form suits us individually without having to agree on anything, not even the names of symbols. Then
> we can use a tool to automatically canonicalize our code into the form most widely acceptable to
> the community when it's time to submit a patch.

+1

> Taking this idea to its logical conclusion & building on Guile's multi-language-syntax paradigm, I
> can picture using a futuristic tool to edit package definitions in Emacs lisp or JavaScript, again
> without requiring that anybody upstream adopt those languages or even recognize that I'm using
> them.
> 
> I'll see what I can hack together for a naïve implementation of this concept and present it for
> review.

Thank you Ryan!

>> Moving forward, I will try hard not to do this. But can I ask you all to allow these cosmetic
>> commits for my existing projects (i.e. commits from wip-desktop and pidgin patch-set) please?
> 
> It doesn't bug me, but I know it does bug other people and imagine we want to avoid creating
> unnecessary review work for people who follow the commit stream.

I see.

>> I understand that these commits were a burden to everyone and my sincere apologies for that.
> 
> I appreciate the grace and consideration you brought to your response & all your contributions to
> Guix!

:-)

> [1] https://www.unisonweb.org
> [2] https://www.unisonweb.org/2020/04/10/reducing-churn/#definitions-getting-renamed-or-moved

Regards,
RG.

Re: Questionable "cosmetic changes" commits

[Danny Milosavljevic]

[-- Attachment #1: Type: text/plain, Size: 3114 bytes --]

Hi Raghav,

first, let me say that as far as I'm concerned, you did nothing wrong--although
it caused a lot of work for you to do the rearranging in the first place
(and also some work for us).

Guix irregularities also annoy the hell out of me.  You can check out some
earlier patches by me (when I joined the project) in the archives.  I wanted
(and still want!) to rearrange a LOT of stuff--because, let's face it, Guix's
module layout in general is a mess.  But for better or for worse, it's
not easy to do anything about it and also keep backward compatibility with
what external projects expect of Guix and also keep using the tools that we
have been using so far (git, diff, patch etc).  So even though I still would
prefer Guix module layout not to suck, it cannot be changed in the short term.
So I know where you are coming from.

But in general, arguing about *formatting* is bike-shedding at its best,
especially in a language where formatting does not matter.

However, we do use textual diff, blame and merge tools--and those do not
understand the tree structure of a Lisp program at all.  Rearranging stuff
especially will (and did) cause diff and patch to mistake the insertion
point for changes.  (It will patch it wrong if applied out of order or when
skipping patches--and will often NOT fail)

So for example it's very difficult to leave off the cosmetic patch and just
apply the non-cosmetic patches that came later.

Furthermore, understand that the package fields (and guix record fields in
general) can refer to previously defined fields--so a package with reordered
fields is NOT necessarily semantically equivalent to the original one.

Try these in guix repl:

(let ((name "OUTER"))
  (package-version
   (package
     (name "INNER")
     (version name)
     (source #f)
     (build-system #f)
     (synopsis #f)
     (description #f)
     (license #f)
     (home-page #f))))

(that gives "INNER")

vs

(let ((name "OUTER"))
  (package-version
   (package
     (version name)
     (name "name")
     (source #f)
     (build-system #f)
     (synopsis #f)
     (description #f)
     (license #f)
     (home-page #f))))

(that gives "OUTER")

That means each cosmetic patch of you required some extra manual review effort
by me in order to make sure that this does not introduce semantic changes.

That said, if people post reformatting patches (especially if part of a patchset
that was presumably already tested by that person) I usually do not say anything
about it because I don't want to cause extra work that is useful to nobody (and
potentially invalidate all the testing done).

FWIW, I do find it strange that Lisp projects, despite using a minimal-syntax
language (mostly in order to conserve its regular tree structure), do not
usually automatically format source code as they check in, but Go projects,
using the prime example of an irregular C-like language, DOES usually use
code formatters automatically when checking in.  That is some strange
reversal of strengths that I wouldn't have expected.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Questionable "cosmetic changes" commits

[Mark H Weaver]

Hi Raghav,

"Raghav Gururajan" <raghavgururajan@disroot.org> writes:

> Yeah, my brain laterally connects fields of different package
> definitions. Like a spread-sheet, where each columns are different
> package definitions and each row is a fields of a package's
> definition.
>
> For example, if column 1 is glib and column 2 is gtk+, my brain spots
> pattern or errors when [arguments] field of both the packages are in
> the same row. Let's say [arguments] field of glib pack-def (column) is
> in 4th place (row); and; if the 4th place (row) of gtk+ pack-def
> (column) is [propagated-inputs]; my brain goes haywire. So I first do
> the cosmetic change of rearranging the fields of gtk+ pack-def to
> match with pack-def of gtk+. This is just one example.

If your goal is to make the ordering of package fields more consistent
across Guix -- which is something that I could support -- I can report
that your commits are making that problem worse, not better.

One of the common features of your "cosmetic changes" commits is that
they all move the 'home-page' field from its conventional place above
the 'synopsis' to below the 'description', if it wasn't there already.

I just hacked up a little script to determine which ordering is more
common.  For simplicity, it only considers top-level declarations of the
form (define-public <pkg-name> (package ...)).  Out of 11446 packages of
that form in gnu/packages/*.scm, 88% of them (10078) have the
'home-page' field above the 'description' field.

So, if consistency of ordering is your goal, you're going in the wrong
direction.

* * *

Meanwhile, you've only provided a rationale for 1 out of 3 of the kinds
of changes made in these commits.

Do you have an explanation for why you are removing comments in your
"cosmetic changes" commits?  For example, the following two commits
remove comments that explain why 'propagated-inputs' are needed:

https://git.sv.gnu.org/cgit/guix.git/commit/?id=c3264f9e100ad6aefe5216002b68f3bfdcf6be95
https://git.sv.gnu.org/cgit/guix.git/commit/?id=416b1b9f56b514677660b56992cea1c78e00f519

What's your rationale for doing this?  Am I the only one here who finds
this practice objectionable?  It's not even mentioned in the commit logs.

       Mark

Re: Questionable "cosmetic changes" commits

[Mark H Weaver]

I wrote:
> I just hacked up a little script to determine which ordering is more
> common.  For simplicity, it only considers top-level declarations of the
> form (define-public <pkg-name> (package ...)).

To be more precise, it only considers packages of that form where the
'...' contains both 'home-page' and 'description' fields, so it excludes
most packages that inherit from another package.

      Mark

Re: Questionable "cosmetic changes" commits

[Raghav Gururajan]

Hi Mark!

> Meanwhile, you've only provided a rationale for 1 out of 3 of the kinds
> of changes made in these commits.
> 
> Do you have an explanation for why you are removing comments in your
> "cosmetic changes" commits? For example, the following two commits
> remove comments that explain why 'propagated-inputs' are needed:
> 
> https://git.sv.gnu.org/cgit/guix.git/commit/?id=c3264f9e100ad6aefe5216002b68f3bfdcf6be95
> https://git.sv.gnu.org/cgit/guix.git/commit/?id=416b1b9f56b514677660b56992cea1c78e00f519
> 
> What's your rationale for doing this? Am I the only one here who finds
> this practice objectionable? It's not even mentioned in the commit logs.

I think the comments are useful for non-trivial cases. In these definitions, the inputs were propagated because they were mentioned in .pc files. Propagation because of pkg-config is trivial. So I removed the comments.

Regards,
RG.

Re: Questionable "cosmetic changes" commits

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 1862 bytes --]


Raghav Gururajan <raghavgururajan@disroot.org> writes:

> Hi Mark!
>
>> Meanwhile, you've only provided a rationale for 1 out of 3 of the kinds
>> of changes made in these commits.
>> 
>> Do you have an explanation for why you are removing comments in your
>> "cosmetic changes" commits? For example, the following two commits
>> remove comments that explain why 'propagated-inputs' are needed:
>> 
>> https://git.sv.gnu.org/cgit/guix.git/commit/?id=c3264f9e100ad6aefe5216002b68f3bfdcf6be95
>> https://git.sv.gnu.org/cgit/guix.git/commit/?id=416b1b9f56b514677660b56992cea1c78e00f519
>> 
>> What's your rationale for doing this? Am I the only one here who finds
>> this practice objectionable? It's not even mentioned in the commit logs.
>
> I think the comments are useful for non-trivial cases. In these
> definitions, the inputs were propagated because they were mentioned in
> .pc files. Propagation because of pkg-config is trivial. So I removed
> the comments.

In the context of writing Guix packages, propagating the necessary
inputs to support other packages finding the library via pkg-config is a
serious thing, not trivial. If it breaks, dependent packages will likely
change in behaviour or stop building entirely.

As for the comments, personally, I think the reasons behind propagated
inputs are individual enough and important enough to each package that
it's useful to write them down, even if that comment is "these things
are referenced by the .pc file". That way others looking at the package
definition don't have to wonder or try and dig through the Git history
to find information about what's going on.

Anyway, I think the most useful output from this discussion is amending
or adding to the packaging guilelines to cover this:

  https://guix.gnu.org/manual/en/html_node/Packaging-Guidelines.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 987 bytes --]

Cosmetic changes commits as a potential security risk (was Re: Questionable "cosmetic changes" commits)

[Mark H Weaver]

Hi Raghav,

I asked:
>> Do you have an explanation for why you are removing comments in your
>> "cosmetic changes" commits?

"Raghav Gururajan" <raghavgururajan@disroot.org> replied:
> I think the comments are useful for non-trivial cases.  In these
> definitions, the inputs were propagated because they were mentioned in
> .pc files.  Propagation because of pkg-config is trivial.  So I
> removed the comments.

Thanks for the explanation.

Please keep in mind that every comment in Guix was deliberately put
there by a Guix developer, which means that at least one developer
thought the comment was worth including.

I'm concerned that you felt so confident in your assessment that these
comments were superfluous that you felt justified in removing them
without telling anyone, let alone asking your mentors if they agreed.

My larger concern is that these removals were effectively hidden within
a commit that ostensibly only rearranged and reindented code.

* * *

It occurs to me that commits that rearrange or reindent code are a
potential security risk, because they obscure other changes made within
the same commit.  Even developers who try to keep an eye on changes
being made to Guix tend to simply *assume* that commits like these are
what they claim to be, because it's too tedious to verify them.

If we allow unannounced changes to be obscured within "cosmetic changes"
commits without reprimand, we invite the future possibility of
deliberate corruption of our code base via such commits, by attackers
who have compromised our developers' machines or signing keys.

* * *

Having said all of this, I should also say that I truly appreciate your
contributions, Raghav, and I'm glad that you are here.

      Regards,
        Mark

Re: Questionable "cosmetic changes" commits

[Bengt Richter]

Hi Christopher and Raghav,

On +2020-12-05 21:54:36 +0000, Christopher Baines wrote:
> 
> Raghav Gururajan <raghavgururajan@disroot.org> writes:
> 
> > Hi Mark!
> >
> >> Meanwhile, you've only provided a rationale for 1 out of 3 of the kinds
> >> of changes made in these commits.
> >> 
> >> Do you have an explanation for why you are removing comments in your
> >> "cosmetic changes" commits? For example, the following two commits
> >> remove comments that explain why 'propagated-inputs' are needed:
> >> 
> >> https://git.sv.gnu.org/cgit/guix.git/commit/?id=c3264f9e100ad6aefe5216002b68f3bfdcf6be95
> >> https://git.sv.gnu.org/cgit/guix.git/commit/?id=416b1b9f56b514677660b56992cea1c78e00f519
> >> 
> >> What's your rationale for doing this? Am I the only one here who finds
> >> this practice objectionable? It's not even mentioned in the commit logs.
> >
> > I think the comments are useful for non-trivial cases. In these
> > definitions, the inputs were propagated because they were mentioned in
> > .pc files. Propagation because of pkg-config is trivial. So I removed
> > the comments.
>
┌──────────────────────────────┐
│ "So I removed the comments." │
└──────────────────────────────┘
Raghav, I think you may not grok the social signalling of a statement like that :)

It sounds like you are overlooking the _social_ need for consensus
in modifying a shared environment.

Taking a picture off the wall of a shared living room is different
from taking the same picture off the wall in your private room.

A git commit in a jointly developed FLOSS project is modifying a shared living room.
(But do what you like in your own git repo ;-)

The social aspect is not about the technical merits of of your changes,
it's about the difference between joint ownership and private ownership,
and the differences in exercising owner rights.

> In the context of writing Guix packages, propagating the necessary
> inputs to support other packages finding the library via pkg-config is a
> serious thing, not trivial. If it breaks, dependent packages will likely
> change in behaviour or stop building entirely.
> 
> As for the comments, personally, I think the reasons behind propagated
> inputs are individual enough and important enough to each package that
> it's useful to write them down, even if that comment is "these things
> are referenced by the .pc file". That way others looking at the package
> definition don't have to wonder or try and dig through the Git history
> to find information about what's going on.
> 
> Anyway, I think the most useful output from this discussion is amending
> or adding to the packaging guilelines to cover this:
> 
>   https://guix.gnu.org/manual/en/html_node/Packaging-Guidelines.html

-- 
Regards,
Bengt Richter

Re: Questionable "cosmetic changes" commits

[Raghav Gururajan via Development of GNU Guix and the GNU System distribution.]

Hi Chris!

> In the context of writing Guix packages, propagating the necessary
> inputs to support other packages finding the library via pkg-config is a
> serious thing, not trivial. If it breaks, dependent packages will likely
> change in behaviour or stop building entirely.

I understand. I didn't mean trivial as in importance but as in reason. Like most common reason why one propagates an input. :-)
 
> As for the comments, personally, I think the reasons behind propagated
> inputs are individual enough and important enough to each package that
> it's useful to write them down, even if that comment is "these things
> are referenced by the .pc file". That way others looking at the package
> definition don't have to wonder or try and dig through the Git history
> to find information about what's going on.

Agreed.

> Anyway, I think the most useful output from this discussion is amending
> or adding to the packaging guilelines to cover this:
> 
> https://guix.gnu.org/manual/en/html_node/Packaging-Guidelines.html

Yes, will be useful.

Regards,
RG.

Re: Cosmetic changes commits as a potential security risk (was Re: Questionable "cosmetic changes" commits)

[Raghav Gururajan via Development of GNU Guix and the GNU System distribution.]

Hi Mark!

> Thanks for the explanation.
> 
> Please keep in mind that every comment in Guix was deliberately put
> there by a Guix developer, which means that at least one developer
> thought the comment was worth including.
> 
> I'm concerned that you felt so confident in your assessment that these
> comments were superfluous that you felt justified in removing them
> without telling anyone, let alone asking your mentors if they agreed.
> 
> My larger concern is that these removals were effectively hidden within
> a commit that ostensibly only rearranged and reindented code.

My apologies, I should have mentioned in the commit message. Anyway, I will be deferring from removing any existing comments. 

> It occurs to me that commits that rearrange or reindent code are a
> potential security risk, because they obscure other changes made within
> the same commit. Even developers who try to keep an eye on changes
> being made to Guix tend to simply *assume* that commits like these are
> what they claim to be, because it's too tedious to verify them.
> 
> If we allow unannounced changes to be obscured within "cosmetic changes"
> commits without reprimand, we invite the future possibility of
> deliberate corruption of our code base via such commits, by attackers
> who have compromised our developers' machines or signing keys.

I see. I haven't thought about this, but will consider it.

Thanks!

Regards,
RG.

Re: Questionable "cosmetic changes" commits

[Raghav Gururajan via Development of GNU Guix and the GNU System distribution.]

Hi Bengt!

> ┌──────────────────────────────┐
> │ "So I removed the comments." │
> └──────────────────────────────┘
> Raghav, I think you may not grok the social signalling of a statement like that :)

My apologies! I didn't mean that with a negative connotation.

> It sounds like you are overlooking the _social_ need for consensus
> in modifying a shared environment.
> 
> Taking a picture off the wall of a shared living room is different
> from taking the same picture off the wall in your private room.
> 
> A git commit in a jointly developed FLOSS project is modifying a shared living room.
> (But do what you like in your own git repo ;-)
> 
> The social aspect is not about the technical merits of of your changes,
> it's about the difference between joint ownership and private ownership,
> and the differences in exercising owner rights.

I understand. When the patch was in the mail-list for a while, I assumed folks were okay with the changes. I am glad that Mark shared the concerns. This gave me an opportunity to be in consensus with others. :-)

I will be changing my work-flow to prevent this issue from happening again.

Regards,
RG.