Okay to commit purely cosmetic (indendation) fixes?

Okay to commit purely cosmetic (indendation) fixes?

[Karl Fogel]

Are we okay with purely cosmetic commits to fix source-level formatting issues?

Specifically, in lisp/gnus/gnus-start.el, in `gnus-get-unread-articles', there is a multi-line `funcall' that is improperly indented.  Here's the fix I'd like to commit:

  --- lisp/gnus/gnus-start.el
  +++ lisp/gnus/gnus-start.el
  @@ -1637,10 +1637,10 @@ gnus-get-unread-articles
   	      type-cache))
         ;; Only add groups that need updating.
         (if (or (and foreign-level (null (numberp foreign-level)))
  -	   (funcall (if one-level #'= #'<=) (gnus-info-level info)
  -		    (if (eq (cadr method-group-list) 'foreign)
  -			foreign-level
  -		      alevel)))
  +	      (funcall (if one-level #'= #'<=) (gnus-info-level info)
  +		       (if (eq (cadr method-group-list) 'foreign)
  +			   foreign-level
  +		         alevel)))
   	  (setcar (nthcdr 2 method-group-list)
   		  (cons info (nth 2 method-group-list)))
   	;; The group is inactive, so we nix out the number of unread articles.

The misindentation appears to have been introduced in commit 1f5eeb7be4ac, "Honor docstring of gnus-group-get-new-news" on 4 Feb 2016.  I ran across it while debugging a non-cosmetic problem related to the persistently undocumented `dont-connect' parameter, the details of which I won't go into here, but it led to the `info' variable in this function unexpectedly remaining `nil' since being first bound to `nil' in the surrounding `let*', and then `gnus-info-level' raising an error because it's being passed `nil' instead of a number... Anyway, I may or may not ever get to the bottom of that problem, since I found a workaround for my use case, but in the meantime I could at least fix the mis-indentation I found along the way.

Thoughts on cosmetic fixes in general and in this specific case?

Best regards,
-Karl

Re: Okay to commit purely cosmetic (indendation) fixes?

[Lars Ingebrigtsen]

Karl Fogel <kfogel@red-bean.com> writes:

> Are we okay with purely cosmetic commits to fix source-level formatting issues?

[...]

> Thoughts on cosmetic fixes in general and in this specific case?

In general, doing full-scale white-space fixups should be avoided, but
in this case I think it's fine -- it's limited to one "then" clause and
makes it more readable.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Okay to commit purely cosmetic (indendation) fixes?

[Clive Tovero]

Karl Foger: My comment would be that your subject line was enough 
detail.  I personally wouldn't commit a tiny, syntactically 
insignificant, whitespace-only change.  To each his own, but...

Lars Ingebrigtsen: I don't understand why you wouldn't bundle whitespace 
changes, rather than one at a time, as long as they were to correct 
deviations from a coding standard.  By the way, could you point us to 
the coding standard for Emacs that would would give us guidance?

Re: Okay to commit purely cosmetic (indendation) fixes?

[Lars Ingebrigtsen]

Clive Tovero <clive.tovero@aol.com> writes:

> Lars Ingebrigtsen: I don't understand why you wouldn't bundle
> whitespace changes, rather than one at a time, as long as they were to
> correct deviations from a coding standard.

Whitespace changes make following code changes more difficult when
digging into the history.

> By the way, could you point us to the coding standard for Emacs that
> would would give us guidance?

Nope.  Perhaps somebody else can?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Okay to commit purely cosmetic (indendation) fixes?

[Stefan Monnier]

>> Lars Ingebrigtsen: I don't understand why you wouldn't bundle
>> whitespace changes, rather than one at a time, as long as they were to
>> correct deviations from a coding standard.
> Whitespace changes make following code changes more difficult when
> digging into the history.

They're also a source of painful spurious conflicts for people
managing branches.  So we prefer that whitespace changes are only
installed along with real changes that touch that same code, although we
definitely tolerate a few extra whitespace touch ups here and there.


        Stefan

Re: Okay to commit purely cosmetic (indendation) fixes?

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Mon, 10 Aug 2020 12:53:26 +0200
> Cc: emacs-devel@gnu.org
> 
> Clive Tovero <clive.tovero@aol.com> writes:
> 
> > Lars Ingebrigtsen: I don't understand why you wouldn't bundle
> > whitespace changes, rather than one at a time, as long as they were to
> > correct deviations from a coding standard.
> 
> Whitespace changes make following code changes more difficult when
> digging into the history.

Yes.  They make whole lines look like due to someone who only changed
the whitespace.

> > By the way, could you point us to the coding standard for Emacs that
> > would would give us guidance?
> 
> Nope.  Perhaps somebody else can?

I might, but I don't think I understand what coding standards are
being sought here, and in which programming language.

Re: Okay to commit purely cosmetic (indendation) fixes?

[Clive Tovero]

>> Whitespace changes make following code changes more difficult when
>> digging into the history.
> 
> Yes.  They make whole lines look like due to someone who only changed
> the whitespace.

Thank you, I had heard the recommendation to not make whitespace-only 
commits, I didn't understand why.  I guess I would have to know what 
tools you are using and what task you are doing to completely 
understand--many diff tools are capable of ignoring whitespace changes 
and highlighting whitespace.  If you are looking at raw diff or 
something, I guess that would be a problem.

> 
>>> By the way, could you point us to the coding standard for Emacs that
>>> would would give us guidance?
>>
>> Nope.  Perhaps somebody else can?
> 
> I might, but I don't think I understand what coding standards are
> being sought here, and in which programming language.
> 

My use of "coding standard" was a poor choice of words, I'm sorry for 
that.  I meant documentation of your software development process, 
including issues like this, a "Commiter's Guide" e.g.  Then you could 
tell them please RTFM, and not lead to discussions like this.

Re: Okay to commit purely cosmetic (indendation) fixes?

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Clive Tovero <clive.tovero@aol.com>
> Date: Mon, 10 Aug 2020 11:39:10 -0400
> 
> > Yes.  They make whole lines look like due to someone who only changed
> > the whitespace.
> 
> Thank you, I had heard the recommendation to not make whitespace-only 
> commits, I didn't understand why.  I guess I would have to know what 
> tools you are using and what task you are doing to completely 
> understand--many diff tools are capable of ignoring whitespace changes 
> and highlighting whitespace.  If you are looking at raw diff or 
> something, I guess that would be a problem.

I had "git blame" in mind, and its Emacs equivalent "C-x v L".

> My use of "coding standard" was a poor choice of words, I'm sorry for 
> that.  I meant documentation of your software development process, 
> including issues like this, a "Commiter's Guide" e.g.  Then you could 
> tell them please RTFM, and not lead to discussions like this.

In that case, please start with CONTRIBUTE in the top-level directory
of the Emacs source tree.

Re: Okay to commit purely cosmetic (indendation) fixes?

[Clive Tovero]

 > They're also a source of painful spurious conflicts for people
managing branches.
Thank you for the clarification, that makes sense.

Re: Okay to commit purely cosmetic (indendation) fixes?

[Karl Fogel]

On 10 Aug 2020, Stefan Monnier wrote:
>They're also a source of painful spurious conflicts for people
>managing branches.  So we prefer that whitespace changes are only
>installed along with real changes that touch that same code, although we
>definitely tolerate a few extra whitespace touch ups here and there.

Thanks.  In that case, I'll leave this one alone, until (if it ever happens) I'm making a substantive change that touches the same spot.

Best regards,
-Karl

Re: Okay to commit purely cosmetic (indendation) fixes?

[Clive Tovero]

> In that case, please start with CONTRIBUTE in the top-level directory
> of the Emacs source tree.
> 

I actually started yesterday higher-up, with "GNU *Coding Standards*" 
here: https://www.gnu.org/prep/standards/standards.html  It does address 
some of the software dev standards and practices of GNU in general.

The ./CONTRIBUTE file pointed to ./admin/notes, so I thought this file 
might be patched with the white space wisdom (including the underlying 
rationale, e.g. git blame issues):

./admin/notes/git-workflow

Git blame does have the -w option to ignore whitespace, but I haven't 
had to use git blame.  In general, I use a graphical tool called 
Meld--because the complexity of diffing overwhelms the symbolic part of 
my brain and I have to use more of the visual part.

I realized too, that this whole topic makes a good case for, in any 
project, picking an indentation style, maybe enforcing it in commits, 
and sticking with it.