* 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 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 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?
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
[parent not found: <e0dab5e9-5603-b2cd-0eec-d708a3b44570.ref@aol.com>]
* 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
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 public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).