Re: [PATCH 4/3] branch: fix "copy" to never touch HEAD

[Ævar Arnfjörð Bjarmason]

On Wed, Sep 27 2017, Junio C. Hamano jotted:

> Ævar Arnfjörð Bjarmason  writes:
>
>> I do think however that we also need to update the docs, the relevant
>> origin/master...gitster/sd/branch-copy diff is currently:
>>
>> +The `-c` and `-C` options have the exact same semantics as `-m` and
>> +`-M`, except instead of the branch being renamed it along with its
>> +config and reflog will be copied to a new name.
>>
>> Suggestions welcome, but I think that should probably be changed to
>> something like the following as part of this patch:
>>
>> The `-c` and `-C` options have the exact same semantics as `-m` and
>> `-M`, except instead of the branch being renamed it along with its
>> config and reflog will be copied to a new name. Furthermore, unlike
>> its `-m` and `-M` cousins the `-c` and `-C` options will never move
>> the HEAD, whereas the move option will do so if the source branch is
>> the currently checked-out branch.
>
> I do not think anybody even _imagines_ copy to move HEAD, and do not
> think "unlike -m, it doesn't touch HEAD" a worthwhile thing to say.
>
> It is '-m' whose behaviour may look strange wrt HEAD, so _that_ may
> be worth mentioning, though.
>
> I suspect that your reaction probably comes from being too married
> to the idea that HEAD has a string that is the same as the refname
> of the current branch.  But that is a mere implementation detail.
> Users would think that HEAD points at the current branch and does
> not even care how that pointing is implemented.

To cut to the chase instead of pointlessly replying to this
point-by-point, I think your patch quoted below is good and solves the
minor doc issue I had with your patch.

Yes HEAD is an implementation detail, but it's an exposed implementation
detail.

Thus before your patch it was true to say that "[-c] has the exact same
semantics as [-m] [...] except [ s/move/rename/ ]" since that was the
only behavior change, but with your patch adding another "if (!copy &&
...)" we'd now have two things different in the code, but only one thing
enumerated as being different in the docs.

Just rephrasing it as you did is a better way out of that than my
proposed patch. Thanks.

> Conceptually, you can consider that each branch has its own
> identity, separate from various traits it has, like what its
> upstream branch is, what commit it points at, what its reflog says,
> and (most notably) what its name is.
>
> Then you can think of "branch -m old new" to be (1) finding the
> instance of branch that currently has name 'old' and (2) updating
> only one of its trait, namely, its name, without changing anything
> else.  Creating a new instance of a branch by copying an existing
> one, on the other hand, would then be the matter of (1) finding the
> instance of branch with name 'old' and (2) creating another instance
> of branch with the same traits as the original, modulo its name is
> set to 'new'.
>
> A necessary wrinkle for "branch -m" that falls out of the above
> world model is that HEAD needs to be adjusted in order to keep
> pointing at the same (renamed) instance of branch that now has an
> updated name, if HEAD happened to be pointing at the instance of the
> branch whose name trait has been updated.
>
> So, from that point of view, I'd prefer to do something like the
> attached patch instead.  I notice that "branch -m" does not mention
> configuration variables carried over to the new branch, but I do not
> necessarily think we want to exhaustively enumerate what traits are
> carried over here, so perhaps it is OK as is.
>
>  Documentation/git-branch.txt | 10 ++
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
> index fe029ac6fc..d425e3acd4 100644
> --- a/Documentation/git-branch.txt
> +++ b/Documentation/git-branch.txt
> @@ -63,11 +63,13 @@ With a `-m` or `-M` option,  will be renamed 
> to .
>  If  had a corresponding reflog, it is renamed to match
>  , and a reflog entry is created to remember the branch
>  renaming. If  exists, -M must be used to force the rename
> -to happen.
> +to happen.  If you rename a branch that is currently checked out,
> +`HEAD` is adjusted so that the branch (whose name is now
> +) stays to be the current branch.
>
> -The `-c` and `-C` options have the exact same semantics as `-m` and
> -`-M`, except instead of the branch being renamed it along with its
> -config and reflog will be copied to a new name.
> +With a `-c` or`-C` option, a new branch  is created by
> +copying the traits like the reflog contents and `branch.*.*`
> +configuration from an existing .
>
>  With a `-d` or `-D` option, `` will be deleted.  You may
>  specify more than one branch for deletion.  If the branch currently

Re: [PATCH 4/3] branch: fix "copy" to never touch HEAD

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> I do think however that we also need to update the docs, the relevant
> origin/master...gitster/sd/branch-copy diff is currently:
>
> +The `-c` and `-C` options have the exact same semantics as `-m` and
> +`-M`, except instead of the branch being renamed it along with its
> +config and reflog will be copied to a new name.
>
> Suggestions welcome, but I think that should probably be changed to
> something like the following as part of this patch:
>
> The `-c` and `-C` options have the exact same semantics as `-m` and
> `-M`, except instead of the branch being renamed it along with its
> config and reflog will be copied to a new name. Furthermore, unlike
> its `-m` and `-M` cousins the `-c` and `-C` options will never move
> the HEAD, whereas the move option will do so if the source branch is
> the currently checked-out branch.

I do not think anybody even _imagines_ copy to move HEAD, and do not
think "unlike -m, it doesn't touch HEAD" a worthwhile thing to say.

It is '-m' whose behaviour may look strange wrt HEAD, so _that_ may
be worth mentioning, though.

I suspect that your reaction probably comes from being too married
to the idea that HEAD has a string that is the same as the refname
of the current branch.  But that is a mere implementation detail.
Users would think that HEAD points at the current branch and does
not even care how that pointing is implemented.

Conceptually, you can consider that each branch has its own
identity, separate from various traits it has, like what its
upstream branch is, what commit it points at, what its reflog says,
and (most notably) what its name is.

Then you can think of "branch -m old new" to be (1) finding the
instance of branch that currently has name 'old' and (2) updating
only one of its trait, namely, its name, without changing anything
else.  Creating a new instance of a branch by copying an existing
one, on the other hand, would then be the matter of (1) finding the
instance of branch with name 'old' and (2) creating another instance
of branch with the same traits as the original, modulo its name is
set to 'new'.

A necessary wrinkle for "branch -m" that falls out of the above
world model is that HEAD needs to be adjusted in order to keep
pointing at the same (renamed) instance of branch that now has an
updated name, if HEAD happened to be pointing at the instance of the
branch whose name trait has been updated.

So, from that point of view, I'd prefer to do something like the
attached patch instead.  I notice that "branch -m" does not mention
configuration variables carried over to the new branch, but I do not
necessarily think we want to exhaustively enumerate what traits are
carried over here, so perhaps it is OK as is.

 Documentation/git-branch.txt | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index fe029ac6fc..d425e3acd4 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -63,11 +63,13 @@ With a `-m` or `-M` option,  will be renamed to 
.
 If  had a corresponding reflog, it is renamed to match
 , and a reflog entry is created to remember the branch
 renaming. If  exists, -M must be used to force the rename
-to happen.
+to happen.  If you rename a branch that is currently checked out,
+`HEAD` is adjusted so that the branch (whose name is now
+) stays to be the current branch.
 
-The `-c` and `-C` options have the exact same semantics as `-m` and
-`-M`, except instead of the branch being renamed it along with its
-config and reflog will be copied to a new name.
+With a `-c` or`-C` option, a new branch  is created by
+copying the traits like the reflog contents and `branch.*.*`
+configuration from an existing .
 
 With a `-d` or `-D` option, `` will be deleted.  You may
 specify more than one branch for deletion.  If the branch currently

Re: [PATCH 4/3] branch: fix "copy" to never touch HEAD

[Ævar Arnfjörð Bjarmason]

On Fri, Sep 22 2017, Junio C. Hamano jotted:

> When creating a new branch B by copying the branch A that happens to
> be the current branch, it also updates HEAD to point at the new
> branch.  It probably was made this way because "git branch -c A B"
> piggybacked its implementation on "git branch -m A B",
>
> This does not match the usual expectation.  If I were sitting on a
> blue chair, and somebody comes and repaints it to red, I would
> accept ending up sitting on a chair that is now red (I am also OK to
> stand, instead, as there no longer is my favourite blue chair).  But
> if somebody creates a new red chair, modelling it after the blue
> chair I am sitting on, I do not expect to be booted off of the blue
> chair and ending up on sitting on the new red one.
>
> Let's fix this before it hits 'next'.  Those who want to create a
> new branch and switch to it can do "git checkout B" after doing a
> "git branch -c B", and if that operation is so useful and deserves a
> short-hand way to do so, perhaps extend "git checkout -b B" to copy
> configurations while creating the new branch B.
>
> Signed-off-by: Junio C Hamano 
> ---
>
>  * Let's send an updated one as a follow-up to the discussion thread
>that it is a follow-up to.  The patch in this message is the same
>as the one I previously sent; the proposed log message has been
>updated somewhat.
>
>  builtin/branch.c  |  9 +++--
>  t/t3200-branch.sh | 10 +-
>  2 files changed, 8 insertions(+), 11 deletions(-)

This mostly looks good to me.

I've already spent several screenfuls here on-list arguing for not
applying the equivalent of this patch, which I won't repeat here.

But just a brief comment: I think this doing this is good, and it's
something I agree with given that the consensus is going this way. I
thought "do what -m does" was less confusing, but since everyone (you,
Brad, even Sahil) seems to disagree I'm happy to be shown to be wrong &
to have this go in so thoroughly reviewed & thought about.

I do think however that we also need to update the docs, the relevant
origin/master...gitster/sd/branch-copy diff is currently:

+The `-c` and `-C` options have the exact same semantics as `-m` and
+`-M`, except instead of the branch being renamed it along with its
+config and reflog will be copied to a new name.

Suggestions welcome, but I think that should probably be changed to
something like the following as part of this patch:

The `-c` and `-C` options have the exact same semantics as `-m` and
`-M`, except instead of the branch being renamed it along with its
config and reflog will be copied to a new name. Furthermore, unlike
its `-m` and `-M` cousins the `-c` and `-C` options will never move
the HEAD, whereas the move option will do so if the source branch is
the currently checked-out branch.

> diff --git a/builtin/branch.c b/builtin/branch.c
> index 89f64f4123..e2e3692838 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -506,12 +506,9 @@ static void copy_or_rename_branch(const char *oldname, 
> const char *newname, int
>   oldref.buf + 11);
>   }
>
> - if (replace_each_worktree_head_symref(oldref.buf, newref.buf, 
> logmsg.buf)) {
> - if (copy)
> - die(_("Branch copied to %s, but HEAD is not updated!"), 
> newname);
> - else
> - die(_("Branch renamed to %s, but HEAD is not 
> updated!"), newname);
> - }
> + if (!copy &&
> + replace_each_worktree_head_symref(oldref.buf, newref.buf, 
> logmsg.buf))
> + die(_("Branch renamed to %s, but HEAD is not updated!"), 
> newname);
>
>   strbuf_release();
>
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 5d03ad16f6..be9b3784c6 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -422,7 +422,7 @@ test_expect_success 'git branch --copy is a synonym for 
> -c' '
>   test_cmp expect actual
>  '
>
> -test_expect_success 'git branch -c ee ef should copy and checkout branch ef' 
> '
> +test_expect_success 'git branch -c ee ef should copy to create branch ef' '
>   git checkout -b ee &&
>   git reflog exists refs/heads/ee &&
>   git config branch.ee.dummy Hello &&
> @@ -431,7 +431,7 @@ test_expect_success 'git branch -c ee ef should copy and 
> checkout branch ef' '
>   git reflog exists refs/heads/ef &&
>   test $(git config branch.ee.dummy) = Hello &&
>   test $(git config branch.ef.dummy) = Hello &&
> - test $(git rev-parse --abbrev-ref HEAD) = ef
> + test $(git rev-parse --abbrev-ref HEAD) = ee
>  '
>
>  test_expect_success 'git branch -c f/f g/g should work' '
> @@ -494,12 +494,12 @@ test_expect_success 'git branch -C c1 c2 should succeed 
> when c1 is checked out'
>   git checkout -b c1 &&
>   git branch c2 &&
>   git branch -C c1 c2 &&
> - test $(git rev-parse --abbrev-ref HEAD) = c2
> + test $(git 

Re: [PATCH 4/3] branch: fix "copy" to never touch HEAD

[Brandon Williams]

On 09/22, Junio C Hamano wrote:
> When creating a new branch B by copying the branch A that happens to
> be the current branch, it also updates HEAD to point at the new
> branch.  It probably was made this way because "git branch -c A B"
> piggybacked its implementation on "git branch -m A B",
> 
> This does not match the usual expectation.  If I were sitting on a
> blue chair, and somebody comes and repaints it to red, I would
> accept ending up sitting on a chair that is now red (I am also OK to
> stand, instead, as there no longer is my favourite blue chair).  But
> if somebody creates a new red chair, modelling it after the blue
> chair I am sitting on, I do not expect to be booted off of the blue
> chair and ending up on sitting on the new red one.
> 
> Let's fix this before it hits 'next'.  Those who want to create a
> new branch and switch to it can do "git checkout B" after doing a
> "git branch -c B", and if that operation is so useful and deserves a
> short-hand way to do so, perhaps extend "git checkout -b B" to copy
> configurations while creating the new branch B.
> 
> Signed-off-by: Junio C Hamano 
> ---
> 
>  * Let's send an updated one as a follow-up to the discussion thread
>that it is a follow-up to.  The patch in this message is the same
>as the one I previously sent; the proposed log message has been
>updated somewhat.
> 
>  builtin/branch.c  |  9 +++--
>  t/t3200-branch.sh | 10 +-
>  2 files changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 89f64f4123..e2e3692838 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -506,12 +506,9 @@ static void copy_or_rename_branch(const char *oldname, 
> const char *newname, int
>   oldref.buf + 11);
>   }
>  
> - if (replace_each_worktree_head_symref(oldref.buf, newref.buf, 
> logmsg.buf)) {
> - if (copy)
> - die(_("Branch copied to %s, but HEAD is not updated!"), 
> newname);
> - else
> - die(_("Branch renamed to %s, but HEAD is not 
> updated!"), newname);
> - }
> + if (!copy &&
> + replace_each_worktree_head_symref(oldref.buf, newref.buf, 
> logmsg.buf))
> + die(_("Branch renamed to %s, but HEAD is not updated!"), 
> newname);
>  
>   strbuf_release();
>  
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 5d03ad16f6..be9b3784c6 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -422,7 +422,7 @@ test_expect_success 'git branch --copy is a synonym for 
> -c' '
>   test_cmp expect actual
>  '
>  
> -test_expect_success 'git branch -c ee ef should copy and checkout branch ef' 
> '
> +test_expect_success 'git branch -c ee ef should copy to create branch ef' '

The new wording seems to be missing something.  Maybe it should read:
  
  'git branch -c ee ef should copy branch ee to create branch ef'

>   git checkout -b ee &&
>   git reflog exists refs/heads/ee &&
>   git config branch.ee.dummy Hello &&
> @@ -431,7 +431,7 @@ test_expect_success 'git branch -c ee ef should copy and 
> checkout branch ef' '
>   git reflog exists refs/heads/ef &&
>   test $(git config branch.ee.dummy) = Hello &&
>   test $(git config branch.ef.dummy) = Hello &&
> - test $(git rev-parse --abbrev-ref HEAD) = ef
> + test $(git rev-parse --abbrev-ref HEAD) = ee
>  '
>  
>  test_expect_success 'git branch -c f/f g/g should work' '
> @@ -494,12 +494,12 @@ test_expect_success 'git branch -C c1 c2 should succeed 
> when c1 is checked out'
>   git checkout -b c1 &&
>   git branch c2 &&
>   git branch -C c1 c2 &&
> - test $(git rev-parse --abbrev-ref HEAD) = c2
> + test $(git rev-parse --abbrev-ref HEAD) = c1
>  '
>  
> -test_expect_success 'git branch -C c1 c2 should add entries to 
> .git/logs/HEAD' '
> +test_expect_success 'git branch -C c1 c2 should never touch HEAD' '
>   msg="Branch: copied refs/heads/c1 to refs/heads/c2" &&
> - grep "$msg$" .git/logs/HEAD
> + ! grep "$msg$" .git/logs/HEAD
>  '
>  
>  test_expect_success 'git branch -C master should work when master is checked 
> out' '
> -- 
> 2.14.1-929-g25eae544e9
> 

The rest of the patch lgtm.  I agree that this is probably a better UI
than without this patch.  Especially since the vanilla behavior of git
branch is to create a new branch without moving you to that new branch.

-- 
Brandon Williams

[PATCH 4/3] branch: fix "copy" to never touch HEAD

[Junio C Hamano]

When creating a new branch B by copying the branch A that happens to
be the current branch, it also updates HEAD to point at the new
branch.  It probably was made this way because "git branch -c A B"
piggybacked its implementation on "git branch -m A B",

This does not match the usual expectation.  If I were sitting on a
blue chair, and somebody comes and repaints it to red, I would
accept ending up sitting on a chair that is now red (I am also OK to
stand, instead, as there no longer is my favourite blue chair).  But
if somebody creates a new red chair, modelling it after the blue
chair I am sitting on, I do not expect to be booted off of the blue
chair and ending up on sitting on the new red one.

Let's fix this before it hits 'next'.  Those who want to create a
new branch and switch to it can do "git checkout B" after doing a
"git branch -c B", and if that operation is so useful and deserves a
short-hand way to do so, perhaps extend "git checkout -b B" to copy
configurations while creating the new branch B.

Signed-off-by: Junio C Hamano 
---

 * Let's send an updated one as a follow-up to the discussion thread
   that it is a follow-up to.  The patch in this message is the same
   as the one I previously sent; the proposed log message has been
   updated somewhat.

 builtin/branch.c  |  9 +++--
 t/t3200-branch.sh | 10 +-
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 89f64f4123..e2e3692838 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -506,12 +506,9 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
oldref.buf + 11);
}
 
-   if (replace_each_worktree_head_symref(oldref.buf, newref.buf, 
logmsg.buf)) {
-   if (copy)
-   die(_("Branch copied to %s, but HEAD is not updated!"), 
newname);
-   else
-   die(_("Branch renamed to %s, but HEAD is not 
updated!"), newname);
-   }
+   if (!copy &&
+   replace_each_worktree_head_symref(oldref.buf, newref.buf, 
logmsg.buf))
+   die(_("Branch renamed to %s, but HEAD is not updated!"), 
newname);
 
strbuf_release();
 
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 5d03ad16f6..be9b3784c6 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -422,7 +422,7 @@ test_expect_success 'git branch --copy is a synonym for -c' 
'
test_cmp expect actual
 '
 
-test_expect_success 'git branch -c ee ef should copy and checkout branch ef' '
+test_expect_success 'git branch -c ee ef should copy to create branch ef' '
git checkout -b ee &&
git reflog exists refs/heads/ee &&
git config branch.ee.dummy Hello &&
@@ -431,7 +431,7 @@ test_expect_success 'git branch -c ee ef should copy and 
checkout branch ef' '
git reflog exists refs/heads/ef &&
test $(git config branch.ee.dummy) = Hello &&
test $(git config branch.ef.dummy) = Hello &&
-   test $(git rev-parse --abbrev-ref HEAD) = ef
+   test $(git rev-parse --abbrev-ref HEAD) = ee
 '
 
 test_expect_success 'git branch -c f/f g/g should work' '
@@ -494,12 +494,12 @@ test_expect_success 'git branch -C c1 c2 should succeed 
when c1 is checked out'
git checkout -b c1 &&
git branch c2 &&
git branch -C c1 c2 &&
-   test $(git rev-parse --abbrev-ref HEAD) = c2
+   test $(git rev-parse --abbrev-ref HEAD) = c1
 '
 
-test_expect_success 'git branch -C c1 c2 should add entries to .git/logs/HEAD' 
'
+test_expect_success 'git branch -C c1 c2 should never touch HEAD' '
msg="Branch: copied refs/heads/c1 to refs/heads/c2" &&
-   grep "$msg$" .git/logs/HEAD
+   ! grep "$msg$" .git/logs/HEAD
 '
 
 test_expect_success 'git branch -C master should work when master is checked 
out' '
-- 
2.14.1-929-g25eae544e9