Re: [StGit PATCH] Add a --tree flag to stg push

From: David Kågedal <davidk@lysator.liu.se>
Date: May 19, 2009, 07:50 UTC
Message-ID: <87d4a5fs59.fsf@krank.kagedal.org>
In-Reply-To: <20090519072512.GA8451@diana.vm.bytemark.co.uk>

Karl Hasselström <kha@treskal.com> writes:

Show 13 quoted lines

> On 2009-05-18 16:50:18 +0200, David Kågedal wrote:
>
>> This scratches a long-time itch for me. The typical use case is when
>> you want to break up a larg patch inte smaller ones. You back out
>> the orignal patch, apply a small set of changes from it and then
>> push the patch back again. But then you don't want to do a merge,
>> with the possibility of conflict. You simply want to restore to the
>> tree that the patch had before so you can see what's left to create
>> cleaned-up patches of. The command "stg push --tree" does just that.
>
> Thanks!
>
> There's no sign-off.

I counted on getting comments, so it's not finished yet...

>> The naming of flags and functions isn't very obvious, and
>> suggestions for improvements are welcome.
>
> --set-tree maybe?

Probably better. But perhaps there is a way to not have to talk about "trees" at all?

Show 10 quoted lines

>>  t/t1207-push-tree.sh     |   64 ++++++++++++++++++++++++++++++++++++++++++++++
>
> A test! Very good.
>
>> +    opt('--tree', action = 'store_true',
>> +        short = 'Push the patch with the original tree')
>
> This probably deserves a long description as well. (That most existing
> options lack them is unfortunate---the support for long descriptions
> was added rather recently.)

I didn't look putside push.py, and just followed the pattern there. But a long description sounds like a good idea. It won't be obvious what this does with just a short one.

Show 9 quoted lines

>> +        if any(getattr(cd, a) != getattr(orig_cd, a) for a in
>> +               ['parent', 'tree', 'author', 'message']):
>> +            comm = self.__stack.repository.commit(cd)
>> +            self.head = comm
>> +        else:
>> +            comm = None
>> +            s = ' (unmodified)'
>
> Shouldn't self.head be set in both cases?

I guess so. I'm a bit unsure about the correctness of that whole function.

>> +# Copyright (c) 2006 David Kågedal
>
> Been sitting on this patch long? :-)

Copy/paste error.

Show 8 quoted lines

>> +# don't need this repo, but better not drop it, see t1100
>> +#rm -rf .git
>> +
>> +# Need a repo to clone
>> +test_create_repo foo
>
> Umm, your test doesn't seem to depend on using this separate repo
> instead of the default one.

Call it copy/paste programming or cargo cult programming. I will clean up.

-- 
David Kågedal