Re: master 50020d026c: Untabify rcirc.el

Re: master 50020d026c: Untabify rcirc.el

[Lars Ingebrigtsen]

Philip Kaludercic <philipk@posteo.net> writes:

>     Untabify rcirc.el

We don't do whitespace changes like this because it makes it more
difficult to follow historical changes (and applying any pending
patches).

So please don't do things like this.

Re: master 50020d026c: Untabify rcirc.el

[Philip Kaludercic]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>>     Untabify rcirc.el
>
> We don't do whitespace changes like this because it makes it more
> difficult to follow historical changes 

My apologies, I didn't know that there was a general rule like this.
The last few times I had been working on rcirc I always ran into issues
with whitespaces whenever indenting a function.  Every patch had
unrelated whitespace changes that had to be reverted to that the
historical record isn't polluted.

On a related note, I believe there has been a need to reorder rcirc to
make it easier to follow along for a while.  But it goes without saying
that that would break the git history far more than this change.

>                                        (and applying any pending
> patches).

That shouldn't be an issue, because git am/apply can ignore whitespace.

> So please don't do things like this.

I will keep it in mind, but there is nothing left to do after this
change.

Re: master 50020d026c: Untabify rcirc.el

[Philip Kaludercic]

Philip Kaludercic <philipk@posteo.net> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> Philip Kaludercic <philipk@posteo.net> writes:
>>
>>>     Untabify rcirc.el
>>
>> We don't do whitespace changes like this because it makes it more
>> difficult to follow historical changes 
>
> My apologies, I didn't know that there was a general rule like this.
> The last few times I had been working on rcirc I always ran into issues
> with whitespaces whenever indenting a function.  Every patch had
> unrelated whitespace changes that had to be reverted to that the
> historical record isn't polluted.
>
> On a related note, I believe there has been a need to reorder rcirc to
> make it easier to follow along for a while.  But it goes without saying
> that that would break the git history far more than this change.
>
>>                                        (and applying any pending
>> patches).
>
> That shouldn't be an issue, because git am/apply can ignore whitespace.

I've also just found out that git blame supports this: Setting
`vc-git-annotate-switches' to "-w" ignores all the whitespace changes
when using vc-annotate.

Re: master 50020d026c: Untabify rcirc.el

[Stefan Kangas]

Philip Kaludercic <philipk@posteo.net> writes:

> I've also just found out that git blame supports this: Setting
> `vc-git-annotate-switches' to "-w" ignores all the whitespace changes
> when using vc-annotate.

If that git flag has been around for a while, I think we should consider
doing that by default.

Re: master 50020d026c: Untabify rcirc.el

[Philip Kaludercic]

Stefan Kangas <stefankangas@gmail.com> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> I've also just found out that git blame supports this: Setting
>> `vc-git-annotate-switches' to "-w" ignores all the whitespace changes
>> when using vc-annotate.
>
> If that git flag has been around for a while, I think we should consider
> doing that by default.

It appears to have been added in 2007[0], so this might be viable.


[0] https://git.kernel.org/pub/scm/git/git.git/commit/?id=b82871b3c32faa8a317007f343fdf2d0ddc9954e

Re: master 50020d026c: Untabify rcirc.el

[Stefan Kangas]

Philip Kaludercic <philipk@posteo.net> writes:

> It appears to have been added in 2007[0], so this might be viable.

So I guess the below should be uncontroversial?

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 9dfdd9e7b1..3b15e976e5 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -119,14 +119,14 @@ vc-git-diff-switches
 		 (repeat :tag "Argument List" :value ("") string))
   :version "23.1")

-(defcustom vc-git-annotate-switches nil
+(defcustom vc-git-annotate-switches "-w"
   "String or list of strings specifying switches for Git blame under VC.
 If nil, use the value of `vc-annotate-switches'.  If t, use no switches."
   :type '(choice (const :tag "Unspecified" nil)
 		 (const :tag "None" t)
 		 (string :tag "Argument String")
 		 (repeat :tag "Argument List" :value ("") string))
-  :version "25.1")
+  :version "29.1")

 (defcustom vc-git-log-switches nil
   "String or list of strings specifying switches for Git log under VC."

Re: master 50020d026c: Untabify rcirc.el

[Robert Pluim]

>>>>> On Thu, 8 Sep 2022 05:07:33 -0400, Stefan Kangas <stefankangas@gmail.com> said:

    Stefan> Philip Kaludercic <philipk@posteo.net> writes:
    >> It appears to have been added in 2007[0], so this might be viable.

    Stefan> So I guess the below should be uncontroversial?

That depends. Are you going to call this out in NEWS, and is there any
easy way to invoke `vc-annotate' with nil switches? [1]

It might be better putting it in the emacs .dir-locals.el

Robert

Footnotes:
[1]  The docstring for `vc-annotate-switches' mentions
     `annotate-switches', but I canʼt find that variable

-- 

Re: master 50020d026c: Untabify rcirc.el

[Stefan Kangas]

Robert Pluim <rpluim@gmail.com> writes:

>>>>>> On Thu, 8 Sep 2022 05:07:33 -0400, Stefan Kangas <stefankangas@gmail.com> said:
>
>     Stefan> Philip Kaludercic <philipk@posteo.net> writes:
>     >> It appears to have been added in 2007[0], so this might be viable.
>
>     Stefan> So I guess the below should be uncontroversial?
>
> That depends. Are you going to call this out in NEWS,

That would be the idea, yes.

> and is there any
> easy way to invoke `vc-annotate' with nil switches? [1]

No idea, sorry.

> It might be better putting it in the emacs .dir-locals.el

That's another alternative.

Re: master 50020d026c: Untabify rcirc.el

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Thu, 8 Sep 2022 05:07:33 -0400
> Cc: Lars Ingebrigtsen <larsi@gnus.org>, emacs-devel@gnu.org
> 
> Philip Kaludercic <philipk@posteo.net> writes:
> 
> > It appears to have been added in 2007[0], so this might be viable.
> 
> So I guess the below should be uncontroversial?

No, I see no reason for us to impose our preferences on users.
Program switches should not specify anything by default, unless not
doing so makes no sense, or doesn't allow Emacs to communicate with
the program or produce the effect we need.

Re: master 50020d026c: Untabify rcirc.el

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> Program switches should not specify anything by default, unless not
> doing so makes no sense, or doesn't allow Emacs to communicate with
> the program or produce the effect we need.

How about adding it to our .dir-locals.el instead?

Re: master 50020d026c: Untabify rcirc.el

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Thu, 8 Sep 2022 05:46:04 -0400
> Cc: philipk@posteo.net, larsi@gnus.org, emacs-devel@gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Program switches should not specify anything by default, unless not
> > doing so makes no sense, or doesn't allow Emacs to communicate with
> > the program or produce the effect we need.
> 
> How about adding it to our .dir-locals.el instead?

If that's what people here prefer, I won't mind.

Re: master 50020d026c: Untabify rcirc.el

[Robert Pluim]

>>>>> On Thu, 08 Sep 2022 13:25:29 +0300, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Stefan Kangas <stefankangas@gmail.com>
    >> Date: Thu, 8 Sep 2022 05:46:04 -0400
    >> Cc: philipk@posteo.net, larsi@gnus.org, emacs-devel@gnu.org
    >> 
    >> Eli Zaretskii <eliz@gnu.org> writes:
    >> 
    >> > Program switches should not specify anything by default, unless not
    >> > doing so makes no sense, or doesn't allow Emacs to communicate with
    >> > the program or produce the effect we need.
    >> 
    >> How about adding it to our .dir-locals.el instead?

    Eli> If that's what people here prefer, I won't mind.

I prefer it (although I tend to use magit, which already has '-w' in
its default blame args).

Robert
-- 

Re: master 50020d026c: Untabify rcirc.el

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stefan Kangas <stefankangas@gmail.com>
>> Date: Thu, 8 Sep 2022 05:46:04 -0400
>> Cc: philipk@posteo.net, larsi@gnus.org, emacs-devel@gnu.org
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > Program switches should not specify anything by default, unless not
>> > doing so makes no sense, or doesn't allow Emacs to communicate with
>> > the program or produce the effect we need.
>> 
>> How about adding it to our .dir-locals.el instead?
>
> If that's what people here prefer, I won't mind.

Just as with bug#55040, there will be an issue when checking the master
branch using an older version of Emacs, since `vc-git-annotate-switches'
won't be marked as a safe file local variable.

Re: master 50020d026c: Untabify rcirc.el

[Stefan Kangas]

Philip Kaludercic <philipk@posteo.net> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> If that's what people here prefer, I won't mind.
>
> Just as with bug#55040, there will be an issue when checking the master
> branch using an older version of Emacs, since `vc-git-annotate-switches'
> won't be marked as a safe file local variable.

So I've added it (commit 3ddf1a920d).

If it's too annoying on old versions, we could always comment it out for
a while and re-enable it later, but I'll leave that decision to people
who would be affected.  (I never edit new sources with an old Emacs.)

Re: master 50020d026c: Untabify rcirc.el

[Philip Kaludercic]

Stefan Kangas <stefankangas@gmail.com> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> It appears to have been added in 2007[0], so this might be viable.
>
> So I guess the below should be uncontroversial?
>
> diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
> index 9dfdd9e7b1..3b15e976e5 100644
> --- a/lisp/vc/vc-git.el
> +++ b/lisp/vc/vc-git.el
> @@ -119,14 +119,14 @@ vc-git-diff-switches
>  		 (repeat :tag "Argument List" :value ("") string))
>    :version "23.1")
>
> -(defcustom vc-git-annotate-switches nil
> +(defcustom vc-git-annotate-switches "-w"

It might be useful to add a comment indicating what the point of -w is.

>    "String or list of strings specifying switches for Git blame under VC.
>  If nil, use the value of `vc-annotate-switches'.  If t, use no switches."
>    :type '(choice (const :tag "Unspecified" nil)
>  		 (const :tag "None" t)
>  		 (string :tag "Argument String")
>  		 (repeat :tag "Argument List" :value ("") string))
> -  :version "25.1")
> +  :version "29.1")
>
>  (defcustom vc-git-log-switches nil
>    "String or list of strings specifying switches for Git log under VC."

Re: master 50020d026c: Untabify rcirc.el

[Lars Ingebrigtsen]

Stefan Kangas <stefankangas@gmail.com> writes:

>> I've also just found out that git blame supports this: Setting
>> `vc-git-annotate-switches' to "-w" ignores all the whitespace changes
>> when using vc-annotate.
>
> If that git flag has been around for a while, I think we should consider
> doing that by default.

There are languages where whitespace is significant, so we can't add
"-w" to the default.  But as was also suggested, we could add that to
the Emacs .dir-locals.el.

Re: master 50020d026c: Untabify rcirc.el

[Stefan Monnier]

> My apologies, I didn't know that there was a general rule like this.
> The last few times I had been working on rcirc I always ran into issues
> with whitespaces whenever indenting a function.  Every patch had
> unrelated whitespace changes that had to be reverted to that the
> historical record isn't polluted.

Whitespace changes are "by definition" gratuitous.  We don't disallow
them, but they should be limited to the lines that are changed for other
reasons as well.
[ Note also that there are degrees: e.g. reindentation is not 100%
  gratuitous, contrary to tabify/untabify.  ]

> On a related note, I believe there has been a need to reorder rcirc to
> make it easier to follow along for a while.  But it goes without saying
> that that would break the git history far more than this change.

This is yet elsewhere on the scale of "gratuitous", so we also prefer to
refrain from doing so, but sometimes it's justified (usually together
with a string of other, more substantial, changes).


        Stefan "guilty of many non-whitespace but still gratuitous changes"

Re: master 50020d026c: Untabify rcirc.el

[Philip Kaludercic]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> My apologies, I didn't know that there was a general rule like this.
>> The last few times I had been working on rcirc I always ran into issues
>> with whitespaces whenever indenting a function.  Every patch had
>> unrelated whitespace changes that had to be reverted to that the
>> historical record isn't polluted.
>
> Whitespace changes are "by definition" gratuitous.  We don't disallow
> them, but they should be limited to the lines that are changed for other
> reasons as well.
> [ Note also that there are degrees: e.g. reindentation is not 100%
>   gratuitous, contrary to tabify/untabify.  ]

Thank you for the explanation.

>> On a related note, I believe there has been a need to reorder rcirc to
>> make it easier to follow along for a while.  But it goes without saying
>> that that would break the git history far more than this change.
>
> This is yet elsewhere on the scale of "gratuitous", so we also prefer to
> refrain from doing so, but sometimes it's justified (usually together
> with a string of other, more substantial, changes).

I suspected that being the case, so I'll try to reorder things gradually
as bugs are addressed or new features are added.

>
>         Stefan "guilty of many non-whitespace but still gratuitous changes"

Re: master 50020d026c: Untabify rcirc.el

[Po Lu]

Philip Kaludercic <philipk@posteo.net> writes:

> branch: master
> commit 50020d026cda1fdd70d8a08e08185dbb7183646a
> Author: Philip Kaludercic <philipk@posteo.net>
> Commit: Philip Kaludercic <philipk@posteo.net>
>
>     Untabify rcirc.el

(There's nothing wrong with tabs, not in Lisp code, not elsewhere.  But
that's besides the point.)

Please don't make whitespace changes like this.  It makes vc-annotate
nonfunctional and is very much unnecessary.

Re: master 50020d026c: Untabify rcirc.el

[Jim Porter]

On 9/7/2022 4:23 AM, Po Lu wrote:
> Philip Kaludercic <philipk@posteo.net> writes:
> 
>> branch: master
>> commit 50020d026cda1fdd70d8a08e08185dbb7183646a
>> Author: Philip Kaludercic <philipk@posteo.net>
>> Commit: Philip Kaludercic <philipk@posteo.net>
>>
>>      Untabify rcirc.el
> 
> (There's nothing wrong with tabs, not in Lisp code, not elsewhere.  But
> that's besides the point.)
> 
> Please don't make whitespace changes like this.  It makes vc-annotate
> nonfunctional and is very much unnecessary.

Perhaps we should set 'blame.ignoreRevsFile'[1] in the Emacs repo's Git 
config (and then add this commit SHA to that file)? That should make 
vc-annotate - and anything else that uses "git blame" - ignore this 
commit, along with any other mechanical commits that shouldn't count for 
the purposes of blame.

I believe this was introduced in Git 2.23 (from Aug 2019), so most 
people with relatively-recent Git installs should be able to use this. 
I'm not sure how robust this feature is (i.e. can it properly ignore 
complex changes?), but for changing tabs to spaces, that should be 
pretty trivial for Git to figure out.

[1] 
https://git-scm.com/docs/git-blame#Documentation/git-blame.txt---ignore-revs-fileltfilegt

Re: master 50020d026c: Untabify rcirc.el

[Eli Zaretskii]

> Cc: Philip Kaludercic <philipk@posteo.net>
> From: Jim Porter <jporterbugs@gmail.com>
> Date: Wed, 7 Sep 2022 09:42:54 -0700
> 
> Perhaps we should set 'blame.ignoreRevsFile'[1] in the Emacs repo's Git 
> config (and then add this commit SHA to that file)? That should make 
> vc-annotate - and anything else that uses "git blame" - ignore this 
> commit, along with any other mechanical commits that shouldn't count for 
> the purposes of blame.

It doesn't seem a problem serious enough to justify hiding some
commits.  Especially since the feature is relatively new: two of the
machines on which I work on Emacs use Git older than 2.23.

Thanks.