{"thread":{"id":"34185","subject":"[PATCH 1/2] builtin/checkout.c: don't leak memory in check_tracking_name","startedAt":"2013-06-18T01:40:49Z","lastAt":"2013-06-18T16:17:24Z","messageCount":5,"participants":["Brandon Casey","Jeff King","Pete Wyckoff","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"221116","messageId":"1371519650-17869-1-git-send-email-bcasey@nvidia.com","threadId":"34185","inReplyTo":null,"subject":"[PATCH 1/2] builtin/checkout.c: don't leak memory in check_tracking_name","fromName":"Brandon Casey","fromEmail":"bcasey@nvidia.com","sentAt":"2013-06-18T01:40:49Z","receivedAt":"2013-06-18T01:40:49Z","isPatch":true,"sender":{"key":"bcasey@nvidia.com","avatar":null},"body":"From: Brandon Casey \n\nremote_find_tracking() populates the query struct with an allocated\nstring in the dst member. So, we do not need to xstrdup() the string,\nsince we can transfer ownership from the query struct (which will go\nout of scope at the end of this function) to our callback struct, but\nwe must free the string if it will not be used so we will not leak\nmemory.\n\nLet's do so.\n\nSigned-off-by: Brandon Casey \n---\n builtin/checkout.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex f5b50e5..3be0018 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -838,13 +838,16 @@ static int check_tracking_name(struct remote *remote, void *cb_data)\n \tmemset(&query, 0, sizeof(struct refspec));\n \tquery.src = cb->src_ref;\n \tif (remote_find_tracking(remote, &query) ||\n-\t get_sha1(query.dst, cb->dst_sha1))\n+\t get_sha1(query.dst, cb->dst_sha1)) {\n+\t\tfree(query.dst);\n \t\treturn 0;\n+\t}\n \tif (cb->dst_ref) {\n+\t\tfree(query.dst);\n \t\tcb->unique = 0;\n \t\treturn 0;\n \t}\n-\tcb->dst_ref = xstrdup(query.dst);\n+\tcb->dst_ref = query.dst;\n \treturn 0;\n }\n \n-- \n1.8.2.415.g63cec41\n"},{"id":"221117","messageId":"1371519650-17869-2-git-send-email-bcasey@nvidia.com","threadId":"34185","inReplyTo":"1371519650-17869-1-git-send-email-bcasey@nvidia.com","subject":"[PATCH 2/2] t/t9802: explicitly name the upstream branch to use as a base","fromName":"Brandon Casey","fromEmail":"bcasey@nvidia.com","sentAt":"2013-06-18T01:40:50Z","receivedAt":"2013-06-18T01:40:50Z","isPatch":true,"sender":{"key":"bcasey@nvidia.com","avatar":null},"body":"From: Brandon Casey \n\nPrior to commit fa83a33b, the 'git checkout' DWIMery would create a\nnew local branch if the specified branch name did not exist and it\nmatched exactly one ref in the \"remotes\" namespace. It searched\nthe \"remotes\" namespace for matching refs using a simple comparison\nof the trailing portion of the remote ref names. This approach\ncould sometimes produce false positives or negatives.\n\nSince fa83a33b, the DWIMery more strictly excludes the remote name\nfrom the ref comparison by iterating through the remotes that are\nconfigured in the .gitconfig file. This has the side-effect that\nany refs that exist in the \"remotes\" namespace, but do not match\nthe destination side of any remote refspec, will not be used by\nthe DWIMery.\n\nThis change in behavior breaks the tests in t9802 which relied on\nthe old behavior of searching all refs in the remotes namespace,\nsince the git-p4 script does not configure any remotes in the\n.gitconfig. Let's work around this in these tests by explicitly\nnaming the upstream branch to base the new local branch on when\ncalling 'git checkout'.\n\nSigned-off-by: Brandon Casey \n---\n t/t9802-git-p4-filetype.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh\nindex eeefa67..b0d1d94 100755\n--- a/t/t9802-git-p4-filetype.sh\n+++ b/t/t9802-git-p4-filetype.sh\n@@ -95,7 +95,7 @@ test_expect_success 'gitattributes setting eol=lf produces lf newlines' '\n \t\tgit init &&\n \t\techo \"* eol=lf\" >.gitattributes &&\n \t\tgit p4 sync //depot@all &&\n-\t\tgit checkout master &&\n+\t\tgit checkout -b master p4/master &&\n \t\ttest_cmp \"$cli\"/f-unix-orig f-unix &&\n \t\ttest_cmp \"$cli\"/f-win-as-lf f-win\n \t)\n@@ -109,7 +109,7 @@ test_expect_success 'gitattributes setting eol=crlf produces crlf newlines' '\n \t\tgit init &&\n \t\techo \"* eol=crlf\" >.gitattributes &&\n \t\tgit p4 sync //depot@all &&\n-\t\tgit checkout master &&\n+\t\tgit checkout -b master p4/master &&\n \t\ttest_cmp \"$cli\"/f-unix-as-crlf f-unix &&\n \t\ttest_cmp \"$cli\"/f-win-orig f-win\n \t)\n-- \n1.8.2.415.g63cec41\n"},{"id":"221147","messageId":"20130618061500.GF5916@sigill.intra.peff.net","threadId":"34185","inReplyTo":"1371519650-17869-1-git-send-email-bcasey@nvidia.com","subject":"Re: [PATCH 1/2] builtin/checkout.c: don't leak memory in check_tracking_name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-18T06:15:01Z","receivedAt":"2013-06-18T06:15:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 17, 2013 at 06:40:49PM -0700, Brandon Casey wrote:\n\n> From: Brandon Casey \n> \n> remote_find_tracking() populates the query struct with an allocated\n> string in the dst member. So, we do not need to xstrdup() the string,\n> since we can transfer ownership from the query struct (which will go\n> out of scope at the end of this function) to our callback struct, but\n> we must free the string if it will not be used so we will not leak\n> memory.\n> \n> Let's do so.\n\nThanks, looks obviously correct. I wonder if other callers of\nremote_find_tracking make the same mistake. It looks like\ncheck_tracking_branch does. And add_branch_for_removal. and\nappend_ref_to_tracked_list. Yeesh.\n\n-Peff\n"},{"id":"221217","messageId":"20130618134207.GA28716@padd.com","threadId":"34185","inReplyTo":"1371519650-17869-2-git-send-email-bcasey@nvidia.com","subject":"Re: [PATCH 2/2] t/t9802: explicitly name the upstream branch to use as a base","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2013-06-18T13:42:07Z","receivedAt":"2013-06-18T13:42:07Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"bcasey@nvidia.com wrote on Mon, 17 Jun 2013 18:40 -0700:\n> From: Brandon Casey \n> \n> Prior to commit fa83a33b, the 'git checkout' DWIMery would create a\n> new local branch if the specified branch name did not exist and it\n> matched exactly one ref in the \"remotes\" namespace. It searched\n> the \"remotes\" namespace for matching refs using a simple comparison\n> of the trailing portion of the remote ref names. This approach\n> could sometimes produce false positives or negatives.\n> \n> Since fa83a33b, the DWIMery more strictly excludes the remote name\n> from the ref comparison by iterating through the remotes that are\n> configured in the .gitconfig file. This has the side-effect that\n> any refs that exist in the \"remotes\" namespace, but do not match\n> the destination side of any remote refspec, will not be used by\n> the DWIMery.\n> \n> This change in behavior breaks the tests in t9802 which relied on\n> the old behavior of searching all refs in the remotes namespace,\n> since the git-p4 script does not configure any remotes in the\n> .gitconfig. Let's work around this in these tests by explicitly\n> naming the upstream branch to base the new local branch on when\n> calling 'git checkout'.\n\nThanks for finding and fixing this. Great explanation. I\ntested it locally too.\n\nAcked-by: Pete Wyckoff \n\n\t\t-- Pete\n"},{"id":"221249","messageId":"7vehbz75rf.fsf@alter.siamese.dyndns.org","threadId":"34185","inReplyTo":"20130618134207.GA28716@padd.com","subject":"Re: [PATCH 2/2] t/t9802: explicitly name the upstream branch to use as a base","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-18T16:17:24Z","receivedAt":"2013-06-18T16:17:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pete Wyckoff writes:\n\n> Thanks for finding and fixing this. Great explanation. I\n> tested it locally too.\n>\n> Acked-by: Pete Wyckoff \n\nThanks, both. Queued.\n"}]}