Re: Subject: [PATCH] fix stg edit command

From: Karl Hasselström <kha@treskal.com>
Date: Feb 12, 2008, 22:47 UTC
Message-ID: <20080212224724.GA24993@diana.vm.bytemark.co.uk>
In-Reply-To: <200802122305.05696.kumbayo84@arcor.de>

On 2008-02-12 23:05:05 +0100, Peter Oberndorfer wrote:

> While testing my editor searching ordering patch i found that this
> patch(Refactor --author/--committer options) seems to break "stg
> edit" (without arguments) starting a interactive editor for me. When
> i issue "stg edit" it silently does nothing.

Thanks for the report. And yes, as far as I can tell your analysis is spot on. In the initial patch I remember being careful to not replace cd unless it was actually changed, but obviously I got sloppy after that. :-(

Show 15 quoted lines

> It seems the following comparison does not return True
>
> > # Let user edit the patch manually.
> > if cd == orig_cd or options.edit:
>
> I can work around this by adding a comparison function to Commitdata
> but maybe __eq__ or __ne__ should be used instead(prevent similar
> bugs caused by == comparison)?
>
> +    def is_same(self, other):
> +        return (self.__tree == other.__tree and
> +                self.__parents == other.__parents and
> +                self.__author == other.__author and
> +                self.__committer == other.__committer and
> +                self.__message == other.__message)

Yes, you'd definitely want the common operators to work. But I usually implement __cmp__ instead of __eq__ and __ne__ -- that gives you all of <, <=, =, !=, >=, and > for the price of a single method. And it's usually possible to define it rather simply in terms of cmp() with tuple arguments, like this:

    def __cmp__(self, other):
        return cmp((self.__tree, self.__parents, self.__author,
                    self.__committer, self.__message),
                   (other.__tree, other.__parents, other.__author,
                    other.__committer, other.__message))

This sidesteps the great problem of cmp -- having to remember when to return 1 and when to return -1.

> So another way to fix this might be, to not overwrite cd
> unconditionally.

Yes, this is what we want -- if the user gives --author, we shouldn't open the interactive editor even if the given author is the same as the patch already had.

Updated patch on the way.

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