stgit: bug with refresh and -p

stgit: bug with refresh and -p

[Jon Smirl]

A refresh with -p is not picking up newly added files.

jonsmirl@terra:~/fs$ stg status
? include/linux/i2c/max9485.h
jonsmirl@terra:~/fs$ git add include/linux/i2c/max9485.h
jonsmirl@terra:~/fs$ stg refresh -p max9485
Checking for changes in the working directory ... done
Popping patches "jds-audio" - "jds-psc" ... done
Refreshing patch "max9485" ... done
Fast-forwarded patches "jds-psc" - "jds-audio"
jonsmirl@terra:~/fs$ stg status
? include/linux/i2c/max9485.h
jonsmirl@terra:~/fs$ stg --version
Stacked GIT 0.14.3.163.g06f9
git version 1.5.6.1
Python version 2.5.2 (r252:60911, Apr 21 2008, 11:17:30)
[GCC 4.2.3 (Ubuntu 4.2.3-2ubuntu7)]
jonsmirl@terra:~/fs$

-- 
Jon Smirl
jonsmirl@gmail.com

Re: stgit: bug with refresh and -p

[Karl Hasselström]

On 2008-07-17 21:29:01 -0400, Jon Smirl wrote:

> A refresh with -p is not picking up newly added files.
>
> jonsmirl@terra:~/fs$ stg status
> ? include/linux/i2c/max9485.h
> jonsmirl@terra:~/fs$ git add include/linux/i2c/max9485.h
> jonsmirl@terra:~/fs$ stg refresh -p max9485
> Checking for changes in the working directory ... done
> Popping patches "jds-audio" - "jds-psc" ... done
> Refreshing patch "max9485" ... done
> Fast-forwarded patches "jds-psc" - "jds-audio"
> jonsmirl@terra:~/fs$ stg status
> ? include/linux/i2c/max9485.h
> jonsmirl@terra:~/fs$ stg --version
> Stacked GIT 0.14.3.163.g06f9

Hmm, I don't immediately see why this happens. I've posted a bug about
it: https://gna.org/bugs/index.php?12038

It's fixed by the refresh rewrite in my experimental branch, but
there's no test for it, so I had to check by hand.

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

[StGit PATCH] Test that we can add a new file to a non-topmost patch with refresh -p

[Karl Hasselström]

We currently can't -- this is bug 12038, found by Jon Smirl. See

  https://gna.org/bugs/index.php?12038

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

---

Here's a proper test that demonstrates the bug. It applies to the
stable branch.

 t/t2701-refresh-p.sh |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 46 insertions(+), 0 deletions(-)
 create mode 100755 t/t2701-refresh-p.sh


diff --git a/t/t2701-refresh-p.sh b/t/t2701-refresh-p.sh
new file mode 100755
index 0000000..d42e90f
--- /dev/null
+++ b/t/t2701-refresh-p.sh
@@ -0,0 +1,46 @@
+#!/bin/sh
+
+test_description='Run "stg refresh -p"'
+
+. ./test-lib.sh
+
+# Ignore our own temp files.
+cat >> .git/info/exclude <<EOF
+expected*.txt
+files*.txt
+status*.txt
+EOF
+
+test_expect_success 'Initialize StGit stack' '
+    stg init &&
+    for i in 1 2; do
+        echo x > $i.txt &&
+        git add $i.txt &&
+        stg new p$i -m "Patch $i" &&
+        stg refresh
+    done
+'
+
+touch expected0.txt
+cat > expected1.txt <<EOF
+A 1.txt
+A new.txt
+EOF
+cat > expected2.txt <<EOF
+A 2.txt
+EOF
+test_expect_failure 'Add new file to non-top patch' '
+    stg status > status1.txt &&
+    diff -u expected0.txt status1.txt &&
+    echo y > new.txt &&
+    git add new.txt &&
+    stg refresh -p p1 &&
+    stg status > status2.txt &&
+    diff -u expected0.txt status2.txt &&
+    stg files p1 > files1.txt &&
+    diff -u expected1.txt files1.txt &&
+    stg files p2 > files2.txt &&
+    diff -u expected2.txt files2.txt
+'
+
+test_done

Re: [StGit PATCH] Test that we can add a new file to a non-topmost patch with refresh -p

[Karl Hasselström]

On 2008-07-18 19:03:06 +0200, Karl Hasselström wrote:

> We currently can't -- this is bug 12038, found by Jon Smirl. See
>
>   https://gna.org/bugs/index.php?12038

OK, the problem is that to pop the patches on top of the one we are to
refresh, we call pop_patch(keep = True). This in turn calls
git.switch(keep = True), which resets the index (but is careful to not
touch the worktree).

I'm not quite sure how to fix this in a simple way -- the code simply
assumes that the index contains nothing of interest. And since I
already have a rewrite of refresh that handles this and a handful of
other cases that the old code does not, I'm kind of disinclined to
undertake a larger restructuring of the code.

Catalin, what do you think?

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

Re: [StGit PATCH] Test that we can add a new file to a non-topmost patch with refresh -p

[Jon Smirl]

On 7/18/08, Karl Hasselström <kha@treskal.com> wrote:
> On 2008-07-18 19:03:06 +0200, Karl Hasselström wrote:
>
>  > We currently can't -- this is bug 12038, found by Jon Smirl. See
>  >
>  >   https://gna.org/bugs/index.php?12038
>
>  OK, the problem is that to pop the patches on top of the one we are to
>  refresh, we call pop_patch(keep = True). This in turn calls
>  git.switch(keep = True), which resets the index (but is careful to not
>  touch the worktree).
>
>  I'm not quite sure how to fix this in a simple way -- the code simply
>  assumes that the index contains nothing of interest. And since I
>  already have a rewrite of refresh that handles this and a handful of
>  other cases that the old code does not, I'm kind of disinclined to
>  undertake a larger restructuring of the code.

It's no big deal to me, it is easy to work around. But it did take me
a while to notice that the add was missing.

When is the refresh rewrite going to be ready for prime time?

>
>  Catalin, what do you think?
>
>
>  --
>  Karl Hasselström, kha@treskal.com
>       www.treskal.com/kalle
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [StGit PATCH] Test that we can add a new file to a non-topmost patch with refresh -p

[Karl Hasselström]

On 2008-07-18 13:45:41 -0400, Jon Smirl wrote:

> It's no big deal to me, it is easy to work around. But it did take
> me a while to notice that the add was missing.

Ah, OK, good.

> When is the refresh rewrite going to be ready for prime time?

It depends on the stack log stuff, but that is actually kind of close
to being ready now. It just needs support for hidden patches.

No time estimate, sorry -- vacation time's coming up, so I can't
really predict how much time I'll spend hacking.

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

Re: [StGit PATCH] Test that we can add a new file to a non-topmost patch with refresh -p

[Catalin Marinas]

2008/7/18 Karl Hasselström <kha@treskal.com>:
> On 2008-07-18 19:03:06 +0200, Karl Hasselström wrote:
>
>> We currently can't -- this is bug 12038, found by Jon Smirl. See
>>
>>   https://gna.org/bugs/index.php?12038
>
> OK, the problem is that to pop the patches on top of the one we are to
> refresh, we call pop_patch(keep = True). This in turn calls
> git.switch(keep = True), which resets the index (but is careful to not
> touch the worktree).

Yes.

> I'm not quite sure how to fix this in a simple way -- the code simply
> assumes that the index contains nothing of interest. And since I
> already have a rewrite of refresh that handles this and a handful of
> other cases that the old code does not, I'm kind of disinclined to
> undertake a larger restructuring of the code.
>
> Catalin, what do you think?

I don't think we should spend time on fixing the current code as you
already have a new implementation. Maybe we could add a simple test
and refuse refreshing other than the topmost patch in case of files
added to the index.

-- 
Catalin

Re: [StGit PATCH] Test that we can add a new file to a non-topmost patch with refresh -p

[Karl Hasselström]

On 2008-07-21 23:01:17 +0100, Catalin Marinas wrote:

> I don't think we should spend time on fixing the current code as you
> already have a new implementation. Maybe we could add a simple test
> and refuse refreshing other than the topmost patch in case of files
> added to the index.

Yes, I guess that's OK. Hmm, how do we check that cheaply?

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

Re: [StGit PATCH] Test that we can add a new file to a non-topmost patch with refresh -p

[Catalin Marinas]

2008/7/22 Karl Hasselström <kha@treskal.com>:
> On 2008-07-21 23:01:17 +0100, Catalin Marinas wrote:
>
>> I don't think we should spend time on fixing the current code as you
>> already have a new implementation. Maybe we could add a simple test
>> and refuse refreshing other than the topmost patch in case of files
>> added to the index.
>
> Yes, I guess that's OK. Hmm, how do we check that cheaply?

In the stable branch, we already call git.check_status() in the
refresh implementation, so it's pretty cheap to test.

-- 
Catalin