Re: [PATCH/RFC] branch: add tests for new copy branch feature
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2017-05-28 23:31:13
On Mon, May 29, 2017 at 12:56 AM, Sahil Dua [off-list ref] wrote:
quoted hunk ↗ jump to hunk
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 <redacted> --- 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, constchar *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.