threads / discuss / 11729

stg clean removes conflicting patch

Subject: stg clean removes conflicting patch

## tl;dr

5 messages between Jan 25, 2008 and Jan 25, 2008.

replies: 4people: 3as markdown or json

Pavel Roskin· Jan 25, 2008, 03:55 UTC · lore

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

Karl Hasselström· Jan 25, 2008, 08:04 UTC · re: Pavel Roskin · lore

Re: stg clean removes conflicting patch

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

Pavel Roskin· Jan 25, 2008, 08:40 UTC · re: Karl Hasselström · lore

Re: stg clean removes conflicting patch

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

Catalin Marinas· Jan 25, 2008, 09:53 UTC · re: Karl Hasselström · lore

Re: stg clean removes conflicting patch

On 25/01/2008, Karl Hasselström <kha@treskal.com> wrote:

Show 7 quoted lines

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

Show 9 quoted lines

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

Karl Hasselström· Jan 25, 2008, 10:17 UTC · re: Catalin Marinas · lore

Re: stg clean removes conflicting patch

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

Show 12 quoted lines

> 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