On 2009-02-06 14:46:19 +0000, Catalin Marinas wrote:
> 2009/1/30 Catalin Marinas <catalin.marinas@xxxxxxxxx>:
>
> > 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.)
> 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@xxxxxxxxxxx
www.treskal.com/kalle
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
