{"thread":{"id":"13659","subject":"[PATCH 1/3] Add some tests for git update-ref -d","startedAt":"2008-05-25T16:14:24Z","lastAt":"2008-05-26T14:09:31Z","messageCount":8,"participants":["Karl Hasselström","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"77709","messageId":"20080525161125.25087.18083.stgit@yoghurt","threadId":"13659","inReplyTo":null,"subject":"[PATCH 0/3] Make old sha1 optional with git update-ref -d","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-05-25T16:14:24Z","receivedAt":"2008-05-25T16:14:24Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"This series makes the old ref value optional for update-ref -d (it's\nalready optional for update-ref without -d). But first it adds some\ntest and refactors a bit.\n\n---\n\nKarl Hasselström (3):\n      Make old sha1 optional with git update-ref -d\n      Clean up builtin-update-ref's option parsing\n      Add some tests for git update-ref -d\n\n\n Documentation/git-update-ref.txt |    2 +-\n builtin-update-ref.c             |   32 ++++++++++++++++++--------------\n t/t1400-update-ref.sh            |   24 ++++++++++++++++++++++++\n 3 files changed, 43 insertions(+), 15 deletions(-)\n\n-- \nKarl Hasselström, kha@treskal.com\n      www.treskal.com/kalle\n"},{"id":"77708","messageId":"20080525161429.25087.10930.stgit@yoghurt","threadId":"13659","inReplyTo":"20080525161125.25087.18083.stgit@yoghurt","subject":"[PATCH 1/3] Add some tests for git update-ref -d","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-05-25T16:14:29Z","receivedAt":"2008-05-25T16:14:29Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"Signed-off-by: Karl Hasselström <kha@treskal.com>\n\n---\n\n t/t1400-update-ref.sh |   16 ++++++++++++++++\n 1 files changed, 16 insertions(+), 0 deletions(-)\n\n\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 78cd412..b88e767 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -32,6 +32,14 @@ test_expect_success \\\n \t\"create $m\" \\\n \t\"git update-ref $m $B $A &&\n \t test $B\"' = $(cat .git/'\"$m\"')'\n+test_expect_success \"fail to delete $m with stale ref\" \"\n+\ttest_must_fail git update-ref -d $m $A &&\n+\ttest $B = \\$(cat .git/$m)\n+\"\n+test_expect_success \"delete $m\" \"\n+\tgit update-ref -d $m $B &&\n+\t! test -f .git/$m\n+\"\n rm -f .git/$m\n \n test_expect_success \\\n@@ -49,6 +57,14 @@ test_expect_success \\\n \t\"create $m (by HEAD)\" \\\n \t\"git update-ref HEAD $B $A &&\n \t test $B\"' = $(cat .git/'\"$m\"')'\n+test_expect_success \"fail to delete $m (by HEAD) with stale ref\" \"\n+\ttest_must_fail git update-ref -d HEAD $A &&\n+\ttest $B = \\$(cat .git/$m)\n+\"\n+test_expect_success \"delete $m (by HEAD)\" \"\n+\tgit update-ref -d HEAD $B &&\n+\t! test -f .git/$m\n+\"\n rm -f .git/$m\n \n test_expect_success '(not) create HEAD with old sha1' \"\n"},{"id":"77710","messageId":"20080525161435.25087.82683.stgit@yoghurt","threadId":"13659","inReplyTo":"20080525161125.25087.18083.stgit@yoghurt","subject":"[PATCH 2/3] Clean up builtin-update-ref's option parsing","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-05-25T16:14:35Z","receivedAt":"2008-05-25T16:14:35Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"builtin-update-ref's option parsing was somewhat tricky to follow,\nespecially if the -d option was given. This patch cleans it upp a bit\n(including fixing an out-of-bounds argv access), at the expense of\nmaking it a bit longer.\n\nSigned-off-by: Karl Hasselström <kha@treskal.com>\n\n---\n\n builtin-update-ref.c |   30 +++++++++++++++++-------------\n 1 files changed, 17 insertions(+), 13 deletions(-)\n\n\ndiff --git a/builtin-update-ref.c b/builtin-update-ref.c\nindex e90737c..0d3eb8e 100644\n--- a/builtin-update-ref.c\n+++ b/builtin-update-ref.c\n@@ -27,25 +27,29 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n \tif (msg && !*msg)\n \t\tdie(\"Refusing to perform update with empty message.\");\n \n-\tif (argc < 2 || argc > 3)\n-\t\tusage_with_options(git_update_ref_usage, options);\n-\trefname = argv[0];\n-\tvalue   = argv[1];\n-\toldval  = argv[2];\n-\n-\tif (get_sha1(value, sha1))\n-\t\tdie(\"%s: not a valid SHA1\", value);\n-\n \tif (delete) {\n-\t\tif (oldval)\n+\t\tif (argc != 2)\n \t\t\tusage_with_options(git_update_ref_usage, options);\n-\t\treturn delete_ref(refname, sha1);\n+\t\trefname = argv[0];\n+\t\tvalue = NULL;\n+\t\toldval = argv[1];\n+\t} else {\n+\t\tif (argc < 2 || argc > 3)\n+\t\t\tusage_with_options(git_update_ref_usage, options);\n+\t\trefname = argv[0];\n+\t\tvalue = argv[1];\n+\t\toldval = argc > 2 ? argv[2] : NULL;\n \t}\n \n+\tif (value && *value && get_sha1(value, sha1))\n+\t\tdie(\"%s: not a valid SHA1\", value);\n \thashclr(oldsha1);\n \tif (oldval && *oldval && get_sha1(oldval, oldsha1))\n \t\tdie(\"%s: not a valid old SHA1\", oldval);\n \n-\treturn update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,\n-\t\t\t  no_deref ? REF_NODEREF : 0, DIE_ON_ERR);\n+\tif (delete)\n+\t\treturn delete_ref(refname, oldsha1);\n+\telse\n+\t\treturn update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,\n+\t\t\t\t  no_deref ? REF_NODEREF : 0, DIE_ON_ERR);\n }\n"},{"id":"77711","messageId":"20080525161440.25087.83011.stgit@yoghurt","threadId":"13659","inReplyTo":"20080525161125.25087.18083.stgit@yoghurt","subject":"[PATCH 3/3] Make old sha1 optional with git update-ref -d","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-05-25T16:14:41Z","receivedAt":"2008-05-25T16:14:41Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"Giving the old sha1 is already optional when changing a ref, and it's\nquite handy when running update-ref manually. So make it optional for\ndeleting a ref too.\n\nSigned-off-by: Karl Hasselström <kha@treskal.com>\n\n---\n\n Documentation/git-update-ref.txt |    2 +-\n builtin-update-ref.c             |    8 ++++----\n t/t1400-update-ref.sh            |    8 ++++++++\n 3 files changed, 13 insertions(+), 5 deletions(-)\n\n\ndiff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt\nindex 4dc4759..80b94c3 100644\n--- a/Documentation/git-update-ref.txt\n+++ b/Documentation/git-update-ref.txt\n@@ -7,7 +7,7 @@ git-update-ref - Update the object name stored in a ref safely\n \n SYNOPSIS\n --------\n-'git-update-ref' [-m <reason>] (-d <ref> <oldvalue> | [--no-deref] <ref> <newvalue> [<oldvalue>])\n+'git-update-ref' [-m <reason>] (-d <ref> [<oldvalue>] | [--no-deref] <ref> <newvalue> [<oldvalue>])\n \n DESCRIPTION\n -----------\ndiff --git a/builtin-update-ref.c b/builtin-update-ref.c\nindex 0d3eb8e..054e64d 100644\n--- a/builtin-update-ref.c\n+++ b/builtin-update-ref.c\n@@ -4,7 +4,7 @@\n #include \"parse-options.h\"\n \n static const char * const git_update_ref_usage[] = {\n-\t\"git-update-ref [options] -d <refname> <oldval>\",\n+\t\"git-update-ref [options] -d <refname> [<oldval>]\",\n \t\"git-update-ref [options]    <refname> <newval> [<oldval>]\",\n \tNULL\n };\n@@ -28,11 +28,11 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n \t\tdie(\"Refusing to perform update with empty message.\");\n \n \tif (delete) {\n-\t\tif (argc != 2)\n+\t\tif (argc < 1 || argc > 2)\n \t\t\tusage_with_options(git_update_ref_usage, options);\n \t\trefname = argv[0];\n \t\tvalue = NULL;\n-\t\toldval = argv[1];\n+\t\toldval = argc > 1 ? argv[1] : NULL;\n \t} else {\n \t\tif (argc < 2 || argc > 3)\n \t\t\tusage_with_options(git_update_ref_usage, options);\n@@ -48,7 +48,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n \t\tdie(\"%s: not a valid old SHA1\", oldval);\n \n \tif (delete)\n-\t\treturn delete_ref(refname, oldsha1);\n+\t\treturn delete_ref(refname, oldval ? oldsha1 : NULL);\n \telse\n \t\treturn update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,\n \t\t\t\t  no_deref ? REF_NODEREF : 0, DIE_ON_ERR);\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex b88e767..da82645 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -42,6 +42,14 @@ test_expect_success \"delete $m\" \"\n \"\n rm -f .git/$m\n \n+test_expect_success \"delete $m without oldvalue verification\" \"\n+\tgit update-ref $m $A &&\n+\ttest $A = \\$(cat .git/$m) &&\n+\tgit update-ref -d $m &&\n+\t! test -f .git/$m\n+\"\n+rm -f .git/$m\n+\n test_expect_success \\\n \t\"fail to create $n\" \\\n \t\"touch .git/$n_dir\n"},{"id":"77715","messageId":"7vk5hii5wn.fsf@gitster.siamese.dyndns.org","threadId":"13659","inReplyTo":"20080525161435.25087.82683.stgit@yoghurt","subject":"Re: [PATCH 2/3] Clean up builtin-update-ref's option parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-25T18:40:24Z","receivedAt":"2008-05-25T18:40:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karl Hasselström <kha@treskal.com> writes:\n\n> builtin-update-ref's option parsing was somewhat tricky to follow,\n> especially if the -d option was given. This patch cleans it upp a bit\n> (including fixing an out-of-bounds argv access), at the expense of\n> making it a bit longer.\n>\n> Signed-off-by: Karl Hasselström <kha@treskal.com>\n\nLonger is fine but I am afraid that this patch is not much easier to\nfollow than the original, does not fix anything, and introduces a new bug.\n\n> diff --git a/builtin-update-ref.c b/builtin-update-ref.c\n> index e90737c..0d3eb8e 100644\n> --- a/builtin-update-ref.c\n> +++ b/builtin-update-ref.c\n> @@ -27,25 +27,29 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n>  \tif (msg && !*msg)\n>  \t\tdie(\"Refusing to perform update with empty message.\");\n>  \n> -\tif (argc < 2 || argc > 3)\n> -\t\tusage_with_options(git_update_ref_usage, options);\n> -\trefname = argv[0];\n> -\tvalue   = argv[1];\n> -\toldval  = argv[2];\n> -\n> -\tif (get_sha1(value, sha1))\n> -\t\tdie(\"%s: not a valid SHA1\", value);\n> -\n>  \tif (delete) {\n> -\t\tif (oldval)\n> +\t\tif (argc != 2)\n>  \t\t\tusage_with_options(git_update_ref_usage, options);\n> -\t\treturn delete_ref(refname, sha1);\n> +\t\trefname = argv[0];\n> +\t\tvalue = NULL;\n> +\t\toldval = argv[1];\n> +\t} else {\n> +\t\tif (argc < 2 || argc > 3)\n> +\t\t\tusage_with_options(git_update_ref_usage, options);\n> +\t\trefname = argv[0];\n> +\t\tvalue = argv[1];\n> +\t\toldval = argc > 2 ? argv[2] : NULL;\n\nWhen (argc == 3), argv[2] has the old value string given on the command\nline.  When (argc == 2), argv[2] has the terminating NULL pointer.  So\neither case you can safely use argv[2].  You do not allow other cases\nupfront, so I do not understand why you need this conditional expression?\n\nIOW, I do not see \"an out-of-bounds argv access\" in the original, and you\nare making this assignment harder to follow.\n\n>  \t}\n>  \n> +\tif (value && *value && get_sha1(value, sha1))\n> +\t\tdie(\"%s: not a valid SHA1\", value);\n\nDropping *value in the sequence may fix it but I think this is wrong.\n\nWe used to barf if you said \"git update-ref refs/heads/master '' master\"\nbecause it would be nonsense to give an empty string as the new value of\nthe ref, didn't we?  Doesn't this change break it?  Does your set of\nadditional tests in [1/3] catch it?\n\n>  \thashclr(oldsha1);\n>  \tif (oldval && *oldval && get_sha1(oldval, oldsha1))\n>  \t\tdie(\"%s: not a valid old SHA1\", oldval);\n>  \n> -\treturn update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,\n> -\t\t\t  no_deref ? REF_NODEREF : 0, DIE_ON_ERR);\n> +\tif (delete)\n> +\t\treturn delete_ref(refname, oldsha1);\n> +\telse\n> +\t\treturn update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,\n> +\t\t\t\t  no_deref ? REF_NODEREF : 0, DIE_ON_ERR);\n>  }\n"},{"id":"77718","messageId":"20080525193213.GB5674@diana.vm.bytemark.co.uk","threadId":"13659","inReplyTo":"7vk5hii5wn.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/3] Clean up builtin-update-ref's option parsing","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-05-25T19:32:13Z","receivedAt":"2008-05-25T19:32:13Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-05-25 11:40:24 -0700, Junio C Hamano wrote:\n\n> >  \tif (delete) {\n> > -\t\tif (oldval)\n> > +\t\tif (argc != 2)\n> >  \t\t\tusage_with_options(git_update_ref_usage, options);\n> > -\t\treturn delete_ref(refname, sha1);\n> > +\t\trefname = argv[0];\n> > +\t\tvalue = NULL;\n> > +\t\toldval = argv[1];\n> > +\t} else {\n> > +\t\tif (argc < 2 || argc > 3)\n> > +\t\t\tusage_with_options(git_update_ref_usage, options);\n> > +\t\trefname = argv[0];\n> > +\t\tvalue = argv[1];\n> > +\t\toldval = argc > 2 ? argv[2] : NULL;\n>\n> When (argc == 3), argv[2] has the old value string given on the\n> command line. When (argc == 2), argv[2] has the terminating NULL\n> pointer. So either case you can safely use argv[2]. You do not allow\n> other cases upfront, so I do not understand why you need this\n> conditional expression?\n>\n> IOW, I do not see \"an out-of-bounds argv access\" in the original,\n> and you are making this assignment harder to follow.\n\nMmmph. The problem here was that I didn't know that argv is\nNULL-terminated. I just assumed it was bad to access anything beyond\nthe given array length as is usually the case in C. But I really\nshould have taken a hint from the existing code. Sorry for wasting\nyour time with this.\n\n> >  \t}\n> >  \n> > +\tif (value && *value && get_sha1(value, sha1))\n> > +\t\tdie(\"%s: not a valid SHA1\", value);\n> \n> Dropping *value in the sequence may fix it but I think this is wrong.\n> \n> We used to barf if you said \"git update-ref refs/heads/master '' master\"\n> because it would be nonsense to give an empty string as the new value of\n> the ref, didn't we?  Doesn't this change break it?  Does your set of\n> additional tests in [1/3] catch it?\n\nAaah, I see what you mean. Good point. The empty string should\nobviously not be allowed as the new ref. In the original code it _is_\nallowed as the _old_ value, and interpreted as 0{40}.\n\nI'll update the series. With, among other things, a comment that\nexplains this.\n\n                                 -+-\n\nMany thanks for the review. I'll be back.\n\n-- \nKarl Hasselström, kha@treskal.com\n      www.treskal.com/kalle\n"},{"id":"77758","messageId":"alpine.DEB.1.00.0805261149310.30431@racer","threadId":"13659","inReplyTo":"20080525161440.25087.83011.stgit@yoghurt","subject":"Re: [PATCH 3/3] Make old sha1 optional with git update-ref -d","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-05-26T10:51:12Z","receivedAt":"2008-05-26T10:51:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 25 May 2008, Karl Hasselström wrote:\n\n> Giving the old sha1 is already optional when changing a ref, and it's\n> quite handy when running update-ref manually. So make it optional for\n> deleting a ref too.\n\nIsn't this a bit dangerous, especially given that there is potentially \n_no_ reflog as a safeguard?\n\nSo no, it is not the same as when changing a ref.\n\nI am mildly negative on this (having lost some remote branches, because \nthey were deleted _together with the reflogs_ by \"git push bla :x\"),\nDscho"},{"id":"77777","messageId":"20080526140931.GA18440@diana.vm.bytemark.co.uk","threadId":"13659","inReplyTo":"alpine.DEB.1.00.0805261149310.30431@racer","subject":"Re: [PATCH 3/3] Make old sha1 optional with git update-ref -d","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-05-26T14:09:31Z","receivedAt":"2008-05-26T14:09:31Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-05-26 11:51:12 +0100, Johannes Schindelin wrote:\n\n> On Sun, 25 May 2008, Karl Hasselström wrote:\n>\n> > Giving the old sha1 is already optional when changing a ref, and\n> > it's quite handy when running update-ref manually. So make it\n> > optional for deleting a ref too.\n>\n> Isn't this a bit dangerous, especially given that there is\n> potentially _no_ reflog as a safeguard?\n>\n> So no, it is not the same as when changing a ref.\n>\n> I am mildly negative on this (having lost some remote branches,\n> because they were deleted _together with the reflogs_ by \"git push\n> bla :x\"),\n\nDeleting the wrong thing is never a fun experience. The thing is, this\npatch doesn't really make it any easier to delete the wrong thing.\nBefore, you'd\n\n  git update-ref -d <ref> <some-other-way-to-point-to-the-same-commit>\n\nif you wanted to play it safe, and\n\n  git update-ref -d <ref> <ref>\n\nif you thought you knew what you were doing. All this patch does is\nmake the second usage mode easier to type.\n\n(One might argue that a user who doesn't know what she's doing will\nhave an easier time shooting herself in the foot when the oldvalue is\noptional, but I was under the impression that ease of use without\nconsulting the man page shouldn't be a factor when designing the\nplumbing UI.)\n\nI do agree with you that branch deletion is one of the more\n\"dangerous\" operations in git. But I don't think making update-ref\nhard to use is the answer there; what we'd really need is for old\nreflogs to stay around for a while before being deleted.\n\n-- \nKarl Hasselström, kha@treskal.com\n      www.treskal.com/kalle\n"}]}