[StGit PATCH 0/2] stg refresh -e/--edit

[StGit PATCH 0/2] stg refresh -e/--edit

[Karl Hasselström]

Jakub, this one's for you. ;-)

These live in my experimental branch for a little while yet, but the
plan is to include them in v0.15 -- and release that reasonably soon.

---

Karl Hasselström (2):
      Implement "stg refresh --edit" again
      Refactor stgit.commands.edit


 stgit/commands/coalesce.py |    2 -
 stgit/commands/common.py   |    9 +++-
 stgit/commands/edit.py     |   82 ++++++------------------------------
 stgit/commands/imprt.py    |    2 -
 stgit/commands/new.py      |    3 +
 stgit/commands/refresh.py  |   44 +++++++++++++++-----
 stgit/lib/edit.py          |   99 ++++++++++++++++++++++++++++++++++++++++++++
 stgit/utils.py             |   18 +++++---
 8 files changed, 167 insertions(+), 92 deletions(-)
 create mode 100644 stgit/lib/edit.py

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

[StGit PATCH 1/2] Refactor stgit.commands.edit

[Karl Hasselström]

Reorganize a few existing functions, and break out stuff from the main
function into subroutines.

While we're at it, move one of the old and all of the new functions to
stgit.lib.edit, so that we can use them in a later patch to implement
"stg refresh --edit".

Signed-off-by: Karl Hasselström <kha@treskal.com>

---

 stgit/commands/common.py |    9 +++-
 stgit/commands/edit.py   |   79 +++++--------------------------------
 stgit/commands/imprt.py  |    2 -
 stgit/lib/edit.py        |   99 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 117 insertions(+), 72 deletions(-)
 create mode 100644 stgit/lib/edit.py


diff --git a/stgit/commands/common.py b/stgit/commands/common.py
index 2689a42..c92554d 100644
--- a/stgit/commands/common.py
+++ b/stgit/commands/common.py
@@ -453,12 +453,15 @@ def parse_mail(msg):
 
     return (descr, authname, authemail, authdate, diff)
 
-def parse_patch(text):
+def parse_patch(text, contains_diff):
     """Parse the input text and return (description, authname,
     authemail, authdate, diff)
     """
-    descr, diff = __split_descr_diff(text)
-    descr, authname, authemail, authdate = __parse_description(descr)
+    if contains_diff:
+        (text, diff) = __split_descr_diff(text)
+    else:
+        diff = None
+    (descr, authname, authemail, authdate) = __parse_description(text)
 
     # we don't yet have an agreed place for the creation date.
     # Just return None
diff --git a/stgit/commands/edit.py b/stgit/commands/edit.py
index a8499c6..a9e8991 100644
--- a/stgit/commands/edit.py
+++ b/stgit/commands/edit.py
@@ -22,7 +22,7 @@ from optparse import make_option
 
 from stgit import git, utils
 from stgit.commands import common
-from stgit.lib import git as gitlib, transaction
+from stgit.lib import git as gitlib, transaction, edit
 from stgit.out import *
 
 help = 'edit a patch description or diff'
@@ -64,45 +64,6 @@ options = [make_option('-d', '--diff',
                 + utils.make_author_committer_options()
                 + utils.make_diff_opts_option())
 
-def patch_diff(repository, cd, diff, diff_flags):
-    if diff:
-        diff = repository.diff_tree(cd.parent.data.tree, cd.tree, diff_flags)
-        return '\n'.join([git.diffstat(diff), diff])
-    else:
-        return None
-
-def patch_description(cd, diff):
-    """Generate a string containing the description to edit."""
-
-    desc = ['From: %s <%s>' % (cd.author.name, cd.author.email),
-            'Date: %s' % cd.author.date.isoformat(),
-            '',
-            cd.message]
-    if diff:
-        desc += ['---',
-                 '',
-                diff]
-    return '\n'.join(desc)
-
-def patch_desc(repository, cd, failed_diff, diff, diff_flags):
-    return patch_description(cd, failed_diff or patch_diff(
-            repository, cd, diff, diff_flags))
-
-def update_patch_description(repository, cd, text):
-    message, authname, authemail, authdate, diff = common.parse_patch(text)
-    cd = (cd.set_message(message)
-            .set_author(cd.author.set_name(authname)
-                                 .set_email(authemail)
-                                 .set_date(gitlib.Date.maybe(authdate))))
-    failed_diff = None
-    if diff:
-        tree = repository.apply(cd.parent.data.tree, diff)
-        if tree == None:
-            failed_diff = diff
-        else:
-            cd = cd.set_tree(tree)
-    return cd, failed_diff
-
 def func(parser, options, args):
     """Edit the given patch or the current one.
     """
@@ -122,44 +83,26 @@ def func(parser, options, args):
 
     cd = orig_cd = stack.patches.get(patchname).commit.data
 
-    # Read patch from user-provided description.
-    if options.message == None:
-        failed_diff = None
-    else:
-        cd, failed_diff = update_patch_description(stack.repository, cd,
-                                                   options.message)
-
-    # Modify author and committer data.
-    a, c = options.author(cd.author), options.committer(cd.committer)
-    if (a, c) != (cd.author, cd.committer):
-        cd = cd.set_author(a).set_committer(c)
-
-    # Add Signed-off-by: or similar.
-    if options.sign_str != None:
-        cd = cd.set_message(utils.add_sign_line(
-                cd.message, options.sign_str, gitlib.Person.committer().name,
-                gitlib.Person.committer().email))
+    cd, failed_diff = edit.auto_edit_patch(
+        stack.repository, cd, msg = options.message, contains_diff = True,
+        author = options.author, committer = options.committer,
+        sign_str = options.sign_str)
 
     if options.save_template:
         options.save_template(
-            patch_desc(stack.repository, cd, failed_diff,
-                       options.diff, options.diff_flags))
+            patch_desc(stack.repository, cd,
+                       options.diff, options.diff_flags, failed_diff))
         return utils.STGIT_SUCCESS
 
-    # Let user edit the patch manually.
     if cd == orig_cd or options.edit:
-        fn = '.stgit-edit.' + ['txt', 'patch'][bool(options.diff)]
-        cd, failed_diff = update_patch_description(
-            stack.repository, cd, utils.edit_string(
-                patch_desc(stack.repository, cd, failed_diff,
-                           options.diff, options.diff_flags),
-                fn))
+        cd, failed_diff = edit.interactive_edit_patch(
+            stack.repository, cd, options.diff, options.diff_flags, failed_diff)
 
     def failed():
         fn = '.stgit-failed.patch'
         f = file(fn, 'w')
-        f.write(patch_desc(stack.repository, cd, failed_diff,
-                           options.diff, options.diff_flags))
+        f.write(patch_desc(stack.repository, cd,
+                           options.diff, options.diff_flags, failed_diff))
         f.close()
         out.error('Edited patch did not apply.',
                   'It has been saved to "%s".' % fn)
diff --git a/stgit/commands/imprt.py b/stgit/commands/imprt.py
index 5fb4da3..b958412 100644
--- a/stgit/commands/imprt.py
+++ b/stgit/commands/imprt.py
@@ -220,7 +220,7 @@ def __import_file(filename, options, patch = None):
                  parse_mail(msg)
     else:
         message, author_name, author_email, author_date, diff = \
-                 parse_patch(f.read())
+                 parse_patch(f.read(), contains_diff = True)
 
     if filename:
         f.close()
diff --git a/stgit/lib/edit.py b/stgit/lib/edit.py
new file mode 100644
index 0000000..6874355
--- /dev/null
+++ b/stgit/lib/edit.py
@@ -0,0 +1,99 @@
+"""This module contains utility functions for patch editing."""
+
+from stgit import utils
+from stgit.commands import common
+from stgit.lib import git
+
+def update_patch_description(repo, cd, text, contains_diff):
+    """Update the given L{CommitData<stgit.lib.git.CommitData>} with the
+    given text description, which may contain author name and time
+    stamp in addition to a new commit message. If C{contains_diff} is
+    true, it may also contain a replacement diff.
+
+    Return a pair: the new L{CommitData<stgit.lib.git.CommitData>};
+    and the diff text if it didn't apply, or C{None} otherwise."""
+    (message, authname, authemail, authdate, diff
+     ) = common.parse_patch(text, contains_diff)
+    a = cd.author
+    for val, setter in [(authname, 'set_name'), (authemail, 'set_email'),
+                        (git.Date.maybe(authdate), 'set_date')]:
+        if val != None:
+            a = getattr(a, setter)(val)
+    cd = cd.set_message(message).set_author(a)
+    failed_diff = None
+    if diff:
+        tree = repo.apply(cd.parent.data.tree, diff)
+        if tree == None:
+            failed_diff = diff
+        else:
+            cd = cd.set_tree(tree)
+    return cd, failed_diff
+
+def patch_desc(repo, cd, append_diff, diff_flags, replacement_diff):
+    """Return a description text for the patch, suitable for editing
+    and/or reimporting with L{update_patch_description()}.
+
+    @param cd: The L{CommitData<stgit.lib.git.CommitData>} to generate
+               a description of
+    @param append_diff: Whether to append the patch diff to the
+                        description
+    @type append_diff: C{bool}
+    @param diff_flags: Extra parameters to pass to C{git diff}
+    @param replacement_diff: Diff text to use; or C{None} if it should
+                             be computed from C{cd}
+    @type replacement_diff: C{str} or C{None}"""
+    desc = ['From: %s <%s>' % (cd.author.name, cd.author.email),
+            'Date: %s' % cd.author.date.isoformat(),
+            '',
+            cd.message]
+    if append_diff:
+        if replacement_diff:
+            diff = replacement_diff
+        else:
+            just_diff = repo.diff_tree(cd.parent.data.tree, cd.tree, diff_flags)
+            diff = '\n'.join([git.diffstat(just_diff), just_diff])
+        desc += ['---', '', diff]
+    return '\n'.join(desc)
+
+def interactive_edit_patch(repo, cd, edit_diff, diff_flags, replacement_diff):
+    """Edit the patch interactively. If C{edit_diff} is true, edit the
+    diff as well. If C{replacement_diff} is not C{None}, it contains a
+    diff to edit instead of the patch's real diff.
+
+    Return a pair: the new L{CommitData<stgit.lib.git.CommitData>};
+    and the diff text if it didn't apply, or C{None} otherwise."""
+    return update_patch_description(
+        repo, cd, utils.edit_string(
+            patch_desc(repo, cd, edit_diff, diff_flags, replacement_diff),
+            '.stgit-edit.' + ['txt', 'patch'][bool(edit_diff)]),
+        edit_diff)
+
+def auto_edit_patch(repo, cd, msg, contains_diff, author, committer, sign_str):
+    """Edit the patch noninteractively in a couple of ways:
+
+         - If C{msg} is not C{None}, parse it to find a replacement
+           message, and possibly also replacement author and
+           timestamp. If C{contains_diff} is true, also look for a
+           replacement diff.
+
+         - C{author} and C{committer} are two functions that take the
+           original L{Person<stgit.lib.git.Person>} value as argument,
+           and return the new one.
+
+         - C{sign_str}, if not C{None}, is a sign string to append to
+           the message.
+
+    Return a pair: the new L{CommitData<stgit.lib.git.CommitData>};
+    and the diff text if it didn't apply, or C{None} otherwise."""
+    if msg == None:
+        failed_diff = None
+    else:
+        cd, failed_diff = update_patch_description(repo, cd, msg, contains_diff)
+    a, c = author(cd.author), committer(cd.committer)
+    if (a, c) != (cd.author, cd.committer):
+        cd = cd.set_author(a).set_committer(c)
+    if sign_str != None:
+        cd = cd.set_message(utils.add_sign_line(
+                cd.message, sign_str, git.Person.committer().name,
+                git.Person.committer().email))
+    return cd, failed_diff

[StGit PATCH 2/2] Implement "stg refresh --edit" again

[Karl Hasselström]

The -e/--edit flag to "stg refresh" was dropped between v0.13 and
v0.14, causing severe user dissatisfaction. This patch restores it,
along with -m/--message, -f/--file, --sign, --ack, --author,
--authname, --authemail, and --authdate.

I omitted the --committer options on purpose; I think they are a
mistake. Falsifying the committer info is not a common operation, and
if one wishes to do it for some reason, one can always set the
GIT_COMMITTER_* environment variables.

Signed-off-by: Karl Hasselström <kha@treskal.com>

---

 stgit/commands/coalesce.py |    2 +-
 stgit/commands/edit.py     |    3 ++-
 stgit/commands/new.py      |    3 ++-
 stgit/commands/refresh.py  |   44 +++++++++++++++++++++++++++++++++-----------
 stgit/utils.py             |   18 ++++++++++++------
 5 files changed, 50 insertions(+), 20 deletions(-)


diff --git a/stgit/commands/coalesce.py b/stgit/commands/coalesce.py
index 1a34934..f3954ce 100644
--- a/stgit/commands/coalesce.py
+++ b/stgit/commands/coalesce.py
@@ -35,7 +35,7 @@ done a sequence of pushes and pops yourself."""
 
 directory = common.DirectoryHasRepositoryLib()
 options = [make_option('-n', '--name', help = 'name of coalesced patch')
-           ] + utils.make_message_options()
+           ] + utils.make_message_options(save_template = True)
 
 class SaveTemplateDone(Exception):
     pass
diff --git a/stgit/commands/edit.py b/stgit/commands/edit.py
index a9e8991..a83ec9c 100644
--- a/stgit/commands/edit.py
+++ b/stgit/commands/edit.py
@@ -60,7 +60,8 @@ options = [make_option('-d', '--diff',
                        action = 'store_true'),
            make_option('-e', '--edit', action = 'store_true',
                        help = 'invoke interactive editor'),
-           ] + (utils.make_sign_options() + utils.make_message_options()
+           ] + (utils.make_sign_options()
+                + utils.make_message_options(save_template = True)
                 + utils.make_author_committer_options()
                 + utils.make_diff_opts_option())
 
diff --git a/stgit/commands/new.py b/stgit/commands/new.py
index c4ee4e1..f468ab0 100644
--- a/stgit/commands/new.py
+++ b/stgit/commands/new.py
@@ -38,7 +38,8 @@ line of the commit message."""
 
 directory = common.DirectoryHasRepositoryLib()
 options = (utils.make_author_committer_options()
-           + utils.make_message_options() + utils.make_sign_options())
+           + utils.make_message_options(save_template = True)
+           + utils.make_sign_options())
 
 def func(parser, options, args):
     """Create a new patch."""
diff --git a/stgit/commands/refresh.py b/stgit/commands/refresh.py
index 7afc55e..37cb559 100644
--- a/stgit/commands/refresh.py
+++ b/stgit/commands/refresh.py
@@ -20,7 +20,7 @@ Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
 
 from optparse import make_option
 from stgit.commands import common
-from stgit.lib import git, transaction
+from stgit.lib import git, transaction, edit
 from stgit import utils
 
 help = 'generate a new commit for the current patch'
@@ -53,7 +53,11 @@ options = [make_option('-u', '--update', action = 'store_true',
                        help = ('only include changes from the index, not'
                                ' from the work tree')),
            make_option('-p', '--patch',
-                       help = 'refresh PATCH instead of the topmost patch')]
+                       help = 'refresh PATCH instead of the topmost patch'),
+           make_option('-e', '--edit', action = 'store_true',
+                       help = 'invoke an editor for the patch description')
+           ] + (utils.make_message_options(save_template = False)
+                + utils.make_sign_options() + utils.make_author_options())
 
 def get_patch(stack, given_patch):
     """Get the name of the patch we are to refresh."""
@@ -116,9 +120,12 @@ def make_temp_patch(stack, patch_name, paths, temp_index):
     return trans.run(stack.repository.default_iw,
                      print_current_patch = False), temp_name
 
-def absorb_applied(trans, iw, patch_name, temp_name):
+def absorb_applied(trans, iw, patch_name, temp_name, edit_fun):
     """Absorb the temp patch (C{temp_name}) into the given patch
-    (C{patch_name}), which must be applied.
+    (C{patch_name}), which must be applied. If the absorption
+    succeeds, call C{edit_fun} on the resulting
+    L{CommitData<stgit.lib.git.CommitData>} before committing it and
+    commit the return value.
 
     @return: C{True} if we managed to absorb the temp patch, C{False}
              if we had to leave it for the user to deal with."""
@@ -136,7 +143,7 @@ def absorb_applied(trans, iw, patch_name, temp_name):
         temp_cd = trans.patches[temp_name].data
         assert trans.patches[patch_name] == temp_cd.parent
         trans.patches[patch_name] = trans.stack.repository.commit(
-            trans.patches[patch_name].data.set_tree(temp_cd.tree))
+            edit_fun(trans.patches[patch_name].data.set_tree(temp_cd.tree)))
         popped = trans.delete_patches(lambda pn: pn == temp_name, quiet = True)
         assert not popped # the temp patch was topmost
         temp_absorbed = True
@@ -148,9 +155,12 @@ def absorb_applied(trans, iw, patch_name, temp_name):
         pass
     return temp_absorbed
 
-def absorb_unapplied(trans, iw, patch_name, temp_name):
+def absorb_unapplied(trans, iw, patch_name, temp_name, edit_fun):
     """Absorb the temp patch (C{temp_name}) into the given patch
-    (C{patch_name}), which must be unapplied.
+    (C{patch_name}), which must be unapplied. If the absorption
+    succeeds, call C{edit_fun} on the resulting
+    L{CommitData<stgit.lib.git.CommitData>} before committing it and
+    commit the return value.
 
     @param iw: Not used.
     @return: C{True} if we managed to absorb the temp patch, C{False}
@@ -174,7 +184,7 @@ def absorb_unapplied(trans, iw, patch_name, temp_name):
         # It worked. Refresh the patch with the new tree, and delete
         # the temp patch.
         trans.patches[patch_name] = trans.stack.repository.commit(
-            patch_cd.set_tree(new_tree))
+            edit_fun(patch_cd.set_tree(new_tree)))
         popped = trans.delete_patches(lambda pn: pn == temp_name, quiet = True)
         assert not popped # the temp patch was not applied
         return True
@@ -183,13 +193,13 @@ def absorb_unapplied(trans, iw, patch_name, temp_name):
         # leave the temp patch for the user.
         return False
 
-def absorb(stack, patch_name, temp_name):
+def absorb(stack, patch_name, temp_name, edit_fun):
     """Absorb the temp patch into the target patch."""
     trans = transaction.StackTransaction(stack, 'refresh')
     iw = stack.repository.default_iw
     f = { True: absorb_applied, False: absorb_unapplied
           }[patch_name in trans.applied]
-    if f(trans, iw, patch_name, temp_name):
+    if f(trans, iw, patch_name, temp_name, edit_fun):
         def info_msg(): pass
     else:
         def info_msg():
@@ -223,4 +233,16 @@ def func(parser, options, args):
         stack, patch_name, paths, temp_index = path_limiting)
     if retval != utils.STGIT_SUCCESS:
         return retval
-    return absorb(stack, patch_name, temp_name)
+    def edit_fun(cd):
+        cd, failed_diff = edit.auto_edit_patch(
+            stack.repository, cd, msg = options.message, contains_diff = False,
+            author = options.author, committer = lambda p: p,
+            sign_str = options.sign_str)
+        assert not failed_diff
+        if options.edit:
+            cd, failed_diff = edit.interactive_edit_patch(
+                stack.repository, cd, edit_diff = False,
+                diff_flags = [], replacement_diff = None)
+            assert not failed_diff
+        return cd
+    return absorb(stack, patch_name, temp_name, edit_fun)
diff --git a/stgit/utils.py b/stgit/utils.py
index 2983ea8..c40e666 100644
--- a/stgit/utils.py
+++ b/stgit/utils.py
@@ -264,13 +264,13 @@ def add_sign_line(desc, sign_str, name, email):
         desc = desc + '\n'
     return '%s\n%s\n' % (desc, sign_str)
 
-def make_message_options():
+def make_message_options(save_template):
     def no_dup(parser):
         if parser.values.message != None:
             raise optparse.OptionValueError(
                 'Cannot give more than one --message or --file')
     def no_combine(parser):
-        if (parser.values.message != None
+        if (save_template and parser.values.message != None
             and parser.values.save_template != None):
             raise optparse.OptionValueError(
                 'Cannot give both --message/--file and --save-template')
@@ -299,15 +299,18 @@ def make_message_options():
         parser.values.save_template = w
         no_combine(parser)
     m = optparse.make_option
-    return [m('-m', '--message', action = 'callback', callback = msg_callback,
+    opts = [m('-m', '--message', action = 'callback', callback = msg_callback,
               dest = 'message', type = 'string',
               help = 'use MESSAGE instead of invoking the editor'),
             m('-f', '--file', action = 'callback', callback = file_callback,
               dest = 'message', type = 'string', metavar = 'FILE',
-              help = 'use FILE instead of invoking the editor'),
+              help = 'use FILE instead of invoking the editor')]
+    if save_template:
+        opts.append(
             m('--save-template', action = 'callback', callback = templ_callback,
               metavar = 'FILE', dest = 'save_template', type = 'string',
-              help = 'save the message template to FILE and exit')]
+              help = 'save the message template to FILE and exit'))
+    return opts
 
 def make_diff_opts_option():
     def diff_opts_callback(option, opt_str, value, parser):
@@ -368,8 +371,11 @@ def make_person_options(person, short):
                 callback_args = (f,), help = 'set the %s %s' % (person, f))
                for f in ['name', 'email', 'date']])
 
+def make_author_options():
+    return make_person_options('author', 'auth')
+
 def make_author_committer_options():
-    return (make_person_options('author', 'auth')
+    return (make_author_options()
             + make_person_options('committer', 'comm'))
 
 # Exit codes.

Re: [StGit PATCH 2/2] Implement "stg refresh --edit" again

[Jakub Narebski]

Karl Hasselström napisał:

> The -e/--edit flag to "stg refresh" was dropped between v0.13 and
> v0.14, causing severe user dissatisfaction. This patch restores it

Thanks a lot!
-- 
Jakub Narebski
Poland

Re: [StGit PATCH 2/2] Implement "stg refresh --edit" again

[Catalin Marinas]

2008/7/4 Karl Hasselström <kha@treskal.com>:
> The -e/--edit flag to "stg refresh" was dropped between v0.13 and
> v0.14, causing severe user dissatisfaction. This patch restores it,
> along with -m/--message, -f/--file, --sign, --ack, --author,
> --authname, --authemail, and --authdate.

Thanks.

> I omitted the --committer options on purpose; I think they are a
> mistake. Falsifying the committer info is not a common operation, and
> if one wishes to do it for some reason, one can always set the
> GIT_COMMITTER_* environment variables.

I agree.

-- 
Catalin