{"thread":{"id":"46064","subject":"[PATCH/RFC] branch: add tests for new copy branch feature","startedAt":"2017-05-28T22:57:00Z","lastAt":"2017-06-13T18:09:11Z","messageCount":10,"participants":["Sahil Dua","Ævar Arnfjörð Bjarmason","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"320908","messageId":"0102015c5146c8ca-e5144538-326b-47b8-8c81-af31da4cdfe0-000000@eu-west-1.amazonses.com","threadId":"46064","inReplyTo":null,"subject":"[PATCH/RFC] branch: add tests for new copy branch feature","fromName":"Sahil Dua","fromEmail":"sahildua2305@gmail.com","sentAt":"2017-05-28T22:56:52Z","receivedAt":"2017-05-28T22:57:00Z","isPatch":true,"sender":{"key":"sahildua2305@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5206277?v=4"},"body":"New feature - copying a branch along with its config section.\n\nAim is to have an option -c for copying a branch just like -m option for\nrenaming a branch.\n\nThis commit adds a few basic tests for getting any suggestions/feedback\nabout expected behavior for this new feature.\n\nSigned-off-by: Sahil Dua <sahildua2305@gmail.com>\n---\n t/t3200-branch.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 53 insertions(+)\n\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex fe62e7c775da..2c95ed6ebf3c 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -341,6 +341,59 @@ test_expect_success 'config information was renamed, too' '\n \ttest_must_fail git config branch.s/s/dummy\n '\n \n+test_expect_success 'git branch -c dumps usage' '\n+\ttest_expect_code 128 git branch -c 2>err &&\n+\ttest_i18ngrep \"branch name required\" err\n+'\n+\n+git config branch.d.dummy Hello\n+\n+test_expect_success 'git branch -c d e should work' '\n+\tgit branch -l d &&\n+\tgit reflog exists refs/heads/d &&\n+\tgit branch -c d e &&\n+\tgit reflog exists refs/heads/d &&\n+\tgit reflog exists refs/heads/e\n+'\n+\n+test_expect_success 'config information was copied, too' '\n+\ttest $(git config branch.e.dummy) = Hello &&\n+\ttest $(git config branch.d.dummy) = Hello\n+'\n+\n+git config branch.f/f.dummy Hello\n+\n+test_expect_success 'git branch -c f/f g/g should work' '\n+\tgit branch -l f/f &&\n+\tgit reflog exists refs/heads/f/f &&\n+\tgit branch -c f/f g/g &&\n+\tgit reflog exists refs/heads/f/f &&\n+\tgit reflog exists refs/heads/g/g\n+'\n+\n+test_expect_success 'config information was copied, too' '\n+\ttest $(git config branch.f/f.dummy) = Hello &&\n+\ttest $(git config branch.g/g.dummy) = Hello\n+'\n+\n+test_expect_success 'git branch -c m2 m2 should work' '\n+\tgit branch -l m2 &&\n+\tgit reflog exists refs/heads/m2 &&\n+\tgit branch -c m2 m2 &&\n+\tgit reflog exists refs/heads/m2\n+'\n+\n+test_expect_success 'git branch -c a a/a should fail' '\n+\tgit branch -l a &&\n+\tgit reflog exists refs/heads/a &&\n+\ttest_must_fail git branch -c a a/a\n+'\n+\n+test_expect_success 'git branch -c b/b b should fail' '\n+\tgit branch -l b/b &&\n+\ttest_must_fail git branch -c b/b b\n+'\n+\n test_expect_success 'deleting a symref' '\n \tgit branch target &&\n \tgit symbolic-ref refs/heads/symref refs/heads/target &&\n\n--\nhttps://github.com/git/git/pull/363\n"},{"id":"320909","messageId":"CACBZZX4LpRK5q+cxToWqGeW2ZgN4ggo4h9SJpx0mZkZUR15jNw@mail.gmail.com","threadId":"46064","inReplyTo":"0102015c5146c8ca-e5144538-326b-47b8-8c81-af31da4cdfe0-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH/RFC] branch: add tests for new copy branch feature","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-28T23:30:46Z","receivedAt":"2017-05-28T23:31:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Mon, May 29, 2017 at 12:56 AM, Sahil Dua <sahildua2305@gmail.com> wrote:\n> New feature - copying a branch along with its config section.\n>\n> Aim is to have an option -c for copying a branch just like -m option for\n> renaming a branch.\n>\n> This commit adds a few basic tests for getting any suggestions/feedback\n> about expected behavior for this new feature.\n>\n> Signed-off-by: Sahil Dua <sahildua2305@gmail.com>\n> ---\n>  t/t3200-branch.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n>  1 file changed, 53 insertions(+)\n>\n> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> index fe62e7c775da..2c95ed6ebf3c 100755\n> --- a/t/t3200-branch.sh\n> +++ b/t/t3200-branch.sh\n> @@ -341,6 +341,59 @@ test_expect_success 'config information was renamed, too' '\n>         test_must_fail git config branch.s/s/dummy\n>  '\n>\n> +test_expect_success 'git branch -c dumps usage' '\n> +       test_expect_code 128 git branch -c 2>err &&\n> +       test_i18ngrep \"branch name required\" err\n> +'\n> +\n> +git config branch.d.dummy Hello\n> +\n> +test_expect_success 'git branch -c d e should work' '\n> +       git branch -l d &&\n> +       git reflog exists refs/heads/d &&\n> +       git branch -c d e &&\n> +       git reflog exists refs/heads/d &&\n> +       git reflog exists refs/heads/e\n> +'\n> +\n> +test_expect_success 'config information was copied, too' '\n> +       test $(git config branch.e.dummy) = Hello &&\n> +       test $(git config branch.d.dummy) = Hello\n> +'\n> +\n> +git config branch.f/f.dummy Hello\n> +\n> +test_expect_success 'git branch -c f/f g/g should work' '\n> +       git branch -l f/f &&\n> +       git reflog exists refs/heads/f/f &&\n> +       git branch -c f/f g/g &&\n> +       git reflog exists refs/heads/f/f &&\n> +       git reflog exists refs/heads/g/g\n> +'\n> +\n> +test_expect_success 'config information was copied, too' '\n> +       test $(git config branch.f/f.dummy) = Hello &&\n> +       test $(git config branch.g/g.dummy) = Hello\n> +'\n> +\n> +test_expect_success 'git branch -c m2 m2 should work' '\n> +       git branch -l m2 &&\n> +       git reflog exists refs/heads/m2 &&\n> +       git branch -c m2 m2 &&\n> +       git reflog exists refs/heads/m2\n> +'\n> +\n> +test_expect_success 'git branch -c a a/a should fail' '\n> +       git branch -l a &&\n> +       git reflog exists refs/heads/a &&\n> +       test_must_fail git branch -c a a/a\n> +'\n> +\n> +test_expect_success 'git branch -c b/b b should fail' '\n> +       git branch -l b/b &&\n> +       test_must_fail git branch -c b/b b\n> +'\n> +\n>  test_expect_success 'deleting a symref' '\n>         git branch target &&\n>         git symbolic-ref refs/heads/symref refs/heads/target &&\n>\n\nThis looks good to me. Comments, in no particular order:\n\n* There should be a test for `git branch -c <newbranch>`, i.e. that\nshould implicitly copy from HEAD just like `git branch -m <newbranch>`\ndoes. However, when looking at this I can see there's actually no test\nfor one-argument `git branch -m`, i.e. if you apply this:\n\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -699,8 +699,6 @@ int cmd_branch(int argc, const char **argv, const\nchar *prefix)\n        } else if (rename) {\n                if (!argc)\n                        die(_(\"branch name required\"));\n-               else if (argc == 1)\n-                       rename_branch(head, argv[0], rename > 1);\n                else if (argc == 2)\n                        rename_branch(argv[0], argv[1], rename > 1);\n                else\n\nThe only test that fails is a `git branch -M master`, i.e.\none-argument -M is tested for, but not -m, same codepath though, but\nwhile you're at it a patch/series like this could start by adding a\none-arg -m test, then follow-up by copying that for the new `-c`.\n\n* We should have a -C to force -c just like -M forces -m, i.e. a test\nwhere one-arg `git branch -C alreadyexists` clobbers alreadyexists,\nand `git branch -C source alreadyexists` does the same for two-arg.\n\n* I know this is just something you're copying, but this `git branch\n-l <name>` use gets me every time \"wait how does listing it help isn't\nthat always succeeding ... damnit it's --create-reflog not --list, got\nme again\" :)\n\nJust noting it in case it confuses other reviewers who are skimming\nthis. Might be worth it to just use --create-reflog for new tests\ninstead of the non-obvious -l whatever the existing tests in the file\ndo, or maybe I'm the only one confused by this :)\n\n* When you use `git branch -m` it adds a note to the reflog, your\npatch should have a test like the existing \"git branch -M baz bam\nshould add entries to .git/logs/HEAD\" test in this file except\n\"Branch: copied ...\" instead of \"Branch: renamed...\".\n\n* Should there be tests for `git branch -c master master` like we have\nfor `git branch -m master master` (see 3f59481e33 (\"branch: allow a\nno-op \"branch -M <current-branch> HEAD\"\", 2011-11-25)). Allowing this\nfor -m smells like a bend-over-backwards workaround for some script\nJonathan had, should we be doing this for -c too? I don't know.\n"},{"id":"320918","messageId":"xmqqr2z88lcy.fsf@gitster.mtv.corp.google.com","threadId":"46064","inReplyTo":"0102015c5146c8ca-e5144538-326b-47b8-8c81-af31da4cdfe0-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH/RFC] branch: add tests for new copy branch feature","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-29T02:09:33Z","receivedAt":"2017-05-29T02:09:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sahil Dua <sahildua2305@gmail.com> writes:\n\n> New feature - copying a branch along with its config section.\n\nThat's an unusual non-sentence (without a verb) in our commit message.\n\n> Aim is to have an option -c for copying a branch just like -m option for\n> renaming a branch.\n\nWhat should it copy literally, and what should it copy with\nadjustment (and adjusting what), and what should it refrain from\ncopying?  \n\nFor example, when you create a branch topic-2 by copying from a\nbranch topic-1, do you copy the reflog in such a way that topic-2's\nreflog contains all the entries of topic-1 before the copy happens,\ncapped with a new (and not shared with topic-1) entry that says\n\"Copied from topic-1\"?  When topic-1 is set to pull from a custom\nupstream 'upstream' (i.e. not \"origin\")'s 'trunk' branch, by setting\n'branch.topic-2.remote' to \"upstream\", i.e. the same as the value of\n'branch.topic-1.remote' and 'branch.topic-2.merge' to \"trunk\",\ni.e. the same as 'branch.topic-1.merge'?  Should a \"git push\" while\ntopic-2 is checked out push to the same remote as a \"git push\" would\npush to when topic-1 is checked out?  Should they update the same\nbranch at the remote?  Why and/or why not? [*1*]\n\n> This commit adds a few basic tests for getting any suggestions/feedback\n> about expected behavior for this new feature.\n\nWriting tests is a good way to prepare for an implementation, which\nmust be done according to the design, but that happens after the\ndesign is polished and judged to be a good one.  While soliciting\ncomments on the design from others, tests are a bit too low level to\nexpress what you are aiming at.  It is a bit unhelpful to those who\nmay want to help to figure out answers to questions like the ones in\nthe previos paragraph (the one that begins with \"For example,...\")\nby telling them to \"read my intention from these tests\", and when\nyou want as wide an input as possible, that is counterproductive for\nyour objective ;-).\n\n> Signed-off-by: Sahil Dua <sahildua2305@gmail.com>\n> ---\n>  t/t3200-branch.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n>  1 file changed, 53 insertions(+)\n>\n> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> index fe62e7c775da..2c95ed6ebf3c 100755\n> --- a/t/t3200-branch.sh\n> +++ b/t/t3200-branch.sh\n> @@ -341,6 +341,59 @@ test_expect_success 'config information was renamed, too' '\n>  \ttest_must_fail git config branch.s/s/dummy\n>  '\n>  \n> +test_expect_success 'git branch -c dumps usage' '\n> +\ttest_expect_code 128 git branch -c 2>err &&\n> +\ttest_i18ngrep \"branch name required\" err\n> +'\n\nOK.  Do we want to support a single-name format (i.e. copy from the\ncurrent)?\n\n> +git config branch.d.dummy Hello\n\nWrite your tests to do as little outside test_expct_success block as\npossible, and do a set-up close to where it matters.\n\n> +test_expect_success 'git branch -c d e should work' '\n> +\tgit branch -l d &&\n> +\tgit reflog exists refs/heads/d &&\n> +\tgit branch -c d e &&\n> +\tgit reflog exists refs/heads/d &&\n> +\tgit reflog exists refs/heads/e\n> +'\n\nA reasonable design of \"copy\" is for d's log to be left intact, and\ne's log to begin with a single entry \"created by copying from d\".\nAnother possible design is to copy the log (making them identical),\nand then add one entry to e's log \"created by copying from d\".\n\nThe above test would succeed with either implementation and does not\nanswer \"should reflog be copied?\" (see above about why \"here are the\ntests that shows my thinking--read them and comment\" is a bad\napproach).\n\n\"move\" that makes the source of the movement disappear after the\noperation has a stronger justification of moving the log under the\nnew name (the only alternative is to lose the history of the source\nforever), it is debatable if \"copy\" wants to retain a copy of the\nhistory of an otherwise unrelated branch, which has its own history\nretained after the operation.\n\n> +\n> +test_expect_success 'config information was copied, too' '\n> +\ttest $(git config branch.e.dummy) = Hello &&\n> +\ttest $(git config branch.d.dummy) = Hello\n> +'\n\nThe expectation is that a configuration like 'dummy' that does not\nhave any meaning to Git itself will all blindly be copied, which is\nsimilar to what \"move\" does.\n\n> +git config branch.f/f.dummy Hello\n> +\n> +test_expect_success 'git branch -c f/f g/g should work' '\n> +\tgit branch -l f/f &&\n> +\tgit reflog exists refs/heads/f/f &&\n> +\tgit branch -c f/f g/g &&\n> +\tgit reflog exists refs/heads/f/f &&\n> +\tgit reflog exists refs/heads/g/g\n> +'\n\nHmph.  What's the reason to expect this not to work?\n\n> +test_expect_success 'config information was copied, too' '\n> +\ttest $(git config branch.f/f.dummy) = Hello &&\n> +\ttest $(git config branch.g/g.dummy) = Hello\n> +'\n\nIsn't this part of the \"should work\" test above?  The definition of\nworking is not just reflog is created for the new branch without the\nold branch losing its reflog, right?\n\n> +test_expect_success 'git branch -c m2 m2 should work' '\n> +\tgit branch -l m2 &&\n> +\tgit reflog exists refs/heads/m2 &&\n> +\tgit branch -c m2 m2 &&\n> +\tgit reflog exists refs/heads/m2\n> +'\n\nIs it OK for the configuration be lost silently?\n\nThanks.\n\n\n[Footnote]\n\n*1* One way to avoid having to design an elaborate \"definition of\n    copying\" from scratch is to piggy-back on whatever \"move\" does.\n    Your definition of \"copy\" could be \"copying from A to create B,\n    followed by deleting A, should leave the identical result as\n    moving A to create B\".  And ask people if they agree with that\n    definition as \"the first principle\".\n\n    All the questions in the paragraph that begins with \"For\n    example...\" and similar ones can be answered by followingthe\n    first principle.\n\n\n"},{"id":"320961","messageId":"CALiud+mYqVoVoFOsRDcXyXXRj857vRC3n6f0PPyX0qBvJs8C5A@mail.gmail.com","threadId":"46064","inReplyTo":"xmqqr2z88lcy.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH/RFC] branch: add tests for new copy branch feature","fromName":"Sahil Dua","fromEmail":"sahildua2305@gmail.com","sentAt":"2017-05-29T19:39:50Z","receivedAt":"2017-05-29T19:40:16Z","isPatch":true,"sender":{"key":"sahildua2305@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5206277?v=4"},"body":"Thanks, Junio for raising all these important questions.\n\nIndeed, showing tests in order to explain my thinking about the\nfeature was a bad idea. I realise that I should have explained the\nfeature first instead of getting the tests reviewed without any\nelaboration of the intentions. I will explain it now.\n\nMy definition of \"copy\" for this feature is \"copying from A to create\nB, keeping A intact\". That means \"copy branch A to B\" should do\nwhatever \"move branch A to B\" does except it shouldn't delete A and\nshould keep A unchanged.\n\n1. When a branch topic-2 is created by copying from branch topic-1,\ntopic-2 branch reflog should now contain the all the entries of\ntopic-1 branch (before copying) followed by \"Copied from topic-1\".\n[This is debatable though, I want inputs/suggestions about this.]\n\n2. Copying a branch should also copy the git config section for that\nbranch. This means if topic-2 branch is created from topic-1,\n\"branch.topic-2.remote\" should now be same as \"branch.topic-1.remote\",\nif set.\n\n3. \"git push\" to copied branch for example - topic-2 should push a new\nbranch with the same name in the remote repo. That means if topic-1\nwas previously pushed and a new branch topic-2 is copied from topic-1,\n\"git push\" on topic-2 branch won't push to the same branch as \"git\npush on topic-1 branch would.\n\n4. \"git branch -c new-branch\" should copy the currently checked out\nbranch and create a new branch with name \"new-branch\".\n\nOn Mon, May 29, 2017 at 4:09 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Sahil Dua <sahildua2305@gmail.com> writes:\n>\n> > New feature - copying a branch along with its config section.\n>\n> That's an unusual non-sentence (without a verb) in our commit message.\n\nIndeed terrible. I will remove it.\n\n>\n>\n> > Aim is to have an option -c for copying a branch just like -m option for\n> > renaming a branch.\n>\n> What should it copy literally, and what should it copy with\n> adjustment (and adjusting what), and what should it refrain from\n> copying?\n>\n> For example, when you create a branch topic-2 by copying from a\n> branch topic-1, do you copy the reflog in such a way that topic-2's\n> reflog contains all the entries of topic-1 before the copy happens,\n> capped with a new (and not shared with topic-1) entry that says\n> \"Copied from topic-1\"?  When topic-1 is set to pull from a custom\n> upstream 'upstream' (i.e. not \"origin\")'s 'trunk' branch, by setting\n> 'branch.topic-2.remote' to \"upstream\", i.e. the same as the value of\n> 'branch.topic-1.remote' and 'branch.topic-2.merge' to \"trunk\",\n> i.e. the same as 'branch.topic-1.merge'?  Should a \"git push\" while\n> topic-2 is checked out push to the same remote as a \"git push\" would\n> push to when topic-1 is checked out?  Should they update the same\n> branch at the remote?  Why and/or why not? [*1*]\n>\n> > This commit adds a few basic tests for getting any suggestions/feedback\n> > about expected behavior for this new feature.\n>\n> Writing tests is a good way to prepare for an implementation, which\n> must be done according to the design, but that happens after the\n> design is polished and judged to be a good one.  While soliciting\n> comments on the design from others, tests are a bit too low level to\n> express what you are aiming at.  It is a bit unhelpful to those who\n> may want to help to figure out answers to questions like the ones in\n> the previos paragraph (the one that begins with \"For example,...\")\n> by telling them to \"read my intention from these tests\", and when\n> you want as wide an input as possible, that is counterproductive for\n> your objective ;-).\n>\n> > Signed-off-by: Sahil Dua <sahildua2305@gmail.com>\n> > ---\n> >  t/t3200-branch.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n> >  1 file changed, 53 insertions(+)\n> >\n> > diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> > index fe62e7c775da..2c95ed6ebf3c 100755\n> > --- a/t/t3200-branch.sh\n> > +++ b/t/t3200-branch.sh\n> > @@ -341,6 +341,59 @@ test_expect_success 'config information was renamed, too' '\n> >       test_must_fail git config branch.s/s/dummy\n> >  '\n> >\n> > +test_expect_success 'git branch -c dumps usage' '\n> > +     test_expect_code 128 git branch -c 2>err &&\n> > +     test_i18ngrep \"branch name required\" err\n> > +'\n>\n> OK.  Do we want to support a single-name format (i.e. copy from the\n> current)?\n>\n>\n>\n> > +git config branch.d.dummy Hello\n>\n> Write your tests to do as little outside test_expct_success block as\n> possible, and do a set-up close to where it matters.\n>\n\nRegarding putting code outside test_expect_success block, I wrote this\nto keep tests consistent within the same file as I saw other test\ndoing it this way. How can I improve this?\n\nHere, I needed to set branch config before I copy so that I can\nconfirm that the new \"copied\" branch has the same config as well. How\ncan I improve this and take it as close to where-it-matters as\npossible?\n\n>\n> > +test_expect_success 'git branch -c d e should work' '\n> > +     git branch -l d &&\n> > +     git reflog exists refs/heads/d &&\n> > +     git branch -c d e &&\n> > +     git reflog exists refs/heads/d &&\n> > +     git reflog exists refs/heads/e\n> > +'\n>\n> A reasonable design of \"copy\" is for d's log to be left intact, and\n> e's log to begin with a single entry \"created by copying from d\".\n> Another possible design is to copy the log (making them identical),\n> and then add one entry to e's log \"created by copying from d\".\n>\n\nYes, this is debatable as I mentioned above. My personal choice will\nbe to use the second approach mentioned by you here.\n\n>\n> The above test would succeed with either implementation and does not\n> answer \"should reflog be copied?\" (see above about why \"here are the\n> tests that shows my thinking--read them and comment\" is a bad\n> approach).\n>\n> \"move\" that makes the source of the movement disappear after the\n> operation has a stronger justification of moving the log under the\n> new name (the only alternative is to lose the history of the source\n> forever), it is debatable if \"copy\" wants to retain a copy of the\n> history of an otherwise unrelated branch, which has its own history\n> retained after the operation.\n>\n\nFor example, if branch topic-2 is copied from branch topic-1, \"copy\"\noption should make sure the history of branch topic-1 is retained. I\nwill need suggestions about this too, in case I'm missing some\nconsequences/corner-cases of this.\n\n>\n> > +\n> > +test_expect_success 'config information was copied, too' '\n> > +     test $(git config branch.e.dummy) = Hello &&\n> > +     test $(git config branch.d.dummy) = Hello\n> > +'\n>\n> The expectation is that a configuration like 'dummy' that does not\n> have any meaning to Git itself will all blindly be copied, which is\n> similar to what \"move\" does.\n\nYes, any configuration for topic-1 branch should be copied to topic-2\nbranch while keeping the configuration for topic-1 branch retained.\n\n>\n>\n> > +git config branch.f/f.dummy Hello\n> > +\n> > +test_expect_success 'git branch -c f/f g/g should work' '\n> > +     git branch -l f/f &&\n> > +     git reflog exists refs/heads/f/f &&\n> > +     git branch -c f/f g/g &&\n> > +     git reflog exists refs/heads/f/f &&\n> > +     git reflog exists refs/heads/g/g\n> > +'\n>\n> Hmph.  What's the reason to expect this not to work?\n>\n>\n>\n> > +test_expect_success 'config information was copied, too' '\n> > +     test $(git config branch.f/f.dummy) = Hello &&\n> > +     test $(git config branch.g/g.dummy) = Hello\n> > +'\n>\n> Isn't this part of the \"should work\" test above?  The definition of\n> working is not just reflog is created for the new branch without the\n> old branch losing its reflog, right?\n>\n\nRight, I will put them in one test together.\n\n> > +test_expect_success 'git branch -c m2 m2 should work' '\n> > +     git branch -l m2 &&\n> > +     git reflog exists refs/heads/m2 &&\n> > +     git branch -c m2 m2 &&\n> > +     git reflog exists refs/heads/m2\n> > +'\n>\n> Is it OK for the configuration be lost silently?\n\nNo, I will change this accordingly as well.\n\n>\n>\n> Thanks.\n>\n>\n> [Footnote]\n>\n> *1* One way to avoid having to design an elaborate \"definition of\n>     copying\" from scratch is to piggy-back on whatever \"move\" does.\n>     Your definition of \"copy\" could be \"copying from A to create B,\n>     followed by deleting A, should leave the identical result as\n>     moving A to create B\".  And ask people if they agree with that\n>     definition as \"the first principle\".\n>\n>     All the questions in the paragraph that begins with \"For\n>     example...\" and similar ones can be answered by followingthe\n>     first principle.\n>\n>\n\nThanks.\n"},{"id":"320966","messageId":"CALiud+mGovAoPriJxzCB93jwkChyRb49yxVtNijfag2zx-jZ0Q@mail.gmail.com","threadId":"46064","inReplyTo":"CACBZZX4LpRK5q+cxToWqGeW2ZgN4ggo4h9SJpx0mZkZUR15jNw@mail.gmail.com","subject":"Re: [PATCH/RFC] branch: add tests for new copy branch feature","fromName":"Sahil Dua","fromEmail":"sahildua2305@gmail.com","sentAt":"2017-05-29T20:41:27Z","receivedAt":"2017-05-29T20:41:52Z","isPatch":true,"sender":{"key":"sahildua2305@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5206277?v=4"},"body":"On Mon, May 29, 2017 at 1:30 AM, Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n> On Mon, May 29, 2017 at 12:56 AM, Sahil Dua <sahildua2305@gmail.com> wrote:\n>> New feature - copying a branch along with its config section.\n>>\n>> Aim is to have an option -c for copying a branch just like -m option for\n>> renaming a branch.\n>>\n>> This commit adds a few basic tests for getting any suggestions/feedback\n>> about expected behavior for this new feature.\n>>\n>> Signed-off-by: Sahil Dua <sahildua2305@gmail.com>\n>> ---\n>>  t/t3200-branch.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n>>  1 file changed, 53 insertions(+)\n>>\n>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n>> index fe62e7c775da..2c95ed6ebf3c 100755\n>> --- a/t/t3200-branch.sh\n>> +++ b/t/t3200-branch.sh\n>> @@ -341,6 +341,59 @@ test_expect_success 'config information was renamed, too' '\n>>         test_must_fail git config branch.s/s/dummy\n>>  '\n>>\n>> +test_expect_success 'git branch -c dumps usage' '\n>> +       test_expect_code 128 git branch -c 2>err &&\n>> +       test_i18ngrep \"branch name required\" err\n>> +'\n>> +\n>> +git config branch.d.dummy Hello\n>> +\n>> +test_expect_success 'git branch -c d e should work' '\n>> +       git branch -l d &&\n>> +       git reflog exists refs/heads/d &&\n>> +       git branch -c d e &&\n>> +       git reflog exists refs/heads/d &&\n>> +       git reflog exists refs/heads/e\n>> +'\n>> +\n>> +test_expect_success 'config information was copied, too' '\n>> +       test $(git config branch.e.dummy) = Hello &&\n>> +       test $(git config branch.d.dummy) = Hello\n>> +'\n>> +\n>> +git config branch.f/f.dummy Hello\n>> +\n>> +test_expect_success 'git branch -c f/f g/g should work' '\n>> +       git branch -l f/f &&\n>> +       git reflog exists refs/heads/f/f &&\n>> +       git branch -c f/f g/g &&\n>> +       git reflog exists refs/heads/f/f &&\n>> +       git reflog exists refs/heads/g/g\n>> +'\n>> +\n>> +test_expect_success 'config information was copied, too' '\n>> +       test $(git config branch.f/f.dummy) = Hello &&\n>> +       test $(git config branch.g/g.dummy) = Hello\n>> +'\n>> +\n>> +test_expect_success 'git branch -c m2 m2 should work' '\n>> +       git branch -l m2 &&\n>> +       git reflog exists refs/heads/m2 &&\n>> +       git branch -c m2 m2 &&\n>> +       git reflog exists refs/heads/m2\n>> +'\n>> +\n>> +test_expect_success 'git branch -c a a/a should fail' '\n>> +       git branch -l a &&\n>> +       git reflog exists refs/heads/a &&\n>> +       test_must_fail git branch -c a a/a\n>> +'\n>> +\n>> +test_expect_success 'git branch -c b/b b should fail' '\n>> +       git branch -l b/b &&\n>> +       test_must_fail git branch -c b/b b\n>> +'\n>> +\n>>  test_expect_success 'deleting a symref' '\n>>         git branch target &&\n>>         git symbolic-ref refs/heads/symref refs/heads/target &&\n>>\n>\n> This looks good to me. Comments, in no particular order:\n>\n> * There should be a test for `git branch -c <newbranch>`, i.e. that\n> should implicitly copy from HEAD just like `git branch -m <newbranch>`\n> does. However, when looking at this I can see there's actually no test\n> for one-argument `git branch -m`, i.e. if you apply this:\n>\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -699,8 +699,6 @@ int cmd_branch(int argc, const char **argv, const\n> char *prefix)\n>         } else if (rename) {\n>                 if (!argc)\n>                         die(_(\"branch name required\"));\n> -               else if (argc == 1)\n> -                       rename_branch(head, argv[0], rename > 1);\n>                 else if (argc == 2)\n>                         rename_branch(argv[0], argv[1], rename > 1);\n>                 else\n>\n> The only test that fails is a `git branch -M master`, i.e.\n> one-argument -M is tested for, but not -m, same codepath though, but\n> while you're at it a patch/series like this could start by adding a\n> one-arg -m test, then follow-up by copying that for the new `-c`.\n>\n\nThanks for the suggestion. Yes, I will add one-arg test for -c. Is it\nok to send a different patch for adding a one-arg test for existing -m\noption?\n\n> * We should have a -C to force -c just like -M forces -m, i.e. a test\n> where one-arg `git branch -C alreadyexists` clobbers alreadyexists,\n> and `git branch -C source alreadyexists` does the same for two-arg.\n>\nYes, I missed this. I will add -C option too.\n\n> * I know this is just something you're copying, but this `git branch\n> -l <name>` use gets me every time \"wait how does listing it help isn't\n> that always succeeding ... damnit it's --create-reflog not --list, got\n> me again\" :)\n>\n\nYes, it was confusing to me too in the beginning. I will use --create-reflog.\n\n> Just noting it in case it confuses other reviewers who are skimming\n> this. Might be worth it to just use --create-reflog for new tests\n> instead of the non-obvious -l whatever the existing tests in the file\n> do, or maybe I'm the only one confused by this :)\n>\n> * When you use `git branch -m` it adds a note to the reflog, your\n> patch should have a test like the existing \"git branch -M baz bam\n> should add entries to .git/logs/HEAD\" test in this file except\n> \"Branch: copied ...\" instead of \"Branch: renamed...\".\n>\n\nNice, I will add it. Thanks.\n\n> * Should there be tests for `git branch -c master master` like we have\n> for `git branch -m master master` (see 3f59481e33 (\"branch: allow a\n> no-op \"branch -M <current-branch> HEAD\"\", 2011-11-25)). Allowing this\n> for -m smells like a bend-over-backwards workaround for some script\n> Jonathan had, should we be doing this for -c too? I don't know.\n\nNot sure I understand this. Can you please elaborate?\nThanks.\n"},{"id":"320967","messageId":"CACBZZX6LcTzFKTe0fENj95Vm7GrxT5HHs3pzrqajG0XApN-rbw@mail.gmail.com","threadId":"46064","inReplyTo":"CALiud+mGovAoPriJxzCB93jwkChyRb49yxVtNijfag2zx-jZ0Q@mail.gmail.com","subject":"Re: [PATCH/RFC] branch: add tests for new copy branch feature","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-29T20:50:20Z","receivedAt":"2017-05-29T20:50:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Mon, May 29, 2017 at 10:41 PM, Sahil Dua <sahildua2305@gmail.com> wrote:\n> On Mon, May 29, 2017 at 1:30 AM, Ævar Arnfjörð Bjarmason\n> <avarab@gmail.com> wrote:\n>> On Mon, May 29, 2017 at 12:56 AM, Sahil Dua <sahildua2305@gmail.com> wrote:\n>>> New feature - copying a branch along with its config section.\n>>>\n>>> Aim is to have an option -c for copying a branch just like -m option for\n>>> renaming a branch.\n>>>\n>>> This commit adds a few basic tests for getting any suggestions/feedback\n>>> about expected behavior for this new feature.\n>>>\n>>> Signed-off-by: Sahil Dua <sahildua2305@gmail.com>\n>>> ---\n>>>  t/t3200-branch.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n>>>  1 file changed, 53 insertions(+)\n>>>\n>>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n>>> index fe62e7c775da..2c95ed6ebf3c 100755\n>>> --- a/t/t3200-branch.sh\n>>> +++ b/t/t3200-branch.sh\n>>> @@ -341,6 +341,59 @@ test_expect_success 'config information was renamed, too' '\n>>>         test_must_fail git config branch.s/s/dummy\n>>>  '\n>>>\n>>> +test_expect_success 'git branch -c dumps usage' '\n>>> +       test_expect_code 128 git branch -c 2>err &&\n>>> +       test_i18ngrep \"branch name required\" err\n>>> +'\n>>> +\n>>> +git config branch.d.dummy Hello\n>>> +\n>>> +test_expect_success 'git branch -c d e should work' '\n>>> +       git branch -l d &&\n>>> +       git reflog exists refs/heads/d &&\n>>> +       git branch -c d e &&\n>>> +       git reflog exists refs/heads/d &&\n>>> +       git reflog exists refs/heads/e\n>>> +'\n>>> +\n>>> +test_expect_success 'config information was copied, too' '\n>>> +       test $(git config branch.e.dummy) = Hello &&\n>>> +       test $(git config branch.d.dummy) = Hello\n>>> +'\n>>> +\n>>> +git config branch.f/f.dummy Hello\n>>> +\n>>> +test_expect_success 'git branch -c f/f g/g should work' '\n>>> +       git branch -l f/f &&\n>>> +       git reflog exists refs/heads/f/f &&\n>>> +       git branch -c f/f g/g &&\n>>> +       git reflog exists refs/heads/f/f &&\n>>> +       git reflog exists refs/heads/g/g\n>>> +'\n>>> +\n>>> +test_expect_success 'config information was copied, too' '\n>>> +       test $(git config branch.f/f.dummy) = Hello &&\n>>> +       test $(git config branch.g/g.dummy) = Hello\n>>> +'\n>>> +\n>>> +test_expect_success 'git branch -c m2 m2 should work' '\n>>> +       git branch -l m2 &&\n>>> +       git reflog exists refs/heads/m2 &&\n>>> +       git branch -c m2 m2 &&\n>>> +       git reflog exists refs/heads/m2\n>>> +'\n>>> +\n>>> +test_expect_success 'git branch -c a a/a should fail' '\n>>> +       git branch -l a &&\n>>> +       git reflog exists refs/heads/a &&\n>>> +       test_must_fail git branch -c a a/a\n>>> +'\n>>> +\n>>> +test_expect_success 'git branch -c b/b b should fail' '\n>>> +       git branch -l b/b &&\n>>> +       test_must_fail git branch -c b/b b\n>>> +'\n>>> +\n>>>  test_expect_success 'deleting a symref' '\n>>>         git branch target &&\n>>>         git symbolic-ref refs/heads/symref refs/heads/target &&\n>>>\n>>\n>> This looks good to me. Comments, in no particular order:\n>>\n>> * There should be a test for `git branch -c <newbranch>`, i.e. that\n>> should implicitly copy from HEAD just like `git branch -m <newbranch>`\n>> does. However, when looking at this I can see there's actually no test\n>> for one-argument `git branch -m`, i.e. if you apply this:\n>>\n>> --- a/builtin/branch.c\n>> +++ b/builtin/branch.c\n>> @@ -699,8 +699,6 @@ int cmd_branch(int argc, const char **argv, const\n>> char *prefix)\n>>         } else if (rename) {\n>>                 if (!argc)\n>>                         die(_(\"branch name required\"));\n>> -               else if (argc == 1)\n>> -                       rename_branch(head, argv[0], rename > 1);\n>>                 else if (argc == 2)\n>>                         rename_branch(argv[0], argv[1], rename > 1);\n>>                 else\n>>\n>> The only test that fails is a `git branch -M master`, i.e.\n>> one-argument -M is tested for, but not -m, same codepath though, but\n>> while you're at it a patch/series like this could start by adding a\n>> one-arg -m test, then follow-up by copying that for the new `-c`.\n>>\n>\n> Thanks for the suggestion. Yes, I will add one-arg test for -c. Is it\n> ok to send a different patch for adding a one-arg test for existing -m\n> option?\n\nYeah, it makes sense to just make the first patch in the series be\nsome cleanup / improvement of the existing tests, which the subsequent\ntests for -c then make use of / copy. It could even be sent on its\nown, but probably makes sense to just bundle them up. Up to you\nthough, in this case you won't need patch A for patch B to work, so\nthe that's one argument against bundling them up. Personally I'd do it\nif I was hacking this just because it's more convenient to keep track\nof fewer things.\n\n>> * We should have a -C to force -c just like -M forces -m, i.e. a test\n>> where one-arg `git branch -C alreadyexists` clobbers alreadyexists,\n>> and `git branch -C source alreadyexists` does the same for two-arg.\n>>\n> Yes, I missed this. I will add -C option too.\n>\n>> * I know this is just something you're copying, but this `git branch\n>> -l <name>` use gets me every time \"wait how does listing it help isn't\n>> that always succeeding ... damnit it's --create-reflog not --list, got\n>> me again\" :)\n>>\n>\n> Yes, it was confusing to me too in the beginning. I will use --create-reflog.\n>\n>> Just noting it in case it confuses other reviewers who are skimming\n>> this. Might be worth it to just use --create-reflog for new tests\n>> instead of the non-obvious -l whatever the existing tests in the file\n>> do, or maybe I'm the only one confused by this :)\n>>\n>> * When you use `git branch -m` it adds a note to the reflog, your\n>> patch should have a test like the existing \"git branch -M baz bam\n>> should add entries to .git/logs/HEAD\" test in this file except\n>> \"Branch: copied ...\" instead of \"Branch: renamed...\".\n>>\n>\n> Nice, I will add it. Thanks.\n>\n>> * Should there be tests for `git branch -c master master` like we have\n>> for `git branch -m master master` (see 3f59481e33 (\"branch: allow a\n>> no-op \"branch -M <current-branch> HEAD\"\", 2011-11-25)). Allowing this\n>> for -m smells like a bend-over-backwards workaround for some script\n>> Jonathan had, should we be doing this for -c too? I don't know.\n>\n> Not sure I understand this. Can you please elaborate?\n> Thanks.\n\nSo the reason we have this for -m is:\n\n    commit 3f59481e33\n    Author: Jonathan Nieder <jrnieder@gmail.com>\n    Date:   Fri Nov 25 20:30:02 2011 -0600\n\n    branch: allow a no-op \"branch -M <current-branch> HEAD\"\n\n    Overwriting the current branch with a different commit is forbidden, as it\n    will make the status recorded in the index and the working tree out of\n    sync with respect to the HEAD. There however is no reason to forbid it if\n    the current branch is renamed to itself, which admittedly is something\n    only an insane user would do, but is handy for scripts.\n\nMy understanding of that last part is that Jonathan/someone (see\nreported-by in that patch) had some script which was renaming\nbranches, and it was easier for whatever reason to just make it no-op\nif the rename would have yielded the same result as doing nothing at\nall.\n\nMost likely your implementation will consist of just re-using the\nlogic in rename_branch() (and renaming it to e.g.\ncopy_or_rename_branch() ...) so you could just re-use the no-op\nbehavior we use for -m, or if there's some reason not to no-op and\nerror instead for -c we could just do that, but in any case this case\nof `git branch -c master master` or `git branch -c currentbranch`\nshould be tested for.\n"},{"id":"320971","messageId":"CALiud+n3TPNCy4WNRjdaKpEU0yNh=UNAKx96pHqmi-X1sYChxA@mail.gmail.com","threadId":"46064","inReplyTo":"CACBZZX6LcTzFKTe0fENj95Vm7GrxT5HHs3pzrqajG0XApN-rbw@mail.gmail.com","subject":"Re: [PATCH/RFC] branch: add tests for new copy branch feature","fromName":"Sahil Dua","fromEmail":"sahildua2305@gmail.com","sentAt":"2017-05-29T22:23:36Z","receivedAt":"2017-05-29T22:24:03Z","isPatch":true,"sender":{"key":"sahildua2305@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5206277?v=4"},"body":"On Mon, May 29, 2017 at 10:50 PM, Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n> On Mon, May 29, 2017 at 10:41 PM, Sahil Dua <sahildua2305@gmail.com> wrote:\n>> On Mon, May 29, 2017 at 1:30 AM, Ævar Arnfjörð Bjarmason\n>> <avarab@gmail.com> wrote:\n>>> On Mon, May 29, 2017 at 12:56 AM, Sahil Dua <sahildua2305@gmail.com> wrote:\n>>>> New feature - copying a branch along with its config section.\n>>>>\n>>>> Aim is to have an option -c for copying a branch just like -m option for\n>>>> renaming a branch.\n>>>>\n>>>> This commit adds a few basic tests for getting any suggestions/feedback\n>>>> about expected behavior for this new feature.\n>>>>\n>>>> Signed-off-by: Sahil Dua <sahildua2305@gmail.com>\n>>>> ---\n>>>>  t/t3200-branch.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n>>>>  1 file changed, 53 insertions(+)\n>>>>\n>>>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n>>>> index fe62e7c775da..2c95ed6ebf3c 100755\n>>>> --- a/t/t3200-branch.sh\n>>>> +++ b/t/t3200-branch.sh\n>>>> @@ -341,6 +341,59 @@ test_expect_success 'config information was renamed, too' '\n>>>>         test_must_fail git config branch.s/s/dummy\n>>>>  '\n>>>>\n>>>> +test_expect_success 'git branch -c dumps usage' '\n>>>> +       test_expect_code 128 git branch -c 2>err &&\n>>>> +       test_i18ngrep \"branch name required\" err\n>>>> +'\n>>>> +\n>>>> +git config branch.d.dummy Hello\n>>>> +\n>>>> +test_expect_success 'git branch -c d e should work' '\n>>>> +       git branch -l d &&\n>>>> +       git reflog exists refs/heads/d &&\n>>>> +       git branch -c d e &&\n>>>> +       git reflog exists refs/heads/d &&\n>>>> +       git reflog exists refs/heads/e\n>>>> +'\n>>>> +\n>>>> +test_expect_success 'config information was copied, too' '\n>>>> +       test $(git config branch.e.dummy) = Hello &&\n>>>> +       test $(git config branch.d.dummy) = Hello\n>>>> +'\n>>>> +\n>>>> +git config branch.f/f.dummy Hello\n>>>> +\n>>>> +test_expect_success 'git branch -c f/f g/g should work' '\n>>>> +       git branch -l f/f &&\n>>>> +       git reflog exists refs/heads/f/f &&\n>>>> +       git branch -c f/f g/g &&\n>>>> +       git reflog exists refs/heads/f/f &&\n>>>> +       git reflog exists refs/heads/g/g\n>>>> +'\n>>>> +\n>>>> +test_expect_success 'config information was copied, too' '\n>>>> +       test $(git config branch.f/f.dummy) = Hello &&\n>>>> +       test $(git config branch.g/g.dummy) = Hello\n>>>> +'\n>>>> +\n>>>> +test_expect_success 'git branch -c m2 m2 should work' '\n>>>> +       git branch -l m2 &&\n>>>> +       git reflog exists refs/heads/m2 &&\n>>>> +       git branch -c m2 m2 &&\n>>>> +       git reflog exists refs/heads/m2\n>>>> +'\n>>>> +\n>>>> +test_expect_success 'git branch -c a a/a should fail' '\n>>>> +       git branch -l a &&\n>>>> +       git reflog exists refs/heads/a &&\n>>>> +       test_must_fail git branch -c a a/a\n>>>> +'\n>>>> +\n>>>> +test_expect_success 'git branch -c b/b b should fail' '\n>>>> +       git branch -l b/b &&\n>>>> +       test_must_fail git branch -c b/b b\n>>>> +'\n>>>> +\n>>>>  test_expect_success 'deleting a symref' '\n>>>>         git branch target &&\n>>>>         git symbolic-ref refs/heads/symref refs/heads/target &&\n>>>>\n>>>\n>>> This looks good to me. Comments, in no particular order:\n>>>\n>>> * There should be a test for `git branch -c <newbranch>`, i.e. that\n>>> should implicitly copy from HEAD just like `git branch -m <newbranch>`\n>>> does. However, when looking at this I can see there's actually no test\n>>> for one-argument `git branch -m`, i.e. if you apply this:\n>>>\n>>> --- a/builtin/branch.c\n>>> +++ b/builtin/branch.c\n>>> @@ -699,8 +699,6 @@ int cmd_branch(int argc, const char **argv, const\n>>> char *prefix)\n>>>         } else if (rename) {\n>>>                 if (!argc)\n>>>                         die(_(\"branch name required\"));\n>>> -               else if (argc == 1)\n>>> -                       rename_branch(head, argv[0], rename > 1);\n>>>                 else if (argc == 2)\n>>>                         rename_branch(argv[0], argv[1], rename > 1);\n>>>                 else\n>>>\n>>> The only test that fails is a `git branch -M master`, i.e.\n>>> one-argument -M is tested for, but not -m, same codepath though, but\n>>> while you're at it a patch/series like this could start by adding a\n>>> one-arg -m test, then follow-up by copying that for the new `-c`.\n>>>\n>>\n>> Thanks for the suggestion. Yes, I will add one-arg test for -c. Is it\n>> ok to send a different patch for adding a one-arg test for existing -m\n>> option?\n>\n> Yeah, it makes sense to just make the first patch in the series be\n> some cleanup / improvement of the existing tests, which the subsequent\n> tests for -c then make use of / copy. It could even be sent on its\n> own, but probably makes sense to just bundle them up. Up to you\n> though, in this case you won't need patch A for patch B to work, so\n> the that's one argument against bundling them up. Personally I'd do it\n> if I was hacking this just because it's more convenient to keep track\n> of fewer things.\n>\n\nGot it. I will submit a patch for the tests for -m option.\n\n>>> * We should have a -C to force -c just like -M forces -m, i.e. a test\n>>> where one-arg `git branch -C alreadyexists` clobbers alreadyexists,\n>>> and `git branch -C source alreadyexists` does the same for two-arg.\n>>>\n>> Yes, I missed this. I will add -C option too.\n>>\n>>> * I know this is just something you're copying, but this `git branch\n>>> -l <name>` use gets me every time \"wait how does listing it help isn't\n>>> that always succeeding ... damnit it's --create-reflog not --list, got\n>>> me again\" :)\n>>>\n>>\n>> Yes, it was confusing to me too in the beginning. I will use --create-reflog.\n>>\n>>> Just noting it in case it confuses other reviewers who are skimming\n>>> this. Might be worth it to just use --create-reflog for new tests\n>>> instead of the non-obvious -l whatever the existing tests in the file\n>>> do, or maybe I'm the only one confused by this :)\n>>>\n>>> * When you use `git branch -m` it adds a note to the reflog, your\n>>> patch should have a test like the existing \"git branch -M baz bam\n>>> should add entries to .git/logs/HEAD\" test in this file except\n>>> \"Branch: copied ...\" instead of \"Branch: renamed...\".\n>>>\n>>\n>> Nice, I will add it. Thanks.\n>>\n>>> * Should there be tests for `git branch -c master master` like we have\n>>> for `git branch -m master master` (see 3f59481e33 (\"branch: allow a\n>>> no-op \"branch -M <current-branch> HEAD\"\", 2011-11-25)). Allowing this\n>>> for -m smells like a bend-over-backwards workaround for some script\n>>> Jonathan had, should we be doing this for -c too? I don't know.\n>>\n>> Not sure I understand this. Can you please elaborate?\n>> Thanks.\n>\n> So the reason we have this for -m is:\n>\n>     commit 3f59481e33\n>     Author: Jonathan Nieder <jrnieder@gmail.com>\n>     Date:   Fri Nov 25 20:30:02 2011 -0600\n>\n>     branch: allow a no-op \"branch -M <current-branch> HEAD\"\n>\n>     Overwriting the current branch with a different commit is forbidden, as it\n>     will make the status recorded in the index and the working tree out of\n>     sync with respect to the HEAD. There however is no reason to forbid it if\n>     the current branch is renamed to itself, which admittedly is something\n>     only an insane user would do, but is handy for scripts.\n>\n> My understanding of that last part is that Jonathan/someone (see\n> reported-by in that patch) had some script which was renaming\n> branches, and it was easier for whatever reason to just make it no-op\n> if the rename would have yielded the same result as doing nothing at\n> all.\n>\n> Most likely your implementation will consist of just re-using the\n> logic in rename_branch() (and renaming it to e.g.\n> copy_or_rename_branch() ...) so you could just re-use the no-op\n> behavior we use for -m, or if there's some reason not to no-op and\n> error instead for -c we could just do that, but in any case this case\n> of `git branch -c master master` or `git branch -c currentbranch`\n> should be tested for.\n\nUnderstood. Thanks. I will add tests for this too.\n"},{"id":"322122","messageId":"20170613175501.GH133952@aiede.mtv.corp.google.com","threadId":"46064","inReplyTo":"CACBZZX6LcTzFKTe0fENj95Vm7GrxT5HHs3pzrqajG0XApN-rbw@mail.gmail.com","subject":"Re: [PATCH/RFC] branch: add tests for new copy branch feature","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-06-13T17:55:01Z","receivedAt":"2017-06-13T17:55:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nÆvar Arnfjörð Bjarmason wrote:\n\n> So the reason we have this for -m is:\n>\n>     commit 3f59481e33\n>     Author: Jonathan Nieder <jrnieder@gmail.com>\n>     Date:   Fri Nov 25 20:30:02 2011 -0600\n>\n>     branch: allow a no-op \"branch -M <current-branch> HEAD\"\n>\n>     Overwriting the current branch with a different commit is forbidden, as it\n>     will make the status recorded in the index and the working tree out of\n>     sync with respect to the HEAD. There however is no reason to forbid it if\n>     the current branch is renamed to itself, which admittedly is something\n>     only an insane user would do, but is handy for scripts.\n>\n> My understanding of that last part is that Jonathan/someone (see\n> reported-by in that patch) had some script which was renaming\n> branches, and it was easier for whatever reason to just make it no-op\n> if the rename would have yielded the same result as doing nothing at\n> all.\n>\n> Most likely your implementation will consist of just re-using the\n> logic in rename_branch() (and renaming it to e.g.\n> copy_or_rename_branch() ...) so you could just re-use the no-op\n> behavior we use for -m, or if there's some reason not to no-op and\n> error instead for -c we could just do that, but in any case this case\n> of `git branch -c master master` or `git branch -c currentbranch`\n> should be tested for.\n\nI may be missing some context, but notice that the above mentioned\ncommit is about -M, not -m.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"322124","messageId":"87o9tr3gxs.fsf@gmail.com","threadId":"46064","inReplyTo":"20170613175501.GH133952@aiede.mtv.corp.google.com","subject":"Re: [PATCH/RFC] branch: add tests for new copy branch feature","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-06-13T18:01:35Z","receivedAt":"2017-06-13T18:02:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jun 13 2017, Jonathan Nieder jotted:\n\n> Hi,\n>\n> Ævar Arnfjörð Bjarmason wrote:\n>\n>> So the reason we have this for -m is:\n>>\n>>     commit 3f59481e33\n>>     Author: Jonathan Nieder <jrnieder@gmail.com>\n>>     Date:   Fri Nov 25 20:30:02 2011 -0600\n>>\n>>     branch: allow a no-op \"branch -M <current-branch> HEAD\"\n>>\n>>     Overwriting the current branch with a different commit is forbidden, as it\n>>     will make the status recorded in the index and the working tree out of\n>>     sync with respect to the HEAD. There however is no reason to forbid it if\n>>     the current branch is renamed to itself, which admittedly is something\n>>     only an insane user would do, but is handy for scripts.\n>>\n>> My understanding of that last part is that Jonathan/someone (see\n>> reported-by in that patch) had some script which was renaming\n>> branches, and it was easier for whatever reason to just make it no-op\n>> if the rename would have yielded the same result as doing nothing at\n>> all.\n>>\n>> Most likely your implementation will consist of just re-using the\n>> logic in rename_branch() (and renaming it to e.g.\n>> copy_or_rename_branch() ...) so you could just re-use the no-op\n>> behavior we use for -m, or if there's some reason not to no-op and\n>> error instead for -c we could just do that, but in any case this case\n>> of `git branch -c master master` or `git branch -c currentbranch`\n>> should be tested for.\n>\n> I may be missing some context, but notice that the above mentioned\n> commit is about -M, not -m.\n\nThe context was just that that commit added a change in how -M\ninteracted when clobbering the current HEAD, and that -C should have a\ntest for that behavior, which the patch now submitted to the list has:\n\n    +test_expect_success 'git branch -C master master should work when master is checked out' '\n    +       git checkout master &&\n    +       git branch -C master master\n    +'\n"},{"id":"322126","messageId":"20170613180857.GI133952@aiede.mtv.corp.google.com","threadId":"46064","inReplyTo":"87o9tr3gxs.fsf@gmail.com","subject":"Re: [PATCH/RFC] branch: add tests for new copy branch feature","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-06-13T18:08:57Z","receivedAt":"2017-06-13T18:09:11Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> On Tue, Jun 13 2017, Jonathan Nieder jotted:\n> > Ævar Arnfjörð Bjarmason wrote:\n\n>>> My understanding of that last part is that Jonathan/someone (see\n>>> reported-by in that patch) had some script which was renaming\n>>> branches, and it was easier for whatever reason to just make it no-op\n>>> if the rename would have yielded the same result as doing nothing at\n>>> all.\n>>>\n>>> Most likely your implementation will consist of just re-using the\n>>> logic in rename_branch() (and renaming it to e.g.\n>>> copy_or_rename_branch() ...) so you could just re-use the no-op\n>>> behavior we use for -m, or if there's some reason not to no-op and\n>>> error instead for -c we could just do that, but in any case this case\n>>> of `git branch -c master master` or `git branch -c currentbranch`\n>>> should be tested for.\n>>\n>> I may be missing some context, but notice that the above mentioned\n>> commit is about -M, not -m.\n>\n> The context was just that that commit added a change in how -M\n> interacted when clobbering the current HEAD, and that -C should have a\n> test for that behavior, which the patch now submitted to the list has:\n>\n>     +test_expect_success 'git branch -C master master should work when master is checked out' '\n>     +       git checkout master &&\n>     +       git branch -C master master\n>     +'\n\nPerfect, thanks.  Carry on. :)\n\nJonathan\n"}]}