{"thread":{"id":"35621","subject":"[PATCH] sha1_name: don't resolve refs when core.warnambiguousrefs is false","startedAt":"2014-01-07T03:32:01Z","lastAt":"2014-01-14T11:34:37Z","messageCount":36,"participants":["Brodie Rao","Duy Nguyen","Jeff King","Junio C Hamano","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"232796","messageId":"1389065521-46331-1-git-send-email-brodie@sf.io","threadId":"35621","inReplyTo":null,"subject":"[PATCH] sha1_name: don't resolve refs when core.warnambiguousrefs is false","fromName":"Brodie Rao","fromEmail":"brodie@sf.io","sentAt":"2014-01-07T03:32:01Z","receivedAt":"2014-01-07T03:32:01Z","isPatch":true,"sender":{"key":"brodie@sf.io","avatar":"https://avatars.githubusercontent.com/u/42407?v=4"},"body":"This change ensures get_sha1_basic() doesn't try to resolve full hashes\nas refs when ambiguous ref warnings are disabled.\n\nThis provides a substantial performance improvement when passing many\nhashes to a command (like \"git rev-list --stdin\") when\ncore.warnambiguousrefs is false. The check incurs 6 stat()s for every\nhash supplied, which can be costly over NFS.\n---\n sha1_name.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex e9c2999..10bd007 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -451,9 +451,9 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \tint at, reflog_len, nth_prior = 0;\n \n \tif (len == 40 && !get_sha1_hex(str, sha1)) {\n-\t\tif (warn_on_object_refname_ambiguity) {\n+\t\tif (warn_ambiguous_refs && warn_on_object_refname_ambiguity) {\n \t\t\trefs_found = dwim_ref(str, len, tmp_sha1, &real_ref);\n-\t\t\tif (refs_found > 0 && warn_ambiguous_refs) {\n+\t\t\tif (refs_found > 0) {\n \t\t\t\twarning(warn_msg, len, str);\n \t\t\t\tif (advice_object_name_warning)\n \t\t\t\t\tfprintf(stderr, \"%s\\n\", _(object_name_msg));\n-- \n1.8.3.4 (Apple Git-47)\n"},{"id":"232797","messageId":"CAEfQM484kqLSVeyjhYtg7GfXOQkQNjaO1FV2_U3uAqO=Nargdg@mail.gmail.com","threadId":"35621","inReplyTo":"1389065521-46331-1-git-send-email-brodie@sf.io","subject":"Re: [PATCH] sha1_name: don't resolve refs when core.warnambiguousrefs is false","fromName":"Brodie Rao","fromEmail":"brodie@sf.io","sentAt":"2014-01-07T03:35:04Z","receivedAt":"2014-01-07T03:35:04Z","isPatch":true,"sender":{"key":"brodie@sf.io","avatar":"https://avatars.githubusercontent.com/u/42407?v=4"},"body":"On Mon, Jan 6, 2014 at 7:32 PM, Brodie Rao wrote:\n> This change ensures get_sha1_basic() doesn't try to resolve full hashes\n> as refs when ambiguous ref warnings are disabled.\n>\n> This provides a substantial performance improvement when passing many\n> hashes to a command (like \"git rev-list --stdin\") when\n> core.warnambiguousrefs is false. The check incurs 6 stat()s for every\n> hash supplied, which can be costly over NFS.\n\nForgot to add:\n\nSigned-off-by: Brodie Rao \n\n> ---\n> sha1_name.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/sha1_name.c b/sha1_name.c\n> index e9c2999..10bd007 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -451,9 +451,9 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n> int at, reflog_len, nth_prior = 0;\n>\n> if (len == 40 && !get_sha1_hex(str, sha1)) {\n> - if (warn_on_object_refname_ambiguity) {\n> + if (warn_ambiguous_refs && warn_on_object_refname_ambiguity) {\n> refs_found = dwim_ref(str, len, tmp_sha1, &real_ref);\n> - if (refs_found > 0 && warn_ambiguous_refs) {\n> + if (refs_found > 0) {\n> warning(warn_msg, len, str);\n> if (advice_object_name_warning)\n> fprintf(stderr, \"%s\\n\", _(object_name_msg));\n> --\n> 1.8.3.4 (Apple Git-47)\n>\n"},{"id":"232800","messageId":"CACsJy8CBCb1i3iLevmgR2SZYpFyZGDPqDSKEL4B_78JyE9Mhew@mail.gmail.com","threadId":"35621","inReplyTo":"1389065521-46331-1-git-send-email-brodie@sf.io","subject":"Re: [PATCH] sha1_name: don't resolve refs when core.warnambiguousrefs is false","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-01-07T06:45:22Z","receivedAt":"2014-01-07T06:45:22Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jan 7, 2014 at 10:32 AM, Brodie Rao wrote:\n> This change ensures get_sha1_basic() doesn't try to resolve full hashes\n> as refs when ambiguous ref warnings are disabled.\n>\n> This provides a substantial performance improvement when passing many\n> hashes to a command (like \"git rev-list --stdin\") when\n> core.warnambiguousrefs is false. The check incurs 6 stat()s for every\n> hash supplied, which can be costly over NFS.\n> ---\n> sha1_name.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/sha1_name.c b/sha1_name.c\n> index e9c2999..10bd007 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -451,9 +451,9 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n> int at, reflog_len, nth_prior = 0;\n>\n> if (len == 40 && !get_sha1_hex(str, sha1)) {\n> - if (warn_on_object_refname_ambiguity) {\n> + if (warn_ambiguous_refs && warn_on_object_refname_ambiguity) {\n> refs_found = dwim_ref(str, len, tmp_sha1, &real_ref);\n> - if (refs_found > 0 && warn_ambiguous_refs) {\n> + if (refs_found > 0) {\n> warning(warn_msg, len, str);\n> if (advice_object_name_warning)\n> fprintf(stderr, \"%s\\n\", _(object_name_msg));\n\nLooks obviously correct. Thanks.\n-- \nDuy\n"},{"id":"232811","messageId":"20140107171307.GA19482@sigill.intra.peff.net","threadId":"35621","inReplyTo":"CAEfQM484kqLSVeyjhYtg7GfXOQkQNjaO1FV2_U3uAqO=Nargdg@mail.gmail.com","subject":"Re: [PATCH] sha1_name: don't resolve refs when core.warnambiguousrefs is false","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-07T17:13:08Z","receivedAt":"2014-01-07T17:13:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 06, 2014 at 07:35:04PM -0800, Brodie Rao wrote:\n\n> On Mon, Jan 6, 2014 at 7:32 PM, Brodie Rao wrote:\n> > This change ensures get_sha1_basic() doesn't try to resolve full hashes\n> > as refs when ambiguous ref warnings are disabled.\n> >\n> > This provides a substantial performance improvement when passing many\n> > hashes to a command (like \"git rev-list --stdin\") when\n> > core.warnambiguousrefs is false. The check incurs 6 stat()s for every\n> > hash supplied, which can be costly over NFS.\n> \n> Forgot to add:\n> \n> Signed-off-by: Brodie Rao \n\nLooks good to me.\n\nI wonder if I should have simply gone this route instead of adding\nwarn_on_object_refname_ambiguity, and then people who want \"cat-file\n--batch\" to be fast could just turn off core.warnAmbiguousRefs. I wanted\nit to happen automatically, though. Alternatively, I guess \"cat-file\n--batch\" could just turn off warn_ambiguous_refs itself.\n\n-Peff\n"},{"id":"232812","messageId":"xmqqd2k3g0ww.fsf@gitster.dls.corp.google.com","threadId":"35621","inReplyTo":"1389065521-46331-1-git-send-email-brodie@sf.io","subject":"Re: [PATCH] sha1_name: don't resolve refs when core.warnambiguousrefs is false","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-07T17:24:47Z","receivedAt":"2014-01-07T17:24:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brodie Rao writes:\n\n> This change ensures get_sha1_basic() doesn't try to resolve full hashes\n> as refs when ambiguous ref warnings are disabled.\n>\n> This provides a substantial performance improvement when passing many\n> hashes to a command (like \"git rev-list --stdin\") when\n> core.warnambiguousrefs is false. The check incurs 6 stat()s for every\n> hash supplied, which can be costly over NFS.\n> ---\n\nNeeds sign-off. The patch looks good.\n\nThanks.\n\n> sha1_name.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/sha1_name.c b/sha1_name.c\n> index e9c2999..10bd007 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -451,9 +451,9 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n> \tint at, reflog_len, nth_prior = 0;\n> \n> \tif (len == 40 && !get_sha1_hex(str, sha1)) {\n> -\t\tif (warn_on_object_refname_ambiguity) {\n> +\t\tif (warn_ambiguous_refs && warn_on_object_refname_ambiguity) {\n> \t\t\trefs_found = dwim_ref(str, len, tmp_sha1, &real_ref);\n> -\t\t\tif (refs_found > 0 && warn_ambiguous_refs) {\n> +\t\t\tif (refs_found > 0) {\n> \t\t\t\twarning(warn_msg, len, str);\n> \t\t\t\tif (advice_object_name_warning)\n> \t\t\t\t\tfprintf(stderr, \"%s\\n\", _(object_name_msg));\n"},{"id":"232819","messageId":"xmqqzjn7el4k.fsf@gitster.dls.corp.google.com","threadId":"35621","inReplyTo":"20140107171307.GA19482@sigill.intra.peff.net","subject":"Re: [PATCH] sha1_name: don't resolve refs when core.warnambiguousrefs is false","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-07T17:51:07Z","receivedAt":"2014-01-07T17:51:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Alternatively, I guess \"cat-file\n> --batch\" could just turn off warn_ambiguous_refs itself.\n\nSounds like a sensible way to go, perhaps on top of this change?\n"},{"id":"232820","messageId":"20140107175241.GA20415@sigill.intra.peff.net","threadId":"35621","inReplyTo":"xmqqzjn7el4k.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] sha1_name: don't resolve refs when core.warnambiguousrefs is false","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-07T17:52:42Z","receivedAt":"2014-01-07T17:52:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 07, 2014 at 09:51:07AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Alternatively, I guess \"cat-file\n> > --batch\" could just turn off warn_ambiguous_refs itself.\n> \n> Sounds like a sensible way to go, perhaps on top of this change?\n\nThe downside is that we would not warn about ambiguous refs anymore,\neven if the user was expecting it to. I don't know if that matters much.\nI kind of feel in the --batch situation that it is somewhat useless (I\nwonder if \"rev-list --stdin\" should turn it off, too).\n\n-Peff\n"},{"id":"232832","messageId":"1389122612-48184-1-git-send-email-brodie@sf.io","threadId":"35621","inReplyTo":"xmqqd2k3g0ww.fsf@gitster.dls.corp.google.com","subject":"[PATCH] sha1_name: don't resolve refs when core.warnambiguousrefs is false","fromName":"Brodie Rao","fromEmail":"brodie@sf.io","sentAt":"2014-01-07T19:23:32Z","receivedAt":"2014-01-07T19:23:32Z","isPatch":true,"sender":{"key":"brodie@sf.io","avatar":"https://avatars.githubusercontent.com/u/42407?v=4"},"body":"This change ensures get_sha1_basic() doesn't try to resolve full hashes\nas refs when ambiguous ref warnings are disabled.\n\nThis provides a substantial performance improvement when passing many\nhashes to a command (like \"git rev-list --stdin\") when\ncore.warnambiguousrefs is false. The check incurs 6 stat()s for every\nhash supplied, which can be costly over NFS.\n\nSigned-off-by: Brodie Rao \n---\n sha1_name.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex e9c2999..10bd007 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -451,9 +451,9 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \tint at, reflog_len, nth_prior = 0;\n \n \tif (len == 40 && !get_sha1_hex(str, sha1)) {\n-\t\tif (warn_on_object_refname_ambiguity) {\n+\t\tif (warn_ambiguous_refs && warn_on_object_refname_ambiguity) {\n \t\t\trefs_found = dwim_ref(str, len, tmp_sha1, &real_ref);\n-\t\t\tif (refs_found > 0 && warn_ambiguous_refs) {\n+\t\t\tif (refs_found > 0) {\n \t\t\t\twarning(warn_msg, len, str);\n \t\t\t\tif (advice_object_name_warning)\n \t\t\t\t\tfprintf(stderr, \"%s\\n\", _(object_name_msg));\n-- \n1.8.5.2\n"},{"id":"232835","messageId":"xmqqppo3d1lk.fsf@gitster.dls.corp.google.com","threadId":"35621","inReplyTo":"20140107175241.GA20415@sigill.intra.peff.net","subject":"Re: [PATCH] sha1_name: don't resolve refs when core.warnambiguousrefs is false","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-07T19:38:15Z","receivedAt":"2014-01-07T19:38:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Jan 07, 2014 at 09:51:07AM -0800, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > Alternatively, I guess \"cat-file\n>> > --batch\" could just turn off warn_ambiguous_refs itself.\n>> \n>> Sounds like a sensible way to go, perhaps on top of this change?\n>\n> The downside is that we would not warn about ambiguous refs anymore,\n> even if the user was expecting it to. I don't know if that matters much.\n\nThat is true already with or without Brodie's change, isn't it?\nWith warn_on_object_refname_ambiguity, \"cat-file --batch\" makes us\nignore core.warnambigousrefs setting. If we redo 25fba78d\n(cat-file: disable object/refname ambiguity check for batch mode,\n2013-07-12) to unconditionally disable warn_ambiguous_refs in\n\"cat-file --batch\" and get rid of warn_on_object_refname_ambiguity,\nthe end result would be the same, no?\n\n> I kind of feel in the --batch situation that it is somewhat useless (I\n> wonder if \"rev-list --stdin\" should turn it off, too).\n\nI think doing the same as \"cat-file --batch\" in \"rev-list --stdin\"\nmakes sense. Both interfaces are designed to grok extended SHA-1s,\nand full 40-hex object names could be ambiguous and we are missing\nthe warning for them.\n\nOr are you wondering if we should revert 25fba78d, apply Brodie's\nchange to skip the ref resolution whose result is never used, and\ntell people who want to use \"cat-file --batch\" (or \"rev-list\n--stdin\") to disable the ambiguity warning themselves?\n"},{"id":"232842","messageId":"20140107195844.GA21812@sigill.intra.peff.net","threadId":"35621","inReplyTo":"xmqqppo3d1lk.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] sha1_name: don't resolve refs when core.warnambiguousrefs is false","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-07T19:58:44Z","receivedAt":"2014-01-07T19:58:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 07, 2014 at 11:38:15AM -0800, Junio C Hamano wrote:\n\n> >> > Alternatively, I guess \"cat-file\n> >> > --batch\" could just turn off warn_ambiguous_refs itself.\n> >> \n> >> Sounds like a sensible way to go, perhaps on top of this change?\n> >\n> > The downside is that we would not warn about ambiguous refs anymore,\n> > even if the user was expecting it to. I don't know if that matters much.\n> \n> That is true already with or without Brodie's change, isn't it?\n> With warn_on_object_refname_ambiguity, \"cat-file --batch\" makes us\n> ignore core.warnambigousrefs setting. If we redo 25fba78d\n> (cat-file: disable object/refname ambiguity check for batch mode,\n> 2013-07-12) to unconditionally disable warn_ambiguous_refs in\n> \"cat-file --batch\" and get rid of warn_on_object_refname_ambiguity,\n> the end result would be the same, no?\n\nNo, I don't think the end effect is the same (or maybe we are not\ntalking about the same thing. :) ).\n\nThere are two ambiguity situations:\n\n 1. Ambiguous non-fully-qualified refs (e.g., same tag and head name).\n\n 2. 40-hex sha1 object names which might also be unqualified ref names.\n\nPrior to 25ffba78d, cat-file checked both (like all the rest of git).\nBut checking (2) is very expensive, since otherwise a 40-hex sha1 does\nnot need to do a ref lookup at all, and something like \"rev-list\n--objects | cat-file --batch-check\" processes a large number of these.\n\nDetecting (1) is not nearly as expensive. You must already be doing a\nref lookup to trigger it (so the relative cost is much closer), and your\nquery size is bounded by the number of refs, not the number of objects.\n\nCommit 25ffba78d traded off some safety for a lot of performance by\ndisabling (2), but left (1) in place because the tradeoff is different.\n\nThe two options I was musing over earlier today were (all on top of\nBrodie's patch):\n\n a. Revert 25ffba78d. With Brodie's patch, core.warnAmbiguousRefs\n disables _both_ warnings. So we default to safe-but-slow, but\n people who care about performance can turn off ambiguity warnings.\n The downside is that you have to know to turn it off manually (and\n it's a global config flag, so you end up turning it off\n _everywhere_, not just in big queries where it matters).\n\n b. Revert 25ffba78d, but then on top of it just turn off\n warn_ambiguous_refs unconditionally in \"cat-file --batch-check\".\n The downside is that we drop the safety from (1). The upside is\n that the code is a little simpler, as we drop the extra flag.\n\nAnd obviously:\n\n c. Just leave it at Brodie's patch with nothing else on top.\n\nMy thinking in favor of (b) was basically \"does anybody actually care\nabout ambiguous refs in this situation anyway?\". If they do, then I\nthink (c) is my preferred choice.\n\n> > I kind of feel in the --batch situation that it is somewhat useless (I\n> > wonder if \"rev-list --stdin\" should turn it off, too).\n> \n> I think doing the same as \"cat-file --batch\" in \"rev-list --stdin\"\n> makes sense. Both interfaces are designed to grok extended SHA-1s,\n> and full 40-hex object names could be ambiguous and we are missing\n> the warning for them.\n\nI'm not sure I understand what you are saying. We _do_ have the warning\nfor \"rev-list --stdin\" currently. We do _not_ have the warning for\n\"cat-file --batch\", since my 25ffba78d. I was wondering if rev-list\nshould go the same way as 25ffba78d, for efficiency reasons (e.g., think\npiping to \"rev-list --no-walk --stdin\").\n\n> Or are you wondering if we should revert 25fba78d, apply Brodie's\n> change to skip the ref resolution whose result is never used, and\n> tell people who want to use \"cat-file --batch\" (or \"rev-list\n> --stdin\") to disable the ambiguity warning themselves?\n\nSee above. :)\n\n-Peff\n"},{"id":"232846","messageId":"xmqqd2k3cz42.fsf@gitster.dls.corp.google.com","threadId":"35621","inReplyTo":"20140107195844.GA21812@sigill.intra.peff.net","subject":"Re: [PATCH] sha1_name: don't resolve refs when core.warnambiguousrefs is false","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-07T20:31:57Z","receivedAt":"2014-01-07T20:31:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Jan 07, 2014 at 11:38:15AM -0800, Junio C Hamano wrote:\n>\n>> >> > Alternatively, I guess \"cat-file\n>> >> > --batch\" could just turn off warn_ambiguous_refs itself.\n>> >> \n>> >> Sounds like a sensible way to go, perhaps on top of this change?\n>> >\n>> > The downside is that we would not warn about ambiguous refs anymore,\n>> > even if the user was expecting it to. I don't know if that matters much.\n>> \n>> That is true already with or without Brodie's change, isn't it?\n>> With warn_on_object_refname_ambiguity, \"cat-file --batch\" makes us\n>> ignore core.warnambigousrefs setting. If we redo 25fba78d\n>> (cat-file: disable object/refname ambiguity check for batch mode,\n>> 2013-07-12) to unconditionally disable warn_ambiguous_refs in\n>> \"cat-file --batch\" and get rid of warn_on_object_refname_ambiguity,\n>> the end result would be the same, no?\n>\n> No, I don't think the end effect is the same (or maybe we are not\n> talking about the same thing. :) ).\n>\n> There are two ambiguity situations:\n>\n> 1. Ambiguous non-fully-qualified refs (e.g., same tag and head name).\n>\n> 2. 40-hex sha1 object names which might also be unqualified ref names.\n>\n> Prior to 25ffba78d, cat-file checked both (like all the rest of git).\n> But checking (2) is very expensive,...\n\nAhh, of course. Sorry for forgetting about 1.\n\n> The two options I was musing over earlier today were (all on top of\n> Brodie's patch):\n>\n> a. Revert 25ffba78d. With Brodie's patch, core.warnAmbiguousRefs\n> disables _both_ warnings. So we default to safe-but-slow, but\n> people who care about performance can turn off ambiguity warnings.\n> The downside is that you have to know to turn it off manually (and\n> it's a global config flag, so you end up turning it off\n> _everywhere_, not just in big queries where it matters).\n\nOr \"git -c core.warnambiguousrefs=false cat-file --batch\", but I\nthink a more important point is that it is no longer automatic for\nknown-to-be-heavy operations, and I agree with you that it is a\ndownside.\n\n> b. Revert 25ffba78d, but then on top of it just turn off\n> warn_ambiguous_refs unconditionally in \"cat-file --batch-check\".\n> The downside is that we drop the safety from (1). The upside is\n> that the code is a little simpler, as we drop the extra flag.\n>\n> And obviously:\n>\n> c. Just leave it at Brodie's patch with nothing else on top.\n>\n> My thinking in favor of (b) was basically \"does anybody actually care\n> about ambiguous refs in this situation anyway?\". If they do, then I\n> think (c) is my preferred choice.\n\nOK. I agree with that line of thinking. Let's take it one step at\na time, i.e. do c. and also use warn_on_object_refname_ambiguity in\n\"rev-list --stdin\" first and leave the simplification (i.e. b.) for\nlater.\n\n>> > I kind of feel in the --batch situation that it is somewhat useless (I\n>> > wonder if \"rev-list --stdin\" should turn it off, too).\n>> \n>> I think doing the same as \"cat-file --batch\" in \"rev-list --stdin\"\n>> makes sense. Both interfaces are designed to grok extended SHA-1s,\n>> and full 40-hex object names could be ambiguous and we are missing\n>> the warning for them.\n>\n> I'm not sure I understand what you are saying. We _do_ have the warning\n> for \"rev-list --stdin\" currently. We do _not_ have the warning for\n> \"cat-file --batch\", since my 25ffba78d.\n\nWhat I wanted to say was that we would be discarding the safety for\n\"rev-list --stdin\" with the same argument as we did for \"cat-file\n--batch\". If the argument for the earlier \"cat-file --batch\" were\n\"this interface only takes raw 40-hex object names\", then the\nsituation would have been different, but that is not the case.\n\n> I was wondering if rev-list should go the same way as 25ffba78d,\n> for efficiency reasons (e.g., think piping to \"rev-list --no-walk\n> --stdin\").\n\nYes, and I was trying to agree with that, but apparently I failed\n;-)\n"},{"id":"232868","messageId":"20140107220856.GA10074@sigill.intra.peff.net","threadId":"35621","inReplyTo":"xmqqd2k3cz42.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] sha1_name: don't resolve refs when core.warnambiguousrefs is false","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-07T22:08:56Z","receivedAt":"2014-01-07T22:08:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 07, 2014 at 12:31:57PM -0800, Junio C Hamano wrote:\n\n> > c. Just leave it at Brodie's patch with nothing else on top.\n> >\n> > My thinking in favor of (b) was basically \"does anybody actually care\n> > about ambiguous refs in this situation anyway?\". If they do, then I\n> > think (c) is my preferred choice.\n> \n> OK. I agree with that line of thinking. Let's take it one step at\n> a time, i.e. do c. and also use warn_on_object_refname_ambiguity in\n> \"rev-list --stdin\" first and leave the simplification (i.e. b.) for\n> later.\n\nHere's a series to do that. The first three are just cleanups I noticed\nwhile looking at the problem.\n\nWhile I was writing the commit messages, though, I had a thought. Maybe\nwe could simply do the check faster for the common case that most refs\ndo not look like object names? Right now we blindly call dwim_ref for\neach get_sha1 call, which is the expensive part. If we instead just\nloaded all of the refnames from the dwim_ref location (basically heads,\ntags and the top-level of \"refs/\"), we could build an index of all of\nthe entries matching the 40-hex pattern. In 99% of cases, this would be\nzero entries, and the check would collapse to a simple integer\ncomparison (and even if we did have one, it would be a simple binary\nsearch in memory).\n\nOur index is more racy than actually checking the filesystem, but I\ndon't think it matters here.\n\nAnyway, here is the series I came up with, in the meantime. I can take a\nquick peek at just making it faster, too.\n\n [1/4]: cat-file: refactor error handling of batch_objects\n [2/4]: cat-file: fix a minor memory leak in batch_objects\n [3/4]: cat-file: restore ambiguity warning flag in batch_objects\n [4/4]: revision: turn off object/refname ambiguity check for --stdin\n\n-Peff\n"},{"id":"232869","messageId":"20140107221014.GA10161@sigill.intra.peff.net","threadId":"35621","inReplyTo":"20140107220856.GA10074@sigill.intra.peff.net","subject":"[PATCH 1/4] cat-file: refactor error handling of batch_objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-07T22:10:15Z","receivedAt":"2014-01-07T22:10:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This just pulls the return value for the function out of the\ninner loop, so we can break out of the loop rather than do\nan early return. This will make it easier to put any cleanup\nfor the function in one place.\n\nSigned-off-by: Jeff King \n---\nJust making the subsequent diffs less noisy...\n\n builtin/cat-file.c | 11 +++++------\n 1 file changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex f8288c8..971cdde 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -263,6 +263,7 @@ static int batch_objects(struct batch_options *opt)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct expand_data data;\n+\tint retval = 0;\n \n \tif (!opt->format)\n \t\topt->format = \"%(objectname) %(objecttype) %(objectsize)\";\n@@ -294,8 +295,6 @@ static int batch_objects(struct batch_options *opt)\n \twarn_on_object_refname_ambiguity = 0;\n \n \twhile (strbuf_getline(&buf, stdin, '\\n') != EOF) {\n-\t\tint error;\n-\n \t\tif (data.split_on_whitespace) {\n \t\t\t/*\n \t\t\t * Split at first whitespace, tying off the beginning\n@@ -310,12 +309,12 @@ static int batch_objects(struct batch_options *opt)\n \t\t\tdata.rest = p;\n \t\t}\n \n-\t\terror = batch_one_object(buf.buf, opt, &data);\n-\t\tif (error)\n-\t\t\treturn error;\n+\t\tretval = batch_one_object(buf.buf, opt, &data);\n+\t\tif (retval)\n+\t\t\tbreak;\n \t}\n \n-\treturn 0;\n+\treturn retval;\n }\n \n static const char * const cat_file_usage[] = {\n-- \n1.8.5.2.500.g8060133\n"},{"id":"232870","messageId":"20140107221035.GB10161@sigill.intra.peff.net","threadId":"35621","inReplyTo":"20140107220856.GA10074@sigill.intra.peff.net","subject":"[PATCH 2/4] cat-file: fix a minor memory leak in batch_objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-07T22:10:35Z","receivedAt":"2014-01-07T22:10:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We should always have been freeing our strbuf, but doing so\nconsistently was annoying until the refactoring in the\nprevious patch.\n\nSigned-off-by: Jeff King \n---\n builtin/cat-file.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 971cdde..ce79103 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -314,6 +314,7 @@ static int batch_objects(struct batch_options *opt)\n \t\t\tbreak;\n \t}\n \n+\tstrbuf_release(&buf);\n \treturn retval;\n }\n \n-- \n1.8.5.2.500.g8060133\n"},{"id":"232871","messageId":"20140107221051.GC10161@sigill.intra.peff.net","threadId":"35621","inReplyTo":"20140107220856.GA10074@sigill.intra.peff.net","subject":"[PATCH 3/4] cat-file: restore ambiguity warning flag in batch_objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-07T22:10:51Z","receivedAt":"2014-01-07T22:10:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since commit 25fba78, we turn off the object/refname\nambiguity warning using a global flag. However, we never\nrestore it. This doesn't really matter in the current code,\nsince the program generally exits immediately after the\nfunction is done, but it's good code hygeine to clean up\nafter ourselves.\n\nSigned-off-by: Jeff King \n---\n builtin/cat-file.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex ce79103..c64e287 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -264,6 +264,7 @@ static int batch_objects(struct batch_options *opt)\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct expand_data data;\n \tint retval = 0;\n+\tint save_warning = warn_on_object_refname_ambiguity;\n \n \tif (!opt->format)\n \t\topt->format = \"%(objectname) %(objecttype) %(objectsize)\";\n@@ -314,6 +315,7 @@ static int batch_objects(struct batch_options *opt)\n \t\t\tbreak;\n \t}\n \n+\twarn_on_object_refname_ambiguity = save_warning;\n \tstrbuf_release(&buf);\n \treturn retval;\n }\n-- \n1.8.5.2.500.g8060133\n"},{"id":"232872","messageId":"20140107221118.GD10161@sigill.intra.peff.net","threadId":"35621","inReplyTo":"20140107220856.GA10074@sigill.intra.peff.net","subject":"[PATCH 4/4] revision: turn off object/refname ambiguity check for --stdin","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-07T22:11:18Z","receivedAt":"2014-01-07T22:11:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We currently check that any 40-hex object name we receive is\nnot also a refname, and output a warning if this is the\ncase. When \"rev-list --stdin\" is used to receive object\nnames, we may receive a large number of inputs, and the cost\nof checking each name as a ref is relatively high.\n\nCommit 25fba78d already dropped this warning for \"cat-file\n--batch-check\". The same reasoning applies for \"rev-list\n--stdin\". Let's disable the check in that instance.\n\nHere are before and after numbers:\n\n $ git rev-list --all >commits\n\n [before]\n $ best-of-five -i commits ./git rev-list --stdin --no-walk --pretty=raw\n\n real 0m0.675s\n user 0m0.552s\n sys 0m0.120s\n\n [after]\n $ best-of-five -i commits ./git rev-list --stdin --no-walk --pretty=raw\n\n real 0m0.415s\n user 0m0.400s\n sys 0m0.012s\n\nSigned-off-by: Jeff King \n---\nObviously we drop this one (and revert 25fba78d) if I can just make the\ncheck faster.\n\n revision.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/revision.c b/revision.c\nindex a68fde6..87d04dd 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1576,7 +1576,9 @@ static void read_revisions_from_stdin(struct rev_info *revs,\n {\n \tstruct strbuf sb;\n \tint seen_dashdash = 0;\n+\tint save_warning = warn_on_object_refname_ambiguity;\n \n+\twarn_on_object_refname_ambiguity = 0;\n \tstrbuf_init(&sb, 1000);\n \twhile (strbuf_getwholeline(&sb, stdin, '\\n') != EOF) {\n \t\tint len = sb.len;\n@@ -1595,6 +1597,7 @@ static void read_revisions_from_stdin(struct rev_info *revs,\n \t\t\t\t\tREVARG_CANNOT_BE_FILENAME))\n \t\t\tdie(\"bad revision '%s'\", sb.buf);\n \t}\n+\twarn_on_object_refname_ambiguity = save_warning;\n \tif (seen_dashdash)\n \t\tread_pathspec_from_stdin(revs, &sb, prune);\n \tstrbuf_release(&sb);\n-- \n1.8.5.2.500.g8060133\n"},{"id":"232882","messageId":"20140107235631.GA10503@sigill.intra.peff.net","threadId":"35621","inReplyTo":"20140107220856.GA10074@sigill.intra.peff.net","subject":"[PATCH v2] speeding up 40-hex ambiguity check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-07T23:56:31Z","receivedAt":"2014-01-07T23:56:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 07, 2014 at 05:08:56PM -0500, Jeff King wrote:\n\n> > OK. I agree with that line of thinking. Let's take it one step at\n> > a time, i.e. do c. and also use warn_on_object_refname_ambiguity in\n> > \"rev-list --stdin\" first and leave the simplification (i.e. b.) for\n> > later.\n> \n> Here's a series to do that. The first three are just cleanups I noticed\n> while looking at the problem.\n> \n> While I was writing the commit messages, though, I had a thought. Maybe\n> we could simply do the check faster for the common case that most refs\n> do not look like object names? Right now we blindly call dwim_ref for\n> each get_sha1 call, which is the expensive part. If we instead just\n> loaded all of the refnames from the dwim_ref location (basically heads,\n> tags and the top-level of \"refs/\"), we could build an index of all of\n> the entries matching the 40-hex pattern. In 99% of cases, this would be\n> zero entries, and the check would collapse to a simple integer\n> comparison (and even if we did have one, it would be a simple binary\n> search in memory).\n\nThat turned out very nicely, and I think we can drop the extra flag\nentirely. Brodie's patch still makes sense, for people who do want to\nturn off ambiguity warnings entirely (and I built on his patch, which\nmatters textually for 4 and 5, but it would be easy to rebase).\n\nI'm cc-ing Michael, since it is his ref-traversal code I am butchering\nin the 3rd patch. The first two are the unrelated cleanups from v1. They\nare not necessary, but I do not see any reason not to include them.\n\n [1/5]: cat-file: refactor error handling of batch_objects\n [2/5]: cat-file: fix a minor memory leak in batch_objects\n [3/5]: refs: teach for_each_ref a flag to avoid recursion\n [4/5]: get_sha1: speed up ambiguous 40-hex test\n [5/5]: get_sha1: drop object/refname ambiguity flag\n\n-Peff\n"},{"id":"232883","messageId":"20140107235719.GA10657@sigill.intra.peff.net","threadId":"35621","inReplyTo":"20140107235631.GA10503@sigill.intra.peff.net","subject":"[PATCH v2 1/5] cat-file: refactor error handling of batch_objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-07T23:57:20Z","receivedAt":"2014-01-07T23:57:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This just pulls the return value for the function out of the\ninner loop, so we can break out of the loop rather than do\nan early return. This will make it easier to put any cleanup\nfor the function in one place.\n\nSigned-off-by: Jeff King \n---\n builtin/cat-file.c | 11 +++++------\n 1 file changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex f8288c8..971cdde 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -263,6 +263,7 @@ static int batch_objects(struct batch_options *opt)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct expand_data data;\n+\tint retval = 0;\n \n \tif (!opt->format)\n \t\topt->format = \"%(objectname) %(objecttype) %(objectsize)\";\n@@ -294,8 +295,6 @@ static int batch_objects(struct batch_options *opt)\n \twarn_on_object_refname_ambiguity = 0;\n \n \twhile (strbuf_getline(&buf, stdin, '\\n') != EOF) {\n-\t\tint error;\n-\n \t\tif (data.split_on_whitespace) {\n \t\t\t/*\n \t\t\t * Split at first whitespace, tying off the beginning\n@@ -310,12 +309,12 @@ static int batch_objects(struct batch_options *opt)\n \t\t\tdata.rest = p;\n \t\t}\n \n-\t\terror = batch_one_object(buf.buf, opt, &data);\n-\t\tif (error)\n-\t\t\treturn error;\n+\t\tretval = batch_one_object(buf.buf, opt, &data);\n+\t\tif (retval)\n+\t\t\tbreak;\n \t}\n \n-\treturn 0;\n+\treturn retval;\n }\n \n static const char * const cat_file_usage[] = {\n-- \n1.8.5.2.500.g8060133\n"},{"id":"232884","messageId":"20140107235725.GB10657@sigill.intra.peff.net","threadId":"35621","inReplyTo":"20140107235631.GA10503@sigill.intra.peff.net","subject":"[PATCH v2 2/5] cat-file: fix a minor memory leak in batch_objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-07T23:57:26Z","receivedAt":"2014-01-07T23:57:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We should always have been freeing our strbuf, but doing so\nconsistently was annoying until the refactoring in the\nprevious patch.\n\nSigned-off-by: Jeff King \n---\n builtin/cat-file.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 971cdde..ce79103 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -314,6 +314,7 @@ static int batch_objects(struct batch_options *opt)\n \t\t\tbreak;\n \t}\n \n+\tstrbuf_release(&buf);\n \treturn retval;\n }\n \n-- \n1.8.5.2.500.g8060133\n"},{"id":"232885","messageId":"20140107235850.GC10657@sigill.intra.peff.net","threadId":"35621","inReplyTo":"20140107235631.GA10503@sigill.intra.peff.net","subject":"[PATCH v2 3/5] refs: teach for_each_ref a flag to avoid recursion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-07T23:58:50Z","receivedAt":"2014-01-07T23:58:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The normal for_each_ref traversal descends into\nsubdirectories, returning each ref it finds. However, in\nsome cases we may want to just iterate over the top-level of\na certain part of the tree.\n\nThe introduction of the \"flags\" option is a little\nmysterious. We already have a \"flags\" option that gets stuck\nin a callback struct and ends up interpreted in do_one_ref.\nBut the traversal itself does not currently have any flags,\nand it needs to know about this new flag.\n\nWe _could_ introduce this as a completely separate flag\nparameter. But instead, we simply put both flag types into a\nsingle namespace, and make it available at both sites. This\nis simple, and given that we do not have a proliferation of\nflags (we have had exactly one until now), it is probably\nsufficient.\n\nSigned-off-by: Jeff King \n---\nI think the flags thing is OK as explained above, but Michael may have a\ndifferent suggestion for refactoring.\n\n refs.c | 61 ++++++++++++++++++++++++++++++++++++++-----------------------\n 1 file changed, 38 insertions(+), 23 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 3926136..ca854d6 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -589,6 +589,8 @@ static void sort_ref_dir(struct ref_dir *dir)\n \n /* Include broken references in a do_for_each_ref*() iteration: */\n #define DO_FOR_EACH_INCLUDE_BROKEN 0x01\n+/* Do not recurse into subdirs, just iterate at a single level. */\n+#define DO_FOR_EACH_NO_RECURSE 0x02\n \n /*\n * Return true iff the reference described by entry can be resolved to\n@@ -661,7 +663,8 @@ static int do_one_ref(struct ref_entry *entry, void *cb_data)\n * called for all references, including broken ones.\n */\n static int do_for_each_entry_in_dir(struct ref_dir *dir, int offset,\n-\t\t\t\t each_ref_entry_fn fn, void *cb_data)\n+\t\t\t\t each_ref_entry_fn fn, void *cb_data,\n+\t\t\t\t int flags)\n {\n \tint i;\n \tassert(dir->sorted == dir->nr);\n@@ -669,9 +672,13 @@ static int do_for_each_entry_in_dir(struct ref_dir *dir, int offset,\n \t\tstruct ref_entry *entry = dir->entries[i];\n \t\tint retval;\n \t\tif (entry->flag & REF_DIR) {\n-\t\t\tstruct ref_dir *subdir = get_ref_dir(entry);\n-\t\t\tsort_ref_dir(subdir);\n-\t\t\tretval = do_for_each_entry_in_dir(subdir, 0, fn, cb_data);\n+\t\t\tif (flags & DO_FOR_EACH_NO_RECURSE) {\n+\t\t\t\tstruct ref_dir *subdir = get_ref_dir(entry);\n+\t\t\t\tsort_ref_dir(subdir);\n+\t\t\t\tretval = do_for_each_entry_in_dir(subdir, 0,\n+\t\t\t\t\t\t\t\t fn, cb_data,\n+\t\t\t\t\t\t\t\t flags);\n+\t\t\t}\n \t\t} else {\n \t\t\tretval = fn(entry, cb_data);\n \t\t}\n@@ -691,7 +698,8 @@ static int do_for_each_entry_in_dir(struct ref_dir *dir, int offset,\n */\n static int do_for_each_entry_in_dirs(struct ref_dir *dir1,\n \t\t\t\t struct ref_dir *dir2,\n-\t\t\t\t each_ref_entry_fn fn, void *cb_data)\n+\t\t\t\t each_ref_entry_fn fn, void *cb_data,\n+\t\t\t\t int flags)\n {\n \tint retval;\n \tint i1 = 0, i2 = 0;\n@@ -702,10 +710,12 @@ static int do_for_each_entry_in_dirs(struct ref_dir *dir1,\n \t\tstruct ref_entry *e1, *e2;\n \t\tint cmp;\n \t\tif (i1 == dir1->nr) {\n-\t\t\treturn do_for_each_entry_in_dir(dir2, i2, fn, cb_data);\n+\t\t\treturn do_for_each_entry_in_dir(dir2, i2, fn, cb_data,\n+\t\t\t\t\t\t\tflags);\n \t\t}\n \t\tif (i2 == dir2->nr) {\n-\t\t\treturn do_for_each_entry_in_dir(dir1, i1, fn, cb_data);\n+\t\t\treturn do_for_each_entry_in_dir(dir1, i1, fn, cb_data,\n+\t\t\t\t\t\t\tflags);\n \t\t}\n \t\te1 = dir1->entries[i1];\n \t\te2 = dir2->entries[i2];\n@@ -713,12 +723,15 @@ static int do_for_each_entry_in_dirs(struct ref_dir *dir1,\n \t\tif (cmp == 0) {\n \t\t\tif ((e1->flag & REF_DIR) && (e2->flag & REF_DIR)) {\n \t\t\t\t/* Both are directories; descend them in parallel. */\n-\t\t\t\tstruct ref_dir *subdir1 = get_ref_dir(e1);\n-\t\t\t\tstruct ref_dir *subdir2 = get_ref_dir(e2);\n-\t\t\t\tsort_ref_dir(subdir1);\n-\t\t\t\tsort_ref_dir(subdir2);\n-\t\t\t\tretval = do_for_each_entry_in_dirs(\n-\t\t\t\t\t\tsubdir1, subdir2, fn, cb_data);\n+\t\t\t\tif (!(flags & DO_FOR_EACH_NO_RECURSE)) {\n+\t\t\t\t\tstruct ref_dir *subdir1 = get_ref_dir(e1);\n+\t\t\t\t\tstruct ref_dir *subdir2 = get_ref_dir(e2);\n+\t\t\t\t\tsort_ref_dir(subdir1);\n+\t\t\t\t\tsort_ref_dir(subdir2);\n+\t\t\t\t\tretval = do_for_each_entry_in_dirs(\n+\t\t\t\t\t\t\tsubdir1, subdir2,\n+\t\t\t\t\t\t\tfn, cb_data, flags);\n+\t\t\t\t}\n \t\t\t\ti1++;\n \t\t\t\ti2++;\n \t\t\t} else if (!(e1->flag & REF_DIR) && !(e2->flag & REF_DIR)) {\n@@ -743,7 +756,7 @@ static int do_for_each_entry_in_dirs(struct ref_dir *dir1,\n \t\t\t\tstruct ref_dir *subdir = get_ref_dir(e);\n \t\t\t\tsort_ref_dir(subdir);\n \t\t\t\tretval = do_for_each_entry_in_dir(\n-\t\t\t\t\t\tsubdir, 0, fn, cb_data);\n+\t\t\t\t\t\tsubdir, 0, fn, cb_data, flags);\n \t\t\t} else {\n \t\t\t\tretval = fn(e, cb_data);\n \t\t\t}\n@@ -817,7 +830,7 @@ static int is_refname_available(const char *refname, const char *oldrefname,\n \tdata.conflicting_refname = NULL;\n \n \tsort_ref_dir(dir);\n-\tif (do_for_each_entry_in_dir(dir, 0, name_conflict_fn, &data)) {\n+\tif (do_for_each_entry_in_dir(dir, 0, name_conflict_fn, &data, 0)) {\n \t\terror(\"'%s' exists; cannot create '%s'\",\n \t\t data.conflicting_refname, refname);\n \t\treturn 0;\n@@ -1651,7 +1664,8 @@ void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname)\n * 0.\n */\n static int do_for_each_entry(struct ref_cache *refs, const char *base,\n-\t\t\t each_ref_entry_fn fn, void *cb_data)\n+\t\t\t each_ref_entry_fn fn, void *cb_data,\n+\t\t\t int flags)\n {\n \tstruct packed_ref_cache *packed_ref_cache;\n \tstruct ref_dir *loose_dir;\n@@ -1684,15 +1698,15 @@ static int do_for_each_entry(struct ref_cache *refs, const char *base,\n \t\tsort_ref_dir(packed_dir);\n \t\tsort_ref_dir(loose_dir);\n \t\tretval = do_for_each_entry_in_dirs(\n-\t\t\t\tpacked_dir, loose_dir, fn, cb_data);\n+\t\t\t\tpacked_dir, loose_dir, fn, cb_data, flags);\n \t} else if (packed_dir) {\n \t\tsort_ref_dir(packed_dir);\n \t\tretval = do_for_each_entry_in_dir(\n-\t\t\t\tpacked_dir, 0, fn, cb_data);\n+\t\t\t\tpacked_dir, 0, fn, cb_data, flags);\n \t} else if (loose_dir) {\n \t\tsort_ref_dir(loose_dir);\n \t\tretval = do_for_each_entry_in_dir(\n-\t\t\t\tloose_dir, 0, fn, cb_data);\n+\t\t\t\tloose_dir, 0, fn, cb_data, flags);\n \t}\n \n \trelease_packed_ref_cache(packed_ref_cache);\n@@ -1718,7 +1732,7 @@ static int do_for_each_ref(struct ref_cache *refs, const char *base,\n \tdata.fn = fn;\n \tdata.cb_data = cb_data;\n \n-\treturn do_for_each_entry(refs, base, do_one_ref, &data);\n+\treturn do_for_each_entry(refs, base, do_one_ref, &data, flags);\n }\n \n static int do_head_ref(const char *submodule, each_ref_fn fn, void *cb_data)\n@@ -2200,7 +2214,7 @@ int commit_packed_refs(void)\n \n \tdo_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),\n \t\t\t\t 0, write_packed_entry_fn,\n-\t\t\t\t &packed_ref_cache->lock->fd);\n+\t\t\t\t &packed_ref_cache->lock->fd, 0);\n \tif (commit_lock_file(packed_ref_cache->lock))\n \t\terror = -1;\n \tpacked_ref_cache->lock = NULL;\n@@ -2345,7 +2359,7 @@ int pack_refs(unsigned int flags)\n \tcbdata.packed_refs = get_packed_refs(&ref_cache);\n \n \tdo_for_each_entry_in_dir(get_loose_refs(&ref_cache), 0,\n-\t\t\t\t pack_if_possible_fn, &cbdata);\n+\t\t\t\t pack_if_possible_fn, &cbdata, 0);\n \n \tif (commit_packed_refs())\n \t\tdie_errno(\"unable to overwrite old ref-pack file\");\n@@ -2447,7 +2461,8 @@ static int repack_without_refs(const char **refnames, int n)\n \t}\n \n \t/* Remove any other accumulated cruft */\n-\tdo_for_each_entry_in_dir(packed, 0, curate_packed_ref_fn, &refs_to_delete);\n+\tdo_for_each_entry_in_dir(packed, 0, curate_packed_ref_fn,\n+\t\t\t\t &refs_to_delete, 0);\n \tfor_each_string_list_item(ref_to_delete, &refs_to_delete) {\n \t\tif (remove_entry(packed, ref_to_delete->string) == -1)\n \t\t\tdie(\"internal error\");\n-- \n1.8.5.2.500.g8060133\n"},{"id":"232886","messageId":"20140107235953.GD10657@sigill.intra.peff.net","threadId":"35621","inReplyTo":"20140107235631.GA10503@sigill.intra.peff.net","subject":"[PATCH v2 4/5] get_sha1: speed up ambiguous 40-hex test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-07T23:59:53Z","receivedAt":"2014-01-07T23:59:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since 798c35f (get_sha1: warn about full or short object\nnames that look like refs, 2013-05-29), a 40-hex sha1 causes\nus to call dwim_ref on the result, on the off chance that we\nhave a matching ref. This can cause a noticeable slow-down\nwhen there are a large number of objects. E.g., on\nlinux.git:\n\n [baseline timing]\n $ best-of-five git rev-list --all --pretty=raw\n real 0m3.996s\n user 0m3.900s\n sys 0m0.100s\n\n [same thing, but calling get_sha1 on each commit from stdin]\n $ git rev-list --all >commits\n $ best-of-five -i commits git rev-list --stdin --pretty=raw\n real 0m7.862s\n user 0m6.108s\n sys 0m1.760s\n\nThe problem is that each call to dwim_ref individually stats\nthe possible refs in refs/heads, refs/tags, etc. In the\ncommon case, there are no refs that look like sha1s at all.\nWe can therefore do the same check much faster by loading\nall ambiguous-looking candidates once, and then checking our\nindex for each object.\n\nThis is technically more racy (somebody might create such a\nref after we build our index), but that's OK, as it's just a\nwarning (and we provide no guarantees about whether a\nsimultaneous process ran before or after the ref was created\nanyway).\n\nHere is the time after this patch, which implements the\nstrategy described above:\n\n $ best-of-five -i commits git rev-list --stdin --pretty=raw\n real 0m4.966s\n user 0m4.776s\n sys 0m0.192s\n\nWe still pay some price to read the commits from stdin, but\nnotice the system time is much lower, as we are avoiding\nhundreds of thousands of stat() calls.\n\nSigned-off-by: Jeff King \n---\nI wanted to make the ref traversal as cheap as possible, hence the\nNO_RECURSE flag I added. I thought INCLUDE_BROKEN used to not open up\nthe refs at all, but it looks like it does these days. I wonder if that\nis worth changing or not.\n\n refs.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++\n refs.h | 2 ++\n sha1_name.c | 4 +---\n 3 files changed, 50 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex ca854d6..cddd871 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -4,6 +4,7 @@\n #include \"tag.h\"\n #include \"dir.h\"\n #include \"string-list.h\"\n+#include \"sha1-array.h\"\n \n /*\n * Make sure \"ref\" is something reasonable to have under \".git/refs/\";\n@@ -2042,6 +2043,52 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)\n \treturn logs_found;\n }\n \n+static int check_ambiguous_sha1_ref(const char *refname,\n+\t\t\t\t const unsigned char *sha1,\n+\t\t\t\t int flags,\n+\t\t\t\t void *data)\n+{\n+\tunsigned char tmp_sha1[20];\n+\tif (strlen(refname) == 40 && !get_sha1_hex(refname, tmp_sha1))\n+\t\tsha1_array_append(data, tmp_sha1);\n+\treturn 0;\n+}\n+\n+static void build_ambiguous_sha1_ref_index(struct sha1_array *idx)\n+{\n+\tconst char **rule;\n+\n+\tfor (rule = ref_rev_parse_rules; *rule; rule++) {\n+\t\tconst char *prefix = *rule;\n+\t\tconst char *end = strstr(prefix, \"%.*s\");\n+\t\tchar *buf;\n+\n+\t\tif (!end)\n+\t\t\tcontinue;\n+\n+\t\tbuf = xmemdupz(prefix, end - prefix);\n+\t\tdo_for_each_ref(&ref_cache, buf, check_ambiguous_sha1_ref,\n+\t\t\t\tend - prefix,\n+\t\t\t\tDO_FOR_EACH_INCLUDE_BROKEN |\n+\t\t\t\tDO_FOR_EACH_NO_RECURSE,\n+\t\t\t\tidx);\n+\t\tfree(buf);\n+\t}\n+}\n+\n+int sha1_is_ambiguous_with_ref(const unsigned char *sha1)\n+{\n+\tstruct sha1_array idx = SHA1_ARRAY_INIT;\n+\tstatic int loaded;\n+\n+\tif (!loaded) {\n+\t\tbuild_ambiguous_sha1_ref_index(&idx);\n+\t\tloaded = 1;\n+\t}\n+\n+\treturn sha1_array_lookup(&idx, sha1) >= 0;\n+}\n+\n static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\t\t\t const unsigned char *old_sha1,\n \t\t\t\t\t int flags, int *type_p)\ndiff --git a/refs.h b/refs.h\nindex 87a1a79..c7d5f89 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -229,4 +229,6 @@ int update_refs(const char *action, const struct ref_update **updates,\n extern int parse_hide_refs_config(const char *var, const char *value, const char *);\n extern int ref_is_hidden(const char *);\n \n+int sha1_is_ambiguous_with_ref(const unsigned char *sha1);\n+\n #endif /* REFS_H */\ndiff --git a/sha1_name.c b/sha1_name.c\nindex a5578f7..f83ecb7 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -452,13 +452,11 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \n \tif (len == 40 && !get_sha1_hex(str, sha1)) {\n \t\tif (warn_ambiguous_refs && warn_on_object_refname_ambiguity) {\n-\t\t\trefs_found = dwim_ref(str, len, tmp_sha1, &real_ref);\n-\t\t\tif (refs_found > 0) {\n+\t\t\tif (sha1_is_ambiguous_with_ref(sha1)) {\n \t\t\t\twarning(warn_msg, len, str);\n \t\t\t\tif (advice_object_name_warning)\n \t\t\t\t\tfprintf(stderr, \"%s\\n\", _(object_name_msg));\n \t\t\t}\n-\t\t\tfree(real_ref);\n \t\t}\n \t\treturn 0;\n \t}\n-- \n1.8.5.2.500.g8060133\n"},{"id":"232887","messageId":"20140108000009.GE10657@sigill.intra.peff.net","threadId":"35621","inReplyTo":"20140107235631.GA10503@sigill.intra.peff.net","subject":"[PATCH v2 5/5] get_sha1: drop object/refname ambiguity flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-08T00:00:09Z","receivedAt":"2014-01-08T00:00:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Now that our object/refname ambiguity test is much faster\n(thanks to the previous commit), there is no reason for code\nlike \"cat-file --batch-check\" to turn it off. Here are\nbefore and after timings with this patch (on git.git):\n\n $ git rev-list --objects --all | cut -d' ' -f1 >objects\n\n [with flag]\n $ best-of-five -i objects ./git cat-file --batch-check\n real 0m0.392s\n user 0m0.368s\n sys 0m0.024s\n\n [without flag, without speedup; i.e., pre-25fba78]\n $ best-of-five -i objects ./git cat-file --batch-check\n real 0m1.652s\n user 0m0.904s\n sys 0m0.748s\n\n [without flag, with speedup]\n $ best-of-five -i objects ./git cat-file --batch-check\n real 0m0.388s\n user 0m0.356s\n sys 0m0.028s\n\nSo the new implementation does just as well as we did with\nthe flag turning the whole thing off (better actually, but\nthat is within the noise).\n\nSigned-off-by: Jeff King \n---\n builtin/cat-file.c | 9 ---------\n cache.h | 1 -\n environment.c | 1 -\n sha1_name.c | 2 +-\n 4 files changed, 1 insertion(+), 12 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex ce79103..afba21f 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -285,15 +285,6 @@ static int batch_objects(struct batch_options *opt)\n \tif (opt->print_contents)\n \t\tdata.info.typep = &data.type;\n \n-\t/*\n-\t * We are going to call get_sha1 on a potentially very large number of\n-\t * objects. In most large cases, these will be actual object sha1s. The\n-\t * cost to double-check that each one is not also a ref (just so we can\n-\t * warn) ends up dwarfing the actual cost of the object lookups\n-\t * themselves. We can work around it by just turning off the warning.\n-\t */\n-\twarn_on_object_refname_ambiguity = 0;\n-\n \twhile (strbuf_getline(&buf, stdin, '\\n') != EOF) {\n \t\tif (data.split_on_whitespace) {\n \t\t\t/*\ndiff --git a/cache.h b/cache.h\nindex ce377e1..73afc38 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -566,7 +566,6 @@ extern int assume_unchanged;\n extern int prefer_symlink_refs;\n extern int log_all_ref_updates;\n extern int warn_ambiguous_refs;\n-extern int warn_on_object_refname_ambiguity;\n extern int shared_repository;\n extern const char *apply_default_whitespace;\n extern const char *apply_default_ignorewhitespace;\ndiff --git a/environment.c b/environment.c\nindex 3c76905..c59f6d4 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -22,7 +22,6 @@ int prefer_symlink_refs;\n int is_bare_repository_cfg = -1; /* unspecified */\n int log_all_ref_updates = -1; /* unspecified */\n int warn_ambiguous_refs = 1;\n-int warn_on_object_refname_ambiguity = 1;\n int repository_format_version;\n const char *git_commit_encoding;\n const char *git_log_output_encoding;\ndiff --git a/sha1_name.c b/sha1_name.c\nindex f83ecb7..b9aaf74 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -451,7 +451,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \tint at, reflog_len, nth_prior = 0;\n \n \tif (len == 40 && !get_sha1_hex(str, sha1)) {\n-\t\tif (warn_ambiguous_refs && warn_on_object_refname_ambiguity) {\n+\t\tif (warn_ambiguous_refs) {\n \t\t\tif (sha1_is_ambiguous_with_ref(sha1)) {\n \t\t\t\twarning(warn_msg, len, str);\n \t\t\t\tif (advice_object_name_warning)\n-- \n1.8.5.2.500.g8060133\n"},{"id":"232893","messageId":"20140108034733.GA17198@sigill.intra.peff.net","threadId":"35621","inReplyTo":"20140107235850.GC10657@sigill.intra.peff.net","subject":"[PATCH v3 3/5] refs: teach for_each_ref a flag to avoid recursion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-08T03:47:34Z","receivedAt":"2014-01-08T03:47:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 07, 2014 at 06:58:50PM -0500, Jeff King wrote:\n\n> +\t\t\tif (flags & DO_FOR_EACH_NO_RECURSE) {\n> +\t\t\t\tstruct ref_dir *subdir = get_ref_dir(entry);\n> +\t\t\t\tsort_ref_dir(subdir);\n> +\t\t\t\tretval = do_for_each_entry_in_dir(subdir, 0,\n\nObviously this is totally wrong and inverts the point of the flag. And\ncauses something like half of the test suite to fail.\n\nMichael was nice enough to point it out to me off-list, but well, I have\nto face the brown paper bag at some point. :) In my defense, it was a\nlast minute refactor before going to dinner. That is what I get for\nrushing out the series.\n\nHere's a fixed version of patch 3/5.\n\n-- >8 --\nSubject: refs: teach for_each_ref a flag to avoid recursion\n\nThe normal for_each_ref traversal descends into\nsubdirectories, returning each ref it finds. However, in\nsome cases we may want to just iterate over the top-level of\na certain part of the tree.\n\nThe introduction of the \"flags\" option is a little\nmysterious. We already have a \"flags\" option that gets stuck\nin a callback struct and ends up interpreted in do_one_ref.\nBut the traversal itself does not currently have any flags,\nand it needs to know about this new flag.\n\nWe _could_ introduce this as a completely separate flag\nparameter. But instead, we simply put both flag types into a\nsingle namespace, and make it available at both sites. This\nis simple, and given that we do not have a proliferation of\nflags (we have had exactly one until now), it is probably\nsufficient.\n\nSigned-off-by: Jeff King \n---\n refs.c | 61 ++++++++++++++++++++++++++++++++++++++-----------------------\n 1 file changed, 38 insertions(+), 23 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 3926136..b70b018 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -589,6 +589,8 @@ static void sort_ref_dir(struct ref_dir *dir)\n \n /* Include broken references in a do_for_each_ref*() iteration: */\n #define DO_FOR_EACH_INCLUDE_BROKEN 0x01\n+/* Do not recurse into subdirs, just iterate at a single level. */\n+#define DO_FOR_EACH_NO_RECURSE 0x02\n \n /*\n * Return true iff the reference described by entry can be resolved to\n@@ -661,7 +663,8 @@ static int do_one_ref(struct ref_entry *entry, void *cb_data)\n * called for all references, including broken ones.\n */\n static int do_for_each_entry_in_dir(struct ref_dir *dir, int offset,\n-\t\t\t\t each_ref_entry_fn fn, void *cb_data)\n+\t\t\t\t each_ref_entry_fn fn, void *cb_data,\n+\t\t\t\t int flags)\n {\n \tint i;\n \tassert(dir->sorted == dir->nr);\n@@ -669,9 +672,13 @@ static int do_for_each_entry_in_dir(struct ref_dir *dir, int offset,\n \t\tstruct ref_entry *entry = dir->entries[i];\n \t\tint retval;\n \t\tif (entry->flag & REF_DIR) {\n-\t\t\tstruct ref_dir *subdir = get_ref_dir(entry);\n-\t\t\tsort_ref_dir(subdir);\n-\t\t\tretval = do_for_each_entry_in_dir(subdir, 0, fn, cb_data);\n+\t\t\tif (!(flags & DO_FOR_EACH_NO_RECURSE)) {\n+\t\t\t\tstruct ref_dir *subdir = get_ref_dir(entry);\n+\t\t\t\tsort_ref_dir(subdir);\n+\t\t\t\tretval = do_for_each_entry_in_dir(subdir, 0,\n+\t\t\t\t\t\t\t\t fn, cb_data,\n+\t\t\t\t\t\t\t\t flags);\n+\t\t\t}\n \t\t} else {\n \t\t\tretval = fn(entry, cb_data);\n \t\t}\n@@ -691,7 +698,8 @@ static int do_for_each_entry_in_dir(struct ref_dir *dir, int offset,\n */\n static int do_for_each_entry_in_dirs(struct ref_dir *dir1,\n \t\t\t\t struct ref_dir *dir2,\n-\t\t\t\t each_ref_entry_fn fn, void *cb_data)\n+\t\t\t\t each_ref_entry_fn fn, void *cb_data,\n+\t\t\t\t int flags)\n {\n \tint retval;\n \tint i1 = 0, i2 = 0;\n@@ -702,10 +710,12 @@ static int do_for_each_entry_in_dirs(struct ref_dir *dir1,\n \t\tstruct ref_entry *e1, *e2;\n \t\tint cmp;\n \t\tif (i1 == dir1->nr) {\n-\t\t\treturn do_for_each_entry_in_dir(dir2, i2, fn, cb_data);\n+\t\t\treturn do_for_each_entry_in_dir(dir2, i2, fn, cb_data,\n+\t\t\t\t\t\t\tflags);\n \t\t}\n \t\tif (i2 == dir2->nr) {\n-\t\t\treturn do_for_each_entry_in_dir(dir1, i1, fn, cb_data);\n+\t\t\treturn do_for_each_entry_in_dir(dir1, i1, fn, cb_data,\n+\t\t\t\t\t\t\tflags);\n \t\t}\n \t\te1 = dir1->entries[i1];\n \t\te2 = dir2->entries[i2];\n@@ -713,12 +723,15 @@ static int do_for_each_entry_in_dirs(struct ref_dir *dir1,\n \t\tif (cmp == 0) {\n \t\t\tif ((e1->flag & REF_DIR) && (e2->flag & REF_DIR)) {\n \t\t\t\t/* Both are directories; descend them in parallel. */\n-\t\t\t\tstruct ref_dir *subdir1 = get_ref_dir(e1);\n-\t\t\t\tstruct ref_dir *subdir2 = get_ref_dir(e2);\n-\t\t\t\tsort_ref_dir(subdir1);\n-\t\t\t\tsort_ref_dir(subdir2);\n-\t\t\t\tretval = do_for_each_entry_in_dirs(\n-\t\t\t\t\t\tsubdir1, subdir2, fn, cb_data);\n+\t\t\t\tif (!(flags & DO_FOR_EACH_NO_RECURSE)) {\n+\t\t\t\t\tstruct ref_dir *subdir1 = get_ref_dir(e1);\n+\t\t\t\t\tstruct ref_dir *subdir2 = get_ref_dir(e2);\n+\t\t\t\t\tsort_ref_dir(subdir1);\n+\t\t\t\t\tsort_ref_dir(subdir2);\n+\t\t\t\t\tretval = do_for_each_entry_in_dirs(\n+\t\t\t\t\t\t\tsubdir1, subdir2,\n+\t\t\t\t\t\t\tfn, cb_data, flags);\n+\t\t\t\t}\n \t\t\t\ti1++;\n \t\t\t\ti2++;\n \t\t\t} else if (!(e1->flag & REF_DIR) && !(e2->flag & REF_DIR)) {\n@@ -743,7 +756,7 @@ static int do_for_each_entry_in_dirs(struct ref_dir *dir1,\n \t\t\t\tstruct ref_dir *subdir = get_ref_dir(e);\n \t\t\t\tsort_ref_dir(subdir);\n \t\t\t\tretval = do_for_each_entry_in_dir(\n-\t\t\t\t\t\tsubdir, 0, fn, cb_data);\n+\t\t\t\t\t\tsubdir, 0, fn, cb_data, flags);\n \t\t\t} else {\n \t\t\t\tretval = fn(e, cb_data);\n \t\t\t}\n@@ -817,7 +830,7 @@ static int is_refname_available(const char *refname, const char *oldrefname,\n \tdata.conflicting_refname = NULL;\n \n \tsort_ref_dir(dir);\n-\tif (do_for_each_entry_in_dir(dir, 0, name_conflict_fn, &data)) {\n+\tif (do_for_each_entry_in_dir(dir, 0, name_conflict_fn, &data, 0)) {\n \t\terror(\"'%s' exists; cannot create '%s'\",\n \t\t data.conflicting_refname, refname);\n \t\treturn 0;\n@@ -1651,7 +1664,8 @@ void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname)\n * 0.\n */\n static int do_for_each_entry(struct ref_cache *refs, const char *base,\n-\t\t\t each_ref_entry_fn fn, void *cb_data)\n+\t\t\t each_ref_entry_fn fn, void *cb_data,\n+\t\t\t int flags)\n {\n \tstruct packed_ref_cache *packed_ref_cache;\n \tstruct ref_dir *loose_dir;\n@@ -1684,15 +1698,15 @@ static int do_for_each_entry(struct ref_cache *refs, const char *base,\n \t\tsort_ref_dir(packed_dir);\n \t\tsort_ref_dir(loose_dir);\n \t\tretval = do_for_each_entry_in_dirs(\n-\t\t\t\tpacked_dir, loose_dir, fn, cb_data);\n+\t\t\t\tpacked_dir, loose_dir, fn, cb_data, flags);\n \t} else if (packed_dir) {\n \t\tsort_ref_dir(packed_dir);\n \t\tretval = do_for_each_entry_in_dir(\n-\t\t\t\tpacked_dir, 0, fn, cb_data);\n+\t\t\t\tpacked_dir, 0, fn, cb_data, flags);\n \t} else if (loose_dir) {\n \t\tsort_ref_dir(loose_dir);\n \t\tretval = do_for_each_entry_in_dir(\n-\t\t\t\tloose_dir, 0, fn, cb_data);\n+\t\t\t\tloose_dir, 0, fn, cb_data, flags);\n \t}\n \n \trelease_packed_ref_cache(packed_ref_cache);\n@@ -1718,7 +1732,7 @@ static int do_for_each_ref(struct ref_cache *refs, const char *base,\n \tdata.fn = fn;\n \tdata.cb_data = cb_data;\n \n-\treturn do_for_each_entry(refs, base, do_one_ref, &data);\n+\treturn do_for_each_entry(refs, base, do_one_ref, &data, flags);\n }\n \n static int do_head_ref(const char *submodule, each_ref_fn fn, void *cb_data)\n@@ -2200,7 +2214,7 @@ int commit_packed_refs(void)\n \n \tdo_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),\n \t\t\t\t 0, write_packed_entry_fn,\n-\t\t\t\t &packed_ref_cache->lock->fd);\n+\t\t\t\t &packed_ref_cache->lock->fd, 0);\n \tif (commit_lock_file(packed_ref_cache->lock))\n \t\terror = -1;\n \tpacked_ref_cache->lock = NULL;\n@@ -2345,7 +2359,7 @@ int pack_refs(unsigned int flags)\n \tcbdata.packed_refs = get_packed_refs(&ref_cache);\n \n \tdo_for_each_entry_in_dir(get_loose_refs(&ref_cache), 0,\n-\t\t\t\t pack_if_possible_fn, &cbdata);\n+\t\t\t\t pack_if_possible_fn, &cbdata, 0);\n \n \tif (commit_packed_refs())\n \t\tdie_errno(\"unable to overwrite old ref-pack file\");\n@@ -2447,7 +2461,8 @@ static int repack_without_refs(const char **refnames, int n)\n \t}\n \n \t/* Remove any other accumulated cruft */\n-\tdo_for_each_entry_in_dir(packed, 0, curate_packed_ref_fn, &refs_to_delete);\n+\tdo_for_each_entry_in_dir(packed, 0, curate_packed_ref_fn,\n+\t\t\t\t &refs_to_delete, 0);\n \tfor_each_string_list_item(ref_to_delete, &refs_to_delete) {\n \t\tif (remove_entry(packed, ref_to_delete->string) == -1)\n \t\t\tdie(\"internal error\");\n-- \n1.8.5.2.500.g8060133\n"},{"id":"232904","messageId":"20140108102348.GA30092@sigill.intra.peff.net","threadId":"35621","inReplyTo":"20140108034733.GA17198@sigill.intra.peff.net","subject":"Re: [PATCH v3 3/5] refs: teach for_each_ref a flag to avoid recursion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-08T10:23:48Z","receivedAt":"2014-01-08T10:23:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 07, 2014 at 10:47:33PM -0500, Jeff King wrote:\n\n> On Tue, Jan 07, 2014 at 06:58:50PM -0500, Jeff King wrote:\n> \n> > +\t\t\tif (flags & DO_FOR_EACH_NO_RECURSE) {\n> > +\t\t\t\tstruct ref_dir *subdir = get_ref_dir(entry);\n> > +\t\t\t\tsort_ref_dir(subdir);\n> > +\t\t\t\tretval = do_for_each_entry_in_dir(subdir, 0,\n> \n> Obviously this is totally wrong and inverts the point of the flag. And\n> causes something like half of the test suite to fail.\n\nAnd while we're on the subject of my mistakes...\n\nThe patch needs the fixup below to ensure that retval is always set,\neven when we do not recurse.\n\nI'll hold off on sending a full re-roll of the patch, in the extremely\nunlikely event that there are other small errors to be fixed. :)\n\ndiff --git a/refs.c b/refs.c\nindex aafbae9..99c72d0 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -679,7 +679,8 @@ static int do_for_each_entry_in_dir(struct ref_dir *dir, int offset,\n \t\t\t\tretval = do_for_each_entry_in_dir(subdir, 0,\n \t\t\t\t\t\t\t\t fn, cb_data,\n \t\t\t\t\t\t\t\t flags);\n-\t\t\t}\n+\t\t\t} else\n+\t\t\t\tretval = 0;\n \t\t} else {\n \t\t\tretval = fn(entry, cb_data);\n \t\t}\n@@ -732,7 +733,8 @@ static int do_for_each_entry_in_dirs(struct ref_dir *dir1,\n \t\t\t\t\tretval = do_for_each_entry_in_dirs(\n \t\t\t\t\t\t\tsubdir1, subdir2,\n \t\t\t\t\t\t\tfn, cb_data, flags);\n-\t\t\t\t}\n+\t\t\t\t} else\n+\t\t\t\t\tretval = 0;\n \t\t\t\ti1++;\n \t\t\t\ti2++;\n \t\t\t} else if (!(e1->flag & REF_DIR) && !(e2->flag & REF_DIR)) {\n"},{"id":"232909","messageId":"52CD36AF.2080705@alum.mit.edu","threadId":"35621","inReplyTo":"20140108034733.GA17198@sigill.intra.peff.net","subject":"Re: [PATCH v3 3/5] refs: teach for_each_ref a flag to avoid recursion","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-08T11:29:51Z","receivedAt":"2014-01-08T11:29:51Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 01/08/2014 04:47 AM, Jeff King wrote:\n> On Tue, Jan 07, 2014 at 06:58:50PM -0500, Jeff King wrote:\n> \n>> +\t\t\tif (flags & DO_FOR_EACH_NO_RECURSE) {\n>> +\t\t\t\tstruct ref_dir *subdir = get_ref_dir(entry);\n>> +\t\t\t\tsort_ref_dir(subdir);\n>> +\t\t\t\tretval = do_for_each_entry_in_dir(subdir, 0,\n> \n> Obviously this is totally wrong and inverts the point of the flag. And\n> causes something like half of the test suite to fail.\n> \n> Michael was nice enough to point it out to me off-list, but well, I have\n> to face the brown paper bag at some point. :) In my defense, it was a\n> last minute refactor before going to dinner. That is what I get for\n> rushing out the series.\n> \n> Here's a fixed version of patch 3/5.\n\nv2 4/5 doesn't apply cleanly on top of v3 3/5. So I'm basing my review\non the branch you have at GitHub peff/git \"jk/cat-file-warn-ambiguous\";\nI hope it is the same.\n\n> -- >8 --\n> Subject: refs: teach for_each_ref a flag to avoid recursion\n> \n> The normal for_each_ref traversal descends into\n\nYou haven't changed any for_each_ref*() functions; you have only exposed\nthe DO_FOR_EACH_NO_RECURSE option to the (static) functions\nfor_each_entry*() and do_for_each_ref(). (This is part and parcel of\nyour decision not to expose the new functionality in the refs API.)\nPlease correct the line above.\n\n> subdirectories, returning each ref it finds. However, in\n> some cases we may want to just iterate over the top-level of\n> a certain part of the tree.\n> \n> The introduction of the \"flags\" option is a little\n> mysterious. We already have a \"flags\" option that gets stuck\n> in a callback struct and ends up interpreted in do_one_ref.\n> But the traversal itself does not currently have any flags,\n> and it needs to know about this new flag.\n> \n> We _could_ introduce this as a completely separate flag\n> parameter. But instead, we simply put both flag types into a\n> single namespace, and make it available at both sites. This\n> is simple, and given that we do not have a proliferation of\n> flags (we have had exactly one until now), it is probably\n> sufficient.\n> \n> Signed-off-by: Jeff King \n> ---\n> refs.c | 61 ++++++++++++++++++++++++++++++++++++++-----------------------\n> 1 file changed, 38 insertions(+), 23 deletions(-)\n> \n> diff --git a/refs.c b/refs.c\n> index 3926136..b70b018 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -589,6 +589,8 @@ static void sort_ref_dir(struct ref_dir *dir)\n> \n> /* Include broken references in a do_for_each_ref*() iteration: */\n> #define DO_FOR_EACH_INCLUDE_BROKEN 0x01\n> +/* Do not recurse into subdirs, just iterate at a single level. */\n> +#define DO_FOR_EACH_NO_RECURSE 0x02\n> \n> /*\n> * Return true iff the reference described by entry can be resolved to\n> @@ -661,7 +663,8 @@ static int do_one_ref(struct ref_entry *entry, void *cb_data)\n> * called for all references, including broken ones.\n> */\n> static int do_for_each_entry_in_dir(struct ref_dir *dir, int offset,\n> -\t\t\t\t each_ref_entry_fn fn, void *cb_data)\n> +\t\t\t\t each_ref_entry_fn fn, void *cb_data,\n> +\t\t\t\t int flags)\n> {\n> \tint i;\n> \tassert(dir->sorted == dir->nr);\n\nPlease update the docstring for this function, which still says that it\nrecurses without mentioning DO_FOR_EACH_NO_RECURSE.\n\n> [...]\n> @@ -817,7 +830,7 @@ static int is_refname_available(const char *refname, const char *oldrefname,\n> \tdata.conflicting_refname = NULL;\n> \n> \tsort_ref_dir(dir);\n> -\tif (do_for_each_entry_in_dir(dir, 0, name_conflict_fn, &data)) {\n> +\tif (do_for_each_entry_in_dir(dir, 0, name_conflict_fn, &data, 0)) {\n> \t\terror(\"'%s' exists; cannot create '%s'\",\n> \t\t data.conflicting_refname, refname);\n> \t\treturn 0;\n> @@ -1651,7 +1664,8 @@ void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname)\n> * 0.\n> */\n> static int do_for_each_entry(struct ref_cache *refs, const char *base,\n> -\t\t\t each_ref_entry_fn fn, void *cb_data)\n> +\t\t\t each_ref_entry_fn fn, void *cb_data,\n> +\t\t\t int flags)\n> {\n> \tstruct packed_ref_cache *packed_ref_cache;\n> \tstruct ref_dir *loose_dir;\n\nA few lines after this, do_for_each_entry() calls\nprime_ref_dir(loose_dir) to ensure that all of the loose references that\nwill be iterated over are read before the packed-refs file is checked.\nIt seems to me that prime_ref_dir() should also get a flags parameter to\nprevent it reading more loose references than necessary, something like\nthis:\n\n====================================================================\ndiff --git a/refs.c b/refs.c\nindex b70b018..b8b7354 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -772,13 +772,13 @@ static int do_for_each_entry_in_dirs(struct\nref_dir *dir1,\n * through all of the sub-directories. We do not even need to care about\n * sorting, as traversal order does not matter to us.\n */\n-static void prime_ref_dir(struct ref_dir *dir)\n+static void prime_ref_dir(struct ref_dir *dir, int flags)\n {\n \tint i;\n \tfor (i = 0; i < dir->nr; i++) {\n \t\tstruct ref_entry *entry = dir->entries[i];\n-\t\tif (entry->flag & REF_DIR)\n-\t\t\tprime_ref_dir(get_ref_dir(entry));\n+\t\tif (entry->flag & REF_DIR && !(flags & DO_FOR_EACH_NO_RECURSE))\n+\t\t\tprime_ref_dir(get_ref_dir(entry), flags);\n \t}\n }\n /*\n@@ -1685,7 +1685,7 @@ static int do_for_each_entry(struct ref_cache\n*refs, const char *base,\n \t\tloose_dir = find_containing_dir(loose_dir, base, 0);\n \t}\n \tif (loose_dir)\n-\t\tprime_ref_dir(loose_dir);\n+\t\tprime_ref_dir(loose_dir, flags);\n\n \tpacked_ref_cache = get_packed_ref_cache(refs);\n \tacquire_packed_ref_cache(packed_ref_cache);\n\n====================================================================\n\n> [...]\n> @@ -1718,7 +1732,7 @@ static int do_for_each_ref(struct ref_cache *refs, const char *base,\n> \tdata.fn = fn;\n> \tdata.cb_data = cb_data;\n> \n> -\treturn do_for_each_entry(refs, base, do_one_ref, &data);\n> +\treturn do_for_each_entry(refs, base, do_one_ref, &data, flags);\n> }\n> \n> static int do_head_ref(const char *submodule, each_ref_fn fn, void *cb_data)\n\nThis change makes the DO_FOR_EACH_NO_RECURSE option usable with\ndo_for_each_ref() (even though it is never in fact used). It should\neither be mentioned in the docstring or (if there is a reason not to\nallow it) explicitly prohibited.\n\n> [...]\n\nThe rest looks fine to me.\n\nIt would be possible to use your new flag to speed up\nis_refname_available(), but it would be a little bit of work and I doubt\nthat is_refname_available() is ever a bottleneck.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"232917","messageId":"52CD7835.2020708@alum.mit.edu","threadId":"35621","inReplyTo":"20140107235953.GD10657@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/5] get_sha1: speed up ambiguous 40-hex test","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-08T16:09:25Z","receivedAt":"2014-01-08T16:09:25Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 01/08/2014 12:59 AM, Jeff King wrote:\n> Since 798c35f (get_sha1: warn about full or short object\n> names that look like refs, 2013-05-29), a 40-hex sha1 causes\n> us to call dwim_ref on the result, on the off chance that we\n> have a matching ref. This can cause a noticeable slow-down\n> when there are a large number of objects. E.g., on\n> linux.git:\n> \n> [baseline timing]\n> $ best-of-five git rev-list --all --pretty=raw\n> real 0m3.996s\n> user 0m3.900s\n> sys 0m0.100s\n> \n> [same thing, but calling get_sha1 on each commit from stdin]\n> $ git rev-list --all >commits\n> $ best-of-five -i commits git rev-list --stdin --pretty=raw\n> real 0m7.862s\n> user 0m6.108s\n> sys 0m1.760s\n> \n> The problem is that each call to dwim_ref individually stats\n> the possible refs in refs/heads, refs/tags, etc. In the\n> common case, there are no refs that look like sha1s at all.\n> We can therefore do the same check much faster by loading\n> all ambiguous-looking candidates once, and then checking our\n> index for each object.\n> \n> This is technically more racy (somebody might create such a\n> ref after we build our index), but that's OK, as it's just a\n> warning (and we provide no guarantees about whether a\n> simultaneous process ran before or after the ref was created\n> anyway).\n\nIt's not only racy WRT other processes. If the current git process\nwould create a new reference, it wouldn't be reflected in the cache.\n\nIt's true that the main ref_cache doesn't invalidate itself\nautomatically either when a new reference is created, so it's not really\na fair complaint. However, as we add places where the cache is\ninvalidated, it is easy to overlook this cache that is stuck in static\nvariables within a function definition and it is impossible to\ninvalidate it. Might it not be better to attach the cache to the\nref_cache structure instead, and couple its lifetime to that object?\n\nAlternatively, the cache could be created and managed on the caller\nside, since the caller would know when the cache would have to be\ninvalidated. Also, different callers are likely to have different\nperformance characteristics. It is unlikely that the time to initialize\nthe cache will be amortized in most cases; in fact, \"rev-list --stdin\"\nmight be the *only* plausible use case.\n\nRegarding the overall strategy: you gather all refnames that could be\nconfused with an SHA-1 into a sha1_array, then later look up SHA-1s in\nthe array to see if they are ambiguous. This is a very special-case\noptimization for SHA-1s.\n\nI wonder whether another approach would gain almost the same amount of\nperformance but be more general. We could change dwim_ref() (or a\nversion of it?) to read its data out of a ref_cache instead of going to\ndisk every time. Then, at the cost of populating the relevant parts of\nthe ref_cache once, we would have fast dwim_ref() calls for all strings.\n\nIt's true that the lookups wouldn't be quite so fast--they would require\na few bisects per refname lookup (one for each level in the refname\nhierarchy) and several refname lookups (one for each ref_rev_parse_rule)\nfor every dwim_ref() call, vs. a single bisect in your current design.\nBut this approach it would bring us most of the gain, it might\nnevertheless be preferable.\n\n> Here is the time after this patch, which implements the\n> strategy described above:\n> \n> $ best-of-five -i commits git rev-list --stdin --pretty=raw\n> real 0m4.966s\n> user 0m4.776s\n> sys 0m0.192s\n> \n> We still pay some price to read the commits from stdin, but\n> notice the system time is much lower, as we are avoiding\n> hundreds of thousands of stat() calls.\n> \n> Signed-off-by: Jeff King \n> ---\n> I wanted to make the ref traversal as cheap as possible, hence the\n> NO_RECURSE flag I added. I thought INCLUDE_BROKEN used to not open up\n> the refs at all, but it looks like it does these days. I wonder if that\n> is worth changing or not.\n\nWhat do you mean by \"open up the refs\"? The loose reference files are\nread when populating the cache. (Was that ever different?) But the\ncall to ref_resolves_to_object() in do_one_ref() is skipped when the\nINCLUDE_BROKEN flag is used.\n\n> \n> refs.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++\n> refs.h | 2 ++\n> sha1_name.c | 4 +---\n> 3 files changed, 50 insertions(+), 3 deletions(-)\n> \n> diff --git a/refs.c b/refs.c\n> index ca854d6..cddd871 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -4,6 +4,7 @@\n> #include \"tag.h\"\n> #include \"dir.h\"\n> #include \"string-list.h\"\n> +#include \"sha1-array.h\"\n> \n> /*\n> * Make sure \"ref\" is something reasonable to have under \".git/refs/\";\n> @@ -2042,6 +2043,52 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)\n> \treturn logs_found;\n> }\n> \n> +static int check_ambiguous_sha1_ref(const char *refname,\n> +\t\t\t\t const unsigned char *sha1,\n> +\t\t\t\t int flags,\n> +\t\t\t\t void *data)\n> +{\n> +\tunsigned char tmp_sha1[20];\n> +\tif (strlen(refname) == 40 && !get_sha1_hex(refname, tmp_sha1))\n> +\t\tsha1_array_append(data, tmp_sha1);\n> +\treturn 0;\n> +}\n> +\n> +static void build_ambiguous_sha1_ref_index(struct sha1_array *idx)\n> +{\n> +\tconst char **rule;\n> +\n> +\tfor (rule = ref_rev_parse_rules; *rule; rule++) {\n> +\t\tconst char *prefix = *rule;\n> +\t\tconst char *end = strstr(prefix, \"%.*s\");\n> +\t\tchar *buf;\n> +\n> +\t\tif (!end)\n> +\t\t\tcontinue;\n> +\n> +\t\tbuf = xmemdupz(prefix, end - prefix);\n> +\t\tdo_for_each_ref(&ref_cache, buf, check_ambiguous_sha1_ref,\n> +\t\t\t\tend - prefix,\n> +\t\t\t\tDO_FOR_EACH_INCLUDE_BROKEN |\n> +\t\t\t\tDO_FOR_EACH_NO_RECURSE,\n> +\t\t\t\tidx);\n\nThis doesn't correctly handle the rule\n\n\t\"refs/remotes/%.*s/HEAD\"\n\nWe might be willing to accept this limitation, but it should at least be\nmentioned somewhere. OTOH if we want to handle this pattern as well, we\ncould do use a technique like that of shorten_unambiguous_ref().\n\n> +\t\tfree(buf);\n> +\t}\n> +}\n> +\n> +int sha1_is_ambiguous_with_ref(const unsigned char *sha1)\n> +{\n> +\tstruct sha1_array idx = SHA1_ARRAY_INIT;\n> +\tstatic int loaded;\n> +\n> +\tif (!loaded) {\n> +\t\tbuild_ambiguous_sha1_ref_index(&idx);\n> +\t\tloaded = 1;\n> +\t}\n> +\n> +\treturn sha1_array_lookup(&idx, sha1) >= 0;\n> +}\n> +\n> static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n> \t\t\t\t\t const unsigned char *old_sha1,\n> \t\t\t\t\t int flags, int *type_p)\n> diff --git a/refs.h b/refs.h\n> index 87a1a79..c7d5f89 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -229,4 +229,6 @@ int update_refs(const char *action, const struct ref_update **updates,\n> extern int parse_hide_refs_config(const char *var, const char *value, const char *);\n> extern int ref_is_hidden(const char *);\n> \n> +int sha1_is_ambiguous_with_ref(const unsigned char *sha1);\n> +\n> #endif /* REFS_H */\n\nCould we have a docstring, please?\n\n> diff --git a/sha1_name.c b/sha1_name.c\n> index a5578f7..f83ecb7 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -452,13 +452,11 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n> \n> \tif (len == 40 && !get_sha1_hex(str, sha1)) {\n> \t\tif (warn_ambiguous_refs && warn_on_object_refname_ambiguity) {\n> -\t\t\trefs_found = dwim_ref(str, len, tmp_sha1, &real_ref);\n> -\t\t\tif (refs_found > 0) {\n> +\t\t\tif (sha1_is_ambiguous_with_ref(sha1)) {\n> \t\t\t\twarning(warn_msg, len, str);\n> \t\t\t\tif (advice_object_name_warning)\n> \t\t\t\t\tfprintf(stderr, \"%s\\n\", _(object_name_msg));\n> \t\t\t}\n> -\t\t\tfree(real_ref);\n> \t\t}\n> \t\treturn 0;\n> \t}\n> \n\nDespite all my bellyaching, I think that your optimizing of these\nlookups gives a nice speedup and I think that the approach that you took\nis also OK.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"232918","messageId":"52CD7E23.8020503@alum.mit.edu","threadId":"35621","inReplyTo":"20140108000009.GE10657@sigill.intra.peff.net","subject":"Re: [PATCH v2 5/5] get_sha1: drop object/refname ambiguity flag","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-08T16:34:43Z","receivedAt":"2014-01-08T16:34:43Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 01/08/2014 01:00 AM, Jeff King wrote:\n> Now that our object/refname ambiguity test is much faster\n> (thanks to the previous commit), there is no reason for code\n> like \"cat-file --batch-check\" to turn it off. Here are\n> before and after timings with this patch (on git.git):\n> \n> $ git rev-list --objects --all | cut -d' ' -f1 >objects\n> \n> [with flag]\n> $ best-of-five -i objects ./git cat-file --batch-check\n> real 0m0.392s\n> user 0m0.368s\n> sys 0m0.024s\n> \n> [without flag, without speedup; i.e., pre-25fba78]\n> $ best-of-five -i objects ./git cat-file --batch-check\n> real 0m1.652s\n> user 0m0.904s\n> sys 0m0.748s\n> \n> [without flag, with speedup]\n> $ best-of-five -i objects ./git cat-file --batch-check\n> real 0m0.388s\n> user 0m0.356s\n> sys 0m0.028s\n> \n> So the new implementation does just as well as we did with\n> the flag turning the whole thing off (better actually, but\n> that is within the noise).\n> \n> Signed-off-by: Jeff King \n> [...]\n\nVery nice. Correctness without a performance hit.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"232956","messageId":"xmqqa9f5avs3.fsf@gitster.dls.corp.google.com","threadId":"35621","inReplyTo":"20140108034733.GA17198@sigill.intra.peff.net","subject":"Re: [PATCH v3 3/5] refs: teach for_each_ref a flag to avoid recursion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-09T17:51:24Z","receivedAt":"2014-01-09T17:51:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Jan 07, 2014 at 06:58:50PM -0500, Jeff King wrote:\n>\n>> +\t\t\tif (flags & DO_FOR_EACH_NO_RECURSE) {\n>> +\t\t\t\tstruct ref_dir *subdir = get_ref_dir(entry);\n>> +\t\t\t\tsort_ref_dir(subdir);\n>> +\t\t\t\tretval = do_for_each_entry_in_dir(subdir, 0,\n>\n> Obviously this is totally wrong and inverts the point of the flag. And\n> causes something like half of the test suite to fail.\n>\n> Michael was nice enough to point it out to me off-list, but well, I have\n> to face the brown paper bag at some point. :) In my defense, it was a\n> last minute refactor before going to dinner. That is what I get for\n> rushing out the series.\n\nAnd perhaps a bad naming that calls for double-negation in the\nnormal cases, which might have been less likely to happen it the new\nflag were called \"onelevel only\" or something, perhaps?\n\n> Here's a fixed version of patch 3/5.\n>\n> -- >8 --\n> Subject: refs: teach for_each_ref a flag to avoid recursion\n>\n> The normal for_each_ref traversal descends into\n> subdirectories, returning each ref it finds. However, in\n> some cases we may want to just iterate over the top-level of\n> a certain part of the tree.\n>\n> The introduction of the \"flags\" option is a little\n> mysterious. We already have a \"flags\" option that gets stuck\n> in a callback struct and ends up interpreted in do_one_ref.\n> But the traversal itself does not currently have any flags,\n> and it needs to know about this new flag.\n>\n> We _could_ introduce this as a completely separate flag\n> parameter. But instead, we simply put both flag types into a\n> single namespace, and make it available at both sites. This\n> is simple, and given that we do not have a proliferation of\n> flags (we have had exactly one until now), it is probably\n> sufficient.\n>\n> Signed-off-by: Jeff King \n> ---\n> refs.c | 61 ++++++++++++++++++++++++++++++++++++++-----------------------\n> 1 file changed, 38 insertions(+), 23 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 3926136..b70b018 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -589,6 +589,8 @@ static void sort_ref_dir(struct ref_dir *dir)\n> \n> /* Include broken references in a do_for_each_ref*() iteration: */\n> #define DO_FOR_EACH_INCLUDE_BROKEN 0x01\n> +/* Do not recurse into subdirs, just iterate at a single level. */\n> +#define DO_FOR_EACH_NO_RECURSE 0x02\n> \n> /*\n> * Return true iff the reference described by entry can be resolved to\n> @@ -661,7 +663,8 @@ static int do_one_ref(struct ref_entry *entry, void *cb_data)\n> * called for all references, including broken ones.\n> */\n> static int do_for_each_entry_in_dir(struct ref_dir *dir, int offset,\n> -\t\t\t\t each_ref_entry_fn fn, void *cb_data)\n> +\t\t\t\t each_ref_entry_fn fn, void *cb_data,\n> +\t\t\t\t int flags)\n> {\n> \tint i;\n> \tassert(dir->sorted == dir->nr);\n> @@ -669,9 +672,13 @@ static int do_for_each_entry_in_dir(struct ref_dir *dir, int offset,\n> \t\tstruct ref_entry *entry = dir->entries[i];\n> \t\tint retval;\n> \t\tif (entry->flag & REF_DIR) {\n> -\t\t\tstruct ref_dir *subdir = get_ref_dir(entry);\n> -\t\t\tsort_ref_dir(subdir);\n> -\t\t\tretval = do_for_each_entry_in_dir(subdir, 0, fn, cb_data);\n> +\t\t\tif (!(flags & DO_FOR_EACH_NO_RECURSE)) {\n> +\t\t\t\tstruct ref_dir *subdir = get_ref_dir(entry);\n> +\t\t\t\tsort_ref_dir(subdir);\n> +\t\t\t\tretval = do_for_each_entry_in_dir(subdir, 0,\n> +\t\t\t\t\t\t\t\t fn, cb_data,\n> +\t\t\t\t\t\t\t\t flags);\n> +\t\t\t}\n> \t\t} else {\n> \t\t\tretval = fn(entry, cb_data);\n> \t\t}\n> @@ -691,7 +698,8 @@ static int do_for_each_entry_in_dir(struct ref_dir *dir, int offset,\n> */\n> static int do_for_each_entry_in_dirs(struct ref_dir *dir1,\n> \t\t\t\t struct ref_dir *dir2,\n> -\t\t\t\t each_ref_entry_fn fn, void *cb_data)\n> +\t\t\t\t each_ref_entry_fn fn, void *cb_data,\n> +\t\t\t\t int flags)\n> {\n> \tint retval;\n> \tint i1 = 0, i2 = 0;\n> @@ -702,10 +710,12 @@ static int do_for_each_entry_in_dirs(struct ref_dir *dir1,\n> \t\tstruct ref_entry *e1, *e2;\n> \t\tint cmp;\n> \t\tif (i1 == dir1->nr) {\n> -\t\t\treturn do_for_each_entry_in_dir(dir2, i2, fn, cb_data);\n> +\t\t\treturn do_for_each_entry_in_dir(dir2, i2, fn, cb_data,\n> +\t\t\t\t\t\t\tflags);\n> \t\t}\n> \t\tif (i2 == dir2->nr) {\n> -\t\t\treturn do_for_each_entry_in_dir(dir1, i1, fn, cb_data);\n> +\t\t\treturn do_for_each_entry_in_dir(dir1, i1, fn, cb_data,\n> +\t\t\t\t\t\t\tflags);\n> \t\t}\n> \t\te1 = dir1->entries[i1];\n> \t\te2 = dir2->entries[i2];\n> @@ -713,12 +723,15 @@ static int do_for_each_entry_in_dirs(struct ref_dir *dir1,\n> \t\tif (cmp == 0) {\n> \t\t\tif ((e1->flag & REF_DIR) && (e2->flag & REF_DIR)) {\n> \t\t\t\t/* Both are directories; descend them in parallel. */\n> -\t\t\t\tstruct ref_dir *subdir1 = get_ref_dir(e1);\n> -\t\t\t\tstruct ref_dir *subdir2 = get_ref_dir(e2);\n> -\t\t\t\tsort_ref_dir(subdir1);\n> -\t\t\t\tsort_ref_dir(subdir2);\n> -\t\t\t\tretval = do_for_each_entry_in_dirs(\n> -\t\t\t\t\t\tsubdir1, subdir2, fn, cb_data);\n> +\t\t\t\tif (!(flags & DO_FOR_EACH_NO_RECURSE)) {\n> +\t\t\t\t\tstruct ref_dir *subdir1 = get_ref_dir(e1);\n> +\t\t\t\t\tstruct ref_dir *subdir2 = get_ref_dir(e2);\n> +\t\t\t\t\tsort_ref_dir(subdir1);\n> +\t\t\t\t\tsort_ref_dir(subdir2);\n> +\t\t\t\t\tretval = do_for_each_entry_in_dirs(\n> +\t\t\t\t\t\t\tsubdir1, subdir2,\n> +\t\t\t\t\t\t\tfn, cb_data, flags);\n> +\t\t\t\t}\n> \t\t\t\ti1++;\n> \t\t\t\ti2++;\n> \t\t\t} else if (!(e1->flag & REF_DIR) && !(e2->flag & REF_DIR)) {\n> @@ -743,7 +756,7 @@ static int do_for_each_entry_in_dirs(struct ref_dir *dir1,\n> \t\t\t\tstruct ref_dir *subdir = get_ref_dir(e);\n> \t\t\t\tsort_ref_dir(subdir);\n> \t\t\t\tretval = do_for_each_entry_in_dir(\n> -\t\t\t\t\t\tsubdir, 0, fn, cb_data);\n> +\t\t\t\t\t\tsubdir, 0, fn, cb_data, flags);\n> \t\t\t} else {\n> \t\t\t\tretval = fn(e, cb_data);\n> \t\t\t}\n> @@ -817,7 +830,7 @@ static int is_refname_available(const char *refname, const char *oldrefname,\n> \tdata.conflicting_refname = NULL;\n> \n> \tsort_ref_dir(dir);\n> -\tif (do_for_each_entry_in_dir(dir, 0, name_conflict_fn, &data)) {\n> +\tif (do_for_each_entry_in_dir(dir, 0, name_conflict_fn, &data, 0)) {\n> \t\terror(\"'%s' exists; cannot create '%s'\",\n> \t\t data.conflicting_refname, refname);\n> \t\treturn 0;\n> @@ -1651,7 +1664,8 @@ void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname)\n> * 0.\n> */\n> static int do_for_each_entry(struct ref_cache *refs, const char *base,\n> -\t\t\t each_ref_entry_fn fn, void *cb_data)\n> +\t\t\t each_ref_entry_fn fn, void *cb_data,\n> +\t\t\t int flags)\n> {\n> \tstruct packed_ref_cache *packed_ref_cache;\n> \tstruct ref_dir *loose_dir;\n> @@ -1684,15 +1698,15 @@ static int do_for_each_entry(struct ref_cache *refs, const char *base,\n> \t\tsort_ref_dir(packed_dir);\n> \t\tsort_ref_dir(loose_dir);\n> \t\tretval = do_for_each_entry_in_dirs(\n> -\t\t\t\tpacked_dir, loose_dir, fn, cb_data);\n> +\t\t\t\tpacked_dir, loose_dir, fn, cb_data, flags);\n> \t} else if (packed_dir) {\n> \t\tsort_ref_dir(packed_dir);\n> \t\tretval = do_for_each_entry_in_dir(\n> -\t\t\t\tpacked_dir, 0, fn, cb_data);\n> +\t\t\t\tpacked_dir, 0, fn, cb_data, flags);\n> \t} else if (loose_dir) {\n> \t\tsort_ref_dir(loose_dir);\n> \t\tretval = do_for_each_entry_in_dir(\n> -\t\t\t\tloose_dir, 0, fn, cb_data);\n> +\t\t\t\tloose_dir, 0, fn, cb_data, flags);\n> \t}\n> \n> \trelease_packed_ref_cache(packed_ref_cache);\n> @@ -1718,7 +1732,7 @@ static int do_for_each_ref(struct ref_cache *refs, const char *base,\n> \tdata.fn = fn;\n> \tdata.cb_data = cb_data;\n> \n> -\treturn do_for_each_entry(refs, base, do_one_ref, &data);\n> +\treturn do_for_each_entry(refs, base, do_one_ref, &data, flags);\n> }\n> \n> static int do_head_ref(const char *submodule, each_ref_fn fn, void *cb_data)\n> @@ -2200,7 +2214,7 @@ int commit_packed_refs(void)\n> \n> \tdo_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),\n> \t\t\t\t 0, write_packed_entry_fn,\n> -\t\t\t\t &packed_ref_cache->lock->fd);\n> +\t\t\t\t &packed_ref_cache->lock->fd, 0);\n> \tif (commit_lock_file(packed_ref_cache->lock))\n> \t\terror = -1;\n> \tpacked_ref_cache->lock = NULL;\n> @@ -2345,7 +2359,7 @@ int pack_refs(unsigned int flags)\n> \tcbdata.packed_refs = get_packed_refs(&ref_cache);\n> \n> \tdo_for_each_entry_in_dir(get_loose_refs(&ref_cache), 0,\n> -\t\t\t\t pack_if_possible_fn, &cbdata);\n> +\t\t\t\t pack_if_possible_fn, &cbdata, 0);\n> \n> \tif (commit_packed_refs())\n> \t\tdie_errno(\"unable to overwrite old ref-pack file\");\n> @@ -2447,7 +2461,8 @@ static int repack_without_refs(const char **refnames, int n)\n> \t}\n> \n> \t/* Remove any other accumulated cruft */\n> -\tdo_for_each_entry_in_dir(packed, 0, curate_packed_ref_fn, &refs_to_delete);\n> +\tdo_for_each_entry_in_dir(packed, 0, curate_packed_ref_fn,\n> +\t\t\t\t &refs_to_delete, 0);\n> \tfor_each_string_list_item(ref_to_delete, &refs_to_delete) {\n> \t\tif (remove_entry(packed, ref_to_delete->string) == -1)\n> \t\t\tdie(\"internal error\");\n"},{"id":"232958","messageId":"xmqq61ptau6v.fsf@gitster.dls.corp.google.com","threadId":"35621","inReplyTo":"52CD7835.2020708@alum.mit.edu","subject":"Re: [PATCH v2 4/5] get_sha1: speed up ambiguous 40-hex test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-09T18:25:44Z","receivedAt":"2014-01-09T18:25:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> It's not only racy WRT other processes. If the current git process\n> would create a new reference, it wouldn't be reflected in the cache.\n>\n> It's true that the main ref_cache doesn't invalidate itself\n> automatically either when a new reference is created, so it's not really\n> a fair complaint. However, as we add places where the cache is\n> invalidated, it is easy to overlook this cache that is stuck in static\n> variables within a function definition and it is impossible to\n> invalidate it. Might it not be better to attach the cache to the\n> ref_cache structure instead, and couple its lifetime to that object?\n>\n> Alternatively, the cache could be created and managed on the caller\n> side, since the caller would know when the cache would have to be\n> invalidated. Also, different callers are likely to have different\n> performance characteristics. It is unlikely that the time to initialize\n> the cache will be amortized in most cases; in fact, \"rev-list --stdin\"\n> might be the *only* plausible use case.\n\nTrue.\n\n> Regarding the overall strategy: you gather all refnames that could be\n> confused with an SHA-1 into a sha1_array, then later look up SHA-1s in\n> the array to see if they are ambiguous. This is a very special-case\n> optimization for SHA-1s.\n>\n> I wonder whether another approach would gain almost the same amount of\n> performance but be more general. We could change dwim_ref() (or a\n> version of it?) to read its data out of a ref_cache instead of going to\n> disk every time. Then, at the cost of populating the relevant parts of\n> the ref_cache once, we would have fast dwim_ref() calls for all strings.\n\nIf opendir-readdir to grab only the names (but not values) of many\nrefs is a lot faster than stat-open-read a handful of dwim-ref\nlocations for a given name, that optimization might be worthwhile,\nbut I think that requires an update to read_loose_refs() not to\nread_ref_full() and the users of refs API to instead lazily resolve\nthe refs, no?\n\nIf I ask for five names (say 'maint', 'master', 'next', 'pu',\n'jch'), the current code will do 5 dwim_ref()s, each of which will\nconsult 6 locations with resolve_ref_unsafe(), totalling 30 calls to\nresolve_ref_unsafe(), each of which in turn is essentially an open\nfollowed by either an return on ENOENT or a read. So 30 opens and 5\nreads in total.\n\nWith your lazy ref_cache scheme, instead we would enumerate all the\nloose ones in the same 6 directories (e.g. refs/tags/, refs/heads),\nso 6 opendir()s with as many readdir()s as I have loose refs, plus\nwe open-read them in read_loose_refs() called from get_ref_dir()\nwith the current ref_cache code. For me, \"find .git/refs/heads\"\ngives 500+ lines of output, which suggests that using the ref_cache\nmechanism for dwim_ref() may not be a huge win, unless it is updated\nto be extremely lazy, and readdir()s turns out to be extremely less\nheavier than open-read. Also it is unlikely that the cost to\ninitialize the cache is amortized to be a net win unless we are\ndealing with tons of dwim_ref()s.\n"},{"id":"232969","messageId":"20140109214926.GA32069@sigill.intra.peff.net","threadId":"35621","inReplyTo":"52CD36AF.2080705@alum.mit.edu","subject":"Re: [PATCH v3 3/5] refs: teach for_each_ref a flag to avoid recursion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-09T21:49:26Z","receivedAt":"2014-01-09T21:49:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 08, 2014 at 12:29:51PM +0100, Michael Haggerty wrote:\n\n> > Here's a fixed version of patch 3/5.\n> \n> v2 4/5 doesn't apply cleanly on top of v3 3/5. So I'm basing my review\n> on the branch you have at GitHub peff/git \"jk/cat-file-warn-ambiguous\";\n> I hope it is the same.\n\nHrmph. I didn't have to do any conflict resolution during the rebase, so\nI would think it would apply at least with \"am -3\".\n\n> > -- >8 --\n> > Subject: refs: teach for_each_ref a flag to avoid recursion\n> > \n> > The normal for_each_ref traversal descends into\n> \n> You haven't changed any for_each_ref*() functions; you have only exposed\n> the DO_FOR_EACH_NO_RECURSE option to the (static) functions\n> for_each_entry*() and do_for_each_ref(). (This is part and parcel of\n> your decision not to expose the new functionality in the refs API.)\n> Please correct the line above.\n\nWill do, and I'll add a note on not exposing it (basically because there\nis not an existing \"flags\" parameter in the public API, and nobody needs\nit).\n\n> > static int do_for_each_entry_in_dir(struct ref_dir *dir, int offset,\n> > -\t\t\t\t each_ref_entry_fn fn, void *cb_data)\n> > +\t\t\t\t each_ref_entry_fn fn, void *cb_data,\n> > +\t\t\t\t int flags)\n> > {\n> > \tint i;\n> > \tassert(dir->sorted == dir->nr);\n> \n> Please update the docstring for this function, which still says that it\n> recurses without mentioning DO_FOR_EACH_NO_RECURSE.\n\nWill do (and for the _in_dirs variant).\n\n> > static int do_for_each_entry(struct ref_cache *refs, const char *base,\n> > -\t\t\t each_ref_entry_fn fn, void *cb_data)\n> > +\t\t\t each_ref_entry_fn fn, void *cb_data,\n> > +\t\t\t int flags)\n> > {\n> > \tstruct packed_ref_cache *packed_ref_cache;\n> > \tstruct ref_dir *loose_dir;\n> \n> A few lines after this, do_for_each_entry() calls\n> prime_ref_dir(loose_dir) to ensure that all of the loose references that\n> will be iterated over are read before the packed-refs file is checked.\n> It seems to me that prime_ref_dir() should also get a flags parameter to\n> prevent it reading more loose references than necessary, something like\n> this:\n\nHmm. I hadn't considered that, but yeah, it definitely nullifies part of\nthe purpose of the optimization.\n\nHowever, is it safe to prime only part of the loose ref namespace? The\npoint of that priming is to avoid the race fixed in 98eeb09, which\ndepends on us caching the loose refs before the packed refs. But when we\nread packed-refs, we will be reading and storing _all_ of it, even if we\ndo not touch it in this traversal. So it does not affect the race for\nthis traversal, but have we setup a cache situation where a subsequent\nfor_each_ref in the same process would be subject to the race?\n\nI'm starting to wonder if this optimization is worth it.\n\n> > [...]\n> > @@ -1718,7 +1732,7 @@ static int do_for_each_ref(struct ref_cache *refs, const char *base,\n> > \tdata.fn = fn;\n> > \tdata.cb_data = cb_data;\n> > \n> > -\treturn do_for_each_entry(refs, base, do_one_ref, &data);\n> > +\treturn do_for_each_entry(refs, base, do_one_ref, &data, flags);\n> > }\n> \n> This change makes the DO_FOR_EACH_NO_RECURSE option usable with\n> do_for_each_ref() (even though it is never in fact used). It should\n> either be mentioned in the docstring or (if there is a reason not to\n> allow it) explicitly prohibited.\n\nHrm, yeah. I guess there are no callers, and there is no plan for any.\nSo we could just pass \"0\" here, and then \"flags\" passed to\ndo_for_each_ref really is _just_ for the callback data that goes to\ndo_one_ref. That clears up the weird \"combined namespace\" stuff I\nmentioned in the commit message, and is a bit cleaner. I'll take it in\nthat direction.\n\n> It would be possible to use your new flag to speed up\n> is_refname_available(), but it would be a little bit of work and I doubt\n> that is_refname_available() is ever a bottleneck.\n\nYeah, agreed on both counts.\n\n-Peff\n"},{"id":"232970","messageId":"20140109215536.GB32069@sigill.intra.peff.net","threadId":"35621","inReplyTo":"xmqqa9f5avs3.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 3/5] refs: teach for_each_ref a flag to avoid recursion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-09T21:55:36Z","receivedAt":"2014-01-09T21:55:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 09, 2014 at 09:51:24AM -0800, Junio C Hamano wrote:\n\n> > On Tue, Jan 07, 2014 at 06:58:50PM -0500, Jeff King wrote:\n> >\n> >> +\t\t\tif (flags & DO_FOR_EACH_NO_RECURSE) {\n> >> +\t\t\t\tstruct ref_dir *subdir = get_ref_dir(entry);\n> >> +\t\t\t\tsort_ref_dir(subdir);\n> >> +\t\t\t\tretval = do_for_each_entry_in_dir(subdir, 0,\n> >\n> > Obviously this is totally wrong and inverts the point of the flag. And\n> > causes something like half of the test suite to fail.\n> >\n> > Michael was nice enough to point it out to me off-list, but well, I have\n> > to face the brown paper bag at some point. :) In my defense, it was a\n> > last minute refactor before going to dinner. That is what I get for\n> > rushing out the series.\n> \n> And perhaps a bad naming that calls for double-negation in the\n> normal cases, which might have been less likely to happen it the new\n> flag were called \"onelevel only\" or something, perhaps?\n\nThat may be a nicer name, but it was not the problem here. The problem\nhere is that I wrote:\n\n if (flags & DO_FOR_EACH_NO_RECURSE == 0)\n\nto avoid the extra layer of parentheses, but of course that doesn't\nwork. And then when I switched it back, I screwed up the reversion.\n\nI think the nicest way to write it would be to avoid negation at all,\nas:\n\n if (flags & DO_FOR_EACH_RECURSE) {\n ... do the recursion ...\n\nbut that means flipping the default, requiring us to set the flag\nexplicitly in the existing callers (though there really aren't that\nmany).\n\n-Peff\n"},{"id":"232991","messageId":"52CFB66D.5070800@alum.mit.edu","threadId":"35621","inReplyTo":"20140109214926.GA32069@sigill.intra.peff.net","subject":"Re: [PATCH v3 3/5] refs: teach for_each_ref a flag to avoid recursion","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-10T08:59:25Z","receivedAt":"2014-01-10T08:59:25Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 01/09/2014 10:49 PM, Jeff King wrote:\n> On Wed, Jan 08, 2014 at 12:29:51PM +0100, Michael Haggerty wrote:\n> \n>>> Here's a fixed version of patch 3/5.\n>>\n>> v2 4/5 doesn't apply cleanly on top of v3 3/5. So I'm basing my review\n>> on the branch you have at GitHub peff/git \"jk/cat-file-warn-ambiguous\";\n>> I hope it is the same.\n> \n> Hrmph. I didn't have to do any conflict resolution during the rebase, so\n> I would think it would apply at least with \"am -3\".\n\nThat may be; I didn't try with \"-3\".\n\n> [...]\n>>> static int do_for_each_entry(struct ref_cache *refs, const char *base,\n>>> -\t\t\t each_ref_entry_fn fn, void *cb_data)\n>>> +\t\t\t each_ref_entry_fn fn, void *cb_data,\n>>> +\t\t\t int flags)\n>>> {\n>>> \tstruct packed_ref_cache *packed_ref_cache;\n>>> \tstruct ref_dir *loose_dir;\n>>\n>> A few lines after this, do_for_each_entry() calls\n>> prime_ref_dir(loose_dir) to ensure that all of the loose references that\n>> will be iterated over are read before the packed-refs file is checked.\n>> It seems to me that prime_ref_dir() should also get a flags parameter to\n>> prevent it reading more loose references than necessary, something like\n>> this:\n> \n> Hmm. I hadn't considered that, but yeah, it definitely nullifies part of\n> the purpose of the optimization.\n> \n> However, is it safe to prime only part of the loose ref namespace? The\n> point of that priming is to avoid the race fixed in 98eeb09, which\n> depends on us caching the loose refs before the packed refs. But when we\n> read packed-refs, we will be reading and storing _all_ of it, even if we\n> do not touch it in this traversal. So it does not affect the race for\n> this traversal, but have we setup a cache situation where a subsequent\n> for_each_ref in the same process would be subject to the race?\n\nprime_ref_dir() is called by do_for_each_entry(), which all the\niteration functions pass through. It is always called before the\niteration starts, and it primes only the subtree of the refs hierarchy\nthat is being iterated over. For example, if iterating over\n\"refs/heads\" then it only primes references with that prefix.\n\nThis is OK, because if later somebody iterates over a broader part of\nthe refs hierarchy (say, \"refs\"), then priming is done again, including\nre-checking the packed refs. If the packed-refs file was changed\nbetween the iterations, then the first iteration (if it is still\nrunning) continues using the old packed-refs cache while the second\niteration uses the new packed-refs cache. (So the first iteration will\nhave a stale, but self-consistent, view of the references.)\n\nIf do_for_each_entry() gets the DO_FOR_EACH_NO_RECURSE option, then it\nknows that it will only traverse one level of the refs hierarchy. So if\nit passes the option to prime_ref_dir(), then the same level will be\nprimed. If somebody later iterates over the same part of the hierarchy\nwithout DO_FOR_EACH_NO_RECURSE, they will re-prime without\nDO_FOR_EACH_NO_RECURSE then, if the packed-refs file has been changed,\nload a new version of it. So they will also get a self-consistent view\nof the references and I think everything will be OK.\n\n> I'm starting to wonder if this optimization is worth it.\n\nIt's true that this is quite a special-case optimization.\n\nI think reference handling will have to move in the direction of\ntransactions, to remove one or two known race conditions. That is why I\ndescribed the alternative of having the DWIM function do its lookups in\na ref cache. It would move in the direction of consciously taking a\nsnapshot of the ref tree and using it for a whole \"transaction\", which I\nthink is a style that we will want to use in more places. It's just\nhard to judge whether this alternative would actually solve the\nperformance problem that you were originally trying to address--a point\nthat Junio discussed elsewhere in this thread.\n\n(This is something I would be willing to work on if you feel like I am\npushing you to enlarge the scope of your work beyond what you are\ninterested in.)\n\n>>> [...]\n>>> @@ -1718,7 +1732,7 @@ static int do_for_each_ref(struct ref_cache *refs, const char *base,\n>>> \tdata.fn = fn;\n>>> \tdata.cb_data = cb_data;\n>>> \n>>> -\treturn do_for_each_entry(refs, base, do_one_ref, &data);\n>>> +\treturn do_for_each_entry(refs, base, do_one_ref, &data, flags);\n>>> }\n>>\n>> This change makes the DO_FOR_EACH_NO_RECURSE option usable with\n>> do_for_each_ref() (even though it is never in fact used). It should\n>> either be mentioned in the docstring or (if there is a reason not to\n>> allow it) explicitly prohibited.\n> \n> Hrm, yeah. I guess there are no callers, and there is no plan for any.\n> So we could just pass \"0\" here, and then \"flags\" passed to\n> do_for_each_ref really is _just_ for the callback data that goes to\n> do_one_ref. That clears up the weird \"combined namespace\" stuff I\n> mentioned in the commit message, and is a bit cleaner. I'll take it in\n> that direction.\n\nIt would also be possible to swing in the other direction. I don't\nremember a particular reason why I left the DO_FOR_EACH_INCLUDE_BROKEN\nhandling at the do_for_each_ref() level rather than handling it at the\ndo_for_each_entry() level. But now that you are passing the flags\nparameter all the way down the call stack, it wouldn't cost anything to\nsupport both of the DO_FOR_EACH flags everywhere and just document it\nthat way.\n\n> [...]\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"232992","messageId":"20140110091546.GA17443@sigill.intra.peff.net","threadId":"35621","inReplyTo":"52CFB66D.5070800@alum.mit.edu","subject":"Re: [PATCH v3 3/5] refs: teach for_each_ref a flag to avoid recursion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-10T09:15:46Z","receivedAt":"2014-01-10T09:15:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 10, 2014 at 09:59:25AM +0100, Michael Haggerty wrote:\n\n> > However, is it safe to prime only part of the loose ref namespace?\n> [...]\n> \n> prime_ref_dir() is called by do_for_each_entry(), which all the\n> iteration functions pass through. It is always called before the\n> iteration starts, and it primes only the subtree of the refs hierarchy\n> that is being iterated over. For example, if iterating over\n> \"refs/heads\" then it only primes references with that prefix.\n> \n> This is OK, because if later somebody iterates over a broader part of\n> the refs hierarchy (say, \"refs\"), then priming is done again, including\n> re-checking the packed refs.\n\nAh, right. This is the part I was forgetting: the next for_each_ref will\nre-prime with the expanded view. Thanks for a dose of sanity.\n\nI'll fix that in my re-roll.\n\n> It would also be possible to swing in the other direction. I don't\n> remember a particular reason why I left the DO_FOR_EACH_INCLUDE_BROKEN\n> handling at the do_for_each_ref() level rather than handling it at the\n> do_for_each_entry() level. But now that you are passing the flags\n> parameter all the way down the call stack, it wouldn't cost anything to\n> support both of the DO_FOR_EACH flags everywhere and just document it\n> that way.\n\nI think it was simply that it was an option that the traversal did not\nneed to know about (just like the \"trim\" option), so you kept it as\nencapsulated as possible. I think I'll introduce it as a separate flag\nnamespace, as discussed in the previous email. It is the same amount of\nrefactoring work to merge them later as it is now, if we so choose.\n\n-Peff\n"},{"id":"232993","messageId":"20140110094120.GB17443@sigill.intra.peff.net","threadId":"35621","inReplyTo":"52CD7835.2020708@alum.mit.edu","subject":"Re: [PATCH v2 4/5] get_sha1: speed up ambiguous 40-hex test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-10T09:41:20Z","receivedAt":"2014-01-10T09:41:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 08, 2014 at 05:09:25PM +0100, Michael Haggerty wrote:\n\n> It's not only racy WRT other processes. If the current git process\n> would create a new reference, it wouldn't be reflected in the cache.\n> \n> It's true that the main ref_cache doesn't invalidate itself\n> automatically either when a new reference is created, so it's not really\n> a fair complaint. However, as we add places where the cache is\n> invalidated, it is easy to overlook this cache that is stuck in static\n> variables within a function definition and it is impossible to\n> invalidate it. Might it not be better to attach the cache to the\n> ref_cache structure instead, and couple its lifetime to that object?\n\nYeah, I noticed that we don't ever invalidate the loose ref cache. I\nthink that's mostly fine, as we rely on resolve_ref to cover the cases\nthat need to be ordered properly. And in this particular case, it's\n\"only\" a warning (and a rather obscure one, at that), so I think the\nstakes are low.\n\nThat being said, it should not be hard at all to attach the cache to the\nref_cache. Since we are generated from that cache, the lifetimes should\nbe the same.\n\n> Alternatively, the cache could be created and managed on the caller\n> side, since the caller would know when the cache would have to be\n> invalidated. Also, different callers are likely to have different\n> performance characteristics. It is unlikely that the time to initialize\n> the cache will be amortized in most cases; in fact, \"rev-list --stdin\"\n> might be the *only* plausible use case.\n\nThe two I know of are \"rev-list --stdin\" and \"cat-file --batch-check\".\n\n> Regarding the overall strategy: you gather all refnames that could be\n> confused with an SHA-1 into a sha1_array, then later look up SHA-1s in\n> the array to see if they are ambiguous. This is a very special-case\n> optimization for SHA-1s.\n\nYes, it is very sha1-specific. Part of my goal was that in the common\ncase, the check would collapse to O(# of ambiguous refs), which is\ntypically 0.\n\nThat may be premature optimization, though. As you note below, doing a\nfew binary searches through the in-memory ref cache is _probably_ fine,\ntoo. And we can do that without a separate index.\n\n> I wonder whether another approach would gain almost the same amount of\n> performance but be more general. We could change dwim_ref() (or a\n> version of it?) to read its data out of a ref_cache instead of going to\n> disk every time. Then, at the cost of populating the relevant parts of\n> the ref_cache once, we would have fast dwim_ref() calls for all strings.\n\nI'm very nervous about turning dwim_ref into a cache. As we noted above,\nwe never invalidate the cache, so any write-then-read operations could\nget stale data. That is not as risky as caching, say, resolve_ref, but\nit still makes me nervous. Caching just the warning has much lower\nstakes.\n\n> It's true that the lookups wouldn't be quite so fast--they would require\n> a few bisects per refname lookup (one for each level in the refname\n> hierarchy) and several refname lookups (one for each ref_rev_parse_rule)\n> for every dwim_ref() call, vs. a single bisect in your current design.\n> But this approach it would bring us most of the gain, it might\n> nevertheless be preferable.\n\nI don't think this would be all that hard to measure. I'll see what I\ncan do.\n\n> > I wanted to make the ref traversal as cheap as possible, hence the\n> > NO_RECURSE flag I added. I thought INCLUDE_BROKEN used to not open up\n> > the refs at all, but it looks like it does these days. I wonder if that\n> > is worth changing or not.\n> \n> What do you mean by \"open up the refs\"? The loose reference files are\n> read when populating the cache. (Was that ever different?)\n\nI meant actually open the ref files and read the sha1. But that is me\nbeing dumb. We have always done that, as we must provide the sha1 via\nto the for_each_ref callback.\n\nThat being said, we could further optimize this by not opening the files\nat all (and make that the responsibility of do_one_ref, which we are\navoiding here). I am slightly worried about the open() cost of my\nsolution. It's amortized away in a big call, but it is probably\nnoticeable for something like `git rev-parse <40-hex>`.\n\n> This doesn't correctly handle the rule\n> \n> \t\"refs/remotes/%.*s/HEAD\"\n>\n> We might be willing to accept this limitation, but it should at least be\n> mentioned somewhere. OTOH if we want to handle this pattern as well, we\n> could do use a technique like that of shorten_unambiguous_ref().\n\nYes, you're right. I considered this, but for some reason convinced\nmyself that it would be OK to just look for \"refs/remotes/<40-hex>\" in\nthis case (which is what my patch does). But obviously that's wrong.\nThe ref traversal won't find a _directory_ with that name.\n\nI'll see how painful it is to make it work. I have to say I was tempted\nto simply manually write the rules. It's a duplication that could go out\nof sync with the ref_rev_parse rules, and that's nasty. But that list does\nnot change often, and the reverse-parsing of the rules is error-prone\nand hard to understand itself.\n\n-Peff\n"},{"id":"233084","messageId":"20140114095002.GA32258@sigill.intra.peff.net","threadId":"35621","inReplyTo":"20140110094120.GB17443@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/5] get_sha1: speed up ambiguous 40-hex test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-14T09:50:02Z","receivedAt":"2014-01-14T09:50:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 10, 2014 at 04:41:20AM -0500, Jeff King wrote:\n\n> That being said, we could further optimize this by not opening the files\n> at all (and make that the responsibility of do_one_ref, which we are\n> avoiding here). I am slightly worried about the open() cost of my\n> solution. It's amortized away in a big call, but it is probably\n> noticeable for something like `git rev-parse <40-hex>`.\n\nI took a look at this. It gets a bit hairy. My strategy is to add a flag\nto ask read_loose_refs to create REF_INCOMPLETE values. We currently use\nthis flag for loose REF_DIRs to mean \"we haven't opendir()'d the\nsubdirectory yet\". This would extend it to the non-REF_DIR case to mean\n\"we haven't opened the loose ref file yet\". We'd check REF_INCOMPLETE\nbefore handing the ref_entry to a callback, and complete it if\nnecessary.\n\nIt gets ugly, though, because we need to pass that flag through quite a\nbit of callstack. get_ref_dir() needs to know it, which means all of\nfind_containing_dir, etc need it, meaning it pollutes all of the\npacked-refs code paths too.\n\nI have a half-done patch in this direction if that doesn't sound too\nnasty.\n\n> > This doesn't correctly handle the rule\n> > \n> > \t\"refs/remotes/%.*s/HEAD\"\n> [...]\n\n> I'll see how painful it is to make it work.\n\nIt's actually reasonably painful. I thought at first we could get away\nwith more cleverly parsing the rule, find the prefix (up to the\nplaceholder), and then look for the suffix (\"/HEAD\") inside there. But\nit can never work with the current do_for_each_* code. That code only\ntriggers a callback when we see a concrete ref. It _never_ lets the\ncallbacks see an intermediate directory.\n\nSo a NO_RECURSE flag is not sufficient to handle this case. I'd need to\nteach do_for_each_ref to recurse based on pathspecs, or a custom\ncallback function. And that is getting quite complicated.\n\nI think it might be simpler to just do my own custom traversal. What I\nneed is much simpler than what do_for_each_entry provides. I don't need\nrecursion, and I don't actually need to look at the loose and packed\nrefs together. It's OK for me to do them one at a time because I don't\ncare about the actual value; I just want to know about which refs exist.\n\n-Peff\n"},{"id":"233086","messageId":"52D520CD.7070902@alum.mit.edu","threadId":"35621","inReplyTo":"20140114095002.GA32258@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/5] get_sha1: speed up ambiguous 40-hex test","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-14T11:34:37Z","receivedAt":"2014-01-14T11:34:37Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 01/14/2014 10:50 AM, Jeff King wrote:\n> On Fri, Jan 10, 2014 at 04:41:20AM -0500, Jeff King wrote:\n> \n>> That being said, we could further optimize this by not opening the files\n>> at all (and make that the responsibility of do_one_ref, which we are\n>> avoiding here). I am slightly worried about the open() cost of my\n>> solution. It's amortized away in a big call, but it is probably\n>> noticeable for something like `git rev-parse <40-hex>`.\n> \n> I took a look at this. It gets a bit hairy. My strategy is to add a flag\n> to ask read_loose_refs to create REF_INCOMPLETE values. We currently use\n> this flag for loose REF_DIRs to mean \"we haven't opendir()'d the\n> subdirectory yet\". This would extend it to the non-REF_DIR case to mean\n> \"we haven't opened the loose ref file yet\". We'd check REF_INCOMPLETE\n> before handing the ref_entry to a callback, and complete it if\n> necessary.\n> \n> It gets ugly, though, because we need to pass that flag through quite a\n> bit of callstack. get_ref_dir() needs to know it, which means all of\n> find_containing_dir, etc need it, meaning it pollutes all of the\n> packed-refs code paths too.\n> \n> I have a half-done patch in this direction if that doesn't sound too\n> nasty.\n\nA long time ago I write a patch series to allow incomplete reading of\nreferences, but my version *always* read them lazily, so it was much\nsimpler (no need to pass a new option down the call stack). It didn't\nseem to speed things up in general, so I never submitted it.\n\nReading lazily only from particular callers is more complicated, and I\ncan see how it would get messy.\n\nGiven the race avoidance needed between packed/loose references, lazy\nreading would mean that after each reference is read, the packed-refs\nfile would need to be stat()ted again to make sure that it hasn't been\nchanged since the last check. I know this isn't an issue for your use\ncase, because you plan *never* to read the file contents. But it does\nincrease the price of lazy reference reading to most callers.\n\nOn the other hand, if we ever go in the direction of routing *all*\nreference lookups--including lookups of single references--through the\ncache, then lazy reading of references probably becomes essential to\navoid populating more of the cache than necessary.\n\n>>> This doesn't correctly handle the rule\n>>>\n>>> \t\"refs/remotes/%.*s/HEAD\"\n>> [...]\n> \n>> I'll see how painful it is to make it work.\n> \n> It's actually reasonably painful. I thought at first we could get away\n> with more cleverly parsing the rule, find the prefix (up to the\n> placeholder), and then look for the suffix (\"/HEAD\") inside there. But\n> it can never work with the current do_for_each_* code. That code only\n> triggers a callback when we see a concrete ref. It _never_ lets the\n> callbacks see an intermediate directory.\n> \n> So a NO_RECURSE flag is not sufficient to handle this case. I'd need to\n> teach do_for_each_ref to recurse based on pathspecs, or a custom\n> callback function. And that is getting quite complicated.\n\nAnother possibility would be to have an \"int recurse\" parameter rather\nthan \"bool recurse\", telling how many levels to recurse. Then one could\ndo a\n\n do_for_each_ref(..., \"refs/remotes\", ..., recurse=2)\n\nto get all of the refs/remotes/*/HEAD references. Though since all of\nthe heads for a remote are also siblings of \"refs/remotes/foo/HEAD\", it\ncould still involve a lot of superfluous file reading. And the integer\nwouldn't fit conveniently in the flags parameter.\n\n> I think it might be simpler to just do my own custom traversal. What I\n> need is much simpler than what do_for_each_entry provides. I don't need\n> recursion, and I don't actually need to look at the loose and packed\n> refs together. It's OK for me to do them one at a time because I don't\n> care about the actual value; I just want to know about which refs exist.\n\nYes. Still, the code is really piling up for this one warning for the\ncontrived eventuality that somebody wants to pass SHA-1s and branch\nnames together in a single cat-file invocation *and* wants to pass lots\nof inputs at once and so is worried about performance *and* has\nreference names that look like SHA-1s. Otherwise we could just leave\nthe warning disabled in this case, as now. Or we could add a new\n\"--hashes-only\" option that tells cat-file to treat all of its\narguments/inputs as SHA-1s; such an option would permit an even faster\ncode path for bulk callers.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"}]}