stg clean removes conflicting patch

stg clean removes conflicting patch

[Pavel Roskin]

Hello!

If "stg push" fails, the subsequent "stg clean" will remove the patch
that could not been applied.  I think it's wrong.  Especially when doing
"stg pull", it can happen that I want to run "stg clean" to get rid of
the patches applied upstream so I can concentrate on the conflict.
Instead, the conflicting patch is removed too.

I've made a patch for the testsuite that should pass once the bug is
fixed.  Try removing "stg clean" from the test. and it will pass.  But
"stg clean" should make no difference here.

Add test to ensure that "stg clean" preserves conflicting patches

From: Pavel Roskin <proski@gnu.org>

Signed-off-by: Pavel Roskin <proski@gnu.org>
---

 t/t2500-clean.sh |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)


diff --git a/t/t2500-clean.sh b/t/t2500-clean.sh
index 3364c18..ad8f892 100755
--- a/t/t2500-clean.sh
+++ b/t/t2500-clean.sh
@@ -24,4 +24,21 @@ test_expect_success 'Clean empty patches' '
     [ "$(echo $(stg unapplied))" = "" ]
 '
 
+test_expect_success 'Create a conflict' '
+    stg new p1 -m p1 &&
+    echo bar > foo.txt &&
+    stg refresh &&
+    stg pop &&
+    stg new p2 -m p2
+    echo quux > foo.txt &&
+    stg refresh &&
+    ! stg push
+'
+
+test_expect_success 'Make sure conflicting patches are preserved' '
+    stg clean &&
+    [ "$(echo $(stg applied))" = "p0 p2 p1" ] &&
+    [ "$(echo $(stg unapplied))" = "" ]
+'
+
 test_done


-- 
Regards,
Pavel Roskin

Re: stg clean removes conflicting patch

[Karl Hasselström]

On 2008-01-24 22:55:17 -0500, Pavel Roskin wrote:

> If "stg push" fails, the subsequent "stg clean" will remove the
> patch that could not been applied. I think it's wrong.

I agree. It's consistent -- a conflicting patch is empty -- but
clearly the wrong thing to do from a usability perspective.

> I've made a patch for the testsuite that should pass once the bug is
> fixed. Try removing "stg clean" from the test. and it will pass. But
> "stg clean" should make no difference here.

Good!

For known-to-be-failing tests, you can use test_expect_failure. I'll
amend your patch to do that when I pick it up (if Catalin doesn't beat
me to it).

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

Re: stg clean removes conflicting patch

[Pavel Roskin]

Quoting Karl Hasselström <kha@treskal.com>:

> For known-to-be-failing tests, you can use test_expect_failure. I'll
> amend your patch to do that when I pick it up (if Catalin doesn't beat
> me to it).

Yes, please go ahead with the change.  I just wasn't aware of it.

-- 
Regards,
Pavel Roskin

Re: stg clean removes conflicting patch

[Catalin Marinas]

On 25/01/2008, Karl Hasselström <kha@treskal.com> wrote:
> On 2008-01-24 22:55:17 -0500, Pavel Roskin wrote:
>
> > If "stg push" fails, the subsequent "stg clean" will remove the
> > patch that could not been applied. I think it's wrong.
>
> I agree. It's consistent -- a conflicting patch is empty -- but
> clearly the wrong thing to do from a usability perspective.

Got broken by commit fe1cee2e49d9995852ba92d8fba1d064acf2fca9 which
removes the check_conflicts() call. As I said in a different post, we
should add these back (and to the 'goto' command as well) to make
StGIT safer.

> > I've made a patch for the testsuite that should pass once the bug is
> > fixed. Try removing "stg clean" from the test. and it will pass. But
> > "stg clean" should make no difference here.
>
> Good!
>
> For known-to-be-failing tests, you can use test_expect_failure. I'll
> amend your patch to do that when I pick it up (if Catalin doesn't beat
> me to it).

Probably not, I'm really busy for one more week with a Linux kernel release.

-- 
Catalin

Re: stg clean removes conflicting patch

[Karl Hasselström]

On 2008-01-25 09:53:46 +0000, Catalin Marinas wrote:

> On 25/01/2008, Karl Hasselström <kha@treskal.com> wrote:
>
> > On 2008-01-24 22:55:17 -0500, Pavel Roskin wrote:
> >
> > > If "stg push" fails, the subsequent "stg clean" will remove the
> > > patch that could not been applied. I think it's wrong.
> >
> > I agree. It's consistent -- a conflicting patch is empty -- but
> > clearly the wrong thing to do from a usability perspective.
>
> Got broken by commit fe1cee2e49d9995852ba92d8fba1d064acf2fca9 which
> removes the check_conflicts() call.

Ah, thanks. I didn't realize it used to work.

> As I said in a different post, we should add these back (and to the
> 'goto' command as well) to make StGIT safer.

The right thing to do would be to check for conflicts before
attempting any kind of modification, I guess.

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