Re: [StGit PATCH] edit: Allow setting git tree SHA1 of a patch

Catalin Marinas [off-list ref] writes:

Hi Gustav,

2010/5/16 Gustav Hållberg [off-list ref]:

quoted

I would like to have something similar to this patch, which allows for
setting the (git) tree of a particular patch. I would like to use it
(from the Emacs mode) to make it easier to split an old patch into two
(or more).

It might be that this is too "powerful" (read: unsafe), and maybe a
better (safer) command would use whatever is currently in the index
rather than a SHA1.

I'm not against such option (as long as it is somehow mentioned that's
dangerous) though I don't fully understand how one would use it,
especially when the patch is buried under other patches. With a series
of patches, any easily accessible tree (sha1) belongs to one of the
patches.

The idea is that Gustav wants to allow the editing of a file as it
appears in an earlier version. Lets say you have patches A, B, C and
D. You realize that one of the changes in to foo.c in C shuold really be
done in A. So you open the "A version of foo.c" in your editor, do the
change, and then save it. The save operation needs to update A to be
the new tree that contains the updated foo.c, and the remaining patches
will keep their tree. The effect is that the moved change now appears as
a diff in A, but not in C (nor B or D).

Working like this means that we don't really see the series as a string
of pateches, but as a series of named commits that we can go back and
edit. But this is a natural way of working with it once the tools get
powerful enough to support it.

-- 
David Kågedal

On 21 May 2010 14:59, David Kågedal [off-list ref] wrote:

Catalin Marinas [off-list ref] writes:

quoted

2010/5/16 Gustav Hållberg [off-list ref]:

quoted

I would like to have something similar to this patch, which allows for
setting the (git) tree of a particular patch. I would like to use it
(from the Emacs mode) to make it easier to split an old patch into two
(or more).

It might be that this is too "powerful" (read: unsafe), and maybe a
better (safer) command would use whatever is currently in the index
rather than a SHA1.

I'm not against such option (as long as it is somehow mentioned that's
dangerous) though I don't fully understand how one would use it,
especially when the patch is buried under other patches. With a series
of patches, any easily accessible tree (sha1) belongs to one of the
patches.

The idea is that Gustav wants to allow the editing of a file as it
appears in an earlier version. Lets say you have patches A, B, C and
D. You realize that one of the changes in to foo.c in C shuold really be
done in A. So you open the "A version of foo.c" in your editor, do the
change, and then save it. The save operation needs to update A to be
the new tree that contains the updated foo.c, and the remaining patches
will keep their tree. The effect is that the moved change now appears as
a diff in A, but not in C (nor B or D).

This is currently achieved by "pop B C D", edit file, "refresh", "push
--set-tree B C D".

Can "edit --set-tree <sha1>" make this simpler? Which <sha1> value
would be used with "edit --set-tree" (unless that's done by Emacs mode
behind the scene and it generates the tree that gets passed to edit).

Working like this means that we don't really see the series as a string
of patches, but as a series of named commits that we can go back and
edit. But this is a natural way of working with it once the tools get
powerful enough to support it.

That's looks a bit difficult (at least to me) since the commits are
usually chained. But, yes, as long as the resulting tree remains he
same, we could freely edit the tree corresponding to intermediate
patches.

-- 
Catalin

On 21 May 2010 14:59, David Kågedal [off-list ref] wrote:

quoted

The idea is that Gustav wants to allow the editing of a file as it
appears in an earlier version. Lets say you have patches A, B, C and
D. You realize that one of the changes in to foo.c in C shuold really be
done in A. So you open the "A version of foo.c" in your editor, do the
change, and then save it. The save operation needs to update A to be
the new tree that contains the updated foo.c, and the remaining patches
will keep their tree. The effect is that the moved change now appears as
a diff in A, but not in C (nor B or D).

David's example does not exactly describe the situation I have in
mind. I was only envisaging the possibility to move a change from one
patch to one of its neighbours. This is enforced by keeping all other
trees intact.

On Fri, May 21, 2010 at 5:16 PM, Catalin Marinas
[off-list ref] wro> This is currently achieved by "pop B
C D", edit file, "refresh", "push

--set-tree B C D".

Can "edit --set-tree <sha1>" make this simpler? Which <sha1> value
would be used with "edit --set-tree" (unless that's done by Emacs mode
behind the scene and it generates the tree that gets passed to edit).

This is indeed my assumption. Without a "smart" user interface to hide
the intricacies this operation becomes too complicated. At least
unless you work exclusively with the index. My prototype for the Emacs
mode approximately does 'read-tree <old patch sha1>', 'update-index
--cache-info <new blob>', 'stg edit --set-tree $(write-tree)'.

I actually think it is the use of the Emacs user interface that really
enabled us (me and my colleagues) to see the stack as a living set of
changes that are very easy to edit. This lead to the conclusion that
one wants to make it much easier, light-weight and faster to move
individual changes between (for a start, neighbouring) patches.

As you point out, there are a number of ways to do these things
already; this is all about making it very easy.

- Gustav

On 21 May 2010 16:32, Gustav Hållberg [off-list ref] wrote:

quoted

On 21 May 2010 14:59, David Kågedal [off-list ref] wrote:

quoted

The idea is that Gustav wants to allow the editing of a file as it
appears in an earlier version. Lets say you have patches A, B, C and
D. You realize that one of the changes in to foo.c in C shuold really be
done in A. So you open the "A version of foo.c" in your editor, do the
change, and then save it. The save operation needs to update A to be
the new tree that contains the updated foo.c, and the remaining patches
will keep their tree. The effect is that the moved change now appears as
a diff in A, but not in C (nor B or D).

David's example does not exactly describe the situation I have in
mind. I was only envisaging the possibility to move a change from one
patch to one of its neighbours. This is enforced by keeping all other
trees intact.

Yes, that's something commonly needed.

On Fri, May 21, 2010 at 5:16 PM, Catalin Marinas
[off-list ref] wro> This is currently achieved by "pop B
C D", edit file, "refresh", "push

quoted

--set-tree B C D".

Can "edit --set-tree <sha1>" make this simpler? Which <sha1> value
would be used with "edit --set-tree" (unless that's done by Emacs mode
behind the scene and it generates the tree that gets passed to edit).

This is indeed my assumption. Without a "smart" user interface to hide
the intricacies this operation becomes too complicated. At least
unless you work exclusively with the index. My prototype for the Emacs
mode approximately does 'read-tree <old patch sha1>', 'update-index
--cache-info <new blob>', 'stg edit --set-tree $(write-tree)'.

OK. As I said, I don't have  a problem with the patch. Maybe you could
mention in the help that it's usually meant for tools like Emacs,
otherwise people would wonder how to use it from the command line but
as it is, the patch looks fine.

I actually think it is the use of the Emacs user interface that really
enabled us (me and my colleagues) to see the stack as a living set of
changes that are very easy to edit. This lead to the conclusion that
one wants to make it much easier, light-weight and faster to move
individual changes between (for a start, neighbouring) patches.

I try to reduce the patch editing as much as possible since I need to
have some public branches that have an immutable history (hence the
stg publish command).

BTW, since you are a group of people using stgit, have you found a
useful way to share patches/series easily?

For example, one colleague works on a set of patches and I'd like his
included in my series but I don't want me to maintain those, so
periodically I would have to re-import his patches. There is a way to
use a combination of export and sync but it's not always easy to
follow.

StGit has the patches (diffs) in the a *.stgit branch but solving
conflicts in diff is problematic, so not sure how to use that for easy
synchronisation.

-- 
Catalin

On Fri, May 21, 2010 at 5:58 PM, Catalin Marinas
[off-list ref] wrote:

OK. As I said, I don't have  a problem with the patch. Maybe you could
mention in the help that it's usually meant for tools like Emacs,
otherwise people would wonder how to use it from the command line but
as it is, the patch looks fine.

Here's an updated patch. The only difference to the previous patch is
in the help text.

Note that it doesn't handle incorrect SHA1 sums very well.
In fact they often end up causing EPIPE as 'git commit-tree' exits
before all data has been written to it.

From a brief glance I cannot find any previous code that verifies the
correctness of a SHA1.
Any pointers to how I would add this would be welcome.
Would a new flag to Repository.get_tree() that makes it verify the
correctness be appropriate?
Or a new method in Repository?

- Gustav

----

Also fix capitalization in edit's short description.

Signed-off-by: Gustav Hållberg <redacted>
---
 stgit/commands/edit.py |   22 +++++++++++++++++++---
 t/t3300-edit.sh        |   15 +++++++++++++++
 2 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/stgit/commands/edit.py b/stgit/commands/edit.py
index f3f731f..a863611 100644
--- a/stgit/commands/edit.py
+++ b/stgit/commands/edit.py

@@ -24,7 +24,7 @@ from stgit.commands import common
 from stgit.lib import git as gitlib, transaction, edit
 from stgit.out import *

-help = 'edit a patch description or diff'
+help = 'Edit a patch description or diff'
 kind = 'patch'
 usage = ['[options] [--] [<patch>]']
 description = """

@@ -52,7 +52,14 @@ invoked even if such command-line options are given.)

 If the patch diff is edited but does not apply, no changes are made to
 the patch at all. The edited patch is saved to a file which you can
-feed to "stg edit --file", once you have made sure it does apply."""
+feed to "stg edit --file", once you have made sure it does apply.
+
+With --set-tree you set the git tree of the patch to the specified
+SHA1, without changing the tree of any other patches. When used on the
+top patch, the index and work tree will be updated to match the tree.
+This low-level option is primarily meant to be used by tools built on
+top of stgit, such as the Emacs mode. See also the --set-tree flag of
+stg push."""

 args = [argparse.applied_patches, argparse.unapplied_patches,
         argparse.hidden_patches]

@@ -61,6 +68,9 @@ options = [
         short = 'Edit the patch diff'),
     opt('-e', '--edit', action = 'store_true',
         short = 'Invoke interactive editor'),
+    opt('-t', '--set-tree', action = 'store',
+        metavar = 'SHA1',
+        short = 'Set the git tree of the patch to SHA1'),
     ] + (argparse.sign_options() +
          argparse.message_options(save_template = True) +
          argparse.author_options() + argparse.diff_opts_option())

@@ -86,6 +96,9 @@ def func(parser, options, args):

     cd = orig_cd = stack.patches.get(patchname).commit.data

+    if options.set_tree:
+        cd = cd.set_tree(stack.repository.get_tree(options.set_tree))
+
     cd, failed_diff = edit.auto_edit_patch(
         stack.repository, cd, msg = options.message, contains_diff = True,
         author = options.author, committer = lambda p: p,

@@ -128,7 +141,10 @@ def func(parser, options, args):
     trans.patches[patchname] = stack.repository.commit(cd)
     try:
         for pn in popped:
-            trans.push_patch(pn, iw, allow_interactive = True)
+            if options.set_tree:
+                trans.push_tree(pn)
+            else:
+                trans.push_patch(pn, iw, allow_interactive = True)
     except transaction.TransactionHalted:
         pass
     try:

diff --git a/t/t3300-edit.sh b/t/t3300-edit.sh
index 7003a27..078d4c3 100755
--- a/t/t3300-edit.sh
+++ b/t/t3300-edit.sh

@@ -212,4 +212,19 @@ test_expect_failure 'Fail to set invalid author date' '
     test "$(date HEAD)" = "2013-01-28 22:30:00 -0300"
 '

+test_expect_success 'Set patch tree SHA1' '
+    p2tree=$(git log -1 --pretty=format:%T $(stg id p2)) &&
+    p4tree=$(git log -1 --pretty=format:%T $(stg id p4)) &&
+    stg edit --set-tree $p4tree &&
+    test $(git write-tree) = $p4tree &&
+    grep "^333zz$" foo &&
+    stg pop &&
+    stg edit --set-tree $p2tree p2 &&
+    stg push --set-tree &&
+    test $(git write-tree) = $p2tree &&
+    grep "^333$" foo &&
+    stg edit --set-tree $p2tree p1 &&
+    test "$(echo $(stg series --empty --all))" = "+ p1 0> p2 - p3 ! p4"
+'
+
 test_done

-- 
1.7.0.4

I solved (I think) how to accept (and verify the correctness of) a
tree-ish as argument for 'edit --set-tree'. The first patch in the
series adds support to Repository.rev_parse() for doing that.

The main (the second) is essentially the same as before.

I moved the --set-tree option to be last in the list of options,
making it even more obscure-looking.

---

Gustav Hållberg (2):
      Repository.rev_parse: support commits, trees, and blobs
      edit: Allow setting git tree of a patch


 stgit/commands/edit.py |   39 ++++++++++++++++++++++++++++-----------
 stgit/lib/git.py       |   10 ++++++----
 t/t3300-edit.sh        |   15 +++++++++++++++
 3 files changed, 49 insertions(+), 15 deletions(-)

-- 

Also fix capitalization in edit's short description.

Signed-off-by: Gustav Hållberg <redacted>
---
 stgit/commands/edit.py |   39 ++++++++++++++++++++++++++++-----------
 t/t3300-edit.sh        |   15 +++++++++++++++
 2 files changed, 43 insertions(+), 11 deletions(-)

diff --git a/stgit/commands/edit.py b/stgit/commands/edit.py
index f3f731f..79335d0 100644
--- a/stgit/commands/edit.py
+++ b/stgit/commands/edit.py

@@ -24,7 +24,7 @@ from stgit.commands import common
 from stgit.lib import git as gitlib, transaction, edit
 from stgit.out import *
 
-help = 'edit a patch description or diff'
+help = 'Edit a patch description or diff'
 kind = 'patch'
 usage = ['[options] [--] [<patch>]']
 description = """

@@ -52,18 +52,28 @@ invoked even if such command-line options are given.)
 
 If the patch diff is edited but does not apply, no changes are made to
 the patch at all. The edited patch is saved to a file which you can
-feed to "stg edit --file", once you have made sure it does apply."""
+feed to "stg edit --file", once you have made sure it does apply.
+
+With --set-tree you set the git tree of the patch to the specified
+TREE-ISH without changing the tree of any other patches. When used on
+the top patch, the index and work tree will be updated to match the
+tree.  This low-level option is primarily meant to be used by tools
+built on top of StGit, such as the Emacs mode. See also the --set-tree
+flag of stg push."""
 
 args = [argparse.applied_patches, argparse.unapplied_patches,
         argparse.hidden_patches]
-options = [
-    opt('-d', '--diff', action = 'store_true',
-        short = 'Edit the patch diff'),
-    opt('-e', '--edit', action = 'store_true',
-        short = 'Invoke interactive editor'),
-    ] + (argparse.sign_options() +
-         argparse.message_options(save_template = True) +
-         argparse.author_options() + argparse.diff_opts_option())
+options = (
+    [ opt('-d', '--diff', action = 'store_true',
+          short = 'Edit the patch diff'),
+      opt('-e', '--edit', action = 'store_true',
+          short = 'Invoke interactive editor') ] +
+    argparse.sign_options() +
+    argparse.message_options(save_template = True) +
+    argparse.author_options() + argparse.diff_opts_option() +
+    [ opt('-t', '--set-tree', action = 'store',
+          metavar = 'TREE-ISH',
+          short = 'Set the git tree of the patch to TREE-ISH') ])
 
 directory = common.DirectoryHasRepositoryLib()
 

@@ -86,6 +96,10 @@ def func(parser, options, args):
 
     cd = orig_cd = stack.patches.get(patchname).commit.data
 
+    if options.set_tree:
+        cd = cd.set_tree(stack.repository.rev_parse(
+                options.set_tree, discard_stderr = True, object_type = 'tree'))
+
     cd, failed_diff = edit.auto_edit_patch(
         stack.repository, cd, msg = options.message, contains_diff = True,
         author = options.author, committer = lambda p: p,

@@ -128,7 +142,10 @@ def func(parser, options, args):
     trans.patches[patchname] = stack.repository.commit(cd)
     try:
         for pn in popped:
-            trans.push_patch(pn, iw, allow_interactive = True)
+            if options.set_tree:
+                trans.push_tree(pn)
+            else:
+                trans.push_patch(pn, iw, allow_interactive = True)
     except transaction.TransactionHalted:
         pass
     try:

diff --git a/t/t3300-edit.sh b/t/t3300-edit.sh
index 7003a27..09a2f25 100755
--- a/t/t3300-edit.sh
+++ b/t/t3300-edit.sh

@@ -212,4 +212,19 @@ test_expect_failure 'Fail to set invalid author date' '
     test "$(date HEAD)" = "2013-01-28 22:30:00 -0300"
 '
 
+test_expect_success 'Set patch tree' '
+    p2tree=$(git log -1 --pretty=format:%T $(stg id p2)) &&
+    p4commit=$(stg id p4) &&
+    stg edit --set-tree $p4commit &&
+    test $(git write-tree) = $(git rev-parse ${p4commit}^{tree}) &&
+    grep "^333zz$" foo &&
+    stg pop &&
+    stg edit --set-tree $p2tree p2 &&
+    stg push --set-tree &&
+    test $(git write-tree) = $p2tree &&
+    grep "^333$" foo &&
+    stg edit --set-tree $p2tree p1 &&
+    test "$(echo $(stg series --empty --all))" = "+ p1 0> p2 - p3 ! p4"
+'
+
 test_done

---
 stgit/lib/git.py |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/stgit/lib/git.py b/stgit/lib/git.py
index 65d2a6c..899c1a2 100644
--- a/stgit/lib/git.py
+++ b/stgit/lib/git.py

@@ -581,13 +581,15 @@ class Repository(RunWithEnv):
     refs = property(lambda self: self.__refs)
     def cat_object(self, sha1):
         return self.run(['git', 'cat-file', '-p', sha1]).raw_output()
-    def rev_parse(self, rev, discard_stderr = False):
+    def rev_parse(self, rev, discard_stderr = False, object_type = 'commit'):
+        assert object_type in ('commit', 'tree', 'blob')
+        getter = getattr(self, 'get_' + object_type)
         try:
-            return self.get_commit(self.run(
-                    ['git', 'rev-parse', '%s^{commit}' % rev]
+            return getter(self.run(
+                    ['git', 'rev-parse', '%s^{%s}' % (rev, object_type)]
                     ).discard_stderr(discard_stderr).output_one_line())
         except run.RunException:
-            raise RepositoryException('%s: No such revision' % rev)
+            raise RepositoryException('%s: No such %s' % (rev, object_type))
     def get_blob(self, sha1):
         return self.__blobs[sha1]
     def get_tree(self, sha1):

On 24 May 2010 19:52, Gustav Hållberg [off-list ref] wrote:

I solved (I think) how to accept (and verify the correctness of) a
tree-ish as argument for 'edit --set-tree'. The first patch in the
series adds support to Repository.rev_parse() for doing that.

The main (the second) is essentially the same as before.

I moved the --set-tree option to be last in the list of options,
making it even more obscure-looking.

The patches look fine to me.

Do you want me to merge them directly or I grab them via some git tree
(or Karl's).

Thanks.

-- 
Catalin

On Tue, May 25, 2010 at 2:26 PM, Catalin Marinas
[off-list ref] wrote:

The patches look fine to me.

Do you want me to merge them directly or I grab them via some git tree
(or Karl's).

You can grab them from git://github.com/gustavh/stgit.git in the
'set-tree' branch.

Somewhat unrelated, I have a string of patches in my 'proposed' branch
(same repository) which contain various fixes to the Emacs mode.
Would you like me to post them here as one patch series or would you
prefer to handle it some other way?

Karl has a bit too high latency these days, so it's inconvenient to go
via his branch(es).

- Gustav

On 26 May 2010 16:34, Gustav Hållberg [off-list ref] wrote:

On Tue, May 25, 2010 at 2:26 PM, Catalin Marinas
[off-list ref] wrote:

quoted

The patches look fine to me.

Do you want me to merge them directly or I grab them via some git tree
(or Karl's).

You can grab them from git://github.com/gustavh/stgit.git in the
'set-tree' branch.

I merged it. Thanks.

Somewhat unrelated, I have a string of patches in my 'proposed' branch
(same repository) which contain various fixes to the Emacs mode.
Would you like me to post them here as one patch series or would you
prefer to handle it some other way?

Well, I don't have the knowledge to review the emacs mode patches -
you are the maintainer. Just let me know when and where to pull the
patches from.

Thanks.

-- 
Catalin