[STGIT] stg refresh wish (splitting patches/removing files from a patch)

[STGIT] stg refresh wish (splitting patches/removing files from a patch)

[Peter Oberndorfer]

Hi,
I recently tried to split a stgit patch into 2 parts
and it was not as easy as i would like it to be.

How do i exclude a file from a patch(use version of file present in HEAD^)
without modifying the working dir?

with plain git i would use something like
git reset HEAD^ files_i_do_not_want_in_first_patch
git commit --amend
git add files_i_do_not_want_in_first_patch
git commit

So my idea was to add a --use-index [1] option to stg refresh
When it is passed stg refresh will use the current index for the contenst of the refreshed patch 
instead of looking at the working dir.
This would solve my problem[2] and also make it possible to use git-gui for 
staging hunks.

Do you think this would be a useful/good idea?
Or do we want a separate command for removing files from a patch anyway?

Another thing that might be useful (in my scenario) would be a stg commit --top extension
which commits at the top end of the stack
(unfortunately this will loose the patch history for splitting commits)
then i can edit this commits without being afraid of confusing stgit
and then stg assimilate /stg repair to make them managed by stg again

Greetings Peter

[1] rename by desire

[2] new way for splitting a patch with extension
git reset HEAD^ files_i_do_not_want_in_first_patch
stg refresh --use-index
stg refresh -e
git add files_i_do_not_want_in_first_patch
stg new
stg refresh --use-index

[STG PATCH] refresh: add a --index option which takes the contents of the index as the new commit

[Peter Oberndorfer]

just like git commit would do without the -a option
---
On Sonntag 30 Dezember 2007, Peter Oberndorfer wrote:
> Hi,
> I recently tried to split a stgit patch into 2 parts
> and it was not as easy as i would like it to be.
> 
> How do i exclude a file from a patch(use version of file present in HEAD^)
> without modifying the working dir?
> 
> with plain git i would use something like
> git reset HEAD^ files_i_do_not_want_in_first_patch
> git commit --amend
> git add files_i_do_not_want_in_first_patch
> git commit
> 
> So my idea was to add a --use-index [1] option to stg refresh
> When it is passed stg refresh will use the current index for the contenst of the refreshed patch 
> instead of looking at the working dir.
> This would solve my problem[2] and also make it possible to use git-gui for 
> staging hunks.
> 
OK, i got off my ass and hacked together the following patch
based on git://repo.or.cz/stgit/kha.git experimental
And i used it to create and update this commit a few times :-)
Additionally it passes the (minimal) test i added.
But since i am not a stgit expert it is highly recommend that somebody else carefully
looks at the patch before going wild on real data.

> Do you think this would be a useful/good idea?
> Or do we want a separate command for removing files from a patch anyway?
The question is still open if this is useful for somebody else.

Greetings Peter

PS: i hope i put the right people on CC

 stgit/commands/refresh.py |   25 ++++++++++++++++---
 stgit/stack.py            |    6 ++++
 t/t2700-refresh.sh        |   57 ++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 83 insertions(+), 5 deletions(-)

diff --git a/stgit/commands/refresh.py b/stgit/commands/refresh.py
index 6e8ed0c..0420b98 100644
--- a/stgit/commands/refresh.py
+++ b/stgit/commands/refresh.py
@@ -45,6 +45,9 @@ options = [make_option('-f', '--force',
            make_option('--update',
                        help = 'only update the current patch files',
                        action = 'store_true'),
+           make_option('--index',
+                       help = 'use the current contents of the index instead of looking at the working directory',
+                       action = 'store_true'),
            make_option('--undo',
                        help = 'revert the commit generated by the last refresh',
                        action = 'store_true'),
@@ -76,6 +79,14 @@ def func(parser, options, args):
         if not patch:
             raise CmdException, 'No patches applied'
 
+    if options.index:
+        if args or options.update:
+            raise CmdException, \
+                  'Only full refresh is available with the --index option'
+        if options.patch:
+            raise CmdException, \
+                  '--patch is not (yet) compatible with --index option'
+
     if not options.force:
         check_head_top_equal(crt_series)
 
@@ -85,9 +96,10 @@ def func(parser, options, args):
         out.done()
         return
 
-    files = [path for (stat, path) in git.tree_status(files = args, verbose = True)]
+    if not options.index:
+        files = [path for (stat, path) in git.tree_status(files = args, verbose = True)]
 
-    if files or not crt_series.head_top_equal():
+    if options.index or files or not crt_series.head_top_equal():
         if options.patch:
             applied = crt_series.get_applied()
             between = applied[:applied.index(patch):-1]
@@ -105,8 +117,13 @@ def func(parser, options, args):
 
         if autoresolved == 'yes':
             resolved_all()
-        crt_series.refresh_patch(files = files,
-                                 backup = True, notes = options.annotate)
+
+        if options.index:
+            crt_series.refresh_patch(use_index = True,
+                                     backup = True, notes = options.annotate)
+        else:
+            crt_series.refresh_patch(files = files,
+                                     backup = True, notes = options.annotate)
 
         if crt_series.empty_patch(patch):
             out.done('empty patch')
diff --git a/stgit/stack.py b/stgit/stack.py
index 4203931..7d14261 100644
--- a/stgit/stack.py
+++ b/stgit/stack.py
@@ -668,6 +668,7 @@ class Series(PatchSet):
         config.remove_section('branch.%s.stgit' % self.get_name())
 
     def refresh_patch(self, files = None, message = None, edit = False,
+                      use_index = False,
                       empty = False,
                       show_patch = False,
                       cache_update = True,
@@ -717,6 +718,11 @@ class Series(PatchSet):
         else:
             tree_id = None
 
+        if use_index:
+            tree_id = None
+            files = None
+            cache_update = False
+
         commit_id = git.commit(files = files,
                                message = descr, parents = [bottom],
                                cache_update = cache_update,
diff --git a/t/t2700-refresh.sh b/t/t2700-refresh.sh
index 2e7901c..9eae85d 100755
--- a/t/t2700-refresh.sh
+++ b/t/t2700-refresh.sh
@@ -6,8 +6,10 @@ test_description='Run "stg refresh"'
 
 test_expect_success 'Initialize StGit stack' '
     stg init &&
-    echo expected.txt >> .git/info/exclude &&
+    echo expected*.txt >> .git/info/exclude &&
     echo patches.txt >> .git/info/exclude &&
+    echo show.txt >> .git/info/exclude &&
+    echo diff.txt >> .git/info/exclude &&
     stg new p0 -m "base" &&
     for i in 1 2 3; do
         echo base >> foo$i.txt &&
@@ -62,4 +64,57 @@ test_expect_success 'Refresh bottom patch' '
     diff -u expected.txt patches.txt
 '
 
+cat > expected.txt <<EOF
+p0
+p1
+p4
+EOF
+cat > expected2.txt <<EOF
+diff --git a/foo1.txt b/foo1.txt
+index 728535d..6f34984 100644
+--- a/foo1.txt
++++ b/foo1.txt
+@@ -1,3 +1,4 @@
+ base
+ foo 1
+ bar 1
++baz 1
+EOF
+cat > expected3.txt <<EOF
+diff --git a/foo1.txt b/foo1.txt
+index 6f34984..a80eb63 100644
+--- a/foo1.txt
++++ b/foo1.txt
+@@ -2,3 +2,4 @@ base
+ foo 1
+ bar 1
+ baz 1
++blah 1
+diff --git a/foo2.txt b/foo2.txt
+index 415c9f5..43168f2 100644
+--- a/foo2.txt
++++ b/foo2.txt
+@@ -1,3 +1,4 @@
+ base
+ foo 2
+ bar 2
++baz 2
+EOF
+test_expect_success 'Refresh --index' '
+    stg status &&
+    stg new p4 -m "refresh_index" &&
+    echo baz 1 >> foo1.txt &&
+    git add foo1.txt &&
+    echo blah 1 >> foo1.txt &&
+    echo baz 2 >> foo2.txt &&
+    stg refresh --index &&
+    stg patches foo1.txt > patches.txt &&
+    git diff HEAD^..HEAD > show.txt &&
+    stg diff > diff.txt &&
+    diff -u expected.txt patches.txt &&
+    diff -u expected2.txt show.txt &&
+    diff -u expected3.txt diff.txt &&
+    stg new p5 -m "cleanup again" &&
+    stg refresh
+'
 test_done
-- 
1.5.4.rc2

Re: [STG PATCH] refresh: add a --index option which takes the contents of the index as the new commit

[Karl Hasselström]

On 2008-01-02 20:39:27 +0100, Peter Oberndorfer wrote:

> On Sonntag 30 Dezember 2007, Peter Oberndorfer wrote:
>
> > Do you think this would be a useful/good idea? Or do we want a
> > separate command for removing files from a patch anyway?
>
> The question is still open if this is useful for somebody else.

I think it's a useful addition. Thanks!

> diff --git a/stgit/stack.py b/stgit/stack.py
> index 4203931..7d14261 100644
> --- a/stgit/stack.py
> +++ b/stgit/stack.py
> @@ -668,6 +668,7 @@ class Series(PatchSet):
>          config.remove_section('branch.%s.stgit' % self.get_name())
>  
>      def refresh_patch(self, files = None, message = None, edit = False,
> +                      use_index = False,
>                        empty = False,
>                        show_patch = False,
>                        cache_update = True,
> @@ -717,6 +718,11 @@ class Series(PatchSet):
>          else:
>              tree_id = None
>  
> +        if use_index:
> +            tree_id = None
> +            files = None
> +            cache_update = False
> +
>          commit_id = git.commit(files = files,
>                                 message = descr, parents = [bottom],
>                                 cache_update = cache_update,

So the use_index parameter to refresh_patch is actually not necessary?
In that case I'd rather you didn't add it, since the functions in
stgit/stack.py have quite enough parameters already.

> diff --git a/t/t2700-refresh.sh b/t/t2700-refresh.sh
> index 2e7901c..9eae85d 100755
> --- a/t/t2700-refresh.sh
> +++ b/t/t2700-refresh.sh

Bonus points for adding a test case!

I still haven't rebased my patch stack since Catalin accepted most of
it just before Christmas. Once I've gotten around to that, I'll take
your patch -- hopefully by then updated to not add the exra argument
to refresh_patch(). :-)

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

Re: [STG PATCH] refresh: add a --index option which takes the contents of the index as the new commit

[Karl Hasselström]

On 2008-01-07 11:56:12 +0100, Karl Hasselström wrote:

> hopefully by then updated to not add the exra argument to
> refresh_patch(). :-)

And with a sign-off, too, please.

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

[STG PATCH] add a --index option to refresh which takes the contents of the index as the new commit

[Peter Oberndorfer]

This allows to stage only certain changes to a file by only adding
the desired parts to the index with git-gui, ugit, git add -i or another tool
that manipulates the index and then run stg refresh --index
it also allows removing a file from a patch by running git reset HEAD^ -- file_to_remove
followed by a stg refresh --index

Signed-off-by: Peter Oberndorfer <kumbayo84@arcor.de>
---

On Montag 07 Januar 2008, Karl Hasselström wrote:
> On 2008-01-02 20:39:27 +0100, Peter Oberndorfer wrote:
> 
> > On Sonntag 30 Dezember 2007, Peter Oberndorfer wrote:
> >
> > > Do you think this would be a useful/good idea? Or do we want a
> > > separate command for removing files from a patch anyway?
> >
> > The question is still open if this is useful for somebody else.
> 
> I think it's a useful addition. Thanks!
Good since it was useful for me too even while writing this patch :-)

> So the use_index parameter to refresh_patch is actually not necessary?
> In that case I'd rather you didn't add it, since the functions in
> stgit/stack.py have quite enough parameters already.
> 
In the beginning i was afraid it would be to obscure to call it this way
with all parameters set to some specific values.
But having more parameters does not make it better :-)
Done
> > diff --git a/t/t2700-refresh.sh b/t/t2700-refresh.sh
> > index 2e7901c..9eae85d 100755
> > --- a/t/t2700-refresh.sh
> > +++ b/t/t2700-refresh.sh
> 
> Bonus points for adding a test case!
> 
> I still haven't rebased my patch stack since Catalin accepted most of
> it just before Christmas. Once I've gotten around to that, I'll take
> your patch -- hopefully by then updated to not add the exra argument
> to refresh_patch(). :-)
> 

Patch now comes with a Signed-off-by and a log message that explains
how this feature could be used.
It was tested with the testcase, used during development of this patch
and on another repo, but still take care when using it :-)

 stgit/commands/refresh.py |   25 ++++++++++++++++---
 t/t2700-refresh.sh        |   57 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 77 insertions(+), 5 deletions(-)

diff --git a/stgit/commands/refresh.py b/stgit/commands/refresh.py
index 6e8ed0c..952b1b6 100644
--- a/stgit/commands/refresh.py
+++ b/stgit/commands/refresh.py
@@ -45,6 +45,9 @@ options = [make_option('-f', '--force',
            make_option('--update',
                        help = 'only update the current patch files',
                        action = 'store_true'),
+           make_option('--index',
+                       help = 'use the current contents of the index instead of looking at the working directory',
+                       action = 'store_true'),
            make_option('--undo',
                        help = 'revert the commit generated by the last refresh',
                        action = 'store_true'),
@@ -76,6 +79,14 @@ def func(parser, options, args):
         if not patch:
             raise CmdException, 'No patches applied'
 
+    if options.index:
+        if args or options.update:
+            raise CmdException, \
+                  'Only full refresh is available with the --index option'
+        if options.patch:
+            raise CmdException, \
+                  '--patch is not compatible with the --index option'
+
     if not options.force:
         check_head_top_equal(crt_series)
 
@@ -85,9 +96,10 @@ def func(parser, options, args):
         out.done()
         return
 
-    files = [path for (stat, path) in git.tree_status(files = args, verbose = True)]
+    if not options.index:
+        files = [path for (stat, path) in git.tree_status(files = args, verbose = True)]
 
-    if files or not crt_series.head_top_equal():
+    if options.index or files or not crt_series.head_top_equal():
         if options.patch:
             applied = crt_series.get_applied()
             between = applied[:applied.index(patch):-1]
@@ -105,8 +117,13 @@ def func(parser, options, args):
 
         if autoresolved == 'yes':
             resolved_all()
-        crt_series.refresh_patch(files = files,
-                                 backup = True, notes = options.annotate)
+
+        if options.index:
+            crt_series.refresh_patch(cache_update = False,
+                                     backup = True, notes = options.annotate)
+        else:
+            crt_series.refresh_patch(files = files,
+                                     backup = True, notes = options.annotate)
 
         if crt_series.empty_patch(patch):
             out.done('empty patch')
diff --git a/t/t2700-refresh.sh b/t/t2700-refresh.sh
index 2e7901c..9eae85d 100755
--- a/t/t2700-refresh.sh
+++ b/t/t2700-refresh.sh
@@ -6,8 +6,10 @@ test_description='Run "stg refresh"'
 
 test_expect_success 'Initialize StGit stack' '
     stg init &&
-    echo expected.txt >> .git/info/exclude &&
+    echo expected*.txt >> .git/info/exclude &&
     echo patches.txt >> .git/info/exclude &&
+    echo show.txt >> .git/info/exclude &&
+    echo diff.txt >> .git/info/exclude &&
     stg new p0 -m "base" &&
     for i in 1 2 3; do
         echo base >> foo$i.txt &&
@@ -62,4 +64,57 @@ test_expect_success 'Refresh bottom patch' '
     diff -u expected.txt patches.txt
 '
 
+cat > expected.txt <<EOF
+p0
+p1
+p4
+EOF
+cat > expected2.txt <<EOF
+diff --git a/foo1.txt b/foo1.txt
+index 728535d..6f34984 100644
+--- a/foo1.txt
++++ b/foo1.txt
+@@ -1,3 +1,4 @@
+ base
+ foo 1
+ bar 1
++baz 1
+EOF
+cat > expected3.txt <<EOF
+diff --git a/foo1.txt b/foo1.txt
+index 6f34984..a80eb63 100644
+--- a/foo1.txt
++++ b/foo1.txt
+@@ -2,3 +2,4 @@ base
+ foo 1
+ bar 1
+ baz 1
++blah 1
+diff --git a/foo2.txt b/foo2.txt
+index 415c9f5..43168f2 100644
+--- a/foo2.txt
++++ b/foo2.txt
+@@ -1,3 +1,4 @@
+ base
+ foo 2
+ bar 2
++baz 2
+EOF
+test_expect_success 'Refresh --index' '
+    stg status &&
+    stg new p4 -m "refresh_index" &&
+    echo baz 1 >> foo1.txt &&
+    git add foo1.txt &&
+    echo blah 1 >> foo1.txt &&
+    echo baz 2 >> foo2.txt &&
+    stg refresh --index &&
+    stg patches foo1.txt > patches.txt &&
+    git diff HEAD^..HEAD > show.txt &&
+    stg diff > diff.txt &&
+    diff -u expected.txt patches.txt &&
+    diff -u expected2.txt show.txt &&
+    diff -u expected3.txt diff.txt &&
+    stg new p5 -m "cleanup again" &&
+    stg refresh
+'
 test_done
-- 
1.5.4.rc2

Re: [STG PATCH] add a --index option to refresh which takes the contents of the index as the new commit

[Karl Hasselström]

On 2008-01-08 21:42:46 +0100, Peter Oberndorfer wrote:

> On Montag 07 Januar 2008, Karl Hasselström wrote:
>
> > So the use_index parameter to refresh_patch is actually not
> > necessary? In that case I'd rather you didn't add it, since the
> > functions in stgit/stack.py have quite enough parameters already.
>
> In the beginning i was afraid it would be to obscure to call it this
> way with all parameters set to some specific values. But having more
> parameters does not make it better :-) Done

Thanks.

> Patch now comes with a Signed-off-by and a log message that explains
> how this feature could be used. It was tested with the testcase,
> used during development of this patch and on another repo, but still
> take care when using it :-)

I may be promising too much now, but hopefully I'll get to this
tonight.

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

Re: [STG PATCH] add a --index option to refresh which takes the contents of the index as the new commit

[Karl Hasselström]

On 2008-01-09 08:23:58 +0100, Karl Hasselström wrote:

> On 2008-01-08 21:42:46 +0100, Peter Oberndorfer wrote:
>
> > Patch now comes with a Signed-off-by and a log message that
> > explains how this feature could be used. It was tested with the
> > testcase, used during development of this patch and on another
> > repo, but still take care when using it :-)
>
> I may be promising too much now, but hopefully I'll get to this
> tonight.

I've rebased my patch stack on top of Catalin's master now, and put
your two patches on top. The --index patch is only in experimental and
not stable, since you recommend further testing.

I massaged the commit messages slightly, mainly to get a reasonably
short first line and end all sentences with a period (except on the
first line, of course!).

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