Re: [PATCH/RFC] branch: add tests for new copy branch feature

[Sahil Dua <sahildua2305@gmail.com>]

On Mon, May 29, 2017 at 10:50 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Mon, May 29, 2017 at 10:41 PM, Sahil Dua <sahildua2305@gmail.com> wrote:
>> On Mon, May 29, 2017 at 1:30 AM, Ævar Arnfjörð Bjarmason
>> <avarab@gmail.com> wrote:
>>> On Mon, May 29, 2017 at 12:56 AM, Sahil Dua <sahildua2305@gmail.com> wrote:
>>>> New feature - copying a branch along with its config section.
>>>>
>>>> Aim is to have an option -c for copying a branch just like -m option for
>>>> renaming a branch.
>>>>
>>>> This commit adds a few basic tests for getting any suggestions/feedback
>>>> about expected behavior for this new feature.
>>>>
>>>> Signed-off-by: Sahil Dua <sahildua2305@gmail.com>
>>>> ---
>>>>  t/t3200-branch.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 53 insertions(+)
>>>>
>>>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
>>>> index fe62e7c775da..2c95ed6ebf3c 100755
>>>> --- a/t/t3200-branch.sh
>>>> +++ b/t/t3200-branch.sh
>>>> @@ -341,6 +341,59 @@ test_expect_success 'config information was renamed, too' '
>>>>         test_must_fail git config branch.s/s/dummy
>>>>  '
>>>>
>>>> +test_expect_success 'git branch -c dumps usage' '
>>>> +       test_expect_code 128 git branch -c 2>err &&
>>>> +       test_i18ngrep "branch name required" err
>>>> +'
>>>> +
>>>> +git config branch.d.dummy Hello
>>>> +
>>>> +test_expect_success 'git branch -c d e should work' '
>>>> +       git branch -l d &&
>>>> +       git reflog exists refs/heads/d &&
>>>> +       git branch -c d e &&
>>>> +       git reflog exists refs/heads/d &&
>>>> +       git reflog exists refs/heads/e
>>>> +'
>>>> +
>>>> +test_expect_success 'config information was copied, too' '
>>>> +       test $(git config branch.e.dummy) = Hello &&
>>>> +       test $(git config branch.d.dummy) = Hello
>>>> +'
>>>> +
>>>> +git config branch.f/f.dummy Hello
>>>> +
>>>> +test_expect_success 'git branch -c f/f g/g should work' '
>>>> +       git branch -l f/f &&
>>>> +       git reflog exists refs/heads/f/f &&
>>>> +       git branch -c f/f g/g &&
>>>> +       git reflog exists refs/heads/f/f &&
>>>> +       git reflog exists refs/heads/g/g
>>>> +'
>>>> +
>>>> +test_expect_success 'config information was copied, too' '
>>>> +       test $(git config branch.f/f.dummy) = Hello &&
>>>> +       test $(git config branch.g/g.dummy) = Hello
>>>> +'
>>>> +
>>>> +test_expect_success 'git branch -c m2 m2 should work' '
>>>> +       git branch -l m2 &&
>>>> +       git reflog exists refs/heads/m2 &&
>>>> +       git branch -c m2 m2 &&
>>>> +       git reflog exists refs/heads/m2
>>>> +'
>>>> +
>>>> +test_expect_success 'git branch -c a a/a should fail' '
>>>> +       git branch -l a &&
>>>> +       git reflog exists refs/heads/a &&
>>>> +       test_must_fail git branch -c a a/a
>>>> +'
>>>> +
>>>> +test_expect_success 'git branch -c b/b b should fail' '
>>>> +       git branch -l b/b &&
>>>> +       test_must_fail git branch -c b/b b
>>>> +'
>>>> +
>>>>  test_expect_success 'deleting a symref' '
>>>>         git branch target &&
>>>>         git symbolic-ref refs/heads/symref refs/heads/target &&
>>>>
>>>
>>> This looks good to me. Comments, in no particular order:
>>>
>>> * There should be a test for `git branch -c <newbranch>`, i.e. that
>>> should implicitly copy from HEAD just like `git branch -m <newbranch>`
>>> does. However, when looking at this I can see there's actually no test
>>> for one-argument `git branch -m`, i.e. if you apply this:
>>>
>>> --- a/builtin/branch.c
>>> +++ b/builtin/branch.c
>>> @@ -699,8 +699,6 @@ int cmd_branch(int argc, const char **argv, const
>>> char *prefix)
>>>         } else if (rename) {
>>>                 if (!argc)
>>>                         die(_("branch name required"));
>>> -               else if (argc == 1)
>>> -                       rename_branch(head, argv[0], rename > 1);
>>>                 else if (argc == 2)
>>>                         rename_branch(argv[0], argv[1], rename > 1);
>>>                 else
>>>
>>> The only test that fails is a `git branch -M master`, i.e.
>>> one-argument -M is tested for, but not -m, same codepath though, but
>>> while you're at it a patch/series like this could start by adding a
>>> one-arg -m test, then follow-up by copying that for the new `-c`.
>>>
>>
>> Thanks for the suggestion. Yes, I will add one-arg test for -c. Is it
>> ok to send a different patch for adding a one-arg test for existing -m
>> option?
>
> Yeah, it makes sense to just make the first patch in the series be
> some cleanup / improvement of the existing tests, which the subsequent
> tests for -c then make use of / copy. It could even be sent on its
> own, but probably makes sense to just bundle them up. Up to you
> though, in this case you won't need patch A for patch B to work, so
> the that's one argument against bundling them up. Personally I'd do it
> if I was hacking this just because it's more convenient to keep track
> of fewer things.
>

Got it. I will submit a patch for the tests for -m option.

>>> * We should have a -C to force -c just like -M forces -m, i.e. a test
>>> where one-arg `git branch -C alreadyexists` clobbers alreadyexists,
>>> and `git branch -C source alreadyexists` does the same for two-arg.
>>>
>> Yes, I missed this. I will add -C option too.
>>
>>> * I know this is just something you're copying, but this `git branch
>>> -l <name>` use gets me every time "wait how does listing it help isn't
>>> that always succeeding ... damnit it's --create-reflog not --list, got
>>> me again" :)
>>>
>>
>> Yes, it was confusing to me too in the beginning. I will use --create-reflog.
>>
>>> Just noting it in case it confuses other reviewers who are skimming
>>> this. Might be worth it to just use --create-reflog for new tests
>>> instead of the non-obvious -l whatever the existing tests in the file
>>> do, or maybe I'm the only one confused by this :)
>>>
>>> * When you use `git branch -m` it adds a note to the reflog, your
>>> patch should have a test like the existing "git branch -M baz bam
>>> should add entries to .git/logs/HEAD" test in this file except
>>> "Branch: copied ..." instead of "Branch: renamed...".
>>>
>>
>> Nice, I will add it. Thanks.
>>
>>> * Should there be tests for `git branch -c master master` like we have
>>> for `git branch -m master master` (see 3f59481e33 ("branch: allow a
>>> no-op "branch -M <current-branch> HEAD"", 2011-11-25)). Allowing this
>>> for -m smells like a bend-over-backwards workaround for some script
>>> Jonathan had, should we be doing this for -c too? I don't know.
>>
>> Not sure I understand this. Can you please elaborate?
>> Thanks.
>
> So the reason we have this for -m is:
>
>     commit 3f59481e33
>     Author: Jonathan Nieder <jrnieder@gmail.com>
>     Date:   Fri Nov 25 20:30:02 2011 -0600
>
>     branch: allow a no-op "branch -M <current-branch> HEAD"
>
>     Overwriting the current branch with a different commit is forbidden, as it
>     will make the status recorded in the index and the working tree out of
>     sync with respect to the HEAD. There however is no reason to forbid it if
>     the current branch is renamed to itself, which admittedly is something
>     only an insane user would do, but is handy for scripts.
>
> My understanding of that last part is that Jonathan/someone (see
> reported-by in that patch) had some script which was renaming
> branches, and it was easier for whatever reason to just make it no-op
> if the rename would have yielded the same result as doing nothing at
> all.
>
> Most likely your implementation will consist of just re-using the
> logic in rename_branch() (and renaming it to e.g.
> copy_or_rename_branch() ...) so you could just re-use the no-op
> behavior we use for -m, or if there's some reason not to no-op and
> error instead for -c we could just do that, but in any case this case
> of `git branch -c master master` or `git branch -c currentbranch`
> should be tested for.

Understood. Thanks. I will add tests for this too.