Relics of removed dir-locals-file-2 feature in pretest

classic Classic list List threaded Threaded
22 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Relics of removed dir-locals-file-2 feature in pretest

Kaushal Modi
Hello,

I was excited to see mention of dir-locals-file-2 in NEWS, but then recalled that that feature was added and then removed because of performance issues.


References to dir-locals-file-2 still need to be removed from:

km²~/downloads/:git/emacs> rg 'dir-locals-file-2'                                                                        
ChangeLog.2
9419:   (dir-locals-file-2, dir-locals--all-files): Remove.
9935:   (dir-locals-file-2): New const.

etc/NEWS
360:See the variable 'dir-locals-file-2' for more information.

etc/DOC
46108:See also `dir-locals-file-2', whose values override this one's.
46109:See Info node `(elisp)Directory Local Variables' for details.Vdir-locals-file-2

lisp/files.el
3868:See also `dir-locals-file-2', whose values override this one's.
3871:(defconst dir-locals-file-2 ".dir-locals-2.el"
--

Kaushal Modi

Reply | Threaded
Open this post in threaded view
|

Re: Relics of removed dir-locals-file-2 feature in pretest

Kaushal Modi
Bump. Looks like this email got missed.

On Thu, Oct 12, 2017 at 9:58 AM Kaushal Modi <[hidden email]> wrote:
Hello,

I was excited to see mention of dir-locals-file-2 in NEWS, but then recalled that that feature was added and then removed because of performance issues.


References to dir-locals-file-2 still need to be removed from:

km²~/downloads/:git/emacs> rg 'dir-locals-file-2'                                                                        
ChangeLog.2
9419:   (dir-locals-file-2, dir-locals--all-files): Remove.
9935:   (dir-locals-file-2): New const.

etc/NEWS
360:See the variable 'dir-locals-file-2' for more information.

etc/DOC
46108:See also `dir-locals-file-2', whose values override this one's.
46109:See Info node `(elisp)Directory Local Variables' for details.Vdir-locals-file-2

lisp/files.el
3868:See also `dir-locals-file-2', whose values override this one's.
3871:(defconst dir-locals-file-2 ".dir-locals-2.el"
--

Kaushal Modi

--

Kaushal Modi

Reply | Threaded
Open this post in threaded view
|

Re: Relics of removed dir-locals-file-2 feature in pretest

Eli Zaretskii
> From: Kaushal Modi <[hidden email]>
> Date: Tue, 21 Nov 2017 19:56:51 +0000
>
> Bump. Looks like this email got missed.
>
> On Thu, Oct 12, 2017 at 9:58 AM Kaushal Modi <[hidden email]> wrote:
>
>  Hello,
>
>  I was excited to see mention of dir-locals-file-2 in NEWS, but then recalled that that feature was added
>  and then removed because of performance issues.

It was only removed from the emacs-25 branch, not from the other
branches.

Reply | Threaded
Open this post in threaded view
|

Re: Relics of removed dir-locals-file-2 feature in pretest

Kaushal Modi
On Tue, Nov 21, 2017 at 3:33 PM Eli Zaretskii <[hidden email]> wrote:

It was only removed from the emacs-25 branch, not from the other
branches.

There's a bit of confusion with that "It" in "It was removed".

I see that that dir-locals-2 feature is absent on emacs-26 branch but the mentions are still there in NEWS, DOC and files.el (dir-locals-file docstring and unused defconst dir-locals-file-2).
--

Kaushal Modi

Reply | Threaded
Open this post in threaded view
|

Re: Relics of removed dir-locals-file-2 feature in pretest

Eli Zaretskii
> From: Kaushal Modi <[hidden email]>
> Date: Tue, 21 Nov 2017 21:11:30 +0000
> Cc: [hidden email]
>
> On Tue, Nov 21, 2017 at 3:33 PM Eli Zaretskii <[hidden email]> wrote:
>
>  It was only removed from the emacs-25 branch, not from the other
>  branches.
>
> There's a bit of confusion with that "It" in "It was removed".
>
> I see that that dir-locals-2 feature is absent on emacs-26 branch but the mentions are still there in NEWS,
> DOC and files.el (dir-locals-file docstring and unused defconst dir-locals-file-2).

You keep saying that, but I don't understand why: I still see on the
latest emacs-26 the code which you say was removed.  What am I
missing?

Reply | Threaded
Open this post in threaded view
|

Re: Relics of removed dir-locals-file-2 feature in pretest

Kaushal Modi
On Wed, Nov 22, 2017 at 10:19 AM Eli Zaretskii <[hidden email]> wrote:
You keep saying that, but I don't understand why: I still see on the
latest emacs-26 the code which you say was removed.  What am I
missing?

OK, I grepped for dir-locals-file-2 in the whole emacs repo, and all I get is:

etc/NEWS:369:19:See the variable 'dir-locals-file-2' for more information.
etc/DOC:46209:11:See also `dir-locals-file-2', whose values override this one's.
etc/DOC:46210:64:See Info node `(elisp)Directory Local Variables' for details. Vdir-locals-file-2
lisp/files.el:3908:11:See also `dir-locals-file-2', whose values override this one's.
lisp/files.el:3911:11:(defconst dir-locals-file-2 ".dir-locals-2.el"
ChangeLog.2:9419:3:    (dir-locals-file-2, dir-locals--all-files): Remove.
ChangeLog.2:9935:3:    (dir-locals-file-2): New const.

I see defconst dir-locals-file-2, but I don't see it being used anywhere. So I thought that the feature was removed.

But when you said that emacs-26 still has the feature (yay!), I looked at files.el again.. and of course the feature is there:

I see this in dir-locals--all-files:

           (file-2 (when (string-match "\\.el\\'" file-1)
                     (replace-match "-2.el" t nil file-1)))

So.. there's an issue, though of a different kind.. may be dir-locals-file-2 mention should be removed? Or instead be used in the code?
--

Kaushal Modi

Reply | Threaded
Open this post in threaded view
|

Re: Relics of removed dir-locals-file-2 feature in pretest

Eli Zaretskii
> From: Kaushal Modi <[hidden email]>
> Date: Wed, 22 Nov 2017 15:58:36 +0000
> Cc: [hidden email]
>
> I see defconst dir-locals-file-2, but I don't see it being used anywhere. So I thought that the feature was
> removed.
>
> But when you said that emacs-26 still has the feature (yay!), I looked at files.el again.. and of course the
> feature is there:
>
> I see this in dir-locals--all-files:
>
>            (file-2 (when (string-match "\\.el\\'" file-1)
>                      (replace-match "-2.el" t nil file-1)))
>
> So.. there's an issue, though of a different kind.. may be dir-locals-file-2 mention should be removed? Or
> instead be used in the code?

But it _is_ used, you just show above the code which uses it.  And
that's not the only place; see the commit to which you pointed
originally for the code which is affected by this second dir-locals
file.

So I really don't understand what is the issue that needs to be fixed.

Reply | Threaded
Open this post in threaded view
|

Re: Relics of removed dir-locals-file-2 feature in pretest

Kaushal Modi
On Wed, Nov 22, 2017 at 11:20 AM Eli Zaretskii <[hidden email]> wrote:
So I really don't understand what is the issue that needs to be fixed.

OK, at the risk of sounding very stupid.. this feature was removed and then added again in this commit: http://git.savannah.gnu.org/cgit/emacs.git/commit/?h=emacs-26&id=da976cff352bdea6adc2667582a56eb4061bb5f6

Right?

The defconst dir-locals-file-2 is added in that commit, but is not used anywhere.

Looking at the dir-locals--all-files code in that commit, from what I understand, the let-bound file-2 derivation is hard-coded, and has nothing to do with dir-locals-file-2:

           (file-2 (when (string-match "\\.el\\'" file-1)
                     (replace-match "-2.el" t nil file-1)))

Is that correct? So should the dir-locals-file-2 defconst and all its mentions be removed from doc? Or should that var instead be used in dir-locals--all-files and wherever else applicable?
--

Kaushal Modi

Reply | Threaded
Open this post in threaded view
|

Re: Relics of removed dir-locals-file-2 feature in pretest

Eli Zaretskii
> From: Kaushal Modi <[hidden email]>
> Date: Wed, 22 Nov 2017 16:28:18 +0000
> Cc: [hidden email]
>
> OK, at the risk of sounding very stupid.. this feature was removed and then added again in this commit:
> http://git.savannah.gnu.org/cgit/emacs.git/commit/?h=emacs-26&id=da976cff352bdea6adc2667582a56eb4061bb5f6
>
>
> Right?

No, it was added first, and then removed _only_ from the emacs-25
branch.  It was left on master (where it was merged from emacs-25),
and therefore it stays on today's emacs-26 and on master.

> The defconst dir-locals-file-2 is added in that commit, but is not used anywhere.
>
> Looking at the dir-locals--all-files code in that commit, from what I understand, the let-bound file-2 derivation is
> hard-coded, and has nothing to do with dir-locals-file-2:
>
>            (file-2 (when (string-match "\\.el\\'" file-1)
>                      (replace-match "-2.el" t nil file-1)))
>
> Is that correct?

The facts are correct, yes.

> So should the dir-locals-file-2 defconst and all its mentions be removed from doc? Or should
> that var instead be used in dir-locals--all-files and wherever else applicable?

We could remove the defconst, but just removing it is not enough,
because that would also remove its doc string.  So we will have to do
something else in order to keep that special file name documented
(e.g., so that "M-x apropos-documentation" would find it).  I'm not
sure we should invest such an effort: after all, what's there does
work, so why fix that which ain't broken?

Reply | Threaded
Open this post in threaded view
|

Re: Relics of removed dir-locals-file-2 feature in pretest

Kaushal Modi
On Thu, Nov 23, 2017, 10:54 AM Eli Zaretskii <[hidden email]> wrote:
We could remove the defconst, but just removing it is not enough,
because that would also remove its doc string.  So we will have to do
something else in order to keep that special file name documented
(e.g., so that "M-x apropos-documentation" would find it).  I'm not
sure we should invest such an effort: after all, what's there does
work, so why fix that which ain't broken?

.. because the documentation is incorrect. The dir-locals-file-2 variable is useless at this point. 

I am volunteering to remove references to dir-locals-file-2 everywhere and mention the "-2.el" file instead in dir-locals-file doc-string and other places where dir-locals-file-2 is currently mentioned. 
--

Kaushal Modi

Reply | Threaded
Open this post in threaded view
|

Re: Relics of removed dir-locals-file-2 feature in pretest

Eli Zaretskii
> From: Kaushal Modi <[hidden email]>
> Date: Thu, 23 Nov 2017 17:34:07 +0000
> Cc: Emacs developers <[hidden email]>
>
>  We could remove the defconst, but just removing it is not enough,
>  because that would also remove its doc string.  So we will have to do
>  something else in order to keep that special file name documented
>  (e.g., so that "M-x apropos-documentation" would find it).  I'm not
>  sure we should invest such an effort: after all, what's there does
>  work, so why fix that which ain't broken?
>
> .. because the documentation is incorrect. The dir-locals-file-2 variable is useless at this point.

Which part of it is incorrect?

> I am volunteering to remove references to dir-locals-file-2 everywhere and mention the "-2.el" file instead in
> dir-locals-file doc-string and other places where dir-locals-file-2 is currently mentioned.

Thank you for volunteering, but I'm not yet sure we should do that.

Reply | Threaded
Open this post in threaded view
|

Re: Relics of removed dir-locals-file-2 feature in pretest

Kaushal Modi
On Thu, Nov 23, 2017 at 3:09 PM Eli Zaretskii <[hidden email]> wrote:

Which part of it is incorrect?

Let's say a user reads the NEWS and finds:

> * A second dir-local file (.dir-locals-2.el) is now accepted.
> See the variable 'dir-locals-file-2' for more information.

They do C-h v dir-locals-file-2 and read:

> This essentially a second file that can be used like `dir-locals-file', ..

That is incorrect! dir-locals-file-2 *cannot* be used like `dir-locals-file'! That variable is just declared but not used anywhere (why is it that only I think that that's a problem). A user can

Reading further in that docstring:

> See Info node `(elisp)Directory Local Variables' for details.

That node has no reference to dir-locals-file-2 variable or a mention of the feature that .dir-locals-2.el can be used to override .dir-local.el.

Also in the doc-string of dir-locals-file:

> See also `dir-locals-file-2', whose values override this one's.

No, it *does not*! dir-locals-file-2 is not used in any code.

Thank you for volunteering, but I'm not yet sure we should do that.

Please consider making this change. 

From your earlier email (I missed replying to this part):

> We could remove the defconst, but just removing it is not enough,
> because that would also remove its doc string. 

Why is that bad? That variable is anyways not used.. so what's the point of keeping docstring of an unused variable.

> So we will have to do
> something else in order to keep that special file name documented
> (e.g., so that "M-x apropos-documentation" would find it).  I'm not
> sure we should invest such an effort: after all, what's there does
> work,

Why not just mention that a user can just add "-2" to (file-name-sans-extension dir-locals-file), and that file can be used to override dir-locals-file. Mention that in the dir-locals-file docstring and the (elisp)Directory Local Variables node.

> so why fix that which ain't broken?

The documentation is broken. It is incorrect, misleading.

And I am not just complaining, I am willing to provide a patch that removes that defconst and updates the dir-local-file doc-string and the manual to make this feature better discoverable. So why is that is problem? Why would you want the 26.1 release to go out with this misdocumented feature? What am I missing?
--

Kaushal Modi

Reply | Threaded
Open this post in threaded view
|

Re: Relics of removed dir-locals-file-2 feature in pretest

Eli Zaretskii
> From: Kaushal Modi <[hidden email]>
> Date: Mon, 27 Nov 2017 14:57:26 +0000
> Cc: [hidden email]
>
> Let's say a user reads the NEWS and finds:
>
> > * A second dir-local file (.dir-locals-2.el) is now accepted.
> > See the variable 'dir-locals-file-2' for more information.
>
> They do C-h v dir-locals-file-2 and read:
>
> > This essentially a second file that can be used like `dir-locals-file', ..
>
> That is incorrect! dir-locals-file-2 *cannot* be used like `dir-locals-file'! That variable is just declared but not
> used anywhere (why is it that only I think that that's a problem). A user can

The "second file" is .dir-locals-2.el, i.e. the value.

> Reading further in that docstring:
>
> > See Info node `(elisp)Directory Local Variables' for details.
>
> That node has no reference to dir-locals-file-2 variable or a mention of the feature that .dir-locals-2.el can be
> used to override .dir-local.el.

So maybe we need to add that to the manual.

> Also in the doc-string of dir-locals-file:
>
> > See also `dir-locals-file-2', whose values override this one's.
>
> No, it *does not*! dir-locals-file-2 is not used in any code.

But its (constant) value does override, doesn't it?

>  Thank you for volunteering, but I'm not yet sure we should do that.
>
> Please consider making this change.

Which change?  You are arguing about multiple issues, and I'm confused
about what change do you have in mind, exactly.

> From your earlier email (I missed replying to this part):
>
> > We could remove the defconst, but just removing it is not enough,
> > because that would also remove its doc string.
>
> Why is that bad?

Because iut's the only place where .dir-locals-2.el is documented.

> That variable is anyways not used.. so what's the point of keeping docstring of an unused
> variable.

The doc string documents a feature.  If you remove the doc string and
do nothing else, where will that feature be documented?

> Why not just mention that a user can just add "-2" to (file-name-sans-extension dir-locals-file), and that file can
> be used to override dir-locals-file. Mention that in the dir-locals-file docstring and the (elisp)Directory Local
> Variables node.

That might work.  As I said, just removing the defconst is not enough.

> The documentation is broken. It is incorrect, misleading.

Then let's fix the documentation.

> And I am not just complaining, I am willing to provide a patch that removes that defconst and updates the
> dir-local-file doc-string and the manual to make this feature better discoverable. So why is that is problem?

I didn't say it was a problem, you misunderstood what I said.  Feel
free to submit such a patch, and thanks in advance.

Reply | Threaded
Open this post in threaded view
|

Re: Relics of removed dir-locals-file-2 feature in pretest

Kaushal Modi
On Mon, Nov 27, 2017 at 11:15 AM Eli Zaretskii <[hidden email]> wrote:
The "second file" is .dir-locals-2.el, i.e. the value.

Correct. But the current state of documentation would lead one to believe that the second file name is ".dir-locals-2.el" because of the dir-locals-file-2 constant; it's actually derived from dir-locals-file only.

So maybe we need to add that to the manual.

I have tried to clarify this in the attached patch.

But its (constant) value does override, doesn't it?

".dir-locals-2.el" file does override ".dir-locals.el". But that would happen even if the value of dir-locals-file-2 were "foo.el" i.e. at present dir-locals-file-2 value is completely unused.. it's just a documentation holder. I have now moved that documentation to the doc string of dir-locals-file constant.

Which change?  You are arguing about multiple issues, and I'm confused
about what change do you have in mind, exactly.

Apologies for the confusion, looking at NEWS, I learned about dir-locals-file-2 constant, but then grepping for that variable led me nowhere. So I initially thought that that constant was a relic of now-removed feature. I was wrong about that part.. the feature is still there and just that dir-locals-file-2 was not used. So the "change" which I propose in the attached patch is remove dir-locals-file-2 constant and put the documentation about this feature in the right places.

> That variable is anyways not used.. so what's the point of keeping docstring of an unused
> variable.

The doc string documents a feature.  If you remove the doc string and
do nothing else, where will that feature be documented?

Yes, there was miscommunication.. I proposed to remove that doc string and add references to ".dir-locals-2.el" wherever applicable (http://lists.gnu.org/r/emacs-devel/2017-11/msg00568.html).

Then let's fix the documentation.
I didn't say it was a problem, you misunderstood what I said.  Feel
free to submit such a patch, and thanks in advance.

Thanks! I have attached the patch.
--

Kaushal Modi


0001-Update-documentation-about-.dir-locals-2.el.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Relics of removed dir-locals-file-2 feature in pretest

Eli Zaretskii
> From: Kaushal Modi <[hidden email]>
> Date: Mon, 27 Nov 2017 17:04:21 +0000
> Cc: [hidden email]
>
> Thanks! I have attached the patch.

Thanks, I have a few comments.

> * lisp/files.el: Remove unused constant `dir-locals-file-2'.

This should be formatted as follows:

 * lisp/files.el (dir-locals-file-2): Remove unused constant.

> * lisp/files.el(dir-locals-file):
                ^^
Missing space.

> * doc/lispref/variables.texi (Directory Local Variables): Mention
>   ".dir-locals-2.el".

These two don't really live together well: one is code, the other
documentation.  So the first one is better written as a separate
entry:

 * lisp/files.el (dir-locals-file): Mention '.dir-locals-2.el' in the
   doc string.

>  doc/lispref/variables.texi | 29 +++++++++++++++++------------
>  etc/NEWS                   |  2 +-
>  lisp/files.el              | 18 +++++++++---------
>  3 files changed, 27 insertions(+), 22 deletions(-)

The change in NEWS should also be mentioned in the log message.

> diff --git a/doc/lispref/variables.texi b/doc/lispref/variables.texi
> index a871352b00..c14a440cbf 100644
> --- a/doc/lispref/variables.texi
> +++ b/doc/lispref/variables.texi
> @@ -1911,18 +1911,23 @@ Directory Local Variables
>  @defvr Constant dir-locals-file
>  This constant is the name of the file where Emacs expects to find the
>  directory-local variables.  The name of the file is
> -@file{.dir-locals.el}@footnote{
> -The MS-DOS version of Emacs uses @file{_dir-locals.el} instead, due to
> -limitations of the DOS filesystems.
> -}.  A file by that name in a directory causes Emacs to apply its
> -settings to any file in that directory or any of its subdirectories
> -(optionally, you can exclude subdirectories; see below).
> -If some of the subdirectories have their own @file{.dir-locals.el}
> -files, Emacs uses the settings from the deepest file it finds starting
> -from the file's directory and moving up the directory tree.  The file
> -specifies local variables as a specially formatted list; see
> -@ref{Directory Variables, , Per-directory Local Variables, emacs, The
> -GNU Emacs Manual}, for more details.
> +@file{.dir-locals.el}@footnote{ The MS-DOS version of Emacs uses
> +@file{_dir-locals.el} instead, due to limitations of the DOS
> +filesystems.  }.  A file by that name in a directory causes Emacs to
> +apply its settings to any file in that directory or any of its
> +subdirectories (optionally, you can exclude subdirectories; see
> +below).  If some of the subdirectories have their own
> +@file{.dir-locals.el} files, Emacs uses the settings from the deepest
> +file it finds starting from the file's directory and moving up the
> +directory tree.  This constant is also used to derive the name of a
> +second dir-locals file @file{.dir-locals-2.el}.  If this second
> +dir-locals file is present, then that is loaded instead of
> +@file{.dir-locals.el}.  This is useful when @file{.dir-locals.el} is
> +under version control in a shared repository and cannot be used for
> +personal customizations.  The file specifies local variables as a
> +specially formatted list; see @ref{Directory Variables, ,
> +Per-directory Local Variables, emacs, The GNU Emacs Manual}, for more
> +details.

This looks like a lot of changes, but it actually only changes the
last few lines.  Please try not to refill existing text you don't
modify (unless it's really badly formatted), so that the actual
changes are clearly visible.

> +                                      This second file name is
> +derived by appending \"-2\" to the file name component without
> +extension in `dir-locals-file'.

This sentence is slightly confusing.  Suggest to reword:

  The name of this second file is derived by appending \"-2\" to
  the base name of `dir-locals-file'.

> +                                 For example, if the value of
> +`dir-locals-file' is \".dir-locals.el\", a \".dir-locals-2.el\"
> +file in the same directory will override the \".dir-locals.el\".

And this sentence could be made simpler if you just describe the
default case:

  With the default value of `dir-locals-file', a \".dir-locals-2.el\"
  file in the same directory will override \".dir-locals.el\".

Reply | Threaded
Open this post in threaded view
|

Re: Relics of removed dir-locals-file-2 feature in pretest

Kaushal Modi
Thanks for the quick feedback. My comments are inline below, followed by updated patch.

On Mon, Nov 27, 2017 at 12:28 PM Eli Zaretskii <[hidden email]> wrote:
> * lisp/files.el: Remove unused constant `dir-locals-file-2'.

This should be formatted as follows:

 * lisp/files.el (dir-locals-file-2): Remove unused constant.

Now fixed (I wasn't quite sure if I should do * foo (bar): if bar was being removed).

> * lisp/files.el(dir-locals-file):
                ^^
Missing space.

Fixed.

> * doc/lispref/variables.texi (Directory Local Variables): Mention
>   ".dir-locals-2.el".

These two don't really live together well: one is code, the other
documentation.  So the first one is better written as a separate
entry:

 * lisp/files.el (dir-locals-file): Mention '.dir-locals-2.el' in the
   doc string.

Fixed. Will keep the point in mind about keeping code and doc separate.

>  doc/lispref/variables.texi | 29 +++++++++++++++++------------
>  etc/NEWS                   |  2 +-
>  lisp/files.el              | 18 +++++++++---------
>  3 files changed, 27 insertions(+), 22 deletions(-)

The change in NEWS should also be mentioned in the log message.

Done.

This looks like a lot of changes, but it actually only changes the
last few lines.  Please try not to refill existing text you don't
modify (unless it's really badly formatted), so that the actual
changes are clearly visible.

Understood. I thought that doing M-q was the right thing to do. I have now manually filled only the edited lines.

> +                                      This second file name is
> +derived by appending \"-2\" to the file name component without
> +extension in `dir-locals-file'.

This sentence is slightly confusing.  Suggest to reword:

  The name of this second file is derived by appending \"-2\" to
  the base name of `dir-locals-file'.

> +                                 For example, if the value of
> +`dir-locals-file' is \".dir-locals.el\", a \".dir-locals-2.el\"
> +file in the same directory will override the \".dir-locals.el\".

And this sentence could be made simpler if you just describe the
default case:

  With the default value of `dir-locals-file', a \".dir-locals-2.el\"
  file in the same directory will override \".dir-locals.el\".

Done.

Updated patch is attached. Thank you.
--

Kaushal Modi


0001-Update-documentation-about-.dir-locals-2.el.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Relics of removed dir-locals-file-2 feature in pretest

Eli Zaretskii
> From: Kaushal Modi <[hidden email]>
> Date: Mon, 27 Nov 2017 17:50:33 +0000
> Cc: [hidden email]
>
> Updated patch is attached. Thank you.

Thanks, pushed to the release branch.

Reply | Threaded
Open this post in threaded view
|

Re: Relics of removed dir-locals-file-2 feature in pretest

Kaushal Modi
On Tue, Nov 28, 2017 at 12:20 PM Eli Zaretskii <[hidden email]> wrote:
Thanks, pushed to the release branch.

Thank you.

I see that the reference link was mentioned as:


When I put that link in the message, I couldn't find any rules on how to do that in on http://git.savannah.gnu.org/cgit/emacs.git/tree/CONTRIBUTE?h=emacs-26 or https://www.gnu.org/prep/standards/html_node/Change-Logs.html.

For contributing to the Org repo, the style is to mention links simply within angle brackets:



Can the same rule be adopted for the Emacs repo and documented in /CONTRIBUTE?
--

Kaushal Modi

Reply | Threaded
Open this post in threaded view
|

Re: Relics of removed dir-locals-file-2 feature in pretest

Eli Zaretskii
> From: Kaushal Modi <[hidden email]>
> Date: Tue, 28 Nov 2017 17:34:19 +0000
> Cc: [hidden email]
>
> When I put that link in the message, I couldn't find any rules on how to do that in on
> http://git.savannah.gnu.org/cgit/emacs.git/tree/CONTRIBUTE?h=emacs-26 or
> https://www.gnu.org/prep/standards/html_node/Change-Logs.html.
>
> For contributing to the Org repo, the style is to mention links simply within angle brackets:
>
>     <https://lists.gnu.org/r/emacs-devel/2017-11/msg00649.html>
>
> Example:
> http://orgmode.org/cgit.cgi/org-mode.git/commit/?id=d7940ae2a7ca01f01d17ba85e113ad7cc65b10ea
>
> Can the same rule be adopted for the Emacs repo and documented in /CONTRIBUTE?

Why is that important?  It's a nuisance to remember to add the
brackets, so I'd prefer to avoid that.

Reply | Threaded
Open this post in threaded view
|

Re: Relics of removed dir-locals-file-2 feature in pretest

Kaushal Modi
On Tue, Nov 28, 2017 at 12:52 PM Eli Zaretskii <[hidden email]> wrote:
Why is that important?

It just brings in some consistency.. just as there are rules for other parts in the commit message. It also makes commit messages easily parseable (if ever needed) for links.
 
  It's a nuisance to remember to add the
brackets, so I'd prefer to avoid that.

I have used that style in Org commits many times, and it's kind of become a second nature, so not very difficult to remember if doing that is added as a guideline to CONTRIBUTE in the "** Commit messages" section. Also, the "remembering issue" applies to everything else in that same section too :)

But in any case, I thought of bringing this up as I saw that there were no guidelines on how to put links in commit messages.
--

Kaushal Modi

12