On Mon, May 29, 2017 at 1:30 AM, Ævar Arnfjörð Bjarmason
<avarab@xxxxxxxxx> wrote:
> On Mon, May 29, 2017 at 12:56 AM, Sahil Dua <sahildua2305@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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?
> * 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.