Re: [StGit PATCH] Check for local changes with "goto"

From: Karl Hasselström <kha@treskal.com>
Date: Feb 6, 2009, 15:31 UTC
Message-ID: <20090206153106.GA28897@diana.vm.bytemark.co.uk>
In-Reply-To: <b0943d9e0902060646hd779681x821e74d9a155d97b@mail.gmail.com>

On 2009-02-06 14:46:19 +0000, Catalin Marinas wrote:

Show 7 quoted lines

> 2009/1/30 Catalin Marinas <catalin.marinas@gmail.com>:
>
> > Now, should we add the check_clean argument to
> > Transaction.__init__() rather than run() as we do for the
> > allow_bad_head case?
>
> It looks like this may be a better option.

Sorry for taking so long to respond, but ... I strongly advise against using default_iw in transaction.py. It's library code, and it should take stuff like index and worktree as input parameters from layers that are higher up in the abstraction stack. Compare the kernel policy of having policy in userspace and not in the kernel.

And if you accept that reasoning, the check has to go in run() rather than __init__(), because we don't have an iw in __init__(). (Though we could add such a parameter, I guess.)

Show 5 quoted lines

> The previous patch fails if "goto" pushes a patch with standard
> git-apply followed by another patch with a three-way merge. When
> Transaction.run() is called, even if the patch pushing succeeded,
> the function complains about local changes because of the
> "iw.index.is_clean(self.stack.head)" check.

Hmm, so that would have to be worked around somehow ... I guess doing the check in __init__() might make sense after all, since that's before we start changing things. How about adding a check_clean_relative_to paramter to __init__() that's not a boolean, but an iw to check against? It would default to None, meaning no check.

> It is also a bit weird to push/pop patches and only complain at the
> end of local changes.

You mean the behavior the new infrastructure currently gives you? It's actually convenient in a number of cases. Assume for example that you have patch A that changes file foo, patch B that changes file bar, and then local changes to file bar. At this point you can pop A without problem, even though a middle stage is to pop and push B which touches the same file as your local changes -- the existing checks will only compare the diff between the original and final tree with your local changes, and that diff doesn't contain bar.

> Below is an updated patch which does the checking in
> Transaction.__init__ (only the relevant parts of the patch):

Looks good, except for the hard-coded default_iw as I mentioned above.

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