On Thu, Apr 3, 2014 at 6:54 AM, Erik Carstensen <[email protected]> wrote:
> test_expect_success \
> 'No patch' \
> '! stg move --from foo'
* It's possible to write tests using far fewer backslashes. See
e.g. t/t3300-edit.sh.
* Use general_error, command_error, or conflict from test-lib.sh
instead of !.
> test_expect_success \
> 'move patches within one remote branch' \
> '
> stg branch foo &&
> stg push -a &&
> stg branch bar &&
> stg move --from foo --to foo a &&
> test "$(echo $(stg series -b foo --applied --noprefix))" = "b c a-0 a-1" &&
> test "$(echo $(stg series -b foo --unapplied --noprefix))" = "a-2"
> '
* So moving a patch to the same branch will pop it if necessary, and
give it a new autogenerated name? Wouldn't it make more sense to
just complain and do nothing?
* This test and the next one are 80-column-challenged.
> usage = ['DEST_BRANCH <patch1> [<patch2>] [<patch3>..<patch4>]']
> description = """
> Delete the given patches from the current branch, and add them unapplied to
> DEST_BRANCH."""
This isn't quite true anymore now that --from and --to are mandatory.
> def find_unique_name(oldname, blacklist):
> # TODO: is this the right place?
No, you can delete it because you never call it. But if you did want
it (and I actually think this scheme makes sense), then perhaps near
make_patch_name in stgit/utils.py.
By the way, I realize the existing codebase doesn't exactly shine in
this regard, but this sort of function is practically begging for a
unit test. ;-)
> if options.src:
> iw = None # can't use index/workdir to manipulate another branch
> else:
> iw = src.repository.default_iw
I would have named this src_iw or something, to make it clear which
branch it belongs to.
Also, you don't handle (or test!) the case where -f explicitly
specifies the current branch.
> # Creating patches directly. Not using a transaction, because
> # this cannot fail
Everything from this point on is indented one step more than
necessary.
> lambda name: dest.patches.exists(name)
I'm sure you can think of a way to simplify this expression. ;-)
> out.info('Created %s on branch %s' % (", ".join(created), dest.name))
This line is often going to get very long---automatically generated
patch names are 30 characters long or something like that. My
suggestion would be one patch per line, which would also give you the
opportunity to say if you had to rename the patch while moving it.
Oh, and here you use double quotes for no apparent reason.
-+-
I like the way this is looking---it's very close to how I would've
done it.
--
Karl Wiberg, [email protected]
subrabbit.wordpress.com
www.treskal.com/kalle
_______________________________________________
stgit-users mailing list
[email protected]
https://mail.gna.org/listinfo/stgit-users
