This is an archive of the discontinued Mercurial Phabricator instance.

Differential D3919

grep: restore pre-9ef10437bb88 behavior, enable wdir search by tweakdefaults
ClosedPublic

Authored by yuja on Jul 11 2018, 10:15 AM.

Details

Reviewers
sangeet259
pulkit
Group Reviewers
hg-reviewers
Commits
rHGbfcd5c7cbf9a: grep: restore pre-9ef10437bb88 behavior, enable wdir search by tweakdefaults
Summary

Unfortunately, python-hglib relies on the original grep behavior and is
documented as such. Even though we agreed to introduce the BC, we shouldn't
break existing libraries.

So this patch flips the default again and move the new default to
ui.tweakdefaults. We could instead use HGPLAIN to turn this flag off, but
that would be rather confusing as the old/new behaviors are quite different.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

yuja created this revision.Jul 11 2018, 10:15 AM
pulkit accepted this revision.Jul 11 2018, 1:39 PM
sangeet259 added a comment.Jul 11 2018, 1:44 PM

So what is the way around this, does this mean grep can't be changed at all
?
Also can't we update hglib to work accordingly.

pulkit added a comment.Jul 11 2018, 1:54 PM
In D3919#61156, @sangeet259 wrote:

So what is the way around this, does this mean grep can't be changed at all?
Also can't we update hglib to work accordingly.

I don't know enough about hglib, so can't say about that.

Reason I am accepting this patch is that the current fixing of hg grep is a work in progress and we have not tested that a lot in real life. We should change the deafult once we are fullly confident. Since this is close to release, let's hide the work behind a config option.

Closed by commit rHGbfcd5c7cbf9a: grep: restore pre-9ef10437bb88 behavior, enable wdir search by tweakdefaults (authored by yuja). · Explain WhyJul 11 2018, 2:11 PM
This revision was automatically updated to reflect the committed changes.
durin42 added a subscriber: durin42.Jul 11 2018, 2:21 PM

I think it would make sense to defer the behavior change until we test some more. That said, I do want us to plan to make an intentionally breaking change with the out of the box grep experience *without* tweakdefaults enabled. My reasoning is more or less this: approximately nobody uses hg grep today[0] because its behavior doesn't match what users expect out of the box. I'm willing to call the current behavior a *bug*, and prominently announce that with 4.8 we expect to change the default behavior of hg grep to match user expectations. That's in line with the decision outlined in [1] which is old enough that mpm was part of the decision. I strongly suspect hglib will be the only meaningful breakage, and we can patch hglib to pass the -r 0:tip flag that'll give consistent behavior across all hg versions.

How do we feel about that?

0: Anecdata: it's been run 25 times at work in the last 30 days over our entire userbase, which includes a number of extremely advanced users, and has been renamed to histgrep at Facebook for at least 4 years.
1: https://www.mercurial-scm.org/wiki/GrepPlan

pulkit added a comment.Jul 11 2018, 2:43 PM
In D3919#61245, @durin42 wrote:

I think it would make sense to defer the behavior change until we test some more. That said, I do want us to plan to make an intentionally breaking change with the out of the box grep experience *without* tweakdefaults enabled. My reasoning is more or less this: approximately nobody uses hg grep today[0] because its behavior doesn't match what users expect out of the box. I'm willing to call the current behavior a *bug*, and prominently announce that with 4.8 we expect to change the default behavior of hg grep to match user expectations. That's in line with the decision outlined in [1] which is old enough that mpm was part of the decision. I strongly suspect hglib will be the only meaningful breakage, and we can patch hglib to pass the -r 0:tip flag that'll give consistent behavior across all hg versions.
How do we feel about that?

A big +1 from my side on that.

0: Anecdata: it's been run 25 times at work in the last 30 days over our entire userbase, which includes a number of extremely advanced users, and has been renamed to histgrep at Facebook for at least 4 years.
1: https://www.mercurial-scm.org/wiki/GrepPlan

yuja added a comment.Jul 12 2018, 9:30 AM
I think it would make sense to defer the behavior change until we test some more. That said, I do want us to plan to make an intentionally breaking change with the out of the box grep experience *without* `tweakdefaults` enabled. My reasoning is more or less this: approximately nobody uses `hg grep` today[0] because its behavior doesn't match what users expect out of the box. I'm willing to call the current behavior a *bug*, and prominently announce that with 4.8 we expect to change the default behavior of `hg grep` to match user expectations. That's in line with the decision outlined in [1] which is old enough that mpm was part of the decision. I strongly suspect hglib will be the only meaningful breakage, and we can patch hglib to pass the -r 0:tip flag that'll give consistent behavior across all hg versions.

If we can take the current hg grep behavior as a bug, we can say the
same thing for python-hglib. I don't want to make hglib grep diverged from
hg one since hglib is a thin wrapper.

durin42 added a comment.Jul 13 2018, 7:05 AM

If we can take the current hg grep behavior as a bug, we can say the
same thing for python-hglib. I don't want to make hglib grep diverged from
hg one since hglib is a thin wrapper.

That's not a bad thought. Sounds like a plan to me.

How do you feel about treating the current behavior of hg grep as a bug?

yuja added a comment.Jul 13 2018, 8:43 AM
How do you feel about treating the current behavior of `hg grep` as a bug?

I'm 0 on this. 10 years are long enough to turn a bug into a feature, but
I agree the grep (without --all/--diff) is useless. If we had no easy way
to opt-in BC, I would give thumbs up on changing the default behavior.

sangeet259 added a comment.Jul 13 2018, 2:02 PM

So what's the next step should be ?

yuja added a subscriber: sangeet259.Jul 13 2018, 9:53 PM

(resend)

So what's the next step should be ?

Maybe add support for "--all-files -rMULTIREV" so the feature can be
documented properly?

https://www.mercurial-scm.org/wiki/GrepPlan#hg_grep
"If given a set of revisions, it will do the same but search the given
revision(s)."

Revision Contents
Changeset List

PathPackages
Mmercurial/commands.py (11 lines)
Mmercurial/configitems.py (2 lines)
Mmercurial/ui.py (2 lines)
Mtests/test-grep.t (9 lines)

Diff 9536

mercurial/commands.py

inferrepo=True, inferrepo=True,
intents={INTENT_READONLY}) intents={INTENT_READONLY})
def grep(ui, repo, pattern, *pats, **opts): def grep(ui, repo, pattern, *pats, **opts):
"""search revision history for a pattern in specified files """search revision history for a pattern in specified files
Search revision history for a regular expression in the specified Search revision history for a regular expression in the specified
files or the entire project. files or the entire project.
By default, grep searches the expression on the working directory. By default, grep prints the most recent revision number for each
To search history and show the most recent revision number for each file in which it finds a match. To get it to print every revision
file in which it finds a match, use :hg:`grep -r tip:0`. that contains a change in match status ("-" for a match that becomes
To get it to print every revision that contains a change in match status a non-match, or "+" for a non-match that becomes a match), use the
("-" for a match that becomes a non-match, or "+" for a non-match that --diff flag.
becomes a match), use the --diff flag.
PATTERN can be any Python (roughly Perl-compatible) regular PATTERN can be any Python (roughly Perl-compatible) regular
expression. expression.
If no FILEs are specified (and -f/--follow isn't set), all files in If no FILEs are specified (and -f/--follow isn't set), all files in
the repository are searched, including those that don't exist in the the repository are searched, including those that don't exist in the
current branch or have been deleted in a prior changeset. current branch or have been deleted in a prior changeset.

mercurial/configitems.py

) )
coreconfigitem('color', 'mode', coreconfigitem('color', 'mode',
default='auto', default='auto',
) )
coreconfigitem('color', 'pagermode', coreconfigitem('color', 'pagermode',
default=dynamicdefault, default=dynamicdefault,
) )
coreconfigitem('commands', 'grep.all-files', coreconfigitem('commands', 'grep.all-files',
default=True, default=False,
) )
coreconfigitem('commands', 'show.aliasprefix', coreconfigitem('commands', 'show.aliasprefix',
default=list, default=list,
) )
coreconfigitem('commands', 'status.relative', coreconfigitem('commands', 'status.relative',
default=False, default=False,
) )
coreconfigitem('commands', 'status.skipstates', coreconfigitem('commands', 'status.skipstates',

mercurial/ui.py

# The rollback command is dangerous. As a rule, don't use it. # The rollback command is dangerous. As a rule, don't use it.
rollback = False rollback = False
# Make `hg status` report copy information # Make `hg status` report copy information
statuscopies = yes statuscopies = yes
# Prefer curses UIs when available. Revert to plain-text with `text`. # Prefer curses UIs when available. Revert to plain-text with `text`.
interface = curses interface = curses
[commands] [commands]
# Grep working directory by default.
grep.all-files = True
# Make `hg status` emit cwd-relative paths by default. # Make `hg status` emit cwd-relative paths by default.
status.relative = yes status.relative = yes
# Refuse to perform an `hg update` that would cause a file content merge # Refuse to perform an `hg update` that would cause a file content merge
update.check = noconflict update.check = noconflict
# Show conflicts information in `hg status` # Show conflicts information in `hg status`
status.verbose = True status.verbose = True
# Collapse entire directories that contain only unknown files # Collapse entire directories that contain only unknown files
status.terse = u status.terse = u

tests/test-grep.t

$ cd a $ cd a
$ echo "abracadara" >> a $ echo "abracadara" >> a
$ hg add a $ hg add a
$ hg grep -r "wdir()" "abra" $ hg grep -r "wdir()" "abra"
a:2147483647:abracadara a:2147483647:abracadara
$ cd .. $ cd ..
Change Default of grep, that is, the files not in current working directory Change Default of grep by ui.tweakdefaults, that is, the files not in current
should not be grepp-ed on working directory should not be grepp-ed on
$ hg init ab $ hg init ab
$ cd ab $ cd ab
$ cat <<'EOF' >> .hg/hgrc
> [ui]
> tweakdefaults = True
> EOF
$ echo "some text">>file1 $ echo "some text">>file1
$ hg add file1 $ hg add file1
$ hg commit -m "adds file1" $ hg commit -m "adds file1"
$ hg mv file1 file2 $ hg mv file1 file2
$ hg grep "some" $ hg grep "some"
file2:2147483647:some text file2:2147483647:some text