{"thread":{"id":"19736","subject":"[PATCHv3 1/4] parse-remote: function to get the tracking branch to be merge","startedAt":"2009-06-08T09:00:48Z","lastAt":"2009-06-11T21:30:50Z","messageCount":8,"participants":["Santi Béjar","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"115782","messageId":"1244451651-22651-2-git-send-email-santi@agolina.net","threadId":"19736","inReplyTo":null,"subject":"[PATCHv3 1/4] parse-remote: function to get the tracking branch to be merge","fromName":"Santi Béjar","fromEmail":"santi@agolina.net","sentAt":"2009-06-08T09:00:48Z","receivedAt":"2009-06-08T09:00:48Z","isPatch":false,"sender":{"key":"santi@agolina.net","avatar":null},"body":"The only user of get_remote_refs_for_fetch was \"git pull --rebase\" and\nit only wanted the tracking branch to be merge. So, add a simple\nfunction with this new meaning.\n\nNo behavior changes.\n\nSigned-off-by: Santi Béjar <santi@agolina.net>\n---\n git-parse-remote.sh |   33 +++++++++++++++++++++++++++++++++\n git-pull.sh         |    7 ++-----\n 2 files changed, 35 insertions(+), 5 deletions(-)\n\ndiff --git a/git-parse-remote.sh b/git-parse-remote.sh\nindex a296719..8b3ba72 100755\n--- a/git-parse-remote.sh\n+++ b/git-parse-remote.sh\n@@ -229,6 +229,38 @@ get_remote_refs_for_fetch () {\n \tesac\n }\n \n+get_remote_merge_branch () {\n+\tcase \"$#\" in\n+\t0|1)\n+\t    die \"internal error: get-remote-merge-branch.\" ;;\n+\t*)\n+\t    repo=$1\n+\t    shift\n+\t    # FIXME: It should return the tracking branch\n+\t    #        Currently only works with the default mapping\n+\t    for ref\n+\t    do\n+\t\tcase \"$ref\" in\n+\t\t+*)\n+\t\t\tref=$(expr \"z$ref\" : 'z+\\(.*\\)')\n+\t\t\t;;\n+\t\tesac\n+\t\texpr \"z$ref\" : 'z.*:' >/dev/null || ref=\"${ref}:\"\n+\t\tremote=$(expr \"z$ref\" : 'z\\([^:]*\\):')\n+\t\tcase \"$remote\" in\n+\t\t'' | HEAD ) remote=HEAD ;;\n+\t\theads/*) remote=${remote#heads/} ;;\n+\t\trefs/heads/*) remote=${remote#refs/heads/} ;;\n+\t\trefs/* | tags/* | remotes/* ) break\n+\t\tesac\n+\n+\t\techo \"refs/remotes/$repo/$remote\"\n+\t\tbreak\n+\t    done\n+\t    ;;\n+\tesac\n+}\n+\n resolve_alternates () {\n \t# original URL (xxx.git)\n \ttop_=`expr \"z$1\" : 'z\\([^:]*:/*[^/]*\\)/'`\n@@ -262,3 +294,4 @@ get_uploadpack () {\n \t\t;;\n \tesac\n }\n+\ndiff --git a/git-pull.sh b/git-pull.sh\nindex 3526153..3cf2663 100755\n--- a/git-pull.sh\n+++ b/git-pull.sh\n@@ -125,12 +125,9 @@ test true = \"$rebase\" && {\n \tdie \"refusing to pull with rebase: your working tree is not up-to-date\"\n \n \t. git-parse-remote &&\n-\torigin=\"$1\"\n-\ttest -z \"$origin\" && origin=$(get_default_remote)\n-\treflist=\"$(get_remote_refs_for_fetch \"$@\" 2>/dev/null |\n-\t\tsed \"s|refs/heads/\\(.*\\):|\\1|\")\" &&\n+\treflist=\"$(get_remote_merge_branch \"$@\" 2>/dev/null)\" &&\n \toldremoteref=\"$(git rev-parse -q --verify \\\n-\t\t\"refs/remotes/$origin/$reflist\")\"\n+\t\t\"$reflist\")\"\n }\n orig_head=$(git rev-parse -q --verify HEAD)\n git fetch $verbosity --update-head-ok \"$@\" || exit 1\n-- \n1.6.3.1.308.g426b5\n"},{"id":"115862","messageId":"7v8wk2wbfs.fsf@alter.siamese.dyndns.org","threadId":"19736","inReplyTo":"1244451651-22651-2-git-send-email-santi@agolina.net","subject":"Re: [PATCHv3 1/4] parse-remote: function to get the tracking branch to be merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-06-08T23:30:31Z","receivedAt":"2009-06-08T23:30:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Santi Béjar <santi@agolina.net> writes:\n\n> The only user of get_remote_refs_for_fetch was \"git pull --rebase\" and\n> it only wanted the tracking branch to be merge. So, add a simple\n> function with this new meaning.\n>\n> No behavior changes.\n\nI am all for code reduction, but after following the original logic that\nuses remote_refs_for_fetch (which knows about things like \"git pull there\n+refs/heads/master:refs/heads/origin tag v1.6.0\" from the command line)\nthat in turn calls canon_refs_list_for_fetch (which returns a list e.g.\n+refs/heads/master:refs/heads/origin refs/tags/v1.6.0:refs/tags/v1.6.0),\nand do not quite see how you can casually say \"No behaviour changes.\"\n"},{"id":"115881","messageId":"adf1fd3d0906090029s2aa7fe19j7b1005997d70b92c@mail.gmail.com","threadId":"19736","inReplyTo":"7v8wk2wbfs.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv3 1/4] parse-remote: function to get the tracking branch to be merge","fromName":"Santi Béjar","fromEmail":"santi@agolina.net","sentAt":"2009-06-09T07:29:28Z","receivedAt":"2009-06-09T07:29:28Z","isPatch":false,"sender":{"key":"santi@agolina.net","avatar":null},"body":"2009/6/9 Junio C Hamano <gitster@pobox.com>:\n> Santi Béjar <santi@agolina.net> writes:\n>\n>> The only user of get_remote_refs_for_fetch was \"git pull --rebase\" and\n>> it only wanted the tracking branch to be merge. So, add a simple\n>> function with this new meaning.\n>>\n>> No behavior changes.\n>\n> I am all for code reduction, but after following the original logic that\n> uses remote_refs_for_fetch (which knows about things like \"git pull there\n> +refs/heads/master:refs/heads/origin tag v1.6.0\" from the command line)\n> that in turn calls canon_refs_list_for_fetch (which returns a list e.g.\n> +refs/heads/master:refs/heads/origin refs/tags/v1.6.0:refs/tags/v1.6.0),\n> and do not quite see how you can casually say \"No behaviour changes.\"\n\nUps, you are right. But even in you case the only important branch is\nthe first, so the only possible change in behavior is:\n\ngit pull --rebase tags v1.6.0\n\nI'll see what I can do.\n\nThanks for the review,\nSanti\n"},{"id":"115882","messageId":"adf1fd3d0906090107w994de3chb39071e5911a59ae@mail.gmail.com","threadId":"19736","inReplyTo":"adf1fd3d0906090029s2aa7fe19j7b1005997d70b92c@mail.gmail.com","subject":"Re: [PATCHv3 1/4] parse-remote: function to get the tracking branch to be merge","fromName":"Santi Béjar","fromEmail":"santi@agolina.net","sentAt":"2009-06-09T08:07:22Z","receivedAt":"2009-06-09T08:07:22Z","isPatch":false,"sender":{"key":"santi@agolina.net","avatar":null},"body":"2009/6/9 Santi Béjar <santi@agolina.net>:\n> 2009/6/9 Junio C Hamano <gitster@pobox.com>:\n>> Santi Béjar <santi@agolina.net> writes:\n>>\n>>> The only user of get_remote_refs_for_fetch was \"git pull --rebase\" and\n>>> it only wanted the tracking branch to be merge. So, add a simple\n>>> function with this new meaning.\n>>>\n>>> No behavior changes.\n>>\n>> I am all for code reduction, but after following the original logic that\n>> uses remote_refs_for_fetch (which knows about things like \"git pull there\n>> +refs/heads/master:refs/heads/origin tag v1.6.0\" from the command line)\n>> that in turn calls canon_refs_list_for_fetch (which returns a list e.g.\n>> +refs/heads/master:refs/heads/origin refs/tags/v1.6.0:refs/tags/v1.6.0),\n>> and do not quite see how you can casually say \"No behaviour changes.\"\n>\n> Ups, you are right. But even in you case the only important branch is\n> the first, so the only possible change in behavior is:\n>\n> git pull --rebase tags v1.6.0\n\nIn fact: git pull --rebase remote tags v1.6.0\n\nBut this still works because oldremoteref defaults to defaults_merge.\nSo the only behavior change is when a remote branch is\nrebased/retagged, and you have worst problems then. I think noone used\nthe rebased functionality in this way, so I don't think it is worth to\nsupport it. But if someone think it is important I'll do it.\n\nSanti\n"},{"id":"115887","messageId":"7veittrete.fsf@alter.siamese.dyndns.org","threadId":"19736","inReplyTo":"adf1fd3d0906090107w994de3chb39071e5911a59ae@mail.gmail.com","subject":"Re: [PATCHv3 1/4] parse-remote: function to get the tracking branch to be merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-06-09T08:28:45Z","receivedAt":"2009-06-09T08:28:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Santi Béjar <santi@agolina.net> writes:\n\n>> git pull --rebase tags v1.6.0\n>\n> In fact: git pull --rebase remote tags v1.6.0\n>\n> But this still works because oldremoteref defaults to defaults_merge.\n> So the only behavior change is when a remote branch is\n> rebased/retagged, and you have worst problems then. I think noone used\n> the rebased functionality in this way, so I don't think it is worth to\n> support it. But if someone think it is important I'll do it.\n\nI personally do not think supporting such a form of input is absolutely\nnecessary.  Even though technically it might be a regression, if it is so\nrare a form, we can simply say \"this strange form used to work, but now it\ndoes not; you can use this form instead to do the same thing\", and move\non.\n\nHowever, at least we should describe the change, both in the commit log\nand documentation.  Simply saying \"No behaviour change\" is not acceptable,\nwhen the code clearly is doing something else.  It needs to be backed by\nsome explanation, e.g. \"Even though this returns different results from\nthe original, the caller behaves the same because of such and such\nreasons\".\n\nWhat caught my attention was not the difference between the new code and\nthe original codepath, but your \"FIXME\" comment that said \"Currently only\nworks with the default mapping\".  My initial reaction was \"What?  The new\ncode that introduces a function for the specific task of figuring out the\nmapping does not work if the user uses a custom mapping?  What kind of\nimprovement is that???\".\n\nThe reaction was followed by \"Even if that were the case, if the original\ncode did not work in the case anyway, then it is not a regression.  The\nproposed commit log message claims that there is no behaviour change, so\nthat FIXME might not be so grave an offense.  Is it really the case?  Was\nthe original broken?\"\n\nWhile trying to figure it out, I noticed that the new code does quite a\ndifferent thing (I still haven't figured out the answer to my original\nquestion about FIXME, by the way).\n\nIn any case, if we were changing behaviour by deprecating support for a\nrarely-if-ever used syntax, it would be nice if we at least diagnosed it,\ninstead of failing, or worse yet, silently doing something different from\nthe old behaviour.\n"},{"id":"115889","messageId":"adf1fd3d0906090150k575c538ds28cd8c1a96909e9e@mail.gmail.com","threadId":"19736","inReplyTo":"7veittrete.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv3 1/4] parse-remote: function to get the tracking branch to be merge","fromName":"Santi Béjar","fromEmail":"santi@agolina.net","sentAt":"2009-06-09T08:50:12Z","receivedAt":"2009-06-09T08:50:12Z","isPatch":false,"sender":{"key":"santi@agolina.net","avatar":null},"body":"2009/6/9 Junio C Hamano <gitster@pobox.com>:\n> Santi Béjar <santi@agolina.net> writes:\n>\n>>> git pull --rebase tags v1.6.0\n>>\n>> In fact: git pull --rebase remote tags v1.6.0\n>>\n>> But this still works because oldremoteref defaults to defaults_merge.\n>> So the only behavior change is when a remote branch is\n>> rebased/retagged, and you have worst problems then. I think noone used\n>> the rebased functionality in this way, so I don't think it is worth to\n>> support it. But if someone think it is important I'll do it.\n>\n> I personally do not think supporting such a form of input is absolutely\n> necessary.  Even though technically it might be a regression, if it is so\n> rare a form, we can simply say \"this strange form used to work, but now it\n> does not; you can use this form instead to do the same thing\", and move\n> on.\n\nOK.\n\n>\n> However, at least we should describe the change, both in the commit log\n> and documentation.  Simply saying \"No behaviour change\" is not acceptable,\n> when the code clearly is doing something else.  It needs to be backed by\n> some explanation, e.g. \"Even though this returns different results from\n> the original, the caller behaves the same because of such and such\n> reasons\".\n\nOK.\n\n>\n> What caught my attention was not the difference between the new code and\n> the original codepath, but your \"FIXME\" comment that said \"Currently only\n> works with the default mapping\".  My initial reaction was \"What?  The new\n> code that introduces a function for the specific task of figuring out the\n> mapping does not work if the user uses a custom mapping?  What kind of\n> improvement is that???\".\n\nThe original code (in git-pull.sh) behaves like this, I only made it\nmore explicit.\n\n>\n> The reaction was followed by \"Even if that were the case, if the original\n> code did not work in the case anyway, then it is not a regression.  The\n> proposed commit log message claims that there is no behaviour change, so\n> that FIXME might not be so grave an offense.  Is it really the case?  Was\n> the original broken?\"\n\nYes, the original is broken.\n\n>\n> While trying to figure it out, I noticed that the new code does quite a\n> different thing (I still haven't figured out the answer to my original\n> question about FIXME, by the way).\n>\n> In any case, if we were changing behaviour by deprecating support for a\n> rarely-if-ever used syntax, it would be nice if we at least diagnosed it,\n> instead of failing, or worse yet, silently doing something different from\n> the old behaviour.\n\nI am not changing the behaviour. The old code worked exactly like the new one.\nBut I agree that this has to be documented(/deprecated?) somewhere.\n\nThe only change in behavior is when a remote tag is rebased/retagged.\n\nSanti\n"},{"id":"115902","messageId":"adf1fd3d0906090516l6b1d5ec1rdc78ffffa116b765@mail.gmail.com","threadId":"19736","inReplyTo":"adf1fd3d0906090150k575c538ds28cd8c1a96909e9e@mail.gmail.com","subject":"Re: [PATCHv3 1/4] parse-remote: function to get the tracking branch to be merge","fromName":"Santi Béjar","fromEmail":"santi@agolina.net","sentAt":"2009-06-09T12:16:03Z","receivedAt":"2009-06-09T12:16:03Z","isPatch":false,"sender":{"key":"santi@agolina.net","avatar":null},"body":"2009/6/9 Santi Béjar <santi@agolina.net>:\n> 2009/6/9 Junio C Hamano <gitster@pobox.com>:\n>> Santi Béjar <santi@agolina.net> writes:\n>>\n>>>> git pull --rebase tags v1.6.0\n>>>\n>>> In fact: git pull --rebase remote tags v1.6.0\n>>>\n>>> But this still works because oldremoteref defaults to defaults_merge.\n>>> So the only behavior change is when a remote branch is\n>>> rebased/retagged, and you have worst problems then. I think noone used\n>>> the rebased functionality in this way, so I don't think it is worth to\n>>> support it. But if someone think it is important I'll do it.\n>>\n>> I personally do not think supporting such a form of input is absolutely\n>> necessary.  Even though technically it might be a regression, if it is so\n>> rare a form, we can simply say \"this strange form used to work, but now it\n>> does not; you can use this form instead to do the same thing\", and move\n>> on.\n>\n> OK.\n>\n\nAt the end it was a little patch to get this corner case working. Here\nit is the patch to squash (I'll send later a proper patch mail, with a\ntest).\n\nAnd this additional sentence in the commit log:\n\nNo behavior changes. The new function behaves like the old code used in\n\"git pull --rebase\".\n\ndiff --git i/git-parse-remote.sh w/git-parse-remote.sh\nindex 8b3ba72..4ac277f 100755\n--- i/git-parse-remote.sh\n+++ w/git-parse-remote.sh\n@@ -238,8 +238,12 @@ get_remote_merge_branch () {\n \t    shift\n \t    # FIXME: It should return the tracking branch\n \t    #        Currently only works with the default mapping\n-\t    for ref\n-\t    do\n+\t    case \"$1\" in\n+\t    tag)\n+\t\t[ -n \"$2\" ] && echo \"refs/tags/${ref}\"\n+\t\t;;\n+\t    *)\n+\t\tref=$1\n \t\tcase \"$ref\" in\n \t\t+*)\n \t\t\tref=$(expr \"z$ref\" : 'z+\\(.*\\)')\n@@ -251,12 +255,11 @@ get_remote_merge_branch () {\n \t\t'' | HEAD ) remote=HEAD ;;\n \t\theads/*) remote=${remote#heads/} ;;\n \t\trefs/heads/*) remote=${remote#refs/heads/} ;;\n-\t\trefs/* | tags/* | remotes/* ) break\n+\t\trefs/* | tags/* | remotes/* ) remote=\n \t\tesac\n\n-\t\techo \"refs/remotes/$repo/$remote\"\n-\t\tbreak\n-\t    done\n+\t\t[ -n \"$remote\" ] && echo \"refs/remotes/$repo/$remote\"\n+\t    esac\n \t    ;;\n \tesac\n }\n\n(Sorry, if it gets corrupted)\n\nSanti\n"},{"id":"116096","messageId":"adf1fd3d0906111430m28ef712axd55942b19505a5b3@mail.gmail.com","threadId":"19736","inReplyTo":"adf1fd3d0906090516l6b1d5ec1rdc78ffffa116b765@mail.gmail.com","subject":"Re: [PATCHv3 1/4] parse-remote: function to get the tracking branch to be merge","fromName":"Santi Béjar","fromEmail":"santi@agolina.net","sentAt":"2009-06-11T21:30:50Z","receivedAt":"2009-06-11T21:30:50Z","isPatch":false,"sender":{"key":"santi@agolina.net","avatar":null},"body":"2009/6/9 Santi Béjar <santi@agolina.net>:\n> 2009/6/9 Santi Béjar <santi@agolina.net>:\n>> 2009/6/9 Junio C Hamano <gitster@pobox.com>:\n>>> Santi Béjar <santi@agolina.net> writes:\n>>>\n>>>>> git pull --rebase tags v1.6.0\n>>>>\n>>>> In fact: git pull --rebase remote tags v1.6.0\n>>>>\n>>>> But this still works because oldremoteref defaults to defaults_merge.\n>>>> So the only behavior change is when a remote branch is\n>>>> rebased/retagged, and you have worst problems then. I think noone used\n>>>> the rebased functionality in this way, so I don't think it is worth to\n>>>> support it. But if someone think it is important I'll do it.\n>>>\n>>> I personally do not think supporting such a form of input is absolutely\n>>> necessary.  Even though technically it might be a regression, if it is so\n>>> rare a form, we can simply say \"this strange form used to work, but now it\n>>> does not; you can use this form instead to do the same thing\", and move\n>>> on.\n>>\n>> OK.\n>>\n>\n> At the end it was a little patch to get this corner case working. Here\n> it is the patch to squash (I'll send later a proper patch mail, with a\n> test).\n>\n> And this additional sentence in the commit log:\n>\n> No behavior changes. The new function behaves like the old code used in\n> \"git pull --rebase\".\n>\n\nAt the end the original code does not handle this case, as it asumes\nthat the rebase ref is a remote commit in refs/remotes/$origin/$ref.\nSo the new code behaves exactly like the old one. I'll send all again\nwith an updated commit message explaining all this.\n\nSanti\n"}]}