stg clean removes conflicting patch

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 <redacted>

Signed-off-by: Pavel Roskin <redacted>
---

 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

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

Quoting Karl Hasselström [off-list ref]:

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

On 25/01/2008, Karl Hasselström [off-list ref] wrote:

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

quoted

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.

quoted

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

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

On 25/01/2008, Karl Hasselström [off-list ref] wrote:

quoted

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

quoted

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