all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* Okay to commit purely cosmetic (indendation) fixes?
@ 2020-08-09 20:49 Karl Fogel
  2020-08-09 20:52 ` Lars Ingebrigtsen
  0 siblings, 1 reply; 11+ messages in thread
From: Karl Fogel @ 2020-08-09 20:49 UTC (permalink / raw)
  To: Emacs developers

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



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Okay to commit purely cosmetic (indendation) fixes?
  2020-08-09 20:49 Karl Fogel
@ 2020-08-09 20:52 ` Lars Ingebrigtsen
  0 siblings, 0 replies; 11+ messages in thread
From: Lars Ingebrigtsen @ 2020-08-09 20:52 UTC (permalink / raw)
  To: Karl Fogel; +Cc: Emacs developers

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



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Okay to commit purely cosmetic (indendation) fixes?
       [not found] <94dd40a5-0195-b88b-3cc1-298d32f99ad8.ref@aol.com>
@ 2020-08-09 22:40 ` Clive Tovero
  2020-08-10 10:53   ` Lars Ingebrigtsen
  0 siblings, 1 reply; 11+ messages in thread
From: Clive Tovero @ 2020-08-09 22:40 UTC (permalink / raw)
  To: emacs-devel

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?




^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Okay to commit purely cosmetic (indendation) fixes?
  2020-08-09 22:40 ` Okay to commit purely cosmetic (indendation) fixes? Clive Tovero
@ 2020-08-10 10:53   ` Lars Ingebrigtsen
  2020-08-10 12:23     ` Stefan Monnier
  2020-08-10 14:04     ` Eli Zaretskii
  0 siblings, 2 replies; 11+ messages in thread
From: Lars Ingebrigtsen @ 2020-08-10 10:53 UTC (permalink / raw)
  To: Clive Tovero; +Cc: emacs-devel

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



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Okay to commit purely cosmetic (indendation) fixes?
  2020-08-10 10:53   ` Lars Ingebrigtsen
@ 2020-08-10 12:23     ` Stefan Monnier
  2020-08-10 16:20       ` Karl Fogel
  2020-08-10 14:04     ` Eli Zaretskii
  1 sibling, 1 reply; 11+ messages in thread
From: Stefan Monnier @ 2020-08-10 12:23 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: Clive Tovero, emacs-devel

>> 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




^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Okay to commit purely cosmetic (indendation) fixes?
  2020-08-10 10:53   ` Lars Ingebrigtsen
  2020-08-10 12:23     ` Stefan Monnier
@ 2020-08-10 14:04     ` Eli Zaretskii
  2020-08-10 15:39       ` Clive Tovero
  1 sibling, 1 reply; 11+ messages in thread
From: Eli Zaretskii @ 2020-08-10 14:04 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: clive.tovero, emacs-devel

> 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.



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Okay to commit purely cosmetic (indendation) fixes?
  2020-08-10 14:04     ` Eli Zaretskii
@ 2020-08-10 15:39       ` Clive Tovero
  2020-08-10 15:51         ` Eli Zaretskii
  0 siblings, 1 reply; 11+ messages in thread
From: Clive Tovero @ 2020-08-10 15:39 UTC (permalink / raw)
  Cc: emacs-devel

>> 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.



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Okay to commit purely cosmetic (indendation) fixes?
  2020-08-10 15:39       ` Clive Tovero
@ 2020-08-10 15:51         ` Eli Zaretskii
  2020-08-10 17:55           ` Clive Tovero
  0 siblings, 1 reply; 11+ messages in thread
From: Eli Zaretskii @ 2020-08-10 15:51 UTC (permalink / raw)
  To: Clive Tovero; +Cc: emacs-devel

> 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.



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Okay to commit purely cosmetic (indendation) fixes?
       [not found] <e0dab5e9-5603-b2cd-0eec-d708a3b44570.ref@aol.com>
@ 2020-08-10 15:58 ` Clive Tovero
  0 siblings, 0 replies; 11+ messages in thread
From: Clive Tovero @ 2020-08-10 15:58 UTC (permalink / raw)
  To: emacs-devel

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



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Okay to commit purely cosmetic (indendation) fixes?
  2020-08-10 12:23     ` Stefan Monnier
@ 2020-08-10 16:20       ` Karl Fogel
  0 siblings, 0 replies; 11+ messages in thread
From: Karl Fogel @ 2020-08-10 16:20 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Clive Tovero, Lars Ingebrigtsen, emacs-devel

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



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Okay to commit purely cosmetic (indendation) fixes?
  2020-08-10 15:51         ` Eli Zaretskii
@ 2020-08-10 17:55           ` Clive Tovero
  0 siblings, 0 replies; 11+ messages in thread
From: Clive Tovero @ 2020-08-10 17:55 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: emacs-devel

> 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.



^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-08-10 17:55 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <94dd40a5-0195-b88b-3cc1-298d32f99ad8.ref@aol.com>
2020-08-09 22:40 ` Okay to commit purely cosmetic (indendation) fixes? Clive Tovero
2020-08-10 10:53   ` Lars Ingebrigtsen
2020-08-10 12:23     ` Stefan Monnier
2020-08-10 16:20       ` Karl Fogel
2020-08-10 14:04     ` Eli Zaretskii
2020-08-10 15:39       ` Clive Tovero
2020-08-10 15:51         ` Eli Zaretskii
2020-08-10 17:55           ` Clive Tovero
     [not found] <e0dab5e9-5603-b2cd-0eec-d708a3b44570.ref@aol.com>
2020-08-10 15:58 ` Clive Tovero
2020-08-09 20:49 Karl Fogel
2020-08-09 20:52 ` Lars Ingebrigtsen

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.