bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Dmitry Gutov]

And fill it in dynamically during expansion.

This way, the caller can set grep-highlight-matches dynamically, without
recomputing grep defaults (which is expensive). This is particularly
relevant on MS-Windows, in semantic-symref-grep-use-template and
xref-collect-matches.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 04 Jun 2015 02:42:58 +0300
> 
> And fill it in dynamically during expansion.
> 
> This way, the caller can set grep-highlight-matches dynamically, without
> recomputing grep defaults (which is expensive). This is particularly
> relevant on MS-Windows, in semantic-symref-grep-use-template and
> xref-collect-matches.

I don't understand what you perceive as a problem with the current
code, and why.  Could you elaborate, and perhaps provide an example?

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Dmitry Gutov]

On 06/04/2015 05:43 AM, Eli Zaretskii wrote:

> I don't understand what you perceive as a problem with the current
> code, and why.

The fact that we have to do a replace-regexp-in-string in 
xref-collect-matches, as well as in semantic-symref-grep-use-template.

Whereas we should be able to simply bind grep-highlight-matches to nil 
before calling grep-expand-template.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Eli Zaretskii]

> Cc: 20728@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 4 Jun 2015 11:58:18 +0300
> 
> On 06/04/2015 05:43 AM, Eli Zaretskii wrote:
> 
> > I don't understand what you perceive as a problem with the current
> > code, and why.
> 
> The fact that we have to do a replace-regexp-in-string in 
> xref-collect-matches, as well as in semantic-symref-grep-use-template.
> 
> Whereas we should be able to simply bind grep-highlight-matches to nil 
> before calling grep-expand-template.

Why not allow users of grep.el bind grep-highlight-matches to
whatever, including nil, and make grep.el obey that?  Using a template
for such a simple job sounds like over-engineering to me.  I can
understand why the template is used in find-grep and related commands:
the 'find' or 'xargs' command line is somewhat tricky.  But there's no
such problem with 'grep', so I'm unsure why we want to use a template
there, let alone have a placeholder for that switch in the template.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Dmitry Gutov]

On 06/04/2015 06:01 PM, Eli Zaretskii wrote:

> Why not allow users of grep.el bind grep-highlight-matches to
> whatever, including nil, and make grep.el obey that?

That would be the effect. rgrep (and zrgrep) call grep-expand-template 
to create the command to invoke.

> Using a template
> for such a simple job sounds like over-engineering to me.  I can
> understand why the template is used in find-grep and related commands:
> the 'find' or 'xargs' command line is somewhat tricky.

find-grep is what this issue is about.

> But there's no
> such problem with 'grep', so I'm unsure why we want to use a template
> there, let alone have a placeholder for that switch in the template.

The behavior of M-x grep is beyond my concern. But how would you 
implement automatic support for that variable in it, if not using a 
template?

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Eli Zaretskii]

> Cc: 20728@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 4 Jun 2015 18:18:55 +0300
> 
> how would you implement automatic support for that variable in it,
> if not using a template?

  (cond
    ((eq grep-highlight-matches 'never)
      "--color=never")
    ((eq grep-highlight-matches 'auto)
      "--color=auto"

etc., you get the idea.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Dmitry Gutov]

On 06/04/2015 06:34 PM, Eli Zaretskii wrote:

>    (cond
>      ((eq grep-highlight-matches 'never)
>        "--color=never")
>      ((eq grep-highlight-matches 'auto)
>        "--color=auto"
>
> etc., you get the idea.

And how would the resulting value be used, without a template?

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Eli Zaretskii]

> Cc: 20728@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 4 Jun 2015 18:36:20 +0300
> 
> On 06/04/2015 06:34 PM, Eli Zaretskii wrote:
> 
> >    (cond
> >      ((eq grep-highlight-matches 'never)
> >        "--color=never")
> >      ((eq grep-highlight-matches 'auto)
> >        "--color=auto"
> >
> > etc., you get the idea.
> 
> And how would the resulting value be used, without a template?

By plugging it directly into the command.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Dmitry Gutov]

On 06/04/2015 06:39 PM, Eli Zaretskii wrote:

> By plugging it directly into the command.

Without a placeholder? How?

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Eli Zaretskii]

> Cc: 20728@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 4 Jun 2015 19:55:19 +0300
> 
> On 06/04/2015 06:39 PM, Eli Zaretskii wrote:
> 
> > By plugging it directly into the command.
> 
> Without a placeholder? How?

With a placeholder that is %s, like we do with all the rest when we
format a string.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Dmitry Gutov]

On 06/04/2015 08:16 PM, Eli Zaretskii wrote:

> With a placeholder that is %s, like we do with all the rest when we
> format a string.

Then it'll also be a template. With a downside of using different 
placeholder than the other templates in grep.el.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Eli Zaretskii]

> Cc: 20728@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 4 Jun 2015 20:46:18 +0300
> 
> On 06/04/2015 08:16 PM, Eli Zaretskii wrote:
> 
> > With a placeholder that is %s, like we do with all the rest when we
> > format a string.
> 
> Then it'll also be a template. With a downside of using different 
> placeholder than the other templates in grep.el.

I see a lot of %s in grep.el, so it's not like this is a precedent
that we didn't have before.  That's the usual way we construct
variable-form commands in Emacs.  Why should this one be different?

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Dmitry Gutov]

On 06/04/2015 10:23 PM, Eli Zaretskii wrote:

> I see a lot of %s in grep.el, so it's not like this is a precedent
> that we didn't have before.

Try to look for them outside of grep-compute-defaults definition.

> That's the usual way we construct
> variable-form commands in Emacs.  Why should this one be different?

Have you looked at the values of grep-template and grep-find-template?

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Eli Zaretskii]

> Cc: 20728@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 4 Jun 2015 23:20:31 +0300
> 
> On 06/04/2015 10:23 PM, Eli Zaretskii wrote:
> 
> > I see a lot of %s in grep.el, so it's not like this is a precedent
> > that we didn't have before.
> 
> Try to look for them outside of grep-compute-defaults definition.

Please try to be less cryptic: what should I look for, and how?

> > That's the usual way we construct
> > variable-form commands in Emacs.  Why should this one be different?
> 
> Have you looked at the values of grep-template and grep-find-template?

Yes.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Dmitry Gutov]

On 06/05/2015 09:58 AM, Eli Zaretskii wrote:

> Please try to be less cryptic: what should I look for, and how?

There are no occurrences of '%s' in grep.el outside of 
grep-compute-defaults.

>> Have you looked at the values of grep-template and grep-find-template?
>
> Yes.

That is the way we construct variable-form commands in grep.el.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Eli Zaretskii]

> Cc: 20728@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 5 Jun 2015 11:29:23 +0300
> 
> On 06/05/2015 09:58 AM, Eli Zaretskii wrote:
> 
> > Please try to be less cryptic: what should I look for, and how?
> 
> There are no occurrences of '%s' in grep.el outside of 
> grep-compute-defaults.

I'm suggesting to add one.

> >> Have you looked at the values of grep-template and grep-find-template?
> >
> > Yes.
> 
> That is the way we construct variable-form commands in grep.el.

I'm suggesting to change that, for this specific issue.  We do that in
any number of other packages where a shell command needs to be
dynamically calculated guided by options.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Dmitry Gutov]

On 06/05/2015 11:59 AM, Eli Zaretskii wrote:

> I'm suggesting to change that, for this specific issue.  We do that in
> any number of other packages where a shell command needs to be
> dynamically calculated guided by options.

I fail to understand what would be the benefit of that. We already have 
templates anyway.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Eli Zaretskii]

> Cc: 20728@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 5 Jun 2015 12:07:26 +0300
> 
> On 06/05/2015 11:59 AM, Eli Zaretskii wrote:
> 
> > I'm suggesting to change that, for this specific issue.  We do that in
> > any number of other packages where a shell command needs to be
> > dynamically calculated guided by options.
> 
> I fail to understand what would be the benefit of that.

IMO, it's much simpler, and doesn't require to invent a new
placeholder for such a simple job.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Dmitry Gutov]

On 06/05/2015 12:23 PM, Eli Zaretskii wrote:

> IMO, it's much simpler, and doesn't require to invent a new
> placeholder for such a simple job.

We'll still need that placeholder in grep-find-template (and probably in 
grep-template, for consistency).

Regarding M-x grep, I would rather leave it alone for now, because 
otherwise, %s or not, that would call for renaming the variable 
grep-command, and grep-template is already taken.

Another option would be to perform the substitution with regexp in 
grep-default-command, and it's pretty hairy already.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Juri Linkov]

> We'll still need that placeholder in grep-find-template (and probably in
> grep-template, for consistency).
>
> Regarding M-x grep, I would rather leave it alone for now, because
> otherwise, %s or not, that would call for renaming the variable
> grep-command, and grep-template is already taken.

There is also grep-find-command used by grep-find.

> Another option would be to perform the substitution with regexp in
> grep-default-command, and it's pretty hairy already.

Since there is a lot of %s-substitutions in grep-compute-defaults
that produce command lines depending on many customizations,
yet another option is to let-bind the variables grep-command,
grep-find-command, grep-template, grep-find-template in your function,
then call grep-compute-defaults with a new value of grep-highlight-matches,
and then get new command lines without the --color option from
these local bindings to run them and parse the output programmatically.

In case when users customize grep-highlight-matches interactively,
its defcustom should compute new command lines using grep-compute-defaults.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Dmitry Gutov]

On 06/06/2015 01:17 AM, Juri Linkov wrote:

> There is also grep-find-command used by grep-find.

Indeed. That one I'd rather leave alone too.

> Since there is a lot of %s-substitutions in grep-compute-defaults
> that produce command lines depending on many customizations,

Not that many. grep-program and grep-highlight-matches seem to be the 
only ones a user might want to change. Or rather, a user might change 
the former, and some function might want to change either.

> yet another option is to let-bind the variables grep-command,
> grep-find-command, grep-template, grep-find-template in your function,
> then call grep-compute-defaults with a new value of grep-highlight-matches,
> and then get new command lines without the --color option from
> these local bindings to run them and parse the output programmatically.

That's what zrgrep does, and its quite clunky. And if we have code to 
pre-compute commands and templates (which takes several external program 
calls), it's kind of silly to redo that again each time certain commands 
are called.

Do you know if zrgrep has a good reason for it? Like, it there are 
platforms where we have to use different calling conventions for grep 
and zgrep? Otherwise, we could simply substitute grep-program value in 
the commands.

> In case when users customize grep-highlight-matches interactively,
> its defcustom should compute new command lines using grep-compute-defaults.

Do you think, overall, it will be the better approach?

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Juri Linkov]

> That's what zrgrep does, and its quite clunky. And if we have code to
> pre-compute commands and templates (which takes several external program
> calls), it's kind of silly to redo that again each time certain commands
> are called.

Thanks for mentioning zrgrep!  I noticed that it's broken now and fixed
with the patch that let-binds grep-highlight-matches before calling
grep-compute-defaults since now it's computed here.

> Do you know if zrgrep has a good reason for it? Like, it there are
> platforms where we have to use different calling conventions for grep and
> zgrep? Otherwise, we could simply substitute grep-program value in
> the commands.

String replacement is too unreliable approach.  But I see no problem with
both zrgrep and the code where you want to parse the output programmatically,
to remember the computed command lines in defvar variables such as
zgrep-find-command, zgrep-find-template, etc. to not compute them every
time the command is called.

>> In case when users customize grep-highlight-matches interactively,
>> its defcustom should compute new command lines using grep-compute-defaults.
>
> Do you think, overall, it will be the better approach?

Well, there are two levels of parametrization in grep.el:

1. grep-compute-defaults uses %s-substitutions to prepare
   command line templates with placeholders.

2. templates are expanded on every command call with regexps and files.

When parameters don't vary between command calls (as regexps and files do)
then I think it's better to prepare them in the command line templates.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Dmitry Gutov]

On 06/07/2015 01:04 AM, Juri Linkov wrote:

> Thanks for mentioning zrgrep!  I noticed that it's broken now and fixed
> with the patch that let-binds grep-highlight-matches before calling
> grep-compute-defaults since now it's computed here.

No problem. Could you explain what those filters are, the ones the 
comment says zgrep puts into output?

> String replacement is too unreliable approach.

I meant template substitution, like in grep-expand-template.

Not sure what to do about grep-default-command, but if leaving it as-is 
is not an option, we can reasonably decide that the grep-command was the 
symbol at the beginning of the previous command.

> But I see no problem with
> both zrgrep and the code where you want to parse the output programmatically,
> to remember the computed command lines in defvar variables such as
> zgrep-find-command, zgrep-find-template, etc. to not compute them every
> time the command is called.

Again, why have zgrep-find-template, when grep-find-template could have 
a (new) placeholder for grep-command? Do zgrep and grep take different 
options?

> When parameters don't vary between command calls (as regexps and files do)
> then I think it's better to prepare them in the command line templates.

Since grep-command and grep-highlight-matches can vary between calls, 
should we make a template placeholder for them?

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Juri Linkov]

> No problem. Could you explain what those filters are, the ones the comment
> says zgrep puts into output?

These are pipe filters.  You can see how complex is the command line
constructed in /bin/zgrep

> I meant template substitution, like in grep-expand-template.
>
> Not sure what to do about grep-default-command, but if leaving it as-is is
> not an option, we can reasonably decide that the grep-command was the
> symbol at the beginning of the previous command.

Then you want two additional placeholders: for the command name and options?

>> But I see no problem with
>> both zrgrep and the code where you want to parse the output programmatically,
>> to remember the computed command lines in defvar variables such as
>> zgrep-find-command, zgrep-find-template, etc. to not compute them every
>> time the command is called.
>
> Again, why have zgrep-find-template, when grep-find-template could have
> a (new) placeholder for grep-command? Do zgrep and grep take
> different options?

In an older version of /bin/zgrep I see the text "OPTIONs are the same as for 'grep'."
I don't know about other versions.

>> When parameters don't vary between command calls (as regexps and files do)
>> then I think it's better to prepare them in the command line templates.
>
> Since grep-command and grep-highlight-matches can vary between calls,
> should we make a template placeholder for them?

There are only two possible values for grep-highlight-matches
whereas the number of possible values of the current placeholders
for regexps and filenames is infinite.

I think the rule should be the following: placeholders are needed only for
parameters provided by users, but for internal implementation parameters
it's enough to pre-compute command lines (and cache them).

PS: Somehow reminds me of endless discussions about distinctions
between `error' and `user-error' :)

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Dmitry Gutov]

On 06/10/2015 02:32 AM, Juri Linkov wrote:

> These are pipe filters.  You can see how complex is the command line
> constructed in /bin/zgrep

I see, thanks. Apparently, the upshot is that --color=auto doesn't work.

The nil value of grep-highlight-matches should still work, though.

> Then you want two additional placeholders: for the command name and options?

Yep!

By the way, <C> is already used to conditionally pass '-i'. We can reuse 
it to pass one more option. So, minus one new placeholder.

> In an older version of /bin/zgrep I see the text "OPTIONs are the same as for 'grep'."
> I don't know about other versions.

Let's try it, then.

> There are only two possible values for grep-highlight-matches
> whereas the number of possible values of the current placeholders
> for regexps and filenames is infinite.

Right. That means that you can't precompute regexps and filenames, at 
all. That does not, however, mean that you *have to* precompute 
everything else.

> I think the rule should be the following: placeholders are needed only for
> parameters provided by users, but for internal implementation parameters
> it's enough to pre-compute command lines (and cache them).

Even if "internal" parameters vary between commands that use the same 
template?

I disagree. If we can expand those parameters dynamically, we should.

By the way, `grep-highlight-matches' is an option that the user can 
change via Customize, but the setter only calls `grep-apply-setting'.

> PS: Somehow reminds me of endless discussions about distinctions
> between `error' and `user-error' :)

I don't remember, but `user-error' won, hasn't it? The main question now 
regarding it, is find and change all applicable uses of `error'.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Juri Linkov]

> By the way, <C> is already used to conditionally pass '-i'. We can reuse it
> to pass one more option. So, minus one new placeholder.

With the mnemonics <C> = <Color> :)

OTOH, null-device has a separate placeholder <N>.

>> PS: Somehow reminds me of endless discussions about distinctions
>> between `error' and `user-error' :)
>
> I don't remember, but `user-error' won, hasn't it? The main question now
> regarding it, is find and change all applicable uses of `error'.

I don't care about formal criteria of changing `error' to `user-error',
my approach is driven by practical needs like in bug#20785.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Dmitry Gutov]

On 06/11/2015 02:49 AM, Juri Linkov wrote:

> With the mnemonics <C> = <Color> :)

Rename it, then? But I'd be fine with keeping the "wrong" letter for 
compatibility with templates set by third-party code, if there's any.

> OTOH, null-device has a separate placeholder <N>.

And it seems to never be used anywhere. Again, grep-compute-defaults 
inlines the appropriate value in the templates.

Maybe we should remove the mentions of it from the docstrings.

> I don't care about formal criteria of changing `error' to `user-error',
> my approach is driven by practical needs

I think those are one and the same. :)

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Juri Linkov]

>> With the mnemonics <C> = <Color> :)
>
> Rename it, then? But I'd be fine with keeping the "wrong" letter for
> compatibility with templates set by third-party code, if there's any.

Yes, it's out of the question that we should keep the original letters.

>> OTOH, null-device has a separate placeholder <N>.
>
> And it seems to never be used anywhere. Again, grep-compute-defaults
> inlines the appropriate value in the templates.

Here again the same question: how different is null-device from
grep-highlight-matches that null-device should be inlined in the
templates by grep-compute-defaults while grep-highlight-matches shouldn't?

> Maybe we should remove the mentions of it from the docstrings.

I wonder how many third-party code uses the templates with <N>
that it needs to be documented.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Dmitry Gutov]

On 06/12/2015 02:20 AM, Juri Linkov wrote:

> Here again the same question: how different is null-device from
> grep-highlight-matches that null-device should be inlined in the
> templates by grep-compute-defaults while grep-highlight-matches shouldn't?

I wouldn't say "should", just that so far we haven't seen the need to 
substitute it in (no function in the core binds that value dynamically).

> I wonder how many third-party code uses the templates with <N>
> that it needs to be documented.

Doing a quick search on GitHub, I couldn't find any of those.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Juri Linkov]

>> I wonder how many third-party code uses the templates with <N>
>> that it needs to be documented.
>
> Doing a quick search on GitHub, I couldn't find any of those.

Is there a search on ELPA and other package repositories like
listed in (info "(efaq) Packages that do not come with Emacs")?

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Dmitry Gutov]

On 06/23/2015 01:33 AM, Juri Linkov wrote:

> Is there a search on ELPA and other package repositories like
> listed in (info "(efaq) Packages that do not come with Emacs")?

You can clone the GNU ELPA repo and rgrep for grep-template or 
grep-find-template (there are no matches).

Regarding the other repositories, there's probably nothing better we can 
do aside from doing a search on GitHub or a web search engine.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Dmitry Gutov]

Version: 25.1

On 06/12/2015 02:20 AM, Juri Linkov wrote:

> Yes, it's out of the question that we should keep the original letters.

I've just pushed a commit that reuses <C> for both -i and --color 
(<C>ase and <C>olor, we can still pretend it's mnemonical), simply 
because there are templates out there with <C> in them, which do not 
include "--color".

If anyone really wants to change it to <O>, be my guest.

> I wonder how many third-party code uses the templates with <N>
> that it needs to be documented.

I haven't found any. OTOH, it doesn't hurt anyone either. Maybe the 
right change would be to make the code use it, actually. Are there use 
cases for changing the value of null-decide dynamically?

And speaking of needs, what is the use case for "--color" without 
"always"? Instead of special-casing Windows and DOS, why don't we always 
use `always' for coloring?

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Juri Linkov]

> I've just pushed a commit that reuses <C> for both -i and --color (<C>ase
> and <C>olor, we can still pretend it's mnemonical), simply because there
> are templates out there with <C> in them, which do not include "--color".

Does zgrep still work after your changes?

> I haven't found any. OTOH, it doesn't hurt anyone either. Maybe the right
> change would be to make the code use it, actually. Are there use cases for
> changing the value of null-decide dynamically?

This is only a hypothetical case.

> And speaking of needs, what is the use case for "--color" without "always"?
> Instead of special-casing Windows and DOS, why don't we always use `always'
> for coloring?

Maybe some users don't want highlighting?

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Dmitry Gutov]

On 06/28/2015 01:49 AM, Juri Linkov wrote:

> Does zgrep still work after your changes?

It does.

> This is only a hypothetical case.

Sure.

> Maybe some users don't want highlighting?

Then they'll set grep-highlight-matches to nil, and there won't be a 
--color option in the constructed command at all.

What I'm asking is, on the other hand, why do we sometimes pass --color, 
but not --color=always?

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Dmitry Gutov]

On 06/28/2015 04:22 AM, Dmitry Gutov wrote:

>> Does zgrep still work after your changes?
>
> It does.

Sorry, spoke too soon. The highlighting is fixed now.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sun, 28 Jun 2015 00:11:34 +0300
> Cc: 20728-done@debbugs.gnu.org
> 
> And speaking of needs, what is the use case for "--color" without 
> "always"? Instead of special-casing Windows and DOS, why don't we always 
> use `always' for coloring?

Isn't it the other way around: Windows wants 'always' (except when it
doesn't, see below), while others want 'auto'?

As to when you won't want 'always': it's when Grep is invoked as part
pf a pipeline, where it's not the last part, or when the Lisp program
that invokes it wants to interpret the results, as opposed to showing
them to the user.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Dmitry Gutov]

On 06/28/2015 05:42 AM, Eli Zaretskii wrote:

> Isn't it the other way around: Windows wants 'always' (except when it
> doesn't, see below), while others want 'auto'?

When will `always' not work for the others?

> As to when you won't want 'always': it's when Grep is invoked as part
> pf a pipeline, where it's not the last part, or when the Lisp program
> that invokes it wants to interpret the results, as opposed to showing
> them to the user.

I'd want `nil' in that case, right? And I'd have to specify it 
explicitly anyway, if I want compatibility with Windows.

So, why do we use `auto'?

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Eli Zaretskii]

> Cc: 20728@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sun, 28 Jun 2015 11:05:52 +0300
> 
> On 06/28/2015 05:42 AM, Eli Zaretskii wrote:
> 
> > Isn't it the other way around: Windows wants 'always' (except when it
> > doesn't, see below), while others want 'auto'?
> 
> When will `always' not work for the others?

I answered that below:

> > As to when you won't want 'always': it's when Grep is invoked as part
> > pf a pipeline, where it's not the last part, or when the Lisp program
> > that invokes it wants to interpret the results, as opposed to showing
> > them to the user.

> I'd want `nil' in that case, right?

In which case?  And what do you mean by 'nil' in this context?  We are
talking about the value of the --color= Grep option, don't we?

> And I'd have to specify it explicitly anyway, if I want
> compatibility with Windows.

You lost me.

> So, why do we use `auto'?

My guess would be: because it mostly does what we want, AUTOmatically:
it produces SGR color sequences when Grep is run as a subprocess via a
pty, and does not produce them when Grep's output is a pipe.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Dmitry Gutov]

On 06/28/2015 05:35 PM, Eli Zaretskii wrote:

>>> Isn't it the other way around: Windows wants 'always' (except when it
>>> doesn't, see below), while others want 'auto'?
>>
>> When will `always' not work for the others?
>
> I answered that below:

No: when will a given command's behavior on other operating systems be 
wrong if it was just written to work on Windows (and thus uses either 
`always' or nil, not `auto')?

>>> As to when you won't want 'always': it's when Grep is invoked as part
>>> pf a pipeline, where it's not the last part,

Do we have any such commands? If so, would it hurt them to have to bind 
`grep-highlight-matches' to nil?

>>> or when the Lisp program
>>> that invokes it wants to interpret the results, as opposed to showing
>>> them to the user.

Again, any such command will need to bind `grep-highlight-matches' to 
nil, to work on Windows. Or neutralize some other way the value `always' 
that it gets set to automatically, on Windows and DOS.

>> I'd want `nil' in that case, right?
>
> In which case?  And what do you mean by 'nil' in this context?  We are
> talking about the value of the --color= Grep option, don't we?

In either case: when the pipeline ends with something other than Grep, 
or when we'll process the output programmatically, and don't need the 
ANSI codes.

I'm talking about the value of `grep-highlight-matches'.

>> And I'd have to specify it explicitly anyway, if I want
>> compatibility with Windows.
>
> You lost me.

See above.

>> So, why do we use `auto'?
>
> My guess would be: because it mostly does what we want, AUTOmatically:
> it produces SGR color sequences when Grep is run as a subprocess via a
> pty, and does not produce them when Grep's output is a pipe.

Do we even have Grep-related commands that use a pipeline that doesn't 
end with Grep?

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Eli Zaretskii]

> Cc: 20728@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sun, 28 Jun 2015 22:02:21 +0300
> 
> On 06/28/2015 05:35 PM, Eli Zaretskii wrote:
> 
> >>> Isn't it the other way around: Windows wants 'always' (except when it
> >>> doesn't, see below), while others want 'auto'?
> >>
> >> When will `always' not work for the others?
> >
> > I answered that below:
> 
> No: when will a given command's behavior on other operating systems be 
> wrong if it was just written to work on Windows (and thus uses either 
> `always' or nil, not `auto')?

See cedet/semantic/symref/grep.el for when we don't want 'always'.

> Do we even have Grep-related commands that use a pipeline that doesn't 
> end with Grep?

I don't know.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Dmitry Gutov]

On 06/28/2015 11:28 PM, Eli Zaretskii wrote:

> See cedet/semantic/symref/grep.el for when we don't want 'always'.

We don't want `auto' there either, do we?

>> Do we even have Grep-related commands that use a pipeline that doesn't
>> end with Grep?
>
> I don't know.

So I propose to eliminate the value `auto'. It seems we only had it as a 
maybe-solution for third-party code not to have to recompute the Grep 
defaults with grep-highlight-matches set to nil.

But now we can simply bind it to nil.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Eli Zaretskii]

> Cc: 20728@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sun, 28 Jun 2015 23:31:30 +0300
> 
> On 06/28/2015 11:28 PM, Eli Zaretskii wrote:
> 
> > See cedet/semantic/symref/grep.el for when we don't want 'always'.
> 
> We don't want `auto' there either, do we?

I think 'auto' does TRT there.

> >> Do we even have Grep-related commands that use a pipeline that doesn't
> >> end with Grep?
> >
> > I don't know.
> 
> So I propose to eliminate the value `auto'. It seems we only had it as a 
> maybe-solution for third-party code not to have to recompute the Grep 
> defaults with grep-highlight-matches set to nil.
> 
> But now we can simply bind it to nil.

We could, if we want to test Grep's output ourselves.  That is, we
will have to decide when to bind it to nil and when to non-nil,
depending on the device/file used as Grep's output (which is
system-dependent).

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Dmitry Gutov]

On 06/29/2015 05:36 AM, Eli Zaretskii wrote:

>>> See cedet/semantic/symref/grep.el for when we don't want 'always'.
>>
>> We don't want `auto' there either, do we?
>
> I think 'auto' does TRT there.

In any case, we have to explicitly get rid of `always' in this command.

>> But now we can simply bind it to nil.
>
> We could, if we want to test Grep's output ourselves.  That is, we
> will have to decide when to bind it to nil and when to non-nil,

What do you mean by "test"?

My point is, we have to decide whether to highlight matches or not, if 
only to be compatible with Windows. And by "we", I mean in the 
implementation of each given command.

> depending on the device/file used as Grep's output (which is
> system-dependent).

Whether we want a given command to use a colorized Grep output, is not 
dependent on the OS, is it?

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Eli Zaretskii]

> Cc: 20728@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Mon, 29 Jun 2015 13:08:48 +0300
> 
> On 06/29/2015 05:36 AM, Eli Zaretskii wrote:
> 
> >>> See cedet/semantic/symref/grep.el for when we don't want 'always'.
> >>
> >> We don't want `auto' there either, do we?
> >
> > I think 'auto' does TRT there.
> 
> In any case, we have to explicitly get rid of `always' in this command.

Obviously.  Not sure what is the significance of this conclusion,
though.  But then I don't understand where does this discussion go
anyway.

> >> But now we can simply bind it to nil.
> >
> > We could, if we want to test Grep's output ourselves.  That is, we
> > will have to decide when to bind it to nil and when to non-nil,
> 
> What do you mean by "test"?

"Examine".  The behavior with pipes, files, consoles, and ptys is
different.

> My point is, we have to decide whether to highlight matches or not, if 
> only to be compatible with Windows. And by "we", I mean in the 
> implementation of each given command.

Yes, the alternative is to do everything in Lisp, and then use only
"--color=no" or "--color=always", as appropriate.

> > depending on the device/file used as Grep's output (which is
> > system-dependent).
> 
> Whether we want a given command to use a colorized Grep output, is not 
> dependent on the OS, is it?

I don't think so.  But it might depend on the values of grep-program
etc., i.e. on user customizations.  Users might want to put shell
scripts or pipelines there.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Dmitry Gutov]

On 06/29/2015 05:49 PM, Eli Zaretskii wrote:

>>> We could, if we want to test Grep's output ourselves.  That is, we
>>> will have to decide when to bind it to nil and when to non-nil,
>>
>> What do you mean by "test"?
>
> "Examine".  The behavior with pipes, files, consoles, and ptys is
> different.

I don't understand this. Grep's output happens after we bind, or not 
bind, this variable. So we can't test in beforehand (or do you mean in 
`grep-compute-defaults'?).

>> My point is, we have to decide whether to highlight matches or not, if
>> only to be compatible with Windows. And by "we", I mean in the
>> implementation of each given command.
>
> Yes, the alternative is to do everything in Lisp, and then use only
> "--color=no" or "--color=always", as appropriate.

That's what I'm proposing. This seems simpler and less error-prone. So 
this discussion is about the possible downsides.

>> Whether we want a given command to use a colorized Grep output, is not
>> dependent on the OS, is it?
>
> I don't think so.  But it might depend on the values of grep-program
> etc., i.e. on user customizations.  Users might want to put shell
> scripts or pipelines there.

Well, okay. If it's a significant use case, we might want to continue 
supporting it.

On the other hand, we might want to choose to set 
`grep-highlight-matches' to `always' in grep-compute-defaults, 
irrespective of the OS.

So that every command that knows it does not need the ANSI codes, will 
bind this variable to nil without relying on auto-detection thanks to 
calling Grep via a pipe. Then we won't have to test on Windows to find 
bugs like that.

And if a user customized grep-program, etc, to unexpectedly postprocess 
Grep ouput using a pipe, they can customize `grep-highlight-matches' to 
`auto' as well.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Eli Zaretskii]

> Cc: 20728@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Mon, 29 Jun 2015 18:01:58 +0300
> 
> >> Whether we want a given command to use a colorized Grep output, is not
> >> dependent on the OS, is it?
> >
> > I don't think so.  But it might depend on the values of grep-program
> > etc., i.e. on user customizations.  Users might want to put shell
> > scripts or pipelines there.
> 
> Well, okay. If it's a significant use case, we might want to continue 
> supporting it.

I don't know if it's a significant use case.  Maybe it isn't, and we
should just go ahead and do what you suggest.

> On the other hand, we might want to choose to set 
> `grep-highlight-matches' to `always' in grep-compute-defaults, 
> irrespective of the OS.

Won't that bring back the problem in symref/grep.el again?

> So that every command that knows it does not need the ANSI codes, will 
> bind this variable to nil without relying on auto-detection thanks to 
> calling Grep via a pipe. Then we won't have to test on Windows to find 
> bugs like that.

Assuming we can count on commands that live far away, like
symref/grep.el, to make a point of doing this, it will work, of
course.

> And if a user customized grep-program, etc, to unexpectedly postprocess 
> Grep ouput using a pipe, they can customize `grep-highlight-matches' to 
> `auto' as well.

Yes, given enough documentation that mentions this.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Dmitry Gutov]

On 06/29/2015 06:26 PM, Eli Zaretskii wrote:

>> On the other hand, we might want to choose to set
>> `grep-highlight-matches' to `always' in grep-compute-defaults,
>> irrespective of the OS.
>
> Won't that bring back the problem in symref/grep.el again?

Why would it?

The replace (replace-regexp-in-string "--color=always" ...) call is 
still there. I didn't change that because CEDET supposedly needs to 
retain compatibility with previous Emacs versions.

But there is a similar function: xref-collect-matches.

Looking at it now, I forgot to apply a relevant fix to it (and this is 
an example of a Lisp code author doing the wrong thing because `auto' 
happened to make things right anyway, on my machine). So could you:

- Try M-x xref-find-regexp on a Windows machine (in an Elisp buffer, 
enter something like "xref-find-").
- Probably see it fail for the same reason as symref/grep.el did previously.
- Apply the patch below.
- Hopefully see it succeed and write back about that.

 > Assuming we can count on commands that live far away, like
 > symref/grep.el, to make a point of doing this, it will work, of
 > course.

The idea is to force their authors to do it (because the commands won't 
work otherwise).

Patch:

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 50d52d0..bb2546f 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -749,8 +749,10 @@ tools are used, and when."
    (require 'semantic/fw)
    (grep-compute-defaults)
    (defvar grep-find-template)
+  (defvar grep-highlight-matches)
    (let* ((grep-find-template (replace-regexp-in-string "-e " "-E "
 
grep-find-template t t))
+         (grep-highlight-matches nil)
           (command (rgrep-default-command (xref--regexp-to-extended regexp)
                                           "*.*" dir))
           (orig-buffers (buffer-list))

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Eli Zaretskii]

> Cc: 20728@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Mon, 29 Jun 2015 19:11:19 +0300
> 
> On 06/29/2015 06:26 PM, Eli Zaretskii wrote:
> 
> >> On the other hand, we might want to choose to set
> >> `grep-highlight-matches' to `always' in grep-compute-defaults,
> >> irrespective of the OS.
> >
> > Won't that bring back the problem in symref/grep.el again?
> 
> Why would it?
> 
> The replace (replace-regexp-in-string "--color=always" ...) call is 
> still there.

Ah, OK.  I thought you intended to remove it as part of this.

> - Try M-x xref-find-regexp on a Windows machine (in an Elisp buffer, 
> enter something like "xref-find-").
> - Probably see it fail for the same reason as symref/grep.el did previously.
> - Apply the patch below.
> - Hopefully see it succeed and write back about that.

Will do, thanks.

>  > Assuming we can count on commands that live far away, like
>  > symref/grep.el, to make a point of doing this, it will work, of
>  > course.
> 
> The idea is to force their authors to do it (because the commands won't 
> work otherwise).

Assuming those commands will be used on platforms that reveal the
failures...

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Dmitry Gutov]

On 06/29/2015 07:24 PM, Eli Zaretskii wrote:

>> The replace (replace-regexp-in-string "--color=always" ...) call is
>> still there.
>
> Ah, OK.  I thought you intended to remove it as part of this.

I hope to be able to do that at some point in the future.

> Assuming those commands will be used on platforms that reveal the
> failures...

With --color=always? IIUC, that will be most of the platforms we support.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Eli Zaretskii]

> Date: Mon, 29 Jun 2015 19:24:08 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 20728@debbugs.gnu.org
> 
> > - Try M-x xref-find-regexp on a Windows machine (in an Elisp buffer, 
> > enter something like "xref-find-").
> > - Probably see it fail for the same reason as symref/grep.el did previously.
> > - Apply the patch below.
> > - Hopefully see it succeed and write back about that.
> 
> Will do, thanks.

Sorry for such a long delay: life intervened, and then I simply
forgot.

Yes, indeed, that patch is needed to make it work on MS-Windows.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Dmitry Gutov]

On 07/11/2015 04:58 PM, Eli Zaretskii wrote:

> Yes, indeed, that patch is needed to make it work on MS-Windows.

Thanks for checking, installed.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Dmitry Gutov]

On 06/28/2015 11:28 PM, Eli Zaretskii wrote:

>> No: when will a given command's behavior on other operating systems be
>> wrong if it was just written to work on Windows (and thus uses either
>> `always' or nil, not `auto')?
>
> See cedet/semantic/symref/grep.el for when we don't want 'always'.

This doesn't answer my question, by the way. Would you like me to try to 
rephrase it?

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Eli Zaretskii]

> Cc: 20728@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Mon, 29 Jun 2015 13:05:39 +0300
> 
> On 06/28/2015 11:28 PM, Eli Zaretskii wrote:
> 
> >> No: when will a given command's behavior on other operating systems be
> >> wrong if it was just written to work on Windows (and thus uses either
> >> `always' or nil, not `auto')?
> >
> > See cedet/semantic/symref/grep.el for when we don't want 'always'.
> 
> This doesn't answer my question, by the way. Would you like me to try to 
> rephrase it?

Do you still want to see it answered?

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Dmitry Gutov]

On 06/29/2015 05:44 PM, Eli Zaretskii wrote:

>> This doesn't answer my question, by the way. Would you like me to try to
>> rephrase it?
>
> Do you still want to see it answered?

I'm assuming this is a rhetoric question.

So: the value `auto' behaves as `nil' on Windows, so it's unreliable if 
a given command supports highlighting of the matches.

Why don't we write each command's implementation as if we're targeting 
Windows? That would mean never using the value `auto', and yes, 
explicitly binding grep-highlight-matches to nil in every command that 
doesn't want `always'.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Eli Zaretskii]

> Cc: 20728@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Mon, 29 Jun 2015 17:50:34 +0300
> 
> On 06/29/2015 05:44 PM, Eli Zaretskii wrote:
> 
> >> This doesn't answer my question, by the way. Would you like me to try to
> >> rephrase it?
> >
> > Do you still want to see it answered?
> 
> I'm assuming this is a rhetoric question.

I assumed so was yours.

> So: the value `auto' behaves as `nil' on Windows, so it's unreliable if 
> a given command supports highlighting of the matches.

No, 'auto' does NOT behave as 'nil' on Windows, not in general.  It
only behaves as 'nil' when Grep is invoked as an async subprocess,
because Windows uses pipes, not ptys, to do that.  Any system where
'process-connection-type' is 'nil' will exhibit this behavior.
Moreover, even on systems that do support ptys, Emacs will use pipes
if "all ptys are busy" (citing the ELisp manual).

> Why don't we write each command's implementation as if we're targeting 
> Windows? That would mean never using the value `auto', and yes, 
> explicitly binding grep-highlight-matches to nil in every command that 
> doesn't want `always'.

We could, I just don't know if we want to.  But I don't object to such
a change.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Juri Linkov]

> And fill it in dynamically during expansion.
>
> This way, the caller can set grep-highlight-matches dynamically, without
> recomputing grep defaults (which is expensive). This is particularly
> relevant on MS-Windows, in semantic-symref-grep-use-template and
> xref-collect-matches.

I guess you need reusing the grep command line for purposes
other than highlighting its matches in the output buffer, right?
Then indeed since on Windows grep fails `isatty' detection
we had to use "--color=always" instead of "--color=auto".
Some time ago additional grep options (including "--color=auto")
were set in env GREP_OPTIONS were they could be easily changed,
but not anymore after 2e4c2fe2787785a421f256541de642976e9bd90b.
So maybe now we should add a place holder for grep options
in the template and use it the same way as we used GREP_OPTIONS.

bug#20728: 25.0.50; grep and grep-find templates should have a place holder for the --color argument

[Dmitry Gutov]

On 06/05/2015 01:50 AM, Juri Linkov wrote:

> I guess you need reusing the grep command line for purposes
> other than highlighting its matches in the output buffer, right?

Yep, to run it and parse the output programmatically.

> Some time ago additional grep options (including "--color=auto")
> were set in env GREP_OPTIONS were they could be easily changed,
> but not anymore after 2e4c2fe2787785a421f256541de642976e9bd90b.

Thanks for the background.

> So maybe now we should add a place holder for grep options
> in the template and use it the same way as we used GREP_OPTIONS.

Indeed. I'm inclined toward <O>, for "additional Options".