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

> 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`.
>

> * 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" :)
>

> 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...".
>

> * 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.