[PATCH 0/2] nmbug-config errors for illegible configs

[PATCH 0/2] nmbug-config errors for illegible configs

[W. Trevor King]

More accessible errors and consistent config loading inspired by
Carl's report of the unhelpful 'No JSON object' traceback [1].

Cheers,
Trevor

[1]: id:87wqegr3dm.fsf@yoom.home.cworth.org
     http://article.gmane.org/gmane.mail.notmuch.general/17995

W. Trevor King (2):
  nmbug-status: Clarify errors for illegible configs
  nmbug-status: Use 'show-ref --heads' for loading configs

 NEWS                     | 18 ++++++++++++++++
 devel/nmbug/nmbug        |  1 +
 devel/nmbug/nmbug-status | 54 ++++++++++++++++++++++++++++++++++++++++--------
 3 files changed, 64 insertions(+), 9 deletions(-)

-- 
1.9.1.353.gc66d89d

[PATCH 1/2] nmbug-status: Clarify errors for illegible configs

[W. Trevor King]

Carl Worth pointed out that errors like:

  $ ./nmbug-status
  fatal: Not a git repository: '/home/cworth/.nmbug'
  fatal: Not a git repository: '/home/cworth/.nmbug'
  Traceback (most recent call last):
    File "./nmbug-status", line 254, in <module>
      config = read_config(path=args.config)
    File "./nmbug-status", line 73, in read_config
      return json.load(fp)
    File "/usr/lib/python2.7/json/__init__.py", line 290, in load
      **kw)
    File "/usr/lib/python2.7/json/__init__.py", line 338, in loads
      return _default_decoder.decode(s)
    File "/usr/lib/python2.7/json/decoder.py", line 366, in decode
      obj, end = self.raw_decode(s, idx=_w(s, 0).end())
    File "/usr/lib/python2.7/json/decoder.py", line 384, in raw_decode
      raise ValueError("No JSON object could be decoded")
  ValueError: No JSON object could be decoded

are not particularly clear.  With this commit, we'll get output like:

  $ ./nmbug-status
  fatal: Not a git repository: '/home/wking/.nmbug'
  No local branch 'config' in /home/wking/.nmbug.  Checkout a local
  config branch or explicitly set --config.

which is much more accessible.  I've also added user-friendly messages
for a number of other config-parsing errors.
---
 devel/nmbug/nmbug-status | 54 ++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 45 insertions(+), 9 deletions(-)

diff --git a/devel/nmbug/nmbug-status b/devel/nmbug/nmbug-status
index 03621bd..75a6e47 100755
--- a/devel/nmbug/nmbug-status
+++ b/devel/nmbug/nmbug-status
@@ -49,28 +49,60 @@ if not hasattr(collections, 'OrderedDict'):  # Python 2.6 or earlier
     collections.OrderedDict = _OrderedDict
 
 
+class ConfigError (Exception):
+    """Errors with config file usage
+    """
+    pass
+
+
 def read_config(path=None, encoding=None):
     "Read config from json file"
     if not encoding:
         encoding = _ENCODING
     if path:
-        fp = open(path)
+        try:
+            with open(path, 'rb') as f:
+                config_bytes = f.read()
+        except IOError as e:
+            raise ConfigError('Could not read config from {}'.format(path))
     else:
         nmbhome = os.getenv('NMBGIT', os.path.expanduser('~/.nmbug'))
+        branch = 'config'
+        filename = 'status-config.json'
 
         # read only the first line from the pipe
         sha1_bytes = subprocess.Popen(
-            ['git', '--git-dir', nmbhome, 'show-ref', '-s', 'config'],
+            ['git', '--git-dir', nmbhome, 'show-ref', '-s', branch],
             stdout=subprocess.PIPE).stdout.readline()
         sha1 = sha1_bytes.decode(encoding).rstrip()
+        if not sha1:
+            raise ConfigError(
+                ("No local branch '{branch}' in {nmbgit}.  "
+                 'Checkout a local {branch} branch or explicitly set --config.'
+                ).format(branch=branch, nmbgit=nmbhome))
 
-        fp_byte_stream = subprocess.Popen(
+        p = subprocess.Popen(
             ['git', '--git-dir', nmbhome, 'cat-file', 'blob',
-             sha1+':status-config.json'],
-            stdout=subprocess.PIPE).stdout
-        fp = codecs.getreader(encoding=encoding)(stream=fp_byte_stream)
-
-    return json.load(fp)
+             '{}:{}'.format(sha1, filename)],
+            stdout=subprocess.PIPE)
+        config_bytes, err = p.communicate()
+        status = p.wait()
+        if status != 0:
+            raise ConfigError(
+                ("Missing status-config.json in branch '{branch}' of"
+                 '{nmbgit}.  Add the file or explicitly set --config.'
+                ).format(branch=branch, nmbgit=nmbhome))
+
+    config_json = config_bytes.decode(encoding)
+    try:
+        return json.loads(config_json)
+    except ValueError as e:
+        if not path:
+            path = "{} in branch '{}' of {}".format(
+                filename, branch, nmbhome)
+        raise ConfigError(
+            'Could not parse JSON from the config file {}:\n{}'.format(
+                path, e))
 
 
 class Thread (list):
@@ -254,7 +286,11 @@ parser.add_argument('--get-query', help='get query for view',
 
 args = parser.parse_args()
 
-config = read_config(path=args.config)
+try:
+    config = read_config(path=args.config)
+except ConfigError as e:
+    print(e)
+    sys.exit(1)
 
 _PAGES['text'] = Page()
 _PAGES['html'] = HtmlPage(
-- 
1.9.1.353.gc66d89d

Re: [PATCH 1/2] nmbug-status: Clarify errors for illegible configs

[David Bremner]

"W. Trevor King" <wking@tremily.us> writes:
>   $ ./nmbug-status
>   fatal: Not a git repository: '/home/wking/.nmbug'
>   No local branch 'config' in /home/wking/.nmbug.  Checkout a local
>   config branch or explicitly set --config.
>

This is not _precicisely correct_; it works fine if config is not a local branch; try

        cd ~/.nmbug && git branch -D config && git show-ref -s config

I'm not sure if it matters, it won't hurt for the user to create a local
branch, but maybe you can think of a better wording. And also an easy
way to test this, since deleting the local config branch is not enough.

d

Re: [PATCH 1/2] nmbug-status: Clarify errors for illegible configs

[David Bremner]

"W. Trevor King" <wking@tremily.us> writes:

> are not particularly clear.  With this commit, we'll get output like:
>
>   $ ./nmbug-status
>   fatal: Not a git repository: '/home/wking/.nmbug'
>   No local branch 'config' in /home/wking/.nmbug.  Checkout a local
>   config branch or explicitly set --config.
>
> which is much more accessible.  I've also added user-friendly messages

pushed this one. The second patch touches the perl version of nmbug, so
I'm guessing you rolled it into the python rewrite.

d

Re: [PATCH 1/2] nmbug-status: Clarify errors for illegible configs

[W. Trevor King]

[-- Attachment #1: Type: text/plain, Size: 463 bytes --]

On Sun, Mar 15, 2015 at 09:32:13AM +0100, David Bremner wrote:
> The second patch touches the perl version of nmbug, so I'm guessing
> you rolled it into the python rewrite.

No, the Python bit was pretty much a straight translation.  I'll port
the second patch over and resubmit.

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 2/2] nmbug-status: Use 'show-ref --heads' for loading configs

[W. Trevor King]

When loading configs from Git, the bare branch name (without a
refs/heads/ prefix or similar) matches all branches of that name
(including remote-tracking branches):

  .nmbug $ git show-ref config
  48f3bbf1d1492e5f3d2f01de6ea79a30d3840f20 refs/heads/config
  48f3bbf1d1492e5f3d2f01de6ea79a30d3840f20 refs/remotes/origin/config
  4b6dbd9ffd152e7476f5101eff26747f34497cee refs/remotes/wking/config

Instead of relying on the ordering of the matching references, use
--heads to ensure we only match local branches.
---
 NEWS                     | 18 ++++++++++++++++++
 devel/nmbug/nmbug        |  1 +
 devel/nmbug/nmbug-status |  2 +-
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/NEWS b/NEWS
index bcd311d..5f7d30a 100644
--- a/NEWS
+++ b/NEWS
@@ -1,3 +1,21 @@
+Notmuch 0.19 (UNRELEASED)
+=========================
+
+nmbug
+-----
+
+`nmbug-status` now only matches local branches when reading
+`status-config.json` from the `config` branch of the `NMBGIT`
+repository.  To help new users running `nmbug-status`, `nmbug clone`
+now creates a local `config` branch tracking `origin/config`.  Folks
+who use `nmbug-status` with an in-Git config (i.e. you don't use the
+`--config` option) who already have `NMBGIT` set up are encouraged to
+run:
+
+    git checkout config origin/config
+
+in their `NMBGIT` repository (usually `~/.nmbug`).
+
 Notmuch 0.18~rc0 (2014-04-22)
 =============================
 
diff --git a/devel/nmbug/nmbug b/devel/nmbug/nmbug
index b18ded7..c6793a5 100755
--- a/devel/nmbug/nmbug
+++ b/devel/nmbug/nmbug
@@ -135,6 +135,7 @@ sub do_clone {
     or die "'git clone' exited with nonzero value\n";
   git ('config', '--unset', 'core.worktree');
   git ('config', 'core.bare', 'true');
+  git ('branch', 'config', 'origin/config');
 }
 
 sub is_committed {
diff --git a/devel/nmbug/nmbug-status b/devel/nmbug/nmbug-status
index 75a6e47..610c281 100755
--- a/devel/nmbug/nmbug-status
+++ b/devel/nmbug/nmbug-status
@@ -72,7 +72,7 @@ def read_config(path=None, encoding=None):
 
         # read only the first line from the pipe
         sha1_bytes = subprocess.Popen(
-            ['git', '--git-dir', nmbhome, 'show-ref', '-s', branch],
+            ['git', '--git-dir', nmbhome, 'show-ref', '-s', '--heads', branch],
             stdout=subprocess.PIPE).stdout.readline()
         sha1 = sha1_bytes.decode(encoding).rstrip()
         if not sha1:
-- 
1.9.1.353.gc66d89d

Re: [PATCH 2/2] nmbug-status: Use 'show-ref --heads' for loading configs

[David Bremner]

"W. Trevor King" <wking@tremily.us> writes:

> When loading configs from Git, the bare branch name (without a
> refs/heads/ prefix or similar) matches all branches of that name
> (including remote-tracking branches):
>
>   .nmbug $ git show-ref config
>   48f3bbf1d1492e5f3d2f01de6ea79a30d3840f20 refs/heads/config
>   48f3bbf1d1492e5f3d2f01de6ea79a30d3840f20 refs/remotes/origin/config
>   4b6dbd9ffd152e7476f5101eff26747f34497cee refs/remotes/wking/config
>
> Instead of relying on the ordering of the matching references, use
> --heads to ensure we only match local branches.

ah. I should have read both patches before replying. 

I consider it a useful feature that it works without the user
configuring a local branch.  I agree that in more complex setups this
ambiguity is not as nice, but I'd rather it was only the minority of
users with unusual setups (e.g. multiple remotes) have to do
configuration.

d

Re: [PATCH 2/2] nmbug-status: Use 'show-ref --heads' for loading configs

[W. Trevor King]

[-- Attachment #1: Type: text/plain, Size: 1425 bytes --]

On Sun, Jul 13, 2014 at 09:30:56AM -0300, David Bremner wrote:
> I consider it a useful feature that it works without the user
> configuring a local branch.  I agree that in more complex setups
> this ambiguity is not as nice, but I'd rather it was only the
> minority of users with unusual setups (e.g. multiple remotes) have
> to do configuration.

I could work up a patch that tried ‘git show-ref -s config’ first, and
only fell back to ‘show-ref -s --heads’ if there were multiple
matches.  That way folks with only origin/config wouldn't need a local
branch, but folks with multiple config-carrying remotes (or a single
config-carrying remote and a local branch) would have to have a local
config to break the tie.  That's possible, and not *too* complicated,
but I personally prefer the consistency of just requiring a local
config branch.

There's probably a project-size threshold after which you want better
access controls than a shared public repository like nmbug.tethera.net
gives you.  At that point, most folks are going to want their own,
personal public repository.  Once you have that, the folks using the
‘config’ branch (maybe not very many?) are going to need to keep their
own local branches.

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] nmbug-status: Use 'show-ref --heads' for loading configs

[David Bremner]

"W. Trevor King" <wking@tremily.us> writes:

> On Sun, Jul 13, 2014 at 09:30:56AM -0300, David Bremner wrote:
>> I consider it a useful feature that it works without the user
>> configuring a local branch.  I agree that in more complex setups
>> this ambiguity is not as nice, but I'd rather it was only the
>> minority of users with unusual setups (e.g. multiple remotes) have
>> to do configuration.
>
> I could work up a patch that tried ‘git show-ref -s config’ first, and
> only fell back to ‘show-ref -s --heads’ if there were multiple
> matches.  That way folks with only origin/config wouldn't need a local
> branch, but folks with multiple config-carrying remotes (or a single
> config-carrying remote and a local branch) would have to have a local
> config to break the tie.  That's possible, and not *too* complicated,
> but I personally prefer the consistency of just requiring a local
> config branch.

It might be simpler to implement (and understand) to try first the local
config branch and then fall back to "origin/config".

d

Re: [PATCH 2/2] nmbug-status: Use 'show-ref --heads' for loading configs

[W. Trevor King]

[-- Attachment #1: Type: text/plain, Size: 1797 bytes --]

On Mon, Jul 14, 2014 at 06:51:22AM -0300, David Bremner wrote:
> W. Trevor King writes:
> > On Sun, Jul 13, 2014 at 09:30:56AM -0300, David Bremner wrote:
> >> I consider it a useful feature that it works without the user
> >> configuring a local branch.  I agree that in more complex setups
> >> this ambiguity is not as nice, but I'd rather it was only the
> >> minority of users with unusual setups (e.g. multiple remotes)
> >> have to do configuration.
> >
> > I could work up a patch that tried ‘git show-ref -s config’ first,
> > and only fell back to ‘show-ref -s --heads’ if there were multiple
> > matches.  That way folks with only origin/config wouldn't need a
> > local branch, but folks with multiple config-carrying remotes (or
> > a single config-carrying remote and a local branch) would have to
> > have a local config to break the tie.  That's possible, and not
> > *too* complicated, but I personally prefer the consistency of just
> > requiring a local config branch.
>
> It might be simpler to implement (and understand) to try first the
> local config branch and then fall back to "origin/config".

I'd rather avoid hard-coding an upstream name here.  We do hard-code
‘origin’ as the default remote for ‘fetch’, ‘pull’, and ‘push’.  I'd
rather drop those in favor of the configured Git defaults (for
example, running a bare ‘git fetch’ if the user doesn't specify a
remote).  For most configurations, falling back to “Is ‘git show-ref
-s config’ a unique hash?” should be equivalent to hard-coding ‘git
show-ref -s origin/config’.

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]