# [PATCH 1/3] Add some tests for git update-ref -d 8 messages from 2008-05-25 to 2008-05-26. Participants: Karl Hasselström, Junio C Hamano, Johannes Schindelin. Thread: https://gitlist.dev/t/13659 ## Karl Hasselström, 2008-05-25 16:14 Subject: [PATCH 0/3] Make old sha1 optional with git update-ref -d Message-ID: <20080525161125.25087.18083.stgit@yoghurt> URL: https://gitlist.dev/e/20080525161125.25087.18083.stgit%40yoghurt ``` This series makes the old ref value optional for update-ref -d (it's already optional for update-ref without -d). But first it adds some test and refactors a bit. --- Karl Hasselström (3): Make old sha1 optional with git update-ref -d Clean up builtin-update-ref's option parsing Add some tests for git update-ref -d Documentation/git-update-ref.txt | 2 +- builtin-update-ref.c | 32 ++++++++++++++++++-------------- t/t1400-update-ref.sh | 24 ++++++++++++++++++++++++ 3 files changed, 43 insertions(+), 15 deletions(-) -- Karl Hasselström, kha@treskal.com www.treskal.com/kalle ``` ## Karl Hasselström, 2008-05-25 16:14 Subject: [PATCH 1/3] Add some tests for git update-ref -d Message-ID: <20080525161429.25087.10930.stgit@yoghurt> URL: https://gitlist.dev/e/20080525161429.25087.10930.stgit%40yoghurt In-Reply-To: <20080525161125.25087.18083.stgit@yoghurt> ``` Signed-off-by: Karl Hasselström --- t/t1400-update-ref.sh | 16 ++++++++++++++++ 1 files changed, 16 insertions(+), 0 deletions(-) diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 78cd412..b88e767 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -32,6 +32,14 @@ test_expect_success \ "create $m" \ "git update-ref $m $B $A && test $B"' = $(cat .git/'"$m"')' +test_expect_success "fail to delete $m with stale ref" " + test_must_fail git update-ref -d $m $A && + test $B = \$(cat .git/$m) +" +test_expect_success "delete $m" " + git update-ref -d $m $B && + ! test -f .git/$m +" rm -f .git/$m test_expect_success \ @@ -49,6 +57,14 @@ test_expect_success \ "create $m (by HEAD)" \ "git update-ref HEAD $B $A && test $B"' = $(cat .git/'"$m"')' +test_expect_success "fail to delete $m (by HEAD) with stale ref" " + test_must_fail git update-ref -d HEAD $A && + test $B = \$(cat .git/$m) +" +test_expect_success "delete $m (by HEAD)" " + git update-ref -d HEAD $B && + ! test -f .git/$m +" rm -f .git/$m test_expect_success '(not) create HEAD with old sha1' " ``` ## Karl Hasselström, 2008-05-25 16:14 Subject: [PATCH 2/3] Clean up builtin-update-ref's option parsing Message-ID: <20080525161435.25087.82683.stgit@yoghurt> URL: https://gitlist.dev/e/20080525161435.25087.82683.stgit%40yoghurt In-Reply-To: <20080525161125.25087.18083.stgit@yoghurt> ``` builtin-update-ref's option parsing was somewhat tricky to follow, especially if the -d option was given. This patch cleans it upp a bit (including fixing an out-of-bounds argv access), at the expense of making it a bit longer. Signed-off-by: Karl Hasselström --- builtin-update-ref.c | 30 +++++++++++++++++------------- 1 files changed, 17 insertions(+), 13 deletions(-) diff --git a/builtin-update-ref.c b/builtin-update-ref.c index e90737c..0d3eb8e 100644 --- a/builtin-update-ref.c +++ b/builtin-update-ref.c @@ -27,25 +27,29 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix) if (msg && !*msg) die("Refusing to perform update with empty message."); - if (argc < 2 || argc > 3) - usage_with_options(git_update_ref_usage, options); - refname = argv[0]; - value = argv[1]; - oldval = argv[2]; - - if (get_sha1(value, sha1)) - die("%s: not a valid SHA1", value); - if (delete) { - if (oldval) + if (argc != 2) usage_with_options(git_update_ref_usage, options); - return delete_ref(refname, sha1); + refname = argv[0]; + value = NULL; + oldval = argv[1]; + } else { + if (argc < 2 || argc > 3) + usage_with_options(git_update_ref_usage, options); + refname = argv[0]; + value = argv[1]; + oldval = argc > 2 ? argv[2] : NULL; } + if (value && *value && get_sha1(value, sha1)) + die("%s: not a valid SHA1", value); hashclr(oldsha1); if (oldval && *oldval && get_sha1(oldval, oldsha1)) die("%s: not a valid old SHA1", oldval); - return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL, - no_deref ? REF_NODEREF : 0, DIE_ON_ERR); + if (delete) + return delete_ref(refname, oldsha1); + else + return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL, + no_deref ? REF_NODEREF : 0, DIE_ON_ERR); } ``` ## Karl Hasselström, 2008-05-25 16:14 Subject: [PATCH 3/3] Make old sha1 optional with git update-ref -d Message-ID: <20080525161440.25087.83011.stgit@yoghurt> URL: https://gitlist.dev/e/20080525161440.25087.83011.stgit%40yoghurt In-Reply-To: <20080525161125.25087.18083.stgit@yoghurt> ``` Giving the old sha1 is already optional when changing a ref, and it's quite handy when running update-ref manually. So make it optional for deleting a ref too. Signed-off-by: Karl Hasselström --- Documentation/git-update-ref.txt | 2 +- builtin-update-ref.c | 8 ++++---- t/t1400-update-ref.sh | 8 ++++++++ 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt index 4dc4759..80b94c3 100644 --- a/Documentation/git-update-ref.txt +++ b/Documentation/git-update-ref.txt @@ -7,7 +7,7 @@ git-update-ref - Update the object name stored in a ref safely SYNOPSIS -------- -'git-update-ref' [-m ] (-d | [--no-deref] []) +'git-update-ref' [-m ] (-d [] | [--no-deref] []) DESCRIPTION ----------- diff --git a/builtin-update-ref.c b/builtin-update-ref.c index 0d3eb8e..054e64d 100644 --- a/builtin-update-ref.c +++ b/builtin-update-ref.c @@ -4,7 +4,7 @@ #include "parse-options.h" static const char * const git_update_ref_usage[] = { - "git-update-ref [options] -d ", + "git-update-ref [options] -d []", "git-update-ref [options] []", NULL }; @@ -28,11 +28,11 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix) die("Refusing to perform update with empty message."); if (delete) { - if (argc != 2) + if (argc < 1 || argc > 2) usage_with_options(git_update_ref_usage, options); refname = argv[0]; value = NULL; - oldval = argv[1]; + oldval = argc > 1 ? argv[1] : NULL; } else { if (argc < 2 || argc > 3) usage_with_options(git_update_ref_usage, options); @@ -48,7 +48,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix) die("%s: not a valid old SHA1", oldval); if (delete) - return delete_ref(refname, oldsha1); + return delete_ref(refname, oldval ? oldsha1 : NULL); else return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL, no_deref ? REF_NODEREF : 0, DIE_ON_ERR); diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index b88e767..da82645 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -42,6 +42,14 @@ test_expect_success "delete $m" " " rm -f .git/$m +test_expect_success "delete $m without oldvalue verification" " + git update-ref $m $A && + test $A = \$(cat .git/$m) && + git update-ref -d $m && + ! test -f .git/$m +" +rm -f .git/$m + test_expect_success \ "fail to create $n" \ "touch .git/$n_dir ``` ## Junio C Hamano, 2008-05-25 18:40 Subject: Re: [PATCH 2/3] Clean up builtin-update-ref's option parsing Message-ID: <7vk5hii5wn.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vk5hii5wn.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20080525161435.25087.82683.stgit@yoghurt> ``` Karl Hasselström writes: > builtin-update-ref's option parsing was somewhat tricky to follow, > especially if the -d option was given. This patch cleans it upp a bit > (including fixing an out-of-bounds argv access), at the expense of > making it a bit longer. > > Signed-off-by: Karl Hasselström Longer is fine but I am afraid that this patch is not much easier to follow than the original, does not fix anything, and introduces a new bug. > diff --git a/builtin-update-ref.c b/builtin-update-ref.c > index e90737c..0d3eb8e 100644 > --- a/builtin-update-ref.c > +++ b/builtin-update-ref.c > @@ -27,25 +27,29 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix) > if (msg && !*msg) > die("Refusing to perform update with empty message."); > > - if (argc < 2 || argc > 3) > - usage_with_options(git_update_ref_usage, options); > - refname = argv[0]; > - value = argv[1]; > - oldval = argv[2]; > - > - if (get_sha1(value, sha1)) > - die("%s: not a valid SHA1", value); > - > if (delete) { > - if (oldval) > + if (argc != 2) > usage_with_options(git_update_ref_usage, options); > - return delete_ref(refname, sha1); > + refname = argv[0]; > + value = NULL; > + oldval = argv[1]; > + } else { > + if (argc < 2 || argc > 3) > + usage_with_options(git_update_ref_usage, options); > + refname = argv[0]; > + value = argv[1]; > + oldval = argc > 2 ? argv[2] : NULL; When (argc == 3), argv[2] has the old value string given on the command line. When (argc == 2), argv[2] has the terminating NULL pointer. So either case you can safely use argv[2]. You do not allow other cases upfront, so I do not understand why you need this conditional expression? IOW, I do not see "an out-of-bounds argv access" in the original, and you are making this assignment harder to follow. > } > > + if (value && *value && get_sha1(value, sha1)) > + die("%s: not a valid SHA1", value); Dropping *value in the sequence may fix it but I think this is wrong. We used to barf if you said "git update-ref refs/heads/master '' master" because it would be nonsense to give an empty string as the new value of the ref, didn't we? Doesn't this change break it? Does your set of additional tests in [1/3] catch it? > hashclr(oldsha1); > if (oldval && *oldval && get_sha1(oldval, oldsha1)) > die("%s: not a valid old SHA1", oldval); > > - return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL, > - no_deref ? REF_NODEREF : 0, DIE_ON_ERR); > + if (delete) > + return delete_ref(refname, oldsha1); > + else > + return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL, > + no_deref ? REF_NODEREF : 0, DIE_ON_ERR); > } ``` ## Karl Hasselström, 2008-05-25 19:32 Subject: Re: [PATCH 2/3] Clean up builtin-update-ref's option parsing Message-ID: <20080525193213.GB5674@diana.vm.bytemark.co.uk> URL: https://gitlist.dev/e/20080525193213.GB5674%40diana.vm.bytemark.co.uk In-Reply-To: <7vk5hii5wn.fsf@gitster.siamese.dyndns.org> ``` On 2008-05-25 11:40:24 -0700, Junio C Hamano wrote: > > if (delete) { > > - if (oldval) > > + if (argc != 2) > > usage_with_options(git_update_ref_usage, options); > > - return delete_ref(refname, sha1); > > + refname = argv[0]; > > + value = NULL; > > + oldval = argv[1]; > > + } else { > > + if (argc < 2 || argc > 3) > > + usage_with_options(git_update_ref_usage, options); > > + refname = argv[0]; > > + value = argv[1]; > > + oldval = argc > 2 ? argv[2] : NULL; > > When (argc == 3), argv[2] has the old value string given on the > command line. When (argc == 2), argv[2] has the terminating NULL > pointer. So either case you can safely use argv[2]. You do not allow > other cases upfront, so I do not understand why you need this > conditional expression? > > IOW, I do not see "an out-of-bounds argv access" in the original, > and you are making this assignment harder to follow. Mmmph. The problem here was that I didn't know that argv is NULL-terminated. I just assumed it was bad to access anything beyond the given array length as is usually the case in C. But I really should have taken a hint from the existing code. Sorry for wasting your time with this. > > } > > > > + if (value && *value && get_sha1(value, sha1)) > > + die("%s: not a valid SHA1", value); > > Dropping *value in the sequence may fix it but I think this is wrong. > > We used to barf if you said "git update-ref refs/heads/master '' master" > because it would be nonsense to give an empty string as the new value of > the ref, didn't we? Doesn't this change break it? Does your set of > additional tests in [1/3] catch it? Aaah, I see what you mean. Good point. The empty string should obviously not be allowed as the new ref. In the original code it _is_ allowed as the _old_ value, and interpreted as 0{40}. I'll update the series. With, among other things, a comment that explains this. -+- Many thanks for the review. I'll be back. -- Karl Hasselström, kha@treskal.com www.treskal.com/kalle ``` ## Johannes Schindelin, 2008-05-26 10:51 Subject: Re: [PATCH 3/3] Make old sha1 optional with git update-ref -d Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0805261149310.30431%40racer In-Reply-To: <20080525161440.25087.83011.stgit@yoghurt> ``` Hi, On Sun, 25 May 2008, Karl Hasselström wrote: > Giving the old sha1 is already optional when changing a ref, and it's > quite handy when running update-ref manually. So make it optional for > deleting a ref too. Isn't this a bit dangerous, especially given that there is potentially _no_ reflog as a safeguard? So no, it is not the same as when changing a ref. I am mildly negative on this (having lost some remote branches, because they were deleted _together with the reflogs_ by "git push bla :x"), Dscho ``` ## Karl Hasselström, 2008-05-26 14:09 Subject: Re: [PATCH 3/3] Make old sha1 optional with git update-ref -d Message-ID: <20080526140931.GA18440@diana.vm.bytemark.co.uk> URL: https://gitlist.dev/e/20080526140931.GA18440%40diana.vm.bytemark.co.uk In-Reply-To: ``` On 2008-05-26 11:51:12 +0100, Johannes Schindelin wrote: > On Sun, 25 May 2008, Karl Hasselström wrote: > > > Giving the old sha1 is already optional when changing a ref, and > > it's quite handy when running update-ref manually. So make it > > optional for deleting a ref too. > > Isn't this a bit dangerous, especially given that there is > potentially _no_ reflog as a safeguard? > > So no, it is not the same as when changing a ref. > > I am mildly negative on this (having lost some remote branches, > because they were deleted _together with the reflogs_ by "git push > bla :x"), Deleting the wrong thing is never a fun experience. The thing is, this patch doesn't really make it any easier to delete the wrong thing. Before, you'd git update-ref -d if you wanted to play it safe, and git update-ref -d if you thought you knew what you were doing. All this patch does is make the second usage mode easier to type. (One might argue that a user who doesn't know what she's doing will have an easier time shooting herself in the foot when the oldvalue is optional, but I was under the impression that ease of use without consulting the man page shouldn't be a factor when designing the plumbing UI.) I do agree with you that branch deletion is one of the more "dangerous" operations in git. But I don't think making update-ref hard to use is the answer there; what we'd really need is for old reflogs to stay around for a while before being deleted. -- Karl Hasselström, kha@treskal.com www.treskal.com/kalle ```