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:
Show 78 quoted lines
> 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`.
* 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.
* 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.
Ævar Arnfjörð Bjarmason <avarab@gmail.com>