{"thread":{"id":"14910","subject":"[PATCH 1/2] reflog test: add more tests for 'reflog delete'","startedAt":"2008-08-09T23:33:29Z","lastAt":"2008-08-10T20:22:21Z","messageCount":9,"participants":["Pieter de Bie","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"86625","messageId":"1218324810-35376-1-git-send-email-pdebie@ai.rug.nl","threadId":"14910","inReplyTo":null,"subject":"[PATCH 1/2] reflog test: add more tests for 'reflog delete'","fromName":"Pieter de Bie","fromEmail":"pdebie@ai.rug.nl","sentAt":"2008-08-09T23:33:29Z","receivedAt":"2008-08-09T23:33:29Z","isPatch":true,"sender":{"key":"pdebie@ai.rug.nl","avatar":null},"body":"This adds more tests for 'reflog delete' and marks it as\nbroken, as currently a call to 'git reflog delete HEAD@{1}'\ndeletes entries in the currently checked out branch's log,\nnot the HEAD log.\n\nNoticed by John Wiegley\n\nSigned-off-by: Pieter de Bie \n---\n\n\tjohnw on IRC noticed this. This adds a test that shows the\n\tproblem. The next patch fixes the issue but I'm not sure of\n\tthe implementation. Perhaps we just shouldn't resolve symbolic\n\trefs? I'm not really sure which functions to use then.\n\n t/t1410-reflog.sh | 22 ++++++++++++++++++----\n 1 files changed, 18 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\nindex 73f830d..3b9860e 100755\n--- a/t/t1410-reflog.sh\n+++ b/t/t1410-reflog.sh\n@@ -175,7 +175,7 @@ test_expect_success 'recover and check' '\n \n '\n \n-test_expect_success 'delete' '\n+test_expect_failure 'delete' '\n \techo 1 > C &&\n \ttest_tick &&\n \tgit commit -m rat C &&\n@@ -188,16 +188,30 @@ test_expect_success 'delete' '\n \ttest_tick &&\n \tgit commit -m tiger C &&\n \n-\ttest 5 = $(git reflog | wc -l) &&\n+\tHEAD_entry_count=$(git reflog | wc -l)\n+\tmaster_entry_count=$(git reflog show master | wc -l)\n+\n+\ttest $HEAD_entry_count = 5 &&\n+\ttest $master_entry_count = 5 &&\n+\n \n \tgit reflog delete master@{1} &&\n \tgit reflog show master > output &&\n-\ttest 4 = $(wc -l < output) &&\n+\ttest $(($master_entry_count - 1)) = $(wc -l < output) &&\n+\ttest $HEAD_entry_count = $(git reflog | wc -l) &&\n \t! grep ox < output &&\n \n+\tmaster_entry_count=$(wc -l < output)\n+\n+\tgit reflog delete HEAD@{1} &&\n+\ttest $(($HEAD_entry_count -1)) = $(git reflog | wc -l) &&\n+\ttest $master_entry_count = $(git reflog show master | wc -l) &&\n+\n+\tHEAD_entry_count=$(git reflog | wc -l)\n+\n \tgit reflog delete master@{07.04.2005.15:15:00.-0700} &&\n \tgit reflog show master > output &&\n-\ttest 3 = $(wc -l < output) &&\n+\ttest $(($master_entry_count - 1)) = $(wc -l < output) &&\n \t! grep dragon < output\n \n '\n-- \n1.6.0.rc0.320.g49281\n"},{"id":"86626","messageId":"1218324810-35376-2-git-send-email-pdebie@ai.rug.nl","threadId":"14910","inReplyTo":"1218324810-35376-1-git-send-email-pdebie@ai.rug.nl","subject":"[PATCH 2/2] builtin-reflog: fix deletion of HEAD entries","fromName":"Pieter de Bie","fromEmail":"pdebie@ai.rug.nl","sentAt":"2008-08-09T23:33:30Z","receivedAt":"2008-08-09T23:33:30Z","isPatch":true,"sender":{"key":"pdebie@ai.rug.nl","avatar":null},"body":"dwim_ref() used to resolve HEAD to its symlink (like refs/heads/master),\nmaking a call to 'git reflog delete HEAD@{1}' to actually delete the second\nentry in the master reflog.\n\nThis patch makes a special case for HEAD (as that's the only non-branch\nreflog we keep), fixing the issue.\n\nSigned-off-by: Pieter de Bie \n---\n builtin-reflog.c | 15 ++++++++++++---\n t/t1410-reflog.sh | 2 +-\n 2 files changed, 13 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-reflog.c b/builtin-reflog.c\nindex 0c34e37..5af3f28 100644\n--- a/builtin-reflog.c\n+++ b/builtin-reflog.c\n@@ -604,9 +604,18 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif (!dwim_ref(argv[i], spec - argv[i], sha1, &ref)) {\n-\t\t\tstatus |= error(\"%s points nowhere!\", argv[i]);\n-\t\t\tcontinue;\n+\t\tif (!strncmp(argv[i], \"HEAD\", 4)) {\n+\t\t\tref = xstrdup(\"HEAD\");\n+\t\t\tif (!resolve_ref(ref, sha1, 1, NULL)) {\n+\t\t\t\tstatus |= error(\"%s points nowhere!\", argv[i]);\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t}\n+\t\telse {\n+\t\t\tif (!dwim_ref(argv[i], spec - argv[i], sha1, &ref)) {\n+\t\t\t\tstatus |= error(\"%s points nowhere!\", argv[i]);\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t}\n \n \t\trecno = strtoul(spec + 2, &ep, 10);\ndiff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\nindex 3b9860e..5b24f05 100755\n--- a/t/t1410-reflog.sh\n+++ b/t/t1410-reflog.sh\n@@ -175,7 +175,7 @@ test_expect_success 'recover and check' '\n \n '\n \n-test_expect_failure 'delete' '\n+test_expect_success 'delete' '\n \techo 1 > C &&\n \ttest_tick &&\n \tgit commit -m rat C &&\n-- \n1.6.0.rc0.320.g49281\n"},{"id":"86628","messageId":"7vhc9t4s2c.fsf@gitster.siamese.dyndns.org","threadId":"14910","inReplyTo":"1218324810-35376-2-git-send-email-pdebie@ai.rug.nl","subject":"Re: [PATCH 2/2] builtin-reflog: fix deletion of HEAD entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-10T00:44:27Z","receivedAt":"2008-08-10T00:44:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pieter de Bie writes:\n\n> dwim_ref() used to resolve HEAD to its symlink (like refs/heads/master),\n> making a call to 'git reflog delete HEAD@{1}' to actually delete the second\n> entry in the master reflog.\n>\n> This patch makes a special case for HEAD (as that's the only non-branch\n> reflog we keep), fixing the issue.\n\nWhat happens to remotes/origin/HEAD that points at remotes/origin/master?\n"},{"id":"86629","messageId":"7vd4kh4r9m.fsf@gitster.siamese.dyndns.org","threadId":"14910","inReplyTo":"7vhc9t4s2c.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] builtin-reflog: fix deletion of HEAD entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-10T01:01:41Z","receivedAt":"2008-08-10T01:01:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Pieter de Bie writes:\n>\n>> dwim_ref() used to resolve HEAD to its symlink (like refs/heads/master),\n>> making a call to 'git reflog delete HEAD@{1}' to actually delete the second\n>> entry in the master reflog.\n>>\n>> This patch makes a special case for HEAD (as that's the only non-branch\n>> reflog we keep), fixing the issue.\n>\n> What happens to remotes/origin/HEAD that points at remotes/origin/master?\n\nPerhaps this might work better?\n\n builtin-reflog.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-reflog.c b/builtin-reflog.c\nindex 0c34e37..a48f664 100644\n--- a/builtin-reflog.c\n+++ b/builtin-reflog.c\n@@ -604,7 +604,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif (!dwim_ref(argv[i], spec - argv[i], sha1, &ref)) {\n+\t\tif (!dwim_log(argv[i], spec - argv[i], sha1, &ref)) {\n \t\t\tstatus |= error(\"%s points nowhere!\", argv[i]);\n \t\t\tcontinue;\n \t\t}\n"},{"id":"86658","messageId":"1218360901-36215-1-git-send-email-pdebie@ai.rug.nl","threadId":"14910","inReplyTo":"7vd4kh4r9m.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] builtin-reflog: fix deletion of HEAD entries","fromName":"Pieter de Bie","fromEmail":"pdebie@ai.rug.nl","sentAt":"2008-08-10T09:35:01Z","receivedAt":"2008-08-10T09:35:01Z","isPatch":true,"sender":{"key":"pdebie@ai.rug.nl","avatar":null},"body":"On Aug 10, 2008, at 3:01 AM, Junio C Hamano wrote:\n>-\t\tif (!dwim_ref(argv[i], spec - argv[i], sha1, &ref)) {\n>+\t\tif (!dwim_log(argv[i], spec - argv[i], sha1, &ref)) {\n\nThis is also what add_reflog_for_walk() does, but that function tries to resolve\nthe argv[i] part first, without doing the dwim_log().\n\nPerhaps we can also do this to allow \"git reflog expire master\" instead of\n\"git reflog expire refs/heads/master\"? \n\n--<8--\nSubject: [PATCH] builtin-reflog: Allow reflog expire to name partial ref\n\nThis allows you to specify 'git reflog expire master' without needing\nto give the full refname like 'git reflog expire refs/heads/master'\n\nSigned-off-by: Pieter de Bie \n---\n builtin-reflog.c | 7 ++++---\n 1 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-reflog.c b/builtin-reflog.c\nindex 5af3f28..a8311a6 100644\n--- a/builtin-reflog.c\n+++ b/builtin-reflog.c\n@@ -541,14 +541,15 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t}\n \n \twhile (i < argc) {\n-\t\tconst char *ref = argv[i++];\n+\t\tchar *ref;\n \t\tunsigned char sha1[20];\n-\t\tif (!resolve_ref(ref, sha1, 1, NULL)) {\n-\t\t\tstatus |= error(\"%s points nowhere!\", ref);\n+\t\tif (!dwim_log(argv[i], strlen(argv[i]), sha1, &ref)) {\n+\t\t\tstatus |= error(\"%s points nowhere!\", argv[i]);\n \t\t\tcontinue;\n \t\t}\n \t\tset_reflog_expiry_param(&cb, explicit_expiry, ref);\n \t\tstatus |= expire_reflog(ref, sha1, 0, &cb);\n+\t\ti++;\n \t}\n \treturn status;\n }\n-- \n1.6.0.rc0.320.g49281\n"},{"id":"86661","messageId":"200808101312.48213.johannes.sixt@telecom.at","threadId":"14910","inReplyTo":"1218360901-36215-1-git-send-email-pdebie@ai.rug.nl","subject":"Re: [PATCH 2/2] builtin-reflog: fix deletion of HEAD entries","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2008-08-10T11:12:48Z","receivedAt":"2008-08-10T11:12:48Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Sonntag, 10. August 2008, Pieter de Bie wrote:\n> diff --git a/builtin-reflog.c b/builtin-reflog.c\n> index 5af3f28..a8311a6 100644\n> --- a/builtin-reflog.c\n> +++ b/builtin-reflog.c\n> @@ -541,14 +541,15 @@ static int cmd_reflog_expire(int argc, const char\n> **argv, const char *prefix) }\n>\n> \twhile (i < argc) {\n> -\t\tconst char *ref = argv[i++];\n> +\t\tchar *ref;\n> \t\tunsigned char sha1[20];\n> -\t\tif (!resolve_ref(ref, sha1, 1, NULL)) {\n> -\t\t\tstatus |= error(\"%s points nowhere!\", ref);\n> +\t\tif (!dwim_log(argv[i], strlen(argv[i]), sha1, &ref)) {\n> +\t\t\tstatus |= error(\"%s points nowhere!\", argv[i]);\n> \t\t\tcontinue;\n> \t\t}\n> \t\tset_reflog_expiry_param(&cb, explicit_expiry, ref);\n> \t\tstatus |= expire_reflog(ref, sha1, 0, &cb);\n> +\t\ti++;\n> \t}\n> \treturn status;\n> }\n\nThis runs into an endless loop in the error case because it doesn't increase \ni.\n\n-- Hannes\n"},{"id":"86708","messageId":"7vej4w3dou.fsf@gitster.siamese.dyndns.org","threadId":"14910","inReplyTo":"1218360901-36215-1-git-send-email-pdebie@ai.rug.nl","subject":"Re: [PATCH 2/2] builtin-reflog: fix deletion of HEAD entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-10T18:52:33Z","receivedAt":"2008-08-10T18:52:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pieter de Bie writes:\n\n> On Aug 10, 2008, at 3:01 AM, Junio C Hamano wrote:\n>>-\t\tif (!dwim_ref(argv[i], spec - argv[i], sha1, &ref)) {\n>>+\t\tif (!dwim_log(argv[i], spec - argv[i], sha1, &ref)) {\n>\n> This is also what add_reflog_for_walk() does, but that function tries to resolve\n> the argv[i] part first, without doing the dwim_log().\n>\n> Perhaps we can also ...\n\nSorry, I do not understand what you meant by the above comment.\n\n - \"This is also what add_reflog_for_walk() does\" -- I take it you mean\n the use of dwim_log() instead of dwim_ref()?\n\n - \"... but that function tries to resolve the argv[i] part first\" -- do\n you mean the resolve_ref(\"HEAD\"...) call inside \"if (!*branch)\"\n codepath?\n\n That one serves different purposes than \"delete HEAD@{42}\". It is\n about showing \"@{42}\" --- in order to show reflog for \"the current\n branch\", it figures out the current branch by resolving \"HEAD\".\n\nIn any case, what confuses me is I cannot tell if you do or do not have\nissues that I did not think of with the \"s/dwim_ref/dwim_log/\" change.\nAre you saying \"no that cannot be a correct fix; see the way dwim_log() is\nused in add_reflog_for_walk() -- it does more than your one-liner\"?\n\nBy the way, I think the idea of \"Perhaps we can also...\" part is good.\n"},{"id":"86709","messageId":"9746FE5D-816C-4818-B32F-EE0028918F72@ai.rug.nl","threadId":"14910","inReplyTo":"7vej4w3dou.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] builtin-reflog: fix deletion of HEAD entries","fromName":"Pieter de Bie","fromEmail":"pdebie@ai.rug.nl","sentAt":"2008-08-10T19:04:56Z","receivedAt":"2008-08-10T19:04:56Z","isPatch":true,"sender":{"key":"pdebie@ai.rug.nl","avatar":null},"body":"\nOn Aug 10, 2008, at 8:52 PM, Junio C Hamano wrote:\n\n> Pieter de Bie writes:\n>\n>> On Aug 10, 2008, at 3:01 AM, Junio C Hamano wrote:\n>>> -\t\tif (!dwim_ref(argv[i], spec - argv[i], sha1, &ref)) {\n>>> +\t\tif (!dwim_log(argv[i], spec - argv[i], sha1, &ref)) {\n>>\n>> This is also what add_reflog_for_walk() does, but that function \n>> tries to resolve\n>> the argv[i] part first, without doing the dwim_log().\n>>\n>> Perhaps we can also ...\n>\n> Sorry, I do not understand what you meant by the above comment.\n\nSorry, it was early in the morning ;)\n\n> - \"This is also what add_reflog_for_walk() does\" -- I take it you mean\n> the use of dwim_log() instead of dwim_ref()?\n\nYes\n\n> - \"... but that function tries to resolve the argv[i] part first\" -- \n> do\n> you mean the resolve_ref(\"HEAD\"...) call inside \"if (!*branch)\"\n> codepath?\n>\n> That one serves different purposes than \"delete HEAD@{42}\". It is\n> about showing \"@{42}\" --- in order to show reflog for \"the current\n> branch\", it figures out the current branch by resolving \"HEAD\".\n\nNo, I meant this part:\n\nreflogs = read_complete_reflog(branch);\nif (!reflogs || reflogs->nr == 0)\n\tif (dwim_log(branch, strlen(branch), sha1, &b) == 1) {\n\t\tbranch = b;\n\t\treflogs = read_complete_reflog(branch);\n\t}\n\nWhich seems to suggest that the read_complete_reflog() may produce \ndifferent results if dwim_log() is not called. However, I did not \nfollow the codepath to see why.\n\n> In any case, what confuses me is I cannot tell if you do or do not \n> have\n> issues that I did not think of with the \"s/dwim_ref/dwim_log/\" change.\n> Are you saying \"no that cannot be a correct fix; see the way \n> dwim_log() is\n> used in add_reflog_for_walk() -- it does more than your one-liner\"?\n\nI think the change looks ok. The only 'problem' I had was the chunk \nabove, because I do not know if the double call to \nread_complete_reflog, once without a dwim_log and optionally once \nwith, is significant. However, I'm not familiar enough with the code \nto make any observation other than that, which is why my reply had no \nconclusion ;)\n\nHope that clears things up.\n\n> By the way, I think the idea of \"Perhaps we can also...\" part is good.\n\nI'll send in a better patch.\n\n- Pieter\n"},{"id":"86715","messageId":"1218399741-37049-1-git-send-email-pdebie@ai.rug.nl","threadId":"14910","inReplyTo":"9746FE5D-816C-4818-B32F-EE0028918F72@ai.rug.nl","subject":"[PATCH] builtin-reflog: Allow reflog expire to name partial ref","fromName":"Pieter de Bie","fromEmail":"pdebie@ai.rug.nl","sentAt":"2008-08-10T20:22:21Z","receivedAt":"2008-08-10T20:22:21Z","isPatch":true,"sender":{"key":"pdebie@ai.rug.nl","avatar":null},"body":"This allows you to specify 'git reflog expire master' without needing\nto give the full refname like 'git reflog expire refs/heads/master'\n\nSigned-off-by: Pieter de Bie \n---\n builtin-reflog.c | 8 ++++----\n 1 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-reflog.c b/builtin-reflog.c\nindex 5af3f28..f4d1f32 100644\n--- a/builtin-reflog.c\n+++ b/builtin-reflog.c\n@@ -540,11 +540,11 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\tfree(collected.e);\n \t}\n \n-\twhile (i < argc) {\n-\t\tconst char *ref = argv[i++];\n+\tfor (; i < argc; i++) {\n+\t\tchar *ref;\n \t\tunsigned char sha1[20];\n-\t\tif (!resolve_ref(ref, sha1, 1, NULL)) {\n-\t\t\tstatus |= error(\"%s points nowhere!\", ref);\n+\t\tif (!dwim_log(argv[i], strlen(argv[i]), sha1, &ref)) {\n+\t\t\tstatus |= error(\"%s points nowhere!\", argv[i]);\n \t\t\tcontinue;\n \t\t}\n \t\tset_reflog_expiry_param(&cb, explicit_expiry, ref);\n-- \n1.6.0.rc0.320.g49281\n"}]}