From: Karl Hasselström <kha@treskal.com>
Date: Fri, 13 Mar 2009 01:57:55 GMT
Subject: Re: [StGit PATCH 1/5] Check for local changes with "goto"
Message-ID: <20090313015755.GA15393@diana.vm.bytemark.co.uk>
In-Reply-To: <20090312120856.2992.48548.stgit@pc1117.cambridge.arm.com>

On 2009-03-12 12:08:56 +0000, Catalin Marinas wrote:

> This is done by default, unless the --keep option is passed, for
> consistency with the "pop" command. The index is checked in the
> Transaction.run() function so that other commands could benefit from
> this feature (off by default).
>
> This behaviour can be overridden by setting the stgit.autokeep option.

Two small nits; otherwise,

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

> -    trans = transaction.StackTransaction(stack, 'goto')
> +    clean_iw = not options.keep and iw or None

Add some parentheses here, please! I know that "and" binds tighter
than "or", but I have to think for too long to remember which is
which, and I'll bet I'm not alone ...

> +    def __assert_index_worktree_clean(self, iw):
> +        if not iw.worktree_clean() or \
> +           not iw.index.is_clean(self.stack.head):
> +            self.__halt('Repository not clean. Use "refresh" or '
> +                        '"status --reset"')

"Repository" is misleading here. Maybe something like

   ix_c = iw.index.is_clean(self.stack.head)
   wt_c = iw.worktree_clean()
   if not ix_c or not wt_c:
       self.__halt('%s not clean. Use "refresh" or "status --reset"'
                   % { (False, True): 'Index', (True, False): 'Worktree',
                       (False, False): 'Index and worktree' }[(ix_c, wt_c)])

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