Patchwork [2,of,6] ignore: throw exception for bad syntax (BC)

login
register
mail settings
Submitter Durham Goode
Date May 18, 2015, 6:28 p.m.
Message ID <f0bebbe084ce886b7b42.1431973738@dev2000.prn2.facebook.com>
Download mbox | patch
Permalink /patch/9140/
State Accepted
Headers show

Comments

Durham Goode - May 18, 2015, 6:28 p.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1431815649 25200
#      Sat May 16 15:34:09 2015 -0700
# Node ID f0bebbe084ce886b7b42999d2f8b82cc6d4b6523
# Parent  ef85707822930cf344e59353e784f1f0223e257c
ignore: throw exception for bad syntax (BC)

Previously we would only print warnings if the user had a bad 'syntax:' line in
their ignore file. Let's make it an exception instead. This has a few benefits:
1) it's more consistent with how we treat other ignore file errors (like bad
regex's), 2) it will prevent the user from accidentally mixing different
syntaxs, and 3) it removes the need for this logic to have a 'warn' argument,
which will allow us to move it into match.py without threading ui/warn logic
down through match.py.
Matt Mackall - May 18, 2015, 8:28 p.m.
On Mon, 2015-05-18 at 11:28 -0700, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1431815649 25200
> #      Sat May 16 15:34:09 2015 -0700
> # Node ID f0bebbe084ce886b7b42999d2f8b82cc6d4b6523
> # Parent  ef85707822930cf344e59353e784f1f0223e257c
> ignore: throw exception for bad syntax (BC)

This looks mostly good, except:

> +  abort: ignoring invalid syntax 'invalid'

So.. if we're aborting, we're not ignoring it, right?

I've amended this to:

abort: invalid ignore syntax 'invalid'
Pierre-Yves David - May 18, 2015, 8:48 p.m.
On 05/18/2015 03:28 PM, Matt Mackall wrote:
> On Mon, 2015-05-18 at 11:28 -0700, Durham Goode wrote:
>> # HG changeset patch
>> # User Durham Goode <durham@fb.com>
>> # Date 1431815649 25200
>> #      Sat May 16 15:34:09 2015 -0700
>> # Node ID f0bebbe084ce886b7b42999d2f8b82cc6d4b6523
>> # Parent  ef85707822930cf344e59353e784f1f0223e257c
>> ignore: throw exception for bad syntax (BC)
>
> This looks mostly good, except:

I think this is too harsh to crash here.

Mercurial have been historically very able to pull any repo with an old 
client and use it. With this change if mercurial 4.2 adds another 
ignores syntax and someone uses it. The repo become hard unusable to 
anyone who did not upgraded yet. I think we should ignore the entry with 
a noisy warning. This way, old client still be able to use the 
repository but have warning about probable misbehavior of 
status/commit/… commands.
Matt Mackall - May 18, 2015, 9:52 p.m.
On Mon, 2015-05-18 at 15:48 -0500, Pierre-Yves David wrote:
> 
> On 05/18/2015 03:28 PM, Matt Mackall wrote:
> > On Mon, 2015-05-18 at 11:28 -0700, Durham Goode wrote:
> >> # HG changeset patch
> >> # User Durham Goode <durham@fb.com>
> >> # Date 1431815649 25200
> >> #      Sat May 16 15:34:09 2015 -0700
> >> # Node ID f0bebbe084ce886b7b42999d2f8b82cc6d4b6523
> >> # Parent  ef85707822930cf344e59353e784f1f0223e257c
> >> ignore: throw exception for bad syntax (BC)
> >
> > This looks mostly good, except:
> 
> I think this is too harsh to crash here.

Yeah, on reflection, I agree with this. I think I've successfully fixed
the merge conflicts after dropping 2 of 6, so these are queued for
default.

Patch

diff --git a/mercurial/ignore.py b/mercurial/ignore.py
--- a/mercurial/ignore.py
+++ b/mercurial/ignore.py
@@ -18,7 +18,6 @@  def ignorepats(lines):
     syntaxes = {'re': 'relre:', 'regexp': 'relre:', 'glob': 'relglob:'}
     syntax = 'relre:'
     patterns = []
-    warnings = []
 
     for line in lines:
         if "#" in line:
@@ -38,7 +37,7 @@  def ignorepats(lines):
             try:
                 syntax = syntaxes[s]
             except KeyError:
-                warnings.append(_("ignoring invalid syntax '%s'") % s)
+                raise util.Abort(_("ignoring invalid syntax '%s'") % s)
             continue
 
         linesyntax = syntax
@@ -53,16 +52,14 @@  def ignorepats(lines):
                 break
         patterns.append(linesyntax + line)
 
-    return patterns, warnings
+    return patterns
 
 def readignorefile(filepath, warn):
     try:
         pats = []
         fp = open(filepath)
-        pats, warnings = ignorepats(fp)
+        pats = ignorepats(fp)
         fp.close()
-        for warning in warnings:
-            warn("%s: %s\n" % (filepath, warning))
     except IOError, inst:
         warn(_("skipping unreadable ignore file '%s': %s\n") %
              (filepath, inst.strerror))
diff --git a/tests/test-hgignore.t b/tests/test-hgignore.t
--- a/tests/test-hgignore.t
+++ b/tests/test-hgignore.t
@@ -107,13 +107,8 @@  Test relative ignore path (issue4473):
 
   $ echo "syntax: invalid" > .hgignore
   $ hg status
-  $TESTTMP/.hgignore: ignoring invalid syntax 'invalid' (glob)
-  A dir/b.o
-  ? .hgignore
-  ? a.c
-  ? a.o
-  ? dir/c.o
-  ? syntax
+  abort: ignoring invalid syntax 'invalid'
+  [255]
 
   $ echo "syntax: glob" > .hgignore
   $ echo "*.o" >> .hgignore