[StGit PATCH 0/9] Various StGit updates

[StGit PATCH 0/9] Various StGit updates

[Catalin Marinas]

These are the patches in my "proposed" branch to be moved into master
once reviewed.

Thanks.


Catalin Marinas (9):
      Use the default git colouring scheme rather than specific scripts
      Add the log --clear option
      Reinstate the --annotate option for refresh
      Convert 'unhide' to the lib infrastructure
      Convert 'hide' to the lib infrastructure
      Convert 'clone' to the use stgit.lib
      Do not sleep after the last patch sent by e-mail
      Show some progress information when checking for upstream merges.
      Show "Pushing <patch>...done" when pushing a patch


 contrib/diffcol.sh        |   51 ---------------------------------------------
 examples/gitconfig        |    4 ++--
 setup.py                  |    3 +--
 stgit/commands/clone.py   |   30 +++++++++++---------------
 stgit/commands/common.py  |    8 +++++++
 stgit/commands/diff.py    |    2 ++
 stgit/commands/hide.py    |   44 +++++++++++++++++++++------------------
 stgit/commands/log.py     |    9 +++++++-
 stgit/commands/mail.py    |   15 ++++++++-----
 stgit/commands/refresh.py |   13 +++++++++--
 stgit/commands/show.py    |    1 +
 stgit/commands/unhide.py  |   39 +++++++++++++++++++---------------
 stgit/config.py           |   10 ++++-----
 stgit/lib/git.py          |    4 ++++
 stgit/lib/transaction.py  |   15 ++++++++-----
 15 files changed, 117 insertions(+), 131 deletions(-)
 delete mode 100755 contrib/diffcol.sh

-- 
Catalin

[StGit PATCH 1/9] Show "Pushing <patch>...done" when pushing a patch

[Catalin Marinas]

My main reason is for the automatic invocation of the interactive merge
when I don't know what patch I have to deal with. The other reasons is
for people working over slow filesystems (NFS) where a three-way merging
may take a significant amount of time.

Signed-off-by: Catalin Marinas <catalin.marinas@gmail.com>
---
 stgit/lib/transaction.py |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/stgit/lib/transaction.py b/stgit/lib/transaction.py
index 4148ff3..582ee72 100644
--- a/stgit/lib/transaction.py
+++ b/stgit/lib/transaction.py
@@ -302,6 +302,7 @@ class StackTransaction(object):
         """Attempt to push the named patch. If this results in conflicts,
         halts the transaction. If index+worktree are given, spill any
         conflicts to them."""
+        out.start('Pushing patch "%s"' % pn)
         orig_cd = self.patches[pn].data
         cd = orig_cd.set_committer(None)
         oldparent = cd.parent
@@ -330,12 +331,12 @@ class StackTransaction(object):
                 iw.merge(base, ours, theirs, interactive = interactive)
                 tree = iw.index.write_tree()
                 self.__current_tree = tree
-                s = ' (modified)'
+                s = 'modified'
             except git.MergeConflictException, e:
                 tree = ours
                 merge_conflict = True
                 self.__conflicts = e.conflicts
-                s = ' (conflict)'
+                s = 'conflict'
             except git.MergeException, e:
                 self.__halt(str(e))
         cd = cd.set_tree(tree)
@@ -345,12 +346,12 @@ class StackTransaction(object):
             self.head = comm
         else:
             comm = None
-            s = ' (unmodified)'
+            s = 'unmodified'
         if already_merged:
-            s = ' (merged)'
+            s = 'merged'
         elif not merge_conflict and cd.is_nochange():
-            s = ' (empty)'
-        out.info('Pushed %s%s' % (pn, s))
+            s = 'empty'
+        out.done(s)
         def update():
             if comm:
                 self.patches[pn] = comm

[StGit PATCH 2/9] Show some progress information when checking for upstream merges.

[Catalin Marinas]

Signed-off-by: Catalin Marinas <catalin.marinas@gmail.com>
---
 stgit/lib/transaction.py |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/stgit/lib/transaction.py b/stgit/lib/transaction.py
index 582ee72..13323dd 100644
--- a/stgit/lib/transaction.py
+++ b/stgit/lib/transaction.py
@@ -390,6 +390,7 @@ class StackTransaction(object):
 
     def check_merged(self, patches):
         """Return a subset of patches already merged."""
+        out.start('Checking for patches merged upstream')
         merged = []
         if self.temp_index_tree != self.stack.head.data.tree:
             self.temp_index.read_tree(self.stack.head.data.tree)
@@ -408,4 +409,5 @@ class StackTransaction(object):
                 self.temp_index_tree = None
             except git.MergeException:
                 pass
+        out.done('%d found' % len(merged))
         return merged

[StGit PATCH 3/9] Do not sleep after the last patch sent by e-mail

[Catalin Marinas]

Signed-off-by: Catalin Marinas <catalin.marinas@gmail.com>
---
 stgit/commands/mail.py |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/stgit/commands/mail.py b/stgit/commands/mail.py
index 46e4b55..69b19fa 100644
--- a/stgit/commands/mail.py
+++ b/stgit/commands/mail.py
@@ -219,7 +219,7 @@ def __send_message_smtp(smtpserver, from_addr, to_addr_list, msg,
     s.quit()
 
 def __send_message(smtpserver, from_addr, to_addr_list, msg,
-                   sleep, smtpuser, smtppassword, use_tls):
+                   smtpuser, smtppassword, use_tls):
     """Message sending dispatcher.
     """
     if smtpserver.startswith('/'):
@@ -229,8 +229,6 @@ def __send_message(smtpserver, from_addr, to_addr_list, msg,
         # Use the SMTP server (we have host and port information)
         __send_message_smtp(smtpserver, from_addr, to_addr_list, msg,
                             smtpuser, smtppassword, use_tls)
-    # give recipients a chance of receiving patches in the correct order
-    time.sleep(sleep)
 
 def __build_address_headers(msg, options, extra_cc = []):
     """Build the address headers and check existing headers in the
@@ -619,7 +617,8 @@ def func(parser, options, args):
         else:
             out.start('Sending the cover message')
             __send_message(smtpserver, from_addr, to_addr_list, msg_string,
-                           sleep, smtpuser, smtppassword, smtpusetls)
+                           smtpuser, smtppassword, smtpusetls)
+            time.sleep(sleep)
             out.done()
 
     # send the patches
@@ -633,7 +632,7 @@ def func(parser, options, args):
         if not tmpl:
             raise CmdException, 'No e-mail template file found'
 
-    for (p, patch_nr) in zip(patches, range(1, len(patches) + 1)):
+    for (p, patch_nr) in zip(patches, range(1, total_nr + 1)):
         msg_id = email.Utils.make_msgid('stgit')
         msg = __build_message(tmpl, p, patch_nr, total_nr, msg_id, ref_id,
                               options)
@@ -650,5 +649,9 @@ def func(parser, options, args):
         else:
             out.start('Sending patch "%s"' % p)
             __send_message(smtpserver, from_addr, to_addr_list, msg_string,
-                           sleep, smtpuser, smtppassword, smtpusetls)
+                           smtpuser, smtppassword, smtpusetls)
+            # give recipients a chance of receiving related patches in the
+            # correct order.
+            if patch_nr < total_nr:
+                time.sleep(sleep)
             out.done()

[StGit PATCH 4/9] Convert 'clone' to the use stgit.lib

[Catalin Marinas]

The patch also adds the stgit.lib.git.clone() function.

Signed-off-by: Catalin Marinas <catalin.marinas@gmail.com>
---
 stgit/commands/clone.py |   30 +++++++++++++-----------------
 stgit/lib/git.py        |    4 ++++
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/stgit/commands/clone.py b/stgit/commands/clone.py
index 7fe9c35..369c8a9 100644
--- a/stgit/commands/clone.py
+++ b/stgit/commands/clone.py
@@ -1,5 +1,5 @@
 __copyright__ = """
-Copyright (C) 2005, Catalin Marinas <catalin.marinas@gmail.com>
+Copyright (C) 2009, Catalin Marinas <catalin.marinas@gmail.com>
 
 This program is free software; you can redistribute it and/or modify
 it under the terms of the GNU General Public License version 2 as
@@ -15,10 +15,11 @@ along with this program; if not, write to the Free Software
 Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
 """
 
-import sys, os
-from stgit.commands.common import *
-from stgit.utils import *
-from stgit import argparse, stack, git
+import os
+from stgit.commands import common
+from stgit.lib import git, stack
+from stgit import argparse
+from stgit.out import out
 
 help = 'Make a local clone of a remote repository'
 kind = 'repo'
@@ -38,7 +39,7 @@ not already exist."""
 args = [argparse.repo, argparse.dir]
 options = []
 
-directory = DirectoryAnywhere(needs_current_series = False, log = False)
+directory = common.DirectoryAnywhere(needs_current_series = False, log = False)
 
 def func(parser, options, args):
     """Clone the <repository> into the local <dir> and initialises the
@@ -51,17 +52,12 @@ def func(parser, options, args):
     local_dir = args[1]
 
     if os.path.exists(local_dir):
-        raise CmdException, '"%s" exists. Remove it first' % local_dir
-
-    print 'Cloning "%s" into "%s"...' % (repository, local_dir)
+        raise common.CmdException, '"%s" exists. Remove it first' % local_dir
 
+    out.start('Cloning "%s" into "%s"' % (repository, local_dir))
     git.clone(repository, local_dir)
     os.chdir(local_dir)
-    git.checkout(tree_id = 'HEAD')
-
-    # be sure to forget any cached value for .git, since we're going
-    # to work on a brand new repository
-    basedir.clear_cache()
-    stack.Series().init()
-
-    print 'done'
+    directory = common.DirectoryHasRepositoryLib()
+    directory.setup()
+    stack.Stack.initialise(directory.repository)
+    out.done()
diff --git a/stgit/lib/git.py b/stgit/lib/git.py
index 9c530c7..6f2c977 100644
--- a/stgit/lib/git.py
+++ b/stgit/lib/git.py
@@ -933,3 +933,7 @@ def diffstat(diff):
     """Return the diffstat of the supplied diff."""
     return run.Run('git', 'apply', '--stat', '--summary'
                    ).raw_input(diff).raw_output()
+
+def clone(remote, local):
+    """Clone a remote repository using 'git clone'."""
+    run.Run('git', 'clone', remote, local).run()

[StGit PATCH 5/9] Convert 'hide' to the lib infrastructure

[Catalin Marinas]

Signed-off-by: Catalin Marinas <catalin.marinas@gmail.com>
---
 stgit/commands/hide.py |   44 ++++++++++++++++++++++++--------------------
 1 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/stgit/commands/hide.py b/stgit/commands/hide.py
index 014febb..0b93a59 100644
--- a/stgit/commands/hide.py
+++ b/stgit/commands/hide.py
@@ -1,5 +1,5 @@
 __copyright__ = """
-Copyright (C) 2007, Catalin Marinas <catalin.marinas@gmail.com>
+Copyright (C) 2009, Catalin Marinas <catalin.marinas@gmail.com>
 
 This program is free software; you can redistribute it and/or modify
 it under the terms of the GNU General Public License version 2 as
@@ -15,12 +15,10 @@ along with this program; if not, write to the Free Software
 Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
 """
 
-import sys, os
+from stgit.commands import common
+from stgit.lib import transaction
+from stgit import argparse
 from stgit.argparse import opt
-from stgit.commands.common import *
-from stgit.utils import *
-from stgit.out import *
-from stgit import argparse, stack, git
 
 help = 'Hide a patch in the series'
 kind = 'stack'
@@ -29,25 +27,31 @@ description = """
 Hide a range of unapplied patches so that they are no longer shown in
 the plain 'series' command output."""
 
-args = [argparse.patch_range(argparse.applied_patches,
-                             argparse.unapplied_patches)]
+args = [argparse.patch_range(argparse.unapplied_patches)]
 options = [
     opt('-b', '--branch', args = [argparse.stg_branches],
         short = 'Use BRANCH instead of the default branch')]
 
-directory = DirectoryHasRepository(log = True)
+directory = common.DirectoryHasRepositoryLib()
 
 def func(parser, options, args):
-    """Hide a range of patch in the series
-    """
-    if args:
-        # parsing all the patches for a more meaningful error reporting
-        all_patches = crt_series.get_applied() + crt_series.get_unapplied() \
-                      + crt_series.get_hidden()
-        patches = parse_patches(args, all_patches)
-    else:
+    """Hide a range of patch in the series."""
+    stack = directory.repository.current_stack
+    trans = transaction.StackTransaction(stack, 'hide')
+
+    if not args:
         parser.error('No patches specified')
 
-    for patch in patches:
-        crt_series.hide_patch(patch)
-        out.info('Patch "%s" hidden' % patch)
+    patches = common.parse_patches(args, trans.all_patches)
+    for p in patches:
+        if p in trans.applied:
+            raise common.CmdException('Cannot hide applied patch "%s"' % p)
+        elif p in trans.hidden:
+            raise common.CmdException('Patch "%s" already hidden' % p)
+
+    applied = list(trans.applied)
+    unapplied = [p for p in trans.unapplied if not p in set(patches)]
+    hidden = patches + trans.hidden
+
+    trans.reorder_patches(applied, unapplied, hidden)
+    return trans.run()

[StGit PATCH 6/9] Convert 'unhide' to the lib infrastructure

[Catalin Marinas]

Signed-off-by: Catalin Marinas <catalin.marinas@gmail.com>
---
 stgit/commands/unhide.py |   39 +++++++++++++++++++++------------------
 1 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/stgit/commands/unhide.py b/stgit/commands/unhide.py
index 0c0832a..656ddea 100644
--- a/stgit/commands/unhide.py
+++ b/stgit/commands/unhide.py
@@ -1,5 +1,5 @@
 __copyright__ = """
-Copyright (C) 2007, Catalin Marinas <catalin.marinas@gmail.com>
+Copyright (C) 2009, Catalin Marinas <catalin.marinas@gmail.com>
 
 This program is free software; you can redistribute it and/or modify
 it under the terms of the GNU General Public License version 2 as
@@ -15,12 +15,10 @@ along with this program; if not, write to the Free Software
 Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
 """
 
-import sys, os
+from stgit.commands import common
+from stgit.lib import transaction
+from stgit import argparse
 from stgit.argparse import opt
-from stgit.commands.common import *
-from stgit.utils import *
-from stgit.out import *
-from stgit import argparse, stack, git
 
 help = 'Unhide a hidden patch'
 kind = 'stack'
@@ -34,19 +32,24 @@ options = [
     opt('-b', '--branch', args = [argparse.stg_branches],
         short = 'Use BRANCH instead of the default branch')]
 
-directory = DirectoryHasRepository(log = True)
+directory = common.DirectoryHasRepositoryLib()
 
 def func(parser, options, args):
-    """Unhide a range of patches in the series
-    """
-    if args:
-        # parsing all the patches for a more meaningful error reporting
-        all_patches = crt_series.get_applied() + crt_series.get_unapplied() \
-                      + crt_series.get_hidden()
-        patches = parse_patches(args, all_patches)
-    else:
+    """Unhide a range of patch in the series."""
+    stack = directory.repository.current_stack
+    trans = transaction.StackTransaction(stack, 'hide')
+
+    if not args:
         parser.error('No patches specified')
 
-    for patch in patches:
-        crt_series.unhide_patch(patch)
-        out.info('Patch "%s" unhidden' % patch)
+    patches = common.parse_patches(args, trans.all_patches)
+    for p in patches:
+        if not p in trans.hidden:
+            raise common.CmdException('Patch "%s" not hidden' % p)
+
+    applied = list(trans.applied)
+    unapplied = trans.unapplied + patches
+    hidden = [p for p in trans.hidden if not p in set(patches)]
+
+    trans.reorder_patches(applied, unapplied, hidden)
+    return trans.run()

[StGit PATCH 7/9] Reinstate the --annotate option for refresh

[Catalin Marinas]

It is sometimes useful to add some notes to the log entry when a patch
was refreshed. This option was dropped when the command was updated to
the new infrastructure as there was no logging support at that time.

The note will be visible with 'stg log {-g,-f}'

Signed-off-by: Catalin Marinas <catalin.marinas@gmail.com>
---
 stgit/commands/refresh.py |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/stgit/commands/refresh.py b/stgit/commands/refresh.py
index 5a5f979..c5a0aeb 100644
--- a/stgit/commands/refresh.py
+++ b/stgit/commands/refresh.py
@@ -61,6 +61,8 @@ options = [
         short = 'Refresh (applied) PATCH instead of the top patch'),
     opt('-e', '--edit', action = 'store_true',
         short = 'Invoke an editor for the patch description'),
+    opt('-a', '--annotate', metavar = 'NOTE',
+        short = 'Annotate the patch log entry')
     ] + (argparse.message_options(save_template = False) +
          argparse.sign_options() + argparse.author_options())
 
@@ -200,9 +202,13 @@ def absorb_unapplied(trans, iw, patch_name, temp_name, edit_fun):
         # leave the temp patch for the user.
         return False
 
-def absorb(stack, patch_name, temp_name, edit_fun):
+def absorb(stack, patch_name, temp_name, edit_fun, annotate = None):
     """Absorb the temp patch into the target patch."""
-    trans = transaction.StackTransaction(stack, 'refresh')
+    if annotate:
+        log_msg = 'refresh\n\n' + annotate
+    else:
+        log_msg = 'refresh'
+    trans = transaction.StackTransaction(stack, log_msg)
     iw = stack.repository.default_iw
     f = { True: absorb_applied, False: absorb_unapplied
           }[patch_name in trans.applied]
@@ -252,4 +258,5 @@ def func(parser, options, args):
                 diff_flags = [], replacement_diff = None)
             assert not failed_diff
         return cd
-    return absorb(stack, patch_name, temp_name, edit_fun)
+    return absorb(stack, patch_name, temp_name, edit_fun,
+                  annotate = options.annotate)

[StGit PATCH 8/9] Add the log --clear option

[Catalin Marinas]

This option allows the clearing of the log history which sometimes may
get too large.

Signed-off-by: Catalin Marinas <catalin.marinas@gmail.com>
---
 stgit/commands/log.py |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/stgit/commands/log.py b/stgit/commands/log.py
index 3370e6c..92aaf0b 100644
--- a/stgit/commands/log.py
+++ b/stgit/commands/log.py
@@ -50,7 +50,9 @@ options = [
     opt('-f', '--full', action = 'store_true',
         short = 'Show the full commit ids'),
     opt('-g', '--graphical', action = 'store_true',
-        short = 'Run gitk instead of printing')]
+        short = 'Run gitk instead of printing'),
+    opt('--clear', action = 'store_true',
+        short = 'Clear the log history')]
 
 directory = common.DirectoryHasRepositoryLib()
 
@@ -76,6 +78,11 @@ def func(parser, options, args):
     except KeyError:
         out.info('Log is empty')
         return
+
+    if options.clear:
+        log.delete_log(stack.repository, stack.name)
+        return
+
     stacklog = log.get_log_entry(stack.repository, logref, logcommit)
     pathlim = [os.path.join('patches', pn) for pn in patches]
 

[StGit PATCH 9/9] Use the default git colouring scheme rather than specific scripts

[Catalin Marinas]

This patch adds the mechanism to check if the output is tty for the
diff and show commands and passes the --color option to git if the
color.diff config option is set auto or true. The patch also changes the
default pager to 'less -FRSX' from the diffcol.sh script.

Signed-off-by: Catalin Marinas <catalin.marinas@gmail.com>
---
 contrib/diffcol.sh       |   51 ----------------------------------------------
 examples/gitconfig       |    4 ++--
 setup.py                 |    3 +--
 stgit/commands/common.py |    8 +++++++
 stgit/commands/diff.py   |    2 ++
 stgit/commands/show.py   |    1 +
 stgit/config.py          |   10 +++++----
 7 files changed, 19 insertions(+), 60 deletions(-)
 delete mode 100755 contrib/diffcol.sh

diff --git a/contrib/diffcol.sh b/contrib/diffcol.sh
deleted file mode 100755
index eecc87a..0000000
--- a/contrib/diffcol.sh
+++ /dev/null
@@ -1,51 +0,0 @@
-#!/bin/bash
-
-# Code copied from Quilt (http://savannah.nongnu.org/projects/quilt)
-#
-# Copyright 2006 - the Quilt authors
-#
-#  This script is free software; you can redistribute it and/or modify
-#  it under the terms of the GNU General Public License version 2 as
-#  published by the Free Software Foundation.
-
-setup_colors()
-{
-	local C="diffhdr=1;36:diffhdradd=1;32:diffadd=32:diffhdrmod=1;35:diffmod=35:diffhdrrem=1;31:diffrem=31:diffhunk=36:diffctx=34:diffcctx=33:default=0"
-	[ -n "$DIFF_COLORS" ] && C="$C:$DIFF_COLORS"
-
-	C=${C//=/=\'$'\e'[}
-	C=col${C//:/m\'; col}m\'
-	#coldefault=$(tput op)
-	eval $C
-}
-
-setup_colors
-
-gawk '{
-	if (/^(Index:|diff --git) /)
-		print "'$coldiffhdr'" $0 "'$coldefault'"
-	else if (/^======*$/)
-		print "'$coldiffhdr'" $0 "'$coldefault'"
-	else if (/^\+\+\+/)
-		print "'$coldiffhdradd'" $0 "'$coldefault'"
-	else if (/^\*\*\*/)
-		print "'$coldiffhdrmod'" $0 "'$coldefault'"
-	else if (/^---/)
-		print "'$coldiffhdrrem'" $0 "'$coldefault'"
-	else if (/^(\+|new( file)? mode )/)
-		print "'$coldiffadd'" $0 "'$coldefault'"
-	else if (/^(-|(deleted file|old) mode )/)
-		print "'$coldiffrem'" $0 "'$coldefault'"
-	else if (/^!/)
-		print "'$coldiffmod'" $0 "'$coldefault'"
-	else if (/^@@ \-[0-9]+(,[0-9]+)? \+[0-9]+(,[0-9]+)? @@/)
-		print gensub(/^(@@[^@]*@@)([ \t]*)(.*)/,
-			"'$coldiffhunk'" "\\1" "'$coldefault'" \
-			"\\2" \
-			"'$coldiffctx'" "\\3" "'$coldefault'", "")
-	else if (/^\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*/)
-		print "'$coldiffcctx'" $0 "'$coldefault'"
-	else {
-		print
-	}
-}' $1 | less -R -S
diff --git a/examples/gitconfig b/examples/gitconfig
index f6e3a79..e235e14 100644
--- a/examples/gitconfig
+++ b/examples/gitconfig
@@ -42,8 +42,8 @@
 	#editor = /usr/bin/vi
 
 	# this value overrides the default PAGER environment variable
-	#pager = ~/share/stgit/contrib/diffcol.sh
-	#pager = filterdiff --annotate | colordiff | less -FRX
+	#pager = less -FRSX
+	#pager = filterdiff --annotate | colordiff | less -FRSX
 
 	# GIT pull and fetch commands (should take the same arguments as
 	# git fetch or git pull).  By default:
diff --git a/setup.py b/setup.py
index fb67958..73ce2e5 100755
--- a/setup.py
+++ b/setup.py
@@ -58,8 +58,7 @@ def __run_setup():
             ('share/stgit/templates', glob.glob('templates/*.tmpl')),
             ('share/stgit/examples', glob.glob('examples/*.tmpl')),
             ('share/stgit/examples', ['examples/gitconfig']),
-            ('share/stgit/contrib', ['contrib/diffcol.sh',
-                                     'contrib/stgbashprompt.sh']),
+            ('share/stgit/contrib', ['contrib/stgbashprompt.sh']),
             ('share/stgit/completion', ['stgit-completion.bash'])
             ])
 
diff --git a/stgit/commands/common.py b/stgit/commands/common.py
index 6bb3685..e46412e 100644
--- a/stgit/commands/common.py
+++ b/stgit/commands/common.py
@@ -83,6 +83,14 @@ def git_commit(name, repository, branch_name = None):
     except libgit.RepositoryException:
         raise CmdException('%s: Unknown patch or revision name' % name)
 
+def color_diff_flags():
+    """Return the git flags for coloured diff output if the configuration and
+    stdout allows."""
+    if sys.stdout.isatty() and config.get('color.diff') in ['true', 'auto']:
+        return ['--color']
+    else:
+        return []
+
 def check_local_changes():
     if git.local_changes():
         raise CmdException('local changes in the tree. Use "refresh" or'
diff --git a/stgit/commands/diff.py b/stgit/commands/diff.py
index 7d2f719..8b8ebe3 100644
--- a/stgit/commands/diff.py
+++ b/stgit/commands/diff.py
@@ -72,6 +72,8 @@ def func(parser, options, args):
         rev1 = 'HEAD'
         rev2 = None
 
+    if not options.stat:
+        options.diff_flags.extend(color_diff_flags())
     diff_str = git.diff(args, git_id(crt_series, rev1),
                         rev2 and git_id(crt_series, rev2),
                         diff_flags = options.diff_flags)
diff --git a/stgit/commands/show.py b/stgit/commands/show.py
index 895943a..b7a8aa9 100644
--- a/stgit/commands/show.py
+++ b/stgit/commands/show.py
@@ -61,6 +61,7 @@ def func(parser, options, args):
         # individual patches or commit ids
         patches = args
 
+    options.diff_flags.extend(color_diff_flags())
     commit_ids = [git_id(crt_series, patch) for patch in patches]
     commit_str = '\n'.join([git.pretty_commit(commit_id,
                                               flags = options.diff_flags)
diff --git a/stgit/config.py b/stgit/config.py
index efce097..4f16978 100644
--- a/stgit/config.py
+++ b/stgit/config.py
@@ -37,7 +37,8 @@ class GitConfig:
         'stgit.autoimerge':	'no',
         'stgit.keepoptimized':	'no',
         'stgit.extensions':	'.ancestor .current .patched',
-        'stgit.shortnr':	 '5'
+        'stgit.shortnr': '5',
+        'stgit.pager':  'less -FRSX'
         }
 
     __cache={}
@@ -109,10 +110,9 @@ config=GitConfig()
 def config_setup():
     global config
 
-    # Set the PAGER environment to the config value (if any)
-    pager = config.get('stgit.pager')
-    if pager:
-        os.environ['PAGER'] = pager
+    # Set the PAGER environment to the config value if not already set
+    if not 'PAGER' in os.environ:
+        os.environ['PAGER'] = config.get('stgit.pager')
     # FIXME: handle EDITOR the same way ?
 
 class ConfigOption:

Re: [StGit PATCH 1/9] Show "Pushing <patch>...done" when pushing a patch

[Karl Hasselström]

On 2009-04-28 16:09:40 +0100, Catalin Marinas wrote:

> My main reason is for the automatic invocation of the interactive
> merge when I don't know what patch I have to deal with. The other
> reasons is for people working over slow filesystems (NFS) where a
> three-way merging may take a significant amount of time.

I don't see any problems with this.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: [StGit PATCH 2/9] Show some progress information when checking for upstream merges.

[Karl Hasselström]

Looks good. (I hadn't even realized we were doing this only in the old
infrastructure.)

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: [StGit PATCH 3/9] Do not sleep after the last patch sent by e-mail

[Karl Hasselström]

This is an improvement.

An somewhat related improvement would be to decouple the mail sending
and interactive editing of cover letter and patch mails -- we don't
really need to wait five seconds before letting the user edit the next
patch! (In fact, we should probably let her edit everything first, and
then ask "Send? Y/N" at the end. Or something. Maybe a menu with
options to edit cover letter, edit patches, send, save to mbox, read
from mbox, and discard. But now we're getting into UI land. And most
of this isn't StGit-specific anyway.)

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: [StGit PATCH 4/9] Convert 'clone' to the use stgit.lib

[Karl Hasselström]

On 2009-04-28 16:09:57 +0100, Catalin Marinas wrote:

> The patch also adds the stgit.lib.git.clone() function.

>      if os.path.exists(local_dir):
> -        raise CmdException, '"%s" exists. Remove it first' % local_dir
> -
> -    print 'Cloning "%s" into "%s"...' % (repository, local_dir)
> +        raise common.CmdException, '"%s" exists. Remove it first' % local_dir

As recommended by PEP 8, consider using the "raise Exc(args)" syntax:

    - When raising an exception, use "raise ValueError('message')" instead of
      the older form "raise ValueError, 'message'".

      The paren-using form is preferred because when the exception arguments
      are long or include string formatting, you don't need to use line
      continuation characters thanks to the containing parentheses.  The older
      form will be removed in Python 3000.

> +def clone(remote, local):
> +    """Clone a remote repository using 'git clone'."""
> +    run.Run('git', 'clone', remote, local).run()

You don't capture git's output here, but just let it through. Does
that look good in combination with the enclosing out.start() ...
.done() stuff?

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: [StGit PATCH 5/9] Convert 'hide' to the lib infrastructure

[Karl Hasselström]

On 2009-04-28 16:10:03 +0100, Catalin Marinas wrote:

> -args = [argparse.patch_range(argparse.applied_patches,
> -                             argparse.unapplied_patches)]
> +args = [argparse.patch_range(argparse.unapplied_patches)]

Why not simply allow all patches? reorder_patches() below will happily
pop unapplied patches before hiding them IIRC, and for already hidden
patches you could just say "already hidden".

Hmm, but this is for the tab completion, so I guess we'd only want the
applied and unapplied patches here.

> +    patches = common.parse_patches(args, trans.all_patches)
> +    for p in patches:
> +        if p in trans.applied:
> +            raise common.CmdException('Cannot hide applied patch "%s"' % p)
> +        elif p in trans.hidden:
> +            raise common.CmdException('Patch "%s" already hidden' % p)
> +
> +    applied = list(trans.applied)
> +    unapplied = [p for p in trans.unapplied if not p in set(patches)]
> +    hidden = patches + trans.hidden
> +
> +    trans.reorder_patches(applied, unapplied, hidden)
> +    return trans.run()

As I said,

  * Why not simply allow hiding of applied patches?

  * Hiding a hidden patch should probably be a warning (if that), not
    an error.

Otherwise it looks good.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: [StGit PATCH 6/9] Convert 'unhide' to the lib infrastructure

[Karl Hasselström]

Looks good.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: [StGit PATCH 7/9] Reinstate the --annotate option for refresh

[Karl Hasselström]

On 2009-04-28 16:10:14 +0100, Catalin Marinas wrote:

> It is sometimes useful to add some notes to the log entry when a
> patch was refreshed. This option was dropped when the command was
> updated to the new infrastructure as there was no logging support at
> that time.

It looks like if you get a conflict during the refresh (such as when
doing refresh -p, and we can't reorder the patches) the annotation
will be dropped. That could presumably be fixed by tacking it on to
the log message for the creation of the temp patch as well.

But I don't know if it's that important. I've never felt the need for
this feature myself.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: [StGit PATCH 8/9] Add the log --clear option

[Karl Hasselström]

On 2009-04-28 16:10:20 +0100, Catalin Marinas wrote:

> This option allows the clearing of the log history which sometimes
> may get too large.

Wow, I didn't realize we already had all the pieces for doing this.

Of course, what we'd _really_ want is to delete the log except for the
last N days, or the last N entries. :-) That's gonna be a bit more
work, though, and this is an excellent step in the right direction.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: [StGit PATCH 9/9] Use the default git colouring scheme rather than specific scripts

[Karl Hasselström]

On 2009-04-28 16:10:25 +0100, Catalin Marinas wrote:

> This patch adds the mechanism to check if the output is tty for the
> diff and show commands and passes the --color option to git if the
> color.diff config option is set auto or true. The patch also changes
> the default pager to 'less -FRSX' from the diffcol.sh script.

Seems like a nice improvement.

> +    # Set the PAGER environment to the config value if not already set
> +    if not 'PAGER' in os.environ:
> +        os.environ['PAGER'] = config.get('stgit.pager')

You can replace these two lines with

  os.environ.setdefault('PAGER', config.get('stgit.pager'))

The only downside is that config.get() will be evaluated (and the
result discarded) even if PAGER _is_ set in the environment. But
config.get() should be dirt cheap, because we should be reading in all
the config values at once the first time we need one of them. But as I
recall we don't currently do that, so my one-liner might not be so
clever after all ...

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: [StGit PATCH 9/9] Use the default git colouring scheme rather than specific scripts

[Samuel Tardieu]

Author: Samuel Tardieu <sam@rfc1149.net>
Date:   Wed Apr 29 13:45:06 2009 +0200

Load the whole config at once and cache it for future use

The whole configuration files are read using

  git config --list --null

and cached for future lookups.

Signed-off-by: Samuel Tardieu <sam@rfc1149.net>
---
>>>>> "Karl" == Karl Hasselström <kha@treskal.com> writes:

Karl> But config.get() should be dirt cheap, because we should be
Karl> reading in all the config values at once the first time we need
Karl> one of them. But as I recall we don't currently do that, so my
Karl> one-liner might not be so clever after all ...

Something like this may be useful then.

  Sam

diff --git a/stgit/config.py b/stgit/config.py
index dbca5fb..c40756c 100644
--- a/stgit/config.py
+++ b/stgit/config.py
@@ -40,25 +40,31 @@ class GitConfig:
         'stgit.shortnr':	 '5'
         }
 
-    __cache={}
+    __cache = None
+
+    def load(self):
+        """Load the whole configuration in __cache unless it has been
+        done already."""
+        if self.__cache is not None:
+            return
+        self.__cache = {}
+        lines = Run('git', 'config', '--list', '--null').raw_output()
+        for line in filter(None, lines.split('\0')):
+            key, value = line.split('\n', 1)
+            self.__cache.setdefault(key, []).append(value)
 
     def get(self, name):
-        if self.__cache.has_key(name):
-            return self.__cache[name]
-        try:
-            value = Run('git', 'config', '--get', name).output_one_line()
-        except RunException:
-            value = self.__defaults.get(name, None)
-        self.__cache[name] = value
-        return value
+        self.load()
+        if name not in self.__cache:
+            self.__cache[name] = [self.__defaults.get(name, None)]
+        return self.__cache[name][0]
 
     def getall(self, name):
-        if self.__cache.has_key(name):
+        self.load()
+        try:
             return self.__cache[name]
-        values = Run('git', 'config', '--get-all', name
-                     ).returns([0, 1]).output_lines()
-        self.__cache[name] = values
-        return values
+        except KeyError:
+            return []
 
     def getint(self, name):
         value = self.get(name)

Re: [StGit PATCH 9/9] Use the default git colouring scheme rather than specific scripts

[Samuel Tardieu]

Karl> But config.get() should be dirt cheap, because we should be
Karl> reading in all the config values at once the first time we need
Karl> one of them. But as I recall we don't currently do that, so my
Karl> one-liner might not be so clever after all ...

Sam> Something like this may be useful then.

Btw, the patch is against kha/experimental.

  Sam
-- 
Samuel Tardieu -- sam@rfc1149.net -- http://www.rfc1149.net/

Re: [StGit PATCH 9/9] Use the default git colouring scheme rather than specific scripts

[Karl Hasselström]

On 2009-04-29 13:48:29 +0200, Samuel Tardieu wrote:

> Something like this may be useful then.

Yes, exactly.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: [StGit PATCH 9/9] Use the default git colouring scheme rather than specific scripts

[Shinya Kuribayashi]

Hi,

Catalin Marinas wrote:
> This patch adds the mechanism to check if the output is tty for the
> diff and show commands and passes the --color option to git if the
> color.diff config option is set auto or true. The patch also changes the
> default pager to 'less -FRSX' from the diffcol.sh script.
> 
> Signed-off-by: Catalin Marinas <catalin.marinas@gmail.com>

Cool.

> diff --git a/stgit/commands/common.py b/stgit/commands/common.py
> index 6bb3685..e46412e 100644
> --- a/stgit/commands/common.py
> +++ b/stgit/commands/common.py
> @@ -83,6 +83,14 @@ def git_commit(name, repository, branch_name = None):
>      except libgit.RepositoryException:
>          raise CmdException('%s: Unknown patch or revision name' % name)
>  
> +def color_diff_flags():
> +    """Return the git flags for coloured diff output if the configuration and
> +    stdout allows."""
> +    if sys.stdout.isatty() and config.get('color.diff') in ['true', 'auto']:
> +        return ['--color']
> +    else:
> +        return []
> +
>  def check_local_changes():
>      if git.local_changes():
>          raise CmdException('local changes in the tree. Use "refresh" or'

Junio introduces `color.ui=auto' as one of base settings in his recent
Japanese article for Git newbies:

http://gitster.livejournal.com/2009/04/24/

Is color.ui worth supporting in color_diff_flags()?, or simply having 
additional color.diff would be better?

> diff --git a/stgit/config.py b/stgit/config.py
> index efce097..4f16978 100644
> --- a/stgit/config.py
> +++ b/stgit/config.py
> @@ -37,7 +37,8 @@ class GitConfig:
>          'stgit.autoimerge':	'no',
>          'stgit.keepoptimized':	'no',
>          'stgit.extensions':	'.ancestor .current .patched',
> -        'stgit.shortnr':	 '5'
> +        'stgit.shortnr': '5',
> +        'stgit.pager':  'less -FRSX'
>          }
>  
>      __cache={}

Wrong indentation? :-)


Shinya

Re: [StGit PATCH 5/9] Convert 'hide' to the lib infrastructure

[Catalin Marinas]

2009/4/29 Karl Hasselström <kha@treskal.com>:
> On 2009-04-28 16:10:03 +0100, Catalin Marinas wrote:
>
>> -args = [argparse.patch_range(argparse.applied_patches,
>> -                             argparse.unapplied_patches)]
>> +args = [argparse.patch_range(argparse.unapplied_patches)]
>
> Why not simply allow all patches? reorder_patches() below will happily
> pop unapplied patches before hiding them IIRC, and for already hidden
> patches you could just say "already hidden".
>
> Hmm, but this is for the tab completion, so I guess we'd only want the
> applied and unapplied patches here.

I fixed that in the proposed branch but I was to slow on sending e-mails.

> As I said,
>
>  * Why not simply allow hiding of applied patches?
>
>  * Hiding a hidden patch should probably be a warning (if that), not
>    an error.

Fixed this as well.

-- 
Catalin

Re: [StGit PATCH 4/9] Convert 'clone' to the use stgit.lib

[Catalin Marinas]

2009/4/29 Karl Hasselström <kha@treskal.com>:
> On 2009-04-28 16:09:57 +0100, Catalin Marinas wrote:
>> +def clone(remote, local):
>> +    """Clone a remote repository using 'git clone'."""
>> +    run.Run('git', 'clone', remote, local).run()
>
> You don't capture git's output here, but just let it through. Does
> that look good in combination with the enclosing out.start() ...
> .done() stuff?

I actually dropped the out.*() entirely as git already prints enough
information.

-- 
Catalin

Re: [StGit PATCH 9/9] Use the default git colouring scheme rather than specific scripts

[Catalin Marinas]

2009/5/4 Shinya Kuribayashi <skuribay@ruby.dti.ne.jp>:
> Catalin Marinas wrote:
>>  +def color_diff_flags():
>> +    """Return the git flags for coloured diff output if the configuration
>> and
>> +    stdout allows."""
>> +    if sys.stdout.isatty() and config.get('color.diff') in ['true',
>> 'auto']:
>> +        return ['--color']
>> +    else:
>> +        return []
>> +
>>  def check_local_changes():
>>     if git.local_changes():
>>         raise CmdException('local changes in the tree. Use "refresh" or'
>
> Junio introduces `color.ui=auto' as one of base settings in his recent
> Japanese article for Git newbies:

That's probably a better option. I changed the patch to this (only
showing the relevant parts):

--- a/stgit/commands/common.py
+++ b/stgit/commands/common.py
@@ -97,6 +97,15 @@ def git_commit(name, repository, branch_name = None):
     except libgit.RepositoryException:
         raise CmdException('%s: Unknown patch or revision name' % name)

+def color_diff_flags():
+    """Return the git flags for coloured diff output if the configuration and
+    stdout allows."""
+    stdout_is_tty = (sys.stdout.isatty() and 'true') or 'false'
+    if config.get_colorbool('color.diff', stdout_is_tty) == 'true':
+        return ['--color']
+    else:
+        return []
+
 def check_local_changes():
     if git.local_changes():
         raise CmdException('local changes in the tree. Use "refresh" or'

--- a/stgit/config.py
+++ b/stgit/config.py
@@ -109,16 +110,18 @@ class GitConfig:
             if m:
                 result.append(m.group(1))
         return result
+
+    def get_colorbool(self, name, stdout_is_tty):
+        """Invoke 'git config --get-colorbool' and return the result."""
+        return Run('git', 'config', '--get-colorbool', name,
+                   stdout_is_tty).output_one_line()

 config=GitConfig()

>> diff --git a/stgit/config.py b/stgit/config.py
>> index efce097..4f16978 100644
>> --- a/stgit/config.py
>> +++ b/stgit/config.py
>> @@ -37,7 +37,8 @@ class GitConfig:
>>         'stgit.autoimerge':    'no',
>>         'stgit.keepoptimized': 'no',
>>         'stgit.extensions':    '.ancestor .current .patched',
>> -        'stgit.shortnr':        '5'
>> +        'stgit.shortnr': '5',
>> +        'stgit.pager':  'less -FRSX'
>>         }
>>       __cache={}
>
> Wrong indentation? :-)

The indentation is right, only that in the past there was a tab left
which I removed with this occasion.

-- 
Catalin

Re: [StGit PATCH 9/9] Use the default git colouring scheme rather than specific scripts

[Shinya Kuribayashi]

Catalin Marinas wrote:
> That's probably a better option. I changed the patch to this (only
> showing the relevant parts):
> 
> --- a/stgit/commands/common.py
> +++ b/stgit/commands/common.py
> @@ -97,6 +97,15 @@ def git_commit(name, repository, branch_name = None):
>      except libgit.RepositoryException:
>          raise CmdException('%s: Unknown patch or revision name' % name)
> 
> +def color_diff_flags():
> +    """Return the git flags for coloured diff output if the configuration and
> +    stdout allows."""
> +    stdout_is_tty = (sys.stdout.isatty() and 'true') or 'false'
> +    if config.get_colorbool('color.diff', stdout_is_tty) == 'true':
> +        return ['--color']
> +    else:
> +        return []
> +
>  def check_local_changes():
>      if git.local_changes():
>          raise CmdException('local changes in the tree. Use "refresh" or'
> 
> --- a/stgit/config.py
> +++ b/stgit/config.py
> @@ -109,16 +110,18 @@ class GitConfig:
>              if m:
>                  result.append(m.group(1))
>          return result
> +
> +    def get_colorbool(self, name, stdout_is_tty):
> +        """Invoke 'git config --get-colorbool' and return the result."""
> +        return Run('git', 'config', '--get-colorbool', name,
> +                   stdout_is_tty).output_one_line()
> 
>  config=GitConfig()

Proposed branch now works for me only with color.ui=auto.

Thanks!
--
Shinya Kuribayashi