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

[Catalin Marinas <catalin.marinas@gmail.com>]

2009/2/6 Karl Hasselström <kha@treskal.com>:
> On 2009-02-06 14:46:19 +0000, Catalin Marinas wrote:
>
>> 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.

OK, no problem with that.

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

OK, that's better.

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

Yes, I agree that's a nice feature and it would still be available
with the --keep option (I wrote in the past why I wouldn't leave the
current behaviour to be the default).

Above I was referring to the new behaviour which checks for local
changes by default (--keep not passed). If we do the test in run() you
may push or pop patches and only fail at the end when actually the
operation shouldn't have started. I plan to add the auto interactive
merging to the new infrastructure (by invoking mergetool if
IndexAndWorktree.merge() fails, based on the stgit.autoimerge option)
and pushing may become a more complex operation if enabled.

I'll repost the patch. Thanks.

-- 
Catalin