Re: BUG when trying to delete symbolic refs

2012-10-18 Thread René Scharfe 
Am 16.10.2012 18:09, schrieb Junio C Hamano:
 Having said all that, I think your patch is going in the right
 direction.  If somebody had a symbolic ref in refs/heads/, the
 removal should remove it, not the pointee, which may not even
 exist.  Does branch -d sym work correctly with your patch when
 refs/heads/sym is pointing at something that does not exist?

No, it doesn't, neither with nor without the patch.  But we can make it
work, and also address a UI issue.  This series starts with two patches
that only move code around, then follows the patch you commented on, a
patch addressing dangling symrefs and finally a change to the way
deleted symrefs are reported by git branch.

  branch: factor out check_branch_commit()
  branch: factor out delete_branch_config()
  branch: delete symref branch, not its target
  branch: skip commit checks when deleting symref branches
  branch: show targets of deleted symrefs, not sha1s

 builtin/branch.c  | 75 ---
 t/t3200-branch.sh | 19 ++
 2 files changed, 68 insertions(+), 26 deletions(-)

-- 
1.7.12

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: BUG when trying to delete symbolic refs

2012-10-16 Thread René Scharfe 
Am 15.10.2012 10:50, schrieb Johan Herland:
 Basically, there is a master branch, and an alias symref to
 master. When we naively try to delete the symref with git branch -d
 alias, it ends up:
 
   - NOT deleting the alias symref
   - DELETING the master loose ref
   - NOT deleting the master packed ref
 
 So, from the user perspective, git branch -d alias ends up resetting
 master (and alias) back to the last time we happened to run git
 gc. Needless to say, this is not quite what we had in mind...
 
 AFAICS, there may be three possible acceptable outcomes when we run
 git branch -d alias in the above scenario:
 
   A. The symbolic ref is deleted. This is obviously what we expected...

Below is a patch to do that.

   B. The command fails because alias is a symref. This would be
 understandable if we don't want to teach branch -d about symrefs.
 But then, the error message should ideally explain which command we
 should use to remove the symref.

Renaming of symrefs with branch -m is disallowed because it's more
complicated than it looks at first; this was discussed here:
http://thread.gmane.org/gmane.comp.version-control.git/98825/focus=99206

I can't imagine why deletion should be prohibited as well, though.

   C. The master ref (BOTH loose and packed versions of it) is
 deleted. This would be less helpful for us, but Git would at least be
 internally consistent (in that the symref would be resolved, and the
 command would become git branch -d master).

Are there use cases for this behaviour?

While I don't use symrefs, I'd somehow expect them to behave like
symbolic links on Unix do, where rm removes a link, not its target.

But I wonder why most delete_ref() calls in the code actually don't use
the flag REF_NODEREF, thus deleting symref targets instead of the
symrefs themselves.  I may be missing something important here.

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

diff --git a/builtin/branch.c b/builtin/branch.c
index ffd2684..31af114 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -221,7 +221,7 @@ static int delete_branches(int argc, const char **argv, int 
force, int kinds,
continue;
}
 
-   if (delete_ref(name, sha1, 0)) {
+   if (delete_ref(name, sha1, REF_NODEREF)) {
error(remote_branch
  ? _(Error deleting remote branch '%s')
  : _(Error deleting branch '%s'),
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 79c8d01..4b73406 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -262,6 +262,16 @@ test_expect_success 'config information was renamed, too' \
test $(git config branch.s.dummy) = Hello 
 test_must_fail git config branch.s/s/dummy
 
+test_expect_success 'deleting a symref' '
+   git branch target 
+   git symbolic-ref refs/heads/symlink refs/heads/target 
+
+   git branch -d symlink 
+
+   test_path_is_file .git/refs/heads/target 
+   test_path_is_missing .git/refs/heads/symlink
+'
+
 test_expect_success 'renaming a symref is not allowed' \
 '
git symbolic-ref refs/heads/master2 refs/heads/master 
-- 
1.7.12

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: BUG when trying to delete symbolic refs

2012-10-16 Thread Junio C Hamano 
René Scharfe rene.scha...@lsrfire.ath.cx writes:

 Am 15.10.2012 10:50, schrieb Johan Herland:
 Basically, there is a master branch, and an alias symref to
 master. When we naively try to delete the symref with git branch -d
 alias, it ends up:
 
   - NOT deleting the alias symref
   - DELETING the master loose ref
   - NOT deleting the master packed ref
 
 So, from the user perspective, git branch -d alias ends up resetting
 master (and alias) back to the last time we happened to run git
 gc. Needless to say, this is not quite what we had in mind...
 
 AFAICS, there may be three possible acceptable outcomes when we run
 git branch -d alias in the above scenario:
 
   A. The symbolic ref is deleted. This is obviously what we expected...

 Below is a patch to do that.

   B. The command fails because alias is a symref. This would be
 understandable if we don't want to teach branch -d about symrefs.
 But then, the error message should ideally explain which command we
 should use to remove the symref.

 Renaming of symrefs with branch -m is disallowed because it's more
 complicated than it looks at first; this was discussed here:
 http://thread.gmane.org/gmane.comp.version-control.git/98825/focus=99206

Thanks for a reminder.

 I can't imagine why deletion should be prohibited as well, though.

I am not sure if it is a good idea to let update-ref -d work on a
symref, with or without --no-deref.  There are cases where you want
to remove the pointer (symbolic-ref -d is there for that), and
there are cases where you want to remove the underlying ref (but of
course you can update-ref -d the underlying ref yourself).  If
update-ref -d refused to work on a symref, we do not have to worry
about the confusion which one is removed---the pointer, or the
pointee?

 But I wonder why most delete_ref() calls in the code actually don't use
 the flag REF_NODEREF, thus deleting symref targets instead of the
 symrefs themselves.  I may be missing something important here.

I suspect that is primarily because using a symref to represent
anything other than $GIT_DIR/HEAD and $GIT_DIR/refs/remotes/*/HEAD
is outside the normally supported use case and in the may happen to
work territory.

Having said all that, I think your patch is going in the right
direction.  If somebody had a symbolic ref in refs/heads/, the
removal should remove it, not the pointee, which may not even
exist.  Does branch -d sym work correctly with your patch when
refs/heads/sym is pointing at something that does not exist?

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

 diff --git a/builtin/branch.c b/builtin/branch.c
 index ffd2684..31af114 100644
 --- a/builtin/branch.c
 +++ b/builtin/branch.c
 @@ -221,7 +221,7 @@ static int delete_branches(int argc, const char **argv, 
 int force, int kinds,
   continue;
   }
  
 - if (delete_ref(name, sha1, 0)) {
 + if (delete_ref(name, sha1, REF_NODEREF)) {
   error(remote_branch
 ? _(Error deleting remote branch '%s')
 : _(Error deleting branch '%s'),
 diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
 index 79c8d01..4b73406 100755
 --- a/t/t3200-branch.sh
 +++ b/t/t3200-branch.sh
 @@ -262,6 +262,16 @@ test_expect_success 'config information was renamed, 
 too' \
   test $(git config branch.s.dummy) = Hello 
test_must_fail git config branch.s/s/dummy
  
 +test_expect_success 'deleting a symref' '
 + git branch target 
 + git symbolic-ref refs/heads/symlink refs/heads/target 
 +
 + git branch -d symlink 
 +
 + test_path_is_file .git/refs/heads/target 
 + test_path_is_missing .git/refs/heads/symlink
 +'
 +
  test_expect_success 'renaming a symref is not allowed' \
  '
   git symbolic-ref refs/heads/master2 refs/heads/master 
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html