Re: [StGIT PATCH 4/5] Add stack creation and initialisation support to lib.Stack
- From
Karl Hasselström <kha@treskal.com>
- Date
- Jun 7, 2008, 08:59 UTC
- Message-ID
- <20080607085949.GA32647@diana.vm.bytemark.co.uk>
- In-Reply-To
- <b0943d9e0806050542w663c64a8o93f81503eb811cf9@mail.gmail.com>
On 2008-06-05 13:42:03 +0100, Catalin Marinas wrote:
Show 14 quoted lines
> 2008/6/5 Karl Hasselström <kha@treskal.com>:
>
> > On 2008-06-04 22:13:43 +0100, Catalin Marinas wrote:
> >
> > > +from stgit.config import config
> > > +
> > > +class StackException(exception.StgException):
> > > + """Exception raised by stack objects."""
> >
> > s/stack/L{Stack}/, perhaps?
>
> I was more referring to objects in the lib.stack module, not only
> the Stack class. Can this be expressed in any way with the epydoc
> format?I think L{lib.stack} would work.See http://epydoc.sourceforge.net/manual-epytext.html.
Show 5 quoted lines
> > Wouldn't it be more consistent if the create function actually > > returned a PatchOrder object, like other creation functions? > > It is a bit more complicated. PatchOrder requires a Stack object in > __init__ but the stack is not fully set up at this point.
I see. How about calling the method "create_files" or something, then, to avoid breaking with the newly established convention of having the create() function create new underlying data and returning a Python object that represents it?
Show 7 quoted lines
> > (You might even consider having these files auto-created whenever > > you instantiate a PatchOrder object and they don't yet exist.) > > This is definitely a solution but at the moment we have to support > the old infrastructure which fails if the files aren't present. Once > we don't have any reference to the stgit.stack module, we can do > this more dynamically.
Right.
Show 5 quoted lines
> > Also, the creation function might instead live in the Stack class, > > since it owns the patch order. > > See my reply to a previous patch with my view on ownership and > separation of functionality.
Yes, I agree with you now.
Show 15 quoted lines
> > > + def set_parents(self, remote, localbranch):
> > > + if not localbranch:
> > > + return
> > > + if remote:
> > > + self.set_parent_remote(remote)
> > > + self.set_parent_branch(localbranch)
> > > + config.set('branch.%s.stgit.parentbranch' % self._name, localbranch)
> >
> > Hmm, I don't quite follow. Why is this a no-op if you give a false
> > localbranch? And why is branch.<branchname>.stgit.parentbranch
> > needed, when it's always the same as branch.<branchname>.merge?
> > (Backwards compatibility? Would you mind making a comment about
> > that, in that case?)
>
> I don't fully follow it either :-).Hmmm. :-)
Show 7 quoted lines
> I just copied the code from stgit.stack.Series but I don't make any > use of it yet. I think it was initially written by Yann Dirson for > the "branch" command which I haven't translated yet. I think > branch.<branchname>.stgit.parentbranch might be used by the current > "pull" or "rebase" implementations. I'll comment it out (but still > keep it for now) and put a FIXME so that I remember when converting > the "branch" and "pull" commands.
In cases like this, when one wants to keep existing behavior unchanged but doesn't quite understand the original reasons behind it, I like to simply insert a big comment saying that this piece of code does this strange thing for reasons unknown to me, but I kept them that way because I didn't want to change the behavior at the moment.
( As an aside, I've also been thinking about pull/rebase. Let's have a discussion about that -- in another thread. )
Show 14 quoted lines
> > What I'd really like to see here, I think, is something like this: > > > > 1. You get a Stack object from some stack.Repository method. > > > > 2. The Stack object works without having to be initialized, but > > the operations that need initialization throw an exception. > > > > 3. The Stack object has an initialize() method -- just a normal > > method, not a class method. > > That was the original behaviour in stgit.stack.Series but if you > happen to try to access a patch or patchorder, you get some > exception on missing file form those objects rather than "stack not > initialised" from Stack.
This wouldn't be a problem if all methods that needed initialization begain with a self.make_sure_were_initialized() call, which could throw a suitable exception. (Though I see how this could get a touch unwieldy -- it's probably not something you'd want to do unless you _are_ shooting for auto-initialization.)
Show 6 quoted lines
> I'm not fully convinced with automatic initialisation. I have some > branches where I don't use StGIT but I might type a "stg series" by > mistake. The branch will be promoted to a stack but later Git > doesn't know about extra files in .git/patches and they aren't > handled (removed, renamed etc.). I'm more in favour of the explicit > initialisation.
Automatic initialization should work such that "initialize" is a no-op. For example, the "applied" and "unapplied" files shouldn't even exist if there are no patch names in them. That way, StGit metadata files (and refs) are only created when there's an actual need for them -- meaning that read-only StGit commands would never create any such files.
Related to this: Have you looked in any detail at the "undo" stuff in my experimental branch? The patch stack log that makes undo possible contains all the metadata that StGit needs, accessible from a single ref. I currently keep writing all the old metadata files as well, but my long-term evil plan is to have only the stack log and nothing else. If not earlier, I'd like to propose a move to auto-init at the same time.
--
Karl Hasselström, kha@treskal.com
www.treskal.com/kalle