[PATCH 1/1] compile.el bug in recognizing -o switches among output

[PATCH 1/1] compile.el bug in recognizing -o switches among output

[Jes Bodi Klinke]

Hello emacs developers

I have long been an satisfied user of Emacs for coding, this is my first
attempt at modifying the code of Emacs itself, so please bear with me.

I have noticed that the highlighting of -o switches in the *compilation*
buffer (or rather the filename following any -o switch) does not properly
handle some cases of extended switches.  For example if it encounters
"--omega" in the output, it will highligt "mega" as a file name.  (If
there had been just a single dash, like "-omega", then it would be correct
to highligt "mega".)

I have located the following regex inside compile.el:
" --?o\\(?:utfile\\|utput\\)?[= ]?\\(\\S +\\)"

It seems to me that this was thought to handle three cases:
 -o optionally followed by space or equal sign before filename
 --outfile followed by space or equal sign before filename
 --output followed by space or equal sign before filename

However, the regex also matches cases such as:
 --o followed by any other characters
       "--omega" will highligt "mega"
 --output followed by any other characters (without space or equal)
       "--output-html-file" will highligt "-html-file"

I do not think the two examples above should result in any highlighting,
in fact i propose to limit the regex to the three initial cases only. 
Please correct me if I have failed to consider other cases.

I have attached a patch which changes the regex into:
" -\\(o[= ]?\\|-\\(?:outfile\\|output\\)[= ]\\)\\(\\S +\\)"

I have separated the regex into two cases, single dash followed by o and
optional space or equal sign, or the other case of double dash followed by
any of two long switch names and the mandatory space or equal sign.

I do not (yet) subscribe to the emacs-devel list, so please cc me on any
reply.

Regards
Jes Bodi Klinke

=== modified file 'lisp/progmodes/compile.el'
*** lisp/progmodes/compile.el   2010-09-24 03:06:33 +0000
--- lisp/progmodes/compile.el   2010-10-27 20:44:03 +0000
*** 543,549 ****
       ;; Command output lines.  Recognize `make[n]:' lines too.
       ("^\\([[:alnum:]_/.+-]+\\)\\(\\[\\([0-9]+\\)\\]\\)?[ \t]*:"
        (1 font-lock-function-name-face) (3 compilation-line-face nil t))
!      (" --?o\\(?:utfile\\|utput\\)?[= ]?\\(\\S +\\)" . 1)
       ("^Compilation \\(finished\\).*"
        (0 '(face nil message nil help-echo nil mouse-face nil) t)
        (1 compilation-info-face))
--- 543,549 ----
       ;; Command output lines.  Recognize `make[n]:' lines too.
       ("^\\([[:alnum:]_/.+-]+\\)\\(\\[\\([0-9]+\\)\\]\\)?[ \t]*:"
        (1 font-lock-function-name-face) (3 compilation-line-face nil t))
!      (" -\\(o[= ]?\\|-\\(?:outfile\\|output\\)[= ]\\)\\(\\S +\\)" . 1)
       ("^Compilation \\(finished\\).*"
        (0 '(face nil message nil help-echo nil mouse-face nil) t)
        (1 compilation-info-face))

Re: [PATCH 1/1] compile.el bug in recognizing -o switches among output

[Jes Bodi Klinke]

This is embarassing.  Apparently I was overconfident in my ability to make
some final simplifications of the regex, without needing to retest it.  I
accidentally messed up the subexpression numbering in my previous patch
(by adding an outer pair of parentheses).  The correct regex is:
" -\\(?:o[= ]?\\|-\\(?:outfile\\|output\\)[= ]\\)\\(\\S +\\)"

Please use this patch instead of my previous one.  I am very sorry for the
inconvenience, I hope that I will be able to provide better contributions
in the future.

Regards
Jes Bodi Klinke

=== modified file 'lisp/progmodes/compile.el'
*** lisp/progmodes/compile.el   2010-09-24 03:06:33 +0000
--- lisp/progmodes/compile.el   2010-10-27 21:30:57 +0000
*** 543,549 ****
       ;; Command output lines.  Recognize `make[n]:' lines too.
       ("^\\([[:alnum:]_/.+-]+\\)\\(\\[\\([0-9]+\\)\\]\\)?[ \t]*:"
        (1 font-lock-function-name-face) (3 compilation-line-face nil t))
!      (" --?o\\(?:utfile\\|utput\\)?[= ]?\\(\\S +\\)" . 1)
       ("^Compilation \\(finished\\).*"
        (0 '(face nil message nil help-echo nil mouse-face nil) t)
        (1 compilation-info-face))
--- 543,549 ----
       ;; Command output lines.  Recognize `make[n]:' lines too.
       ("^\\([[:alnum:]_/.+-]+\\)\\(\\[\\([0-9]+\\)\\]\\)?[ \t]*:"
        (1 font-lock-function-name-face) (3 compilation-line-face nil t))
!      (" -\\(?:o[= ]?\\|-\\(?:outfile\\|output\\)[= ]\\)\\(\\S +\\)" . 1)
       ("^Compilation \\(finished\\).*"
        (0 '(face nil message nil help-echo nil mouse-face nil) t)
        (1 compilation-info-face))

Re: [PATCH 1/1] compile.el bug in recognizing -o switches among

[Stefan Monnier]

> (by adding an outer pair of parentheses).  The correct regex is:
> " -\\(?:o[= ]?\\|-\\(?:outfile\\|output\\)[= ]\\)\\(\\S +\\)"

Thanks, installed.
But, could you tell me how you made your patches?
Neither diff-mode nor `patch' understand their format:

> *** lisp/progmodes/compile.el   2010-09-24 03:06:33 +0000
> --- lisp/progmodes/compile.el   2010-10-27 21:30:57 +0000
> *** 543,549 ****
>        ;; Command output lines.  Recognize `make[n]:' lines too.

Usually such context diffs have an additional "***************" line as in:

> *** lisp/progmodes/compile.el   2010-09-24 03:06:33 +0000
> --- lisp/progmodes/compile.el   2010-10-27 21:30:57 +0000
> ***************
> *** 543,549 ****
>        ;; Command output lines.  Recognize `make[n]:' lines too.


        Stefan

Re: [PATCH 1/1] compile.el bug in recognizing -o switches among

[Jes Bodi Klinke]

>> (by adding an outer pair of parentheses).  The correct regex is:
>> " -\\(?:o[= ]?\\|-\\(?:outfile\\|output\\)[= ]\\)\\(\\S +\\)"
>
> Thanks, installed.
> But, could you tell me how you made your patches?
> Neither diff-mode nor `patch' understand their format:

Thank you.  Responses like this to my feedback encourages me to spend more
time on Emacs the next time.

About the patch.  I manually edited the output from bzr diff.  I deleted
what I thought was a comment line.  It looked like bzr had tried to
extract the name of the enclosing function, but it had failed badly and
pulled out part of an unrelated text literal.  I thought that letting it
be could confuse human readers of the patch, but I guess I should have
just deleted the text to the right of the ***********, not the entire
line.

Regards
Jes Bodi Klinke

Re: [PATCH 1/1] compile.el bug in recognizing -o switches among

[Stefan Monnier]

>>> (by adding an outer pair of parentheses).  The correct regex is:
>>> " -\\(?:o[= ]?\\|-\\(?:outfile\\|output\\)[= ]\\)\\(\\S +\\)"
>> Thanks, installed.
>> But, could you tell me how you made your patches?
>> Neither diff-mode nor `patch' understand their format:
> Thank you.  Responses like this to my feedback encourages me to spend more
> time on Emacs the next time.

Thanks.

> About the patch.  I manually edited the output from bzr diff.  I deleted
> what I thought was a comment line.  It looked like bzr had tried to
> extract the name of the enclosing function, but it had failed badly and
> pulled out part of an unrelated text literal.  I thought that letting it
> be could confuse human readers of the patch, but I guess I should have
> just deleted the text to the right of the ***********, not the entire
> line.

That was not specific to Bzr but to diff in general (controlled by the
"-p" flag) which just uses the last line that starts in column 0 as
a very simple heuristic.  So "patch" knows to ignore whatever follows
the ***************, and people used to reading context diffs also know
to ignore it.

FWIW, I usually prefer the unified context diff format (not that it
makes much difference in this case since it can also have this extra
"show function line" text).


        Stefan