2009/2/6 Karl Hasselström <kha@xxxxxxxxxxx>: > 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. 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 -- 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
