Patchwork D2943: grep: fixes errorneous output of grep in forward order

login
register
mail settings
Submitter phabricator
Date March 26, 2018, 2:22 p.m.
Message ID <differential-rev-PHID-DREV-wopytbzqxasecv3rwgyr-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/29863/
State Superseded
Headers show

Comments

phabricator - March 26, 2018, 2:22 p.m.
sangeet259 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  If grep is passed a revset in forwards order via -r , say -r 0:tip
  Then the output is errorneous. This patch fixes that. The ouput was wrong because
  we deleted the last revision key in the matches and when we moved to the next
  revision we didn't had this to comapare the diff. So the pstates dict was always
  empty and in the SequenceMatcher, to convert and empty pstate to the states
  dictionary you would always insert. This patch keeps the matches dictionary until
  the end of this window and clears it at once when this window ends. This solves the
  above mentioned problem and also do not cause any memory leak.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2943

AFFECTED FILES
  mercurial/commands.py

CHANGE DETAILS




To: sangeet259, #hg-reviewers
Cc: mercurial-devel
phabricator - March 26, 2018, 2:22 p.m.
sangeet259 added a comment.


  Since it required some explanation I have written a post on it.
  http://sangeetmishra.me/2018/03/26/demonstrating-the-problem-with-forward-ordered-grep-all-in-mercurial.html

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2943

To: sangeet259, #hg-reviewers
Cc: mercurial-devel
phabricator - March 26, 2018, 2:24 p.m.
sangeet259 added a comment.


  Currently, I am not sure what tests I should add, the tests in test-grep.t are passing.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2943

To: sangeet259, #hg-reviewers
Cc: mercurial-devel
phabricator - March 27, 2018, 12:34 p.m.
yuja requested changes to this revision.
yuja added a comment.
This revision now requires changes to proceed.


  > This patch keeps the matches dictionary until
  >  the end of this window and clears it at once when this window ends.
  
  This is really helpful while reading the patch. Perhaps it's worth adding
  an inline comment?
  
  The change looks good, but can't be applied on the current tip. Can you
  rebase?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2943

To: sangeet259, #hg-reviewers, yuja
Cc: yuja, mercurial-devel
phabricator - March 27, 2018, 1:43 p.m.
sangeet259 added a comment.


  @yuja rebase it on which revision?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2943

To: sangeet259, #hg-reviewers, yuja
Cc: yuja, mercurial-devel
phabricator - March 27, 2018, 3:16 p.m.
yuja accepted this revision.
yuja added a comment.
This revision is now accepted and ready to land.


  Queued, thanks. Adjusted the commit message to close  " (issue3885)".

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2943

To: sangeet259, #hg-reviewers, yuja
Cc: yuja, mercurial-devel
phabricator - March 30, 2018, 6:59 p.m.
pulkit closed this revision.
pulkit added a comment.


  This was committed as https://www.mercurial-scm.org/repo/hg/rev/a2a6755a3def.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2943

To: pulkit, #hg-reviewers, yuja, sangeet259
Cc: yuja, mercurial-devel

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -2610,8 +2610,9 @@ 
                         skip[fn] = True
                         if copy:
                             skip[copy] = True
-            del matches[rev]
             del revfiles[rev]
+            if not revfiles:
+                matches.clear()
     fm.end()
 
     return not found