Fix detection of upstream version branches with continue

[abadger]

cherry_picker has recently grown support for prefixed version branches
(like stable-2.6). The --continue support had a bug with those branches
where it wouldn't account for the fact that those branches could have
extra dashes in them and thus mixing branch name with sha.

This commit should fix those situations.

[webknjaz]

@abadger You need to take into account case when a user might run --continue from a different branch:

=================================== FAILURES =================================== ______________________ test_get_base_branch_without_dash _______________________ deftest_get_base_branch_without_dash(): cherry_pick_branch ='master' > result = get_base_branch(cherry_pick_branch) cherry_picker/test.py:43: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ cherry_pick_branch = 'master' defget_base_branch(cherry_pick_branch): """return'2.7'from'backport-sha-2.7'""" > prefix, sha, base_branch = cherry_pick_branch.split('-', 2) E ValueError: not enough values to unpack (expected 3, got 1) cherry_picker/cherry_picker.py:425: ValueError ===================== 1 failed, 27 passed in 0.35 seconds ======================

[abadger]

Ah... I thought rpartition traced back in that case too but I see now it always returns a three tuple.

I can replicate the previous behaviour here but I'm not sure... is that actually sensible behaviour? From where it's being called, I think that this would cause other bugs.

[abadger]

So the two places that get_base_branch is currently called from are:

• https://github.com/python/core-workflow/blob/master/cherry_picker/cherry_picker/cherry_picker.py#L330
  This is on the --continue codepath. The very next line will traceback if there's no dash in the branch that we pass into get_base_branch.
• https://github.com/python/core-workflow/blob/master/cherry_picker/cherry_picker/cherry_picker.py#L184 In this case it won't traceback later. However, the branch that it's passed in this case is generated here: https://github.com/python/core-workflow/blob/master/cherry_picker/cherry_picker/cherry_picker.py#L277
  which in turn is generated by this function: https://github.com/python/core-workflow/blob/master/cherry_picker/cherry_picker/cherry_picker.py#L98 So in that case it will always be a branch name with at least two dashes.

Thinking about this more, it feels like this function is assuming that it's taking a string of a known format, assigning semantic meaning to pieces of that string based on their position inside of it, and then returning the value that corresponds to one of those meanings. If so, it feels right that the code should raise a ValueError if the string does not conform to the format... Otherwise how would we know what portion of the string was the base branch? Is it all of it? Is it the first piece? The last piece? Or perhaps none of those because we weren't given a string containing the base branch at all?

So I think the test is wrong but perhaps there's some other use case for this code that I am not aware of? @Mariatta ? Input from you?

[abadger]

I've pushed a change to the test cases which removes the "no_dash" test and adds one for "dashes in the base branch".

[webknjaz]

I can replicate the previous behaviour here but I'm not sure... is that actually sensible behaviour? From where it's being called, I think that this would cause other bugs.

Git itself is a very stateful tool, so it's expected that when you rely on it there's some restrictions.

[abadger]

My question doesn't come from what git does; it comes from what cherry-picker is doing with this branch name. AFAICT, the branch name has to be one that we created (with the three dashes), otherwise the present code will fail. So it seems like the current test that no dashes works is testing something that we'll never want the code itself to do.

[webknjaz]

@Mariatta this PR looks ready :)

[Mariatta]

Sorry for the delay in responding to this! 🙇‍♀️
The original implementation was based on CPython's branch names that doesn't include dashes, so it didn't handle ansible's situation.
Thanks for catching and fixing this!