Re: master 501e2096d6: Fix some issues with a recent change

Re: master 501e2096d6: Fix some issues with a recent change

[Stefan Monnier]

Please include some more info in the first line of your commits, such as
the relevant file(s).


        Stefan

Re: master 501e2096d6: Fix some issues with a recent change

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Sun, 26 Dec 2021 12:04:30 -0500
> 
> Please include some more info in the first line of your commits, such as
> the relevant file(s).

According to the format we use, the file(s) don't have to be mentioned
in the first line.  Given enough files, that is simply impossible.

I know that you like that form, but that is not what CONTRIBUTE says,
so let's not confuse contributors by promoting two separate and
somewhat contradicting conventions.

Re: master 501e2096d6: Fix some issues with a recent change

[Stefan Monnier]

>> Please include some more info in the first line of your commits, such as
>> the relevant file(s).
>
> According to the format we use, the file(s) don't have to be mentioned
> in the first line.  Given enough files, that is simply impossible.

Please re-read the `Subject:` of this thread and tell me if you think it
holds any useful information other than "master 501e2096d6"?

This changed a single file, AFAIK, so we're pretty far from the
"difficult" case of having to summarize an arbitrary set of files.


        Stefan

Re: master 501e2096d6: Fix some issues with a recent change

[Po Lu]

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

>>> Please include some more info in the first line of your commits, such as
>>> the relevant file(s).
>>
>> According to the format we use, the file(s) don't have to be mentioned
>> in the first line.  Given enough files, that is simply impossible.
>
> Please re-read the `Subject:` of this thread and tell me if you think it
> holds any useful information other than "master 501e2096d6"?
>
> This changed a single file, AFAIK, so we're pretty far from the
> "difficult" case of having to summarize an arbitrary set of files.

That touched 3 files, lisp/face-remap.el, etc/NEWS, and
doc/lispref/commands.texi.

If it were a single file, I would have found a way to omit the title
altogether.

Re: master 501e2096d6: Fix some issues with a recent change

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: luangruo@yahoo.com,  emacs-devel@gnu.org
> Date: Sun, 26 Dec 2021 16:12:37 -0500
> 
> >> Please include some more info in the first line of your commits, such as
> >> the relevant file(s).
> >
> > According to the format we use, the file(s) don't have to be mentioned
> > in the first line.  Given enough files, that is simply impossible.
> 
> Please re-read the `Subject:` of this thread and tell me if you think it
> holds any useful information other than "master 501e2096d6"?

I find it entirely pertinent.  Changes aren't supposed to be judged by
reading the emacs-diffs mailing list, they are supposed to be judged
by looking at the VCS logs, in which case the "recent change" part
will tell you all you need to know.

Once again, personal workflows should not affect our coding
conventions.

Re: master 501e2096d6: Fix some issues with a recent change

[Stefan Monnier]

>> This changed a single file, AFAIK, so we're pretty far from the
>> "difficult" case of having to summarize an arbitrary set of files.
> That touched 3 files, lisp/face-remap.el, etc/NEWS, and
> doc/lispref/commands.texi.

The main change was to `face-remap.el` so having `face-remap.el` in the
first line would have been very helpful to me.  Another option would have
been to include `text-scale-pinch` in the first line, that would have
worked great for me as well.

The other changes were largely secondary and related, so it's OK if they
don't get any mention in the first line.


        Stefan

Re: master 501e2096d6: Fix some issues with a recent change

[Stefan Monnier]

> Once again, personal workflows should not affect our coding
> conventions.

AFAIK the conventions are designed to help the various workflows out
there.  Some parts of the convention will only be useful for some of
those workflows, of course.  We do have a convention about the first
line being a kind of summary, even though you seem not to care about
that convention.

Why do we bother with such a first-line-convention if it doesn't even
give any indication whatsoever about which part of the codebase
is modified?

A summary should say something non-trivial about the change.
"Fix some issues with a recent change" does say something, indeed.
AFAICT it gives the following info:
- it's related to something "recent".
- it fixes something rather than adding a new feature.
That's still very vague in my book.

I don't care if the first line lists specific files (the space
constraints are obviously much too severe to impose such a requirement),
but it should give some vague idea about which part of the codebase
is affected.


        Stefan

Re: master 501e2096d6: Fix some issues with a recent change

[Po Lu]

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

> The main change was to `face-remap.el` so having `face-remap.el` in the
> first line would have been very helpful to me.  Another option would have
> been to include `text-scale-pinch` in the first line, that would have
> worked great for me as well.

> The other changes were largely secondary and related, so it's OK if they
> don't get any mention in the first line.

None of that change is "secondary": they all fixed issues Eli pointed
out in an earlier discussion.

Re: master 501e2096d6: Fix some issues with a recent change

[Stefan Monnier]

>> The main change was to `face-remap.el` so having `face-remap.el` in the
>> first line would have been very helpful to me.  Another option would have
>> been to include `text-scale-pinch` in the first line, that would have
>> worked great for me as well.
>
>> The other changes were largely secondary and related, so it's OK if they
>> don't get any mention in the first line.
>
> None of that change is "secondary": they all fixed issues Eli pointed
> out in an earlier discussion.

Maybe we can't agree on it being "secondary" but all those changes are
related to pinch or to code that is itself related to that.
So including those words would give some hint as to what it is you've
changed, whereas the first line you've used gave no indication
whatsoever about which part of the codebase was modified.

Basically, think of the reader who'd like to quickly know "could this
change affect `doctor.el`?"


        Stefan

Re: master 501e2096d6: Fix some issues with a recent change

[Po Lu]

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

> Maybe we can't agree on it being "secondary" but all those changes are
> related to pinch or to code that is itself related to that.
> So including those words would give some hint as to what it is you've
> changed, whereas the first line you've used gave no indication
> whatsoever about which part of the codebase was modified.

Okay, so how about "Fix some issues with the recently introduced pinch
support?"

Re: master 501e2096d6: Fix some issues with a recent change

[Óscar Fuentes]

Po Lu <luangruo@yahoo.com> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>> Maybe we can't agree on it being "secondary" but all those changes are
>> related to pinch or to code that is itself related to that.
>> So including those words would give some hint as to what it is you've
>> changed, whereas the first line you've used gave no indication
>> whatsoever about which part of the codebase was modified.
>
> Okay, so how about "Fix some issues with the recently introduced pinch
> support?"

"Fix some issues with pinch support" would be better. "Recently" is
relative and irrelevant for those who read that line. Or even better:
"pinch support: fix some issues".

If the issues are independent one from another, doing a commit for each
one with an specific subject would be the right thing.

The point of that first line is for people who read emacs-diffs or the
output of `git log --oneline' to quickly decide if they can ignore the
change or it is worth to look into.

Emacs has lots of development fronts and many consumers of the vc-log
info are unconcerned with changes to many of those areas.

Re: master 501e2096d6: Fix some issues with a recent change

[Lars Ingebrigtsen]

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

> Please include some more info in the first line of your commits, such as
> the relevant file(s).

Yes, please do.  It makes it a lot easier to know whether it's a commit
I should skim or not.  For instance, this could have been "Fix some
issues with a recent pinch event change".

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

Re: master 501e2096d6: Fix some issues with a recent change

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: luangruo@yahoo.com,  emacs-devel@gnu.org
> Date: Sun, 26 Dec 2021 23:27:09 -0500
> 
> > Once again, personal workflows should not affect our coding
> > conventions.
> 
> AFAIK the conventions are designed to help the various workflows out
> there.

Some of them, but not all of them.

> Some parts of the convention will only be useful for some of
> those workflows, of course.  We do have a convention about the first
> line being a kind of summary, even though you seem not to care about
> that convention.

I do care about that, and try to follow it in my commits.  I just
don't think that file names necessarily belong to the summary.

> Why do we bother with such a first-line-convention if it doesn't even
> give any indication whatsoever about which part of the codebase
> is modified?

Because sometimes that's the best that can be said in a single short
line.  Or because the changes are so minor they don't deserver
anyone's attention on this high level.  Or for any number of other
reasons.

> A summary should say something non-trivial about the change.

What if the change doesn't have anything non-trivial?

And what's non-trivial for me might be trivial for you, or vice versa.

Coming up with that summary is already a kind of burden, in those
cases where there's no useful summary that can be so short.  You are
now requesting on top of that to provide information that is uniquely
interesting to you, because you filter the changes based on that.  I
think this is too much to expect, and I therefore respectively ask you
not to insist on this detail.

Re: master 501e2096d6: Fix some issues with a recent change

[Dmitry Gutov]

On 27.12.2021 06:27, Eli Zaretskii wrote:
> I find it entirely pertinent.  Changes aren't supposed to be judged by
> reading the emacs-diffs mailing list, they are supposed to be judged
> by looking at the VCS logs, in which case the "recent change" part
> will tell you all you need to know.
> 
> Once again, personal workflows should not affect our coding
> conventions.

"Everybody has to read everything" is a common problem (both regarding 
diffs and the bug tracker), so it would be helpful to alter our 
workflows somehow.

RE: [External] : Re: master 501e2096d6: Fix some issues with a recent change

[Drew Adams]

> Basically, think of the reader who'd like to
> quickly know "could this change affect `doctor.el`?"

Those interested would likely be patient
readers or doctor readers, not impatient
readers...

;-)

Re: master 501e2096d6: Fix some issues with a recent change

[Stefan Monnier]

> Okay, so how about "Fix some issues with the recently introduced pinch
> support?"

That would have been a good improvement, yes.


        Stefan

Re: master 501e2096d6: Fix some issues with a recent change

[Stefan Kangas]

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

>> Okay, so how about "Fix some issues with the recently introduced pinch
>> support?"
>
> That would have been a good improvement, yes.

I would try to follow the CONTRIBUTE guidelines of sticking to 50
characters or fewer when possible, so you could probably shorten that to
something like:

    Fix issues with recently introduced pinch support

or even

    Clean up pinch support

This makes it easier to read the line in various situations (avoiding
truncating), but note that this is just a recommendation and not a
requirement.

Re: master 501e2096d6: Fix some issues with a recent change

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Mon, 27 Dec 2021 15:19:15 -0800
> Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel@gnu.org
> 
> I would try to follow the CONTRIBUTE guidelines of sticking to 50
> characters or fewer when possible, so you could probably shorten that to
> something like:
> 
>     Fix issues with recently introduced pinch support
> 
> or even
> 
>     Clean up pinch support
> 
> This makes it easier to read the line in various situations (avoiding
> truncating), but note that this is just a recommendation and not a
> requirement.

But "fix issues" and "clean up" are not necessarily the same.

Re: master 501e2096d6: Fix some issues with a recent change

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> But "fix issues" and "clean up" are not necessarily the same.

That's true; I improvised here as I don't know the content of the patch
we are discussing.  It was mostly just to give an example of how things
in general can be shortened to something less than 50 characters.