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

quoted text
> 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. :-(

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

quoted text
> 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
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html