{"thread":{"id":"12265","subject":"[PATCH 1/2] pull: pass --strategy along to to rebase","startedAt":"2008-02-23T00:52:29Z","lastAt":"2008-02-26T21:37:41Z","messageCount":7,"participants":["Jay Soffian","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"69630","messageId":"1203727950-14980-1-git-send-email-jaysoffian@gmail.com","threadId":"12265","inReplyTo":null,"subject":"[PATCH 1/2] pull: pass --strategy along to to rebase","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2008-02-23T00:52:29Z","receivedAt":"2008-02-23T00:52:29Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"rebase supports --strategy, so pull should pass the option along to it.\n\nSigned-off-by: Jay Soffian \n---\nI noticed the other day that pull didn't pass --strategy along to rebase, but\nforgot I'd noticed, even though Johannes replied w/a patch. Anyway, my next\npatch documents pull's usage via its OPTIONS_SPEC and I noticed the problem\nagain. I fix it here so that I can document its use in the next patch. :-)\n\n git-pull.sh | 3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/git-pull.sh b/git-pull.sh\nindex 46da0f4..3ce32b5 100755\n--- a/git-pull.sh\n+++ b/git-pull.sh\n@@ -174,6 +174,7 @@ fi\n \n merge_name=$(git fmt-merge-msg <\"$GIT_DIR/FETCH_HEAD\") || exit\n test true = \"$rebase\" &&\n-\texec git-rebase --onto $merge_head ${oldremoteref:-$merge_head}\n+\texec git-rebase $strategy_args --onto $merge_head \\\n+\t${oldremoteref:-$merge_head}\n exec git-merge $no_summary $no_commit $squash $no_ff $strategy_args \\\n \t\"$merge_name\" HEAD $merge_head\n-- \n1.5.4.2.285.g1e9f\n"},{"id":"69631","messageId":"1203727950-14980-2-git-send-email-jaysoffian@gmail.com","threadId":"12265","inReplyTo":"1203727950-14980-1-git-send-email-jaysoffian@gmail.com","subject":"[PATCH 2/2] pull: document usage via OPTIONS_SPEC","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2008-02-23T00:52:30Z","receivedAt":"2008-02-23T00:52:30Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"document usage via OPTIONS_SPEC\n\nSigned-off-by: Jay Soffian \n---\n git-pull.sh | 32 +++++++++++++++++++++++++++++---\n 1 files changed, 29 insertions(+), 3 deletions(-)\n\ndiff --git a/git-pull.sh b/git-pull.sh\nindex 3ce32b5..194c1d0 100755\n--- a/git-pull.sh\n+++ b/git-pull.sh\n@@ -4,10 +4,36 @@\n #\n # Fetch one or more remote refs and merge it/them into the current HEAD.\n \n-USAGE='[-n | --no-summary] [--[no-]commit] [--[no-]squash] [--[no-]ff] [-s strategy]... [] ...'\n-LONG_USAGE='Fetch one or more remote refs and merge it/them into the current HEAD.'\n SUBDIRECTORY_OK=Yes\n-OPTIONS_SPEC=\n+OPTIONS_SPEC=\"\\\n+git pull [options] [] []\n+--\n+ fetch options\n+q,quiet make the fetch process less verbose\n+v,verbose make the fetch process more verbose\n+a,append append to FETCH_HEAD instead of overwritting\n+upload-pack=, specify path to git-fetch-pack on remote end\n+f,force force local branch to be updated by remote branch\n+t,tags fetch all remote tags\n+no-tags do not fetch any remote tags\n+k,keep keep downloaded pack\n+u,update-head-ok used internally between git pull and git fetch\n+depth= deep shallow history by specified number of commits\n+ merge options\n+summary show diffstat at end of merge\n+n,no-summary do not show diff stat at end of merge\n+commit commit the result after merging\n+no-commit do not commit the result after merging\n+squash update the index and working tree only\n+no-squash override --squash, perform a normal merge and commit\n+ff only update branch pointer if merge is a fast-forward\n+no-ff do a merge commit even if the merge is a fast-forward\n+s,strategy= use given merge strategy\n+ rebase options\n+rebase rebase after fetching\n+no-rebase merge after fetching\n+s,strategy= use given merge strategy; implies -m to rebase\n+\"\n . git-sh-setup\n set_reflog_action \"pull $*\"\n require_work_tree\n-- \n1.5.4.2.285.g1e9f\n"},{"id":"69633","messageId":"7vk5kw5v79.fsf@gitster.siamese.dyndns.org","threadId":"12265","inReplyTo":"1203727950-14980-2-git-send-email-jaysoffian@gmail.com","subject":"Re: [PATCH 2/2] pull: document usage via OPTIONS_SPEC","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-23T01:15:54Z","receivedAt":"2008-02-23T01:15:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"OPTIONS_SPEC is not just for documentation but it actively mucks\nwith the incoming options list.\n\nHave you checked if you did not break the actual parsing?\n"},{"id":"69637","messageId":"76718490802221740ldbf077ch80aa6ec3fedbba61@mail.gmail.com","threadId":"12265","inReplyTo":"7vk5kw5v79.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] pull: document usage via OPTIONS_SPEC","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2008-02-23T01:40:15Z","receivedAt":"2008-02-23T01:40:15Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Fri, Feb 22, 2008 at 8:15 PM, Junio C Hamano wrote:\n> OPTIONS_SPEC is not just for documentation but it actively mucks\n> with the incoming options list.\n>\n> Have you checked if you did not break the actual parsing?\n\nDuh, no. Sorry. Will do.\n\nj.\n"},{"id":"70026","messageId":"1204058554-74593-1-git-send-email-jaysoffian@gmail.com","threadId":"12265","inReplyTo":"7vk5kw5v79.fsf@gitster.siamese.dyndns.org","subject":"[PATCH v2] pull: document usage via OPTIONS_SPEC","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2008-02-26T20:42:34Z","receivedAt":"2008-02-26T20:42:34Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"Replaced USAGE and LONG_USAGE with OPTIONS_SPEC\n\nSigned-off-by: Jay Soffian \n---\nOkay, I'm pretty sure this is correct now. make test passes, and it also\npasses my \"it looks right\" test after having gone over it a few times.\n\nThis isn't quite ready for inclusion yet though because I want some feedback:\n\n * There is one semantic change. You can't use \"-s=\" anymore. That's not\n really proper usage of a short option (it's either \"-s\" or \n \"-s \"). Is it okay to not accept the \"-s=\" form?\n\n * Is it worth doing this at all? If the plan is to rewrite everything as\n builtins I kinda feel like I'm wasting my time. If not, I'll work on doing\n this for all the other scripts which are missing an OPTIONS_SPEC. Then maybe\n they can all be merged at once?\n\n * You can see I added the GIT_PULL_DEBUG_ARGS blob. Frankly, I can't really\n think of a good way to test this change other than to do something like that\n and then add a test for each individual option. But that's going to really\n bloat the test suite. Is it worth it to do this for every script? Or is\n passing the existing test suite and enough eyeballs looking at it in next\n good enough? Can you think of another way to check for regressions?\n\nj.\n\n git-pull.sh | 94 ++++++++++++++++++++++++++++++++++++++--------------------\n 1 files changed, 61 insertions(+), 33 deletions(-)\n\ndiff --git a/git-pull.sh b/git-pull.sh\nindex 3ce32b5..8b1d732 100755\n--- a/git-pull.sh\n+++ b/git-pull.sh\n@@ -4,10 +4,36 @@\n #\n # Fetch one or more remote refs and merge it/them into the current HEAD.\n \n-USAGE='[-n | --no-summary] [--[no-]commit] [--[no-]squash] [--[no-]ff] [-s strategy]... [] ...'\n-LONG_USAGE='Fetch one or more remote refs and merge it/them into the current HEAD.'\n SUBDIRECTORY_OK=Yes\n-OPTIONS_SPEC=\n+OPTIONS_SPEC=\"\\\n+git pull [options] [] []\n+--\n+ fetch options\n+q,quiet make the fetch process less verbose\n+v,verbose make the fetch process more verbose\n+a,append append to FETCH_HEAD instead of overwritting\n+upload-pack=, specify path to git-fetch-pack on remote end\n+f,force force local branch to be updated by remote branch\n+t,tags fetch all remote tags\n+no-tags do not fetch any remote tags\n+k,keep keep downloaded pack\n+u,update-head-ok used internally between git pull and git fetch\n+depth= deep shallow history by specified number of commits\n+ merge options\n+summary show diffstat at end of merge\n+n,no-summary do not show diff stat at end of merge\n+commit commit the result after merging\n+no-commit do not commit the result after merging\n+squash update the index and working tree only\n+no-squash override --squash, perform a normal merge and commit\n+ff only update branch pointer if merge is a fast-forward\n+no-ff do a merge commit even if the merge is a fast-forward\n+s,strategy= use given merge strategy\n+ rebase options\n+rebase rebase after fetching\n+no-rebase merge after fetching\n+s,strategy= use given merge strategy; implies -m to rebase\n+\"\n . git-sh-setup\n set_reflog_action \"pull $*\"\n require_work_tree\n@@ -23,46 +49,33 @@ rebase=$(git config --bool branch.$curr_branch_short.rebase)\n while :\n do\n \tcase \"$1\" in\n-\t-n|--n|--no|--no-|--no-s|--no-su|--no-sum|--no-summ|\\\n-\t\t--no-summa|--no-summar|--no-summary)\n+\t-n|--no-summary)\n \t\tno_summary=-n ;;\n \t--summary)\n-\t\tno_summary=$1\n-\t\t;;\n-\t--no-c|--no-co|--no-com|--no-comm|--no-commi|--no-commit)\n+\t\tno_summary=$1 ;;\n+\t--no-commit)\n \t\tno_commit=--no-commit ;;\n-\t--c|--co|--com|--comm|--commi|--commit)\n+\t--commit)\n \t\tno_commit=--commit ;;\n-\t--sq|--squ|--squa|--squas|--squash)\n+\t--squash)\n \t\tsquash=--squash ;;\n-\t--no-sq|--no-squ|--no-squa|--no-squas|--no-squash)\n+\t--no-squash)\n \t\tsquash=--no-squash ;;\n \t--ff)\n \t\tno_ff=--ff ;;\n \t--no-ff)\n \t\tno_ff=--no-ff ;;\n-\t-s=*|--s=*|--st=*|--str=*|--stra=*|--strat=*|--strate=*|\\\n-\t\t--strateg=*|--strategy=*|\\\n-\t-s|--s|--st|--str|--stra|--strat|--strate|--strateg|--strategy)\n-\t\tcase \"$#,$1\" in\n-\t\t*,*=*)\n-\t\t\tstrategy=`expr \"z$1\" : 'z-[^=]*=\$.*\$'` ;;\n-\t\t1,*)\n-\t\t\tusage ;;\n-\t\t*)\n-\t\t\tstrategy=\"$2\"\n-\t\t\tshift ;;\n-\t\tesac\n-\t\tstrategy_args=\"${strategy_args}-s $strategy \"\n-\t\t;;\n-\t-r|--r|--re|--reb|--reba|--rebas|--rebase)\n-\t\trebase=true\n-\t\t;;\n-\t--no-r|--no-re|--no-reb|--no-reba|--no-rebas|--no-rebase)\n-\t\trebase=false\n+\t-s|--strategy)\n+\t\tshift\n+\t\tstrategy_args=\"${strategy_args}-s $1 \"\n \t\t;;\n-\t-h|--h|--he|--hel|--help)\n-\t\tusage\n+\t-r|--rebase)\n+\t\trebase=true ;;\n+\t--no-rebase)\n+\t\trebase=false ;;\n+\t--)\n+\t\tshift\n+\t\tbreak\n \t\t;;\n \t*)\n \t\t# Pass thru anything that may be meant for fetch.\n@@ -72,12 +85,27 @@ do\n \tshift\n done\n \n+test '' != \"$GIT_PULL_DEBUG_ARGS\" && {\n+\texec >&2\n+\tfor var in strategy_args no_summary no_commit squash no_ff rebase\n+\tdo\n+\t\techo \"$var='$(eval echo \"\\$$var\")'\"\n+\tdone\n+\targnum=1\n+\tfor opt\n+\tdo\n+\t\techo \"$argnum='$opt'\"\n+\t\targnum=$(($argnum + 1))\n+\tdone\n+\texit 0\n+}\n+\n error_on_no_merge_candidates () {\n \texec >&2\n \tfor opt\n \tdo\n \t\tcase \"$opt\" in\n-\t\t-t|--t|--ta|--tag|--tags)\n+\t\t-t|--tags)\n \t\t\techo \"Fetching tags only, you probably meant:\"\n \t\t\techo \" git fetch --tags\"\n \t\t\texit 1\n-- \n1.5.4.3.345.g651f8\n"},{"id":"70031","messageId":"7v4pbv4dkw.fsf@gitster.siamese.dyndns.org","threadId":"12265","inReplyTo":"1204058554-74593-1-git-send-email-jaysoffian@gmail.com","subject":"Re: [PATCH v2] pull: document usage via OPTIONS_SPEC","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-26T21:23:11Z","receivedAt":"2008-02-26T21:23:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jay Soffian writes:\n\n> * There is one semantic change. You can't use \"-s=\" anymore. That's not\n> really proper usage of a short option (it's either \"-s\" or \n> \"-s \"). Is it okay to not accept the \"-s=\" form?\n\nWell, with my maintainer hat on, I must resist _any_ change ;-).\nPersonally I would not mind this. It is a borderline between\nregression and making the option parameters more consistent.\n\n> * Is it worth doing this at all? If the plan is to rewrite everything as\n> builtins I kinda feel like I'm wasting my time.\n\nIf a contributor feels wasting his time, what should reviewers\nfeel reviewing such patches ;-)?\n\n> * You can see I added the GIT_PULL_DEBUG_ARGS blob. Frankly, I can't really\n> think of a good way to test this change other than to do\n> something like that and then add a test for each individual option.\n\nI see this would be useful while you are developing. But the\ntestsuite is meant to check the behaviour that is externally\nobservable. It does not matter if you change the way you\ninternally represent --verbose option from settting $verbose to\nresetting $quiet, but tests based on GIT_PULL_DEBUG_ARGS would\nnotice the difference in the implementation detail, so I do not\nsee much point leaving this in; it would not be useful for test\nscripts, I suspect.\n\n> +git pull [options] [] []\n> +--\n> + fetch options\n> +q,quiet make the fetch process less verbose\n> +v,verbose make the fetch process more verbose\n> +a,append append to FETCH_HEAD instead of overwritting\n> +upload-pack=, specify path to git-fetch-pack on remote end\n> +f,force force local branch to be updated by remote branch\n> +t,tags fetch all remote tags\n\nWhile it is technically correct that you _can_ feed any option\nmeant for git-fetch to this command, some options do not make\nany sense in the context of git-pull, and we should not\nadvertise them, or better yet, actively reject them if you can.\n\nI think --tags is one of them.\n\n> . git-sh-setup\n> set_reflog_action \"pull $*\"\n> require_work_tree\n> @@ -23,46 +49,33 @@ rebase=$(git config --bool branch.$curr_branch_short.rebase)\n> while :\n> do\n> \tcase \"$1\" in\n> ...\n> \t\t;;\n> \t*)\n> \t\t# Pass thru anything that may be meant for fetch.\n>\t\tbreak\n\nBecause the loop breaks here, the option description above\nshould mention that options meant for fetch should come after\nall the options you want to give to git-pull itself. For\nexample, I do not think \"git pull -q -s stupid $there $that\" is\nmeant to work.\n\nA better long-term alternative would be to lift that restriction.\n\nI do not recall offhand but does the parse-options reorder the\noptions in any way? If that is the case, it makes the above not\na long-term thing but a must-be-done in a patch that starts to\nuse parse-options.\n"},{"id":"70034","messageId":"76718490802261337i16e5f27bg11cf452fdc0542ca@mail.gmail.com","threadId":"12265","inReplyTo":"7v4pbv4dkw.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2] pull: document usage via OPTIONS_SPEC","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2008-02-26T21:37:41Z","receivedAt":"2008-02-26T21:37:41Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Tue, Feb 26, 2008 at 4:23 PM, Junio C Hamano wrote:\n> Jay Soffian writes:\n>\n> > * There is one semantic change. You can't use \"-s=\" anymore. That's not\n> > really proper usage of a short option (it's either \"-s\" or\n> > \"-s \"). Is it okay to not accept the \"-s=\" form?\n>\n> Well, with my maintainer hat on, I must resist _any_ change ;-).\n> Personally I would not mind this. It is a borderline between\n> regression and making the option parameters more consistent.\n\nMy reasoning was that were the script converted to a built-in, the -s= behavior\nwould not likely be maintained (or even noticed...). And that form certainly\nisn't documented. I think it was an accident that it ever worked.\n\n> If a contributor feels wasting his time, what should reviewers\n> feel reviewing such patches ;-)?\n\nI get the smiley, but let me rephrase: is there a long term goal to replace all\nshell scripts with builtins?\n\nAnd to answer your rhetorical question, reviewers should feel appreciated,\nbecause they are.\n\n> While it is technically correct that you _can_ feed any option\n> meant for git-fetch to this command, some options do not make\n> any sense in the context of git-pull, and we should not\n> advertise them, or better yet, actively reject them if you can.\n\nWell then we have a documentation issue then, because it was from the git pull\ndocs that I wrote the option spec. So I suppose this patch should include a\ndocumentation cleanup at the same time.\n\n> Because the loop breaks here, the option description above\n> should mention that options meant for fetch should come after\n> all the options you want to give to git-pull itself. For\n> example, I do not think \"git pull -q -s stupid $there $that\" is\n> meant to work.\n>\n> A better long-term alternative would be to lift that restriction.\n\nSigh, nothing is ever as simple as it seems. And here I was just trying to\nimprove the usage statement. :-(\n\n> I do not recall offhand but does the parse-options reorder the\n> options in any way? If that is the case, it makes the above not\n> a long-term thing but a must-be-done in a patch that starts to\n> use parse-options.\n\nI don't think it does.\n\nBack to the drawing board...\n\nj.\n"}]}