{"thread":{"id":"39356","subject":"[PATCH 0/2] Improve git-pull's option parsing","startedAt":"2015-05-18T13:54:54Z","lastAt":"2015-05-21T08:41:46Z","messageCount":5,"participants":["Paul Tan","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"261443","messageId":"1431957296-31534-1-git-send-email-pyokagan@gmail.com","threadId":"39356","inReplyTo":null,"subject":"[PATCH 0/2] Improve git-pull's option parsing","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-18T13:54:54Z","receivedAt":"2015-05-18T13:54:54Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"While parsing the command-line arguments, git-pull stops parsing at the\nfirst unrecognized option, assuming that any subsequent options are for\ngit-fetch, and can thus be kept in the shell's positional parameters\nlist, so that it can be passed to git-fetch via the expansion of \"$@\".\n\nHowever, certain functions in git-pull assume that the positional\nparameters do not contain any options. Fix this by making git-pull handle\ngit-fetch's options as well at the option parsing stage.\n\nWith this change in place, we can move on to migrate git-pull to use\ngit-rev-parse --parseopt such that its option parsing is consistent with the\nother git commands.\n\nPaul Tan (2):\n  pull: handle git-fetch's options as well\n  pull: use git-rev-parse --parseopt for option parsing\n\n git-pull.sh             | 137 ++++++++++++++++++++++++++++++++++--------------\n t/t5520-pull.sh         |  20 +++++++\n t/t5521-pull-options.sh |  14 +++++\n 3 files changed, 132 insertions(+), 39 deletions(-)\n\n-- \n2.1.4\n"},{"id":"261445","messageId":"1431957296-31534-2-git-send-email-pyokagan@gmail.com","threadId":"39356","inReplyTo":"1431957296-31534-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH 1/2] pull: handle git-fetch's options as well","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-18T13:54:55Z","receivedAt":"2015-05-18T13:54:55Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"While parsing the command-line arguments, git-pull stops parsing at the\nfirst unrecognized option, assuming that any subsequent options are for\ngit-fetch, and can thus be kept in the shell's positional parameters\nlist, so that it can be passed to git-fetch via the expansion of \"$@\".\n\nHowever, certain functions in git-pull assume that the positional\nparameters do not contain any options:\n\n* error_on_no_merge_candidates() uses the number of positional\n  parameters to determine which error message to print out, and will\n  thus print the wrong message if git-fetch's options are passed in as\n  well.\n\n* the call to get_remote_merge_branch() assumes that the positional\n  parameters only contains the optional repo and refspecs, and will\n  thus silently fail if git-fetch's options are passed in as well.\n\n* --dry-run is a valid git-fetch option, but if provided after any\n  git-fetch options, it is not recognized by git-pull and thus git-pull\n  will continue to run the merge or rebase.\n\nFix these bugs by teaching git-pull to parse git-fetch's options as\nwell. Add tests to prevent regressions.\n\nSigned-off-by: Paul Tan <pyokagan@gmail.com>\n---\n git-pull.sh             | 34 ++++++++++++++++++++++++++++++++--\n t/t5520-pull.sh         | 20 ++++++++++++++++++++\n t/t5521-pull-options.sh | 14 ++++++++++++++\n 3 files changed, 66 insertions(+), 2 deletions(-)\n\ndiff --git a/git-pull.sh b/git-pull.sh\nindex 5ff4545..633c385 100755\n--- a/git-pull.sh\n+++ b/git-pull.sh\n@@ -163,11 +163,39 @@ do\n \t--d|--dr|--dry|--dry-|--dry-r|--dry-ru|--dry-run)\n \t\tdry_run=--dry-run\n \t\t;;\n+\t--all|--no-all)\n+\t\tall=$1 ;;\n+\t-a|--append|--no-append)\n+\t\tappend=$1 ;;\n+\t--upload-pack=*|--no-upload-pack)\n+\t\tupload_pack=$1 ;;\n+\t-f|--force|--no-force)\n+\t\tforce=\"$force $1\" ;;\n+\t-t|--tags|--no-tags)\n+\t\ttags=$1 ;;\n+\t-p|--prune|--no-prune)\n+\t\tprune=$1 ;;\n+\t-k|--keep|--no-keep)\n+\t\tkeep=$1 ;;\n+\t--depth=*|--no-depth)\n+\t\tdepth=$1 ;;\n+\t--unshallow|--no-unshallow)\n+\t\tunshallow=$1 ;;\n+\t--update-shallow|--no-update-shallow)\n+\t\tupdate_shallow=$1 ;;\n+\t--refmap=*|--no-refmap)\n+\t\trefmap=$1 ;;\n \t-h|--help-all)\n \t\tusage\n \t\t;;\n+\t--)\n+\t\tshift\n+\t\tbreak\n+\t\t;;\n+\t-*)\n+\t\tusage\n+\t\t;;\n \t*)\n-\t\t# Pass thru anything that may be meant for fetch.\n \t\tbreak\n \t\t;;\n \tesac\n@@ -254,7 +282,9 @@ test true = \"$rebase\" && {\n \toldremoteref=$(git merge-base --fork-point \"$remoteref\" $curr_branch 2>/dev/null)\n }\n orig_head=$(git rev-parse -q --verify HEAD)\n-git fetch $verbosity $progress $dry_run $recurse_submodules --update-head-ok \"$@\" || exit 1\n+git fetch $verbosity $progress $dry_run $recurse_submodules $all $append \\\n+$upload_pack $force $tags $prune $keep $depth $unshallow $update_shallow \\\n+$refmap --update-head-ok \"$@\" || exit 1\n test -z \"$dry_run\" || exit 0\n \n curr_head=$(git rev-parse -q --verify HEAD)\ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex 62dbfb5..ea73f2f 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -160,6 +160,18 @@ test_expect_success 'fail if no configuration for current branch' '\n \ttest \"$(cat file)\" = file\n '\n \n+test_expect_success 'pull --all: fail if no configuration for current branch' '\n+\tgit remote add test_remote . &&\n+\ttest_when_finished \"git remote remove test_remote\" &&\n+\tgit checkout -b test copy^ &&\n+\ttest_when_finished \"git checkout -f copy && git branch -D test\" &&\n+\ttest_config branch.test.remote test_remote &&\n+\ttest \"$(cat file)\" = file &&\n+\ttest_must_fail git pull --all 2>err &&\n+\ttest_i18ngrep \"There is no tracking information\" err &&\n+\ttest \"$(cat file)\" = file\n+'\n+\n test_expect_success 'fail if upstream branch does not exist' '\n \tgit checkout -b test copy^ &&\n \ttest_when_finished \"git checkout -f copy && git branch -D test\" &&\n@@ -366,6 +378,14 @@ test_expect_success '--rebase with rebased upstream' '\n \n '\n \n+test_expect_success '--rebase -f with rebased upstream' '\n+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n+\tgit reset --hard to-rebase-orig &&\n+\tgit pull --rebase -f me copy &&\n+\ttest \"conflicting modification\" = \"$(cat file)\" &&\n+\ttest file = \"$(cat file2)\"\n+'\n+\n test_expect_success '--rebase with rebased default upstream' '\n \n \tgit update-ref refs/remotes/me/copy copy-orig &&\ndiff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh\nindex 56e7377..d61af3d 100755\n--- a/t/t5521-pull-options.sh\n+++ b/t/t5521-pull-options.sh\n@@ -130,4 +130,18 @@ test_expect_success 'git pull --dry-run' '\n \t)\n '\n \n+test_expect_success 'git pull --all --dry-run' '\n+\ttest_when_finished \"rm -rf clonedmulti\" &&\n+\tgit init clonedry &&\n+\t(\n+\t\tcd clonedry &&\n+\t\tgit remote add origin ../parent &&\n+\t\tgit pull --all --dry-run &&\n+\t\ttest_path_is_missing .git/FETCH_HEAD &&\n+\t\ttest_path_is_missing .git/refs/remotes/origin/master &&\n+\t\ttest_path_is_missing .git/index &&\n+\t\ttest_path_is_missing file\n+\t)\n+'\n+\n test_done\n-- \n2.1.4\n"},{"id":"261444","messageId":"1431957296-31534-3-git-send-email-pyokagan@gmail.com","threadId":"39356","inReplyTo":"1431957296-31534-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH 2/2] pull: use git-rev-parse --parseopt for option parsing","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-18T13:54:56Z","receivedAt":"2015-05-18T13:54:56Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"To enable unambiguous parsing of abbreviated options, bundled short\noptions, separate form options and to provide consistent usage help, use\ngit-rev-parse --parseopt for option parsing. This allows the option parsing\ncode to be simplified.\n\nSigned-off-by: Paul Tan <pyokagan@gmail.com>\n---\n git-pull.sh | 109 ++++++++++++++++++++++++++++++++++++++----------------------\n 1 file changed, 69 insertions(+), 40 deletions(-)\n\ndiff --git a/git-pull.sh b/git-pull.sh\nindex 633c385..67f825c 100755\n--- a/git-pull.sh\n+++ b/git-pull.sh\n@@ -4,13 +4,53 @@\n #\n # Fetch one or more remote refs and merge it/them into the current HEAD.\n \n-USAGE='[-n | --no-stat] [--[no-]commit] [--[no-]squash] [--[no-]ff|--ff-only] [--[no-]rebase|--rebase=preserve] [-s strategy]... [<fetch-options>] <repo> <head>...'\n-LONG_USAGE='Fetch one or more remote refs and integrate it/them with the current HEAD.'\n SUBDIRECTORY_OK=Yes\n-OPTIONS_SPEC=\n+OPTIONS_KEEPDASHDASH=Yes\n+OPTIONS_STUCKLONG=Yes\n+OPTIONS_SPEC=\"\\\n+git pull [options] [<repo> [<refspec>...]]\n+\n+Fetch one or more remote refs and integrate it/them with the current HEAD.\n+--\n+v,verbose                  be more verbose\n+q,quiet                    be more quiet\n+progress                   force progress reporting\n+\n+  Options related to merging\n+r,rebase?false|true|preserve incorporate changes by rebasing rather than merging\n+n!                         do not show a diffstat at the end of the merge\n+stat                       show a diffstat at the end of the merge\n+summary                    (synonym to --stat)\n+log?n                      add (at most <n>) entries from shortlog to merge commit message\n+squash                     create a single commit instead of doing a merge\n+commit                     perform a commit if the merge succeeds (default)\n+e,edit                       edit message before committing\n+ff                         allow fast-forward\n+ff-only!                   abort if fast-forward is not possible\n+verify-signatures          verify that the named commit has a valid GPG signature\n+s,strategy=strategy        merge strategy to use\n+X,strategy-option=option   option for selected merge strategy\n+S,gpg-sign?key-id          GPG sign commit\n+\n+  Options related to fetching\n+all                        fetch from all remotes\n+a,append                   append to .git/FETCH_HEAD instead of overwriting\n+upload-pack=path           path to upload pack on remote end\n+f,force                    force overwrite of local branch\n+t,tags                     fetch all tags and associated objects\n+p,prune                    prune remote-tracking branches no longer on remote\n+recurse-submodules?on-demand control recursive fetching of submodules\n+dry-run                    dry run\n+k,keep                     keep downloaded pack\n+depth=depth                deepen history of shallow clone\n+unshallow                  convert to a complete repository\n+update-shallow             accept refs that update .git/shallow\n+refmap=refmap              specify fetch refmap\n+\"\n+test $# -gt 0 && args=\"$*\"\n . git-sh-setup\n . git-sh-i18n\n-set_reflog_action \"pull${1+ $*}\"\n+set_reflog_action \"pull${args+ $args}\"\n require_work_tree_exists\n cd_to_toplevel\n \n@@ -83,17 +123,17 @@ do\n \t\tdiffstat=--stat ;;\n \t--log|--log=*|--no-log)\n \t\tlog_arg=\"$1\" ;;\n-\t--no-c|--no-co|--no-com|--no-comm|--no-commi|--no-commit)\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-e|--edit)\n \t\tedit=--edit ;;\n \t--no-edit)\n \t\tedit=--no-edit ;;\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@@ -101,39 +141,25 @@ do\n \t\tno_ff=--no-ff ;;\n \t--ff-only)\n \t\tff_only=--ff-only ;;\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-s*|--strategy=*)\n+\t\tstrategy_args=\"$strategy_args $1\"\n \t\t;;\n-\t-X*)\n-\t\tcase \"$#,$1\" in\n-\t\t1,-X)\n-\t\t\tusage ;;\n-\t\t*,-X)\n-\t\t\txx=\"-X $(git rev-parse --sq-quote \"$2\")\"\n-\t\t\tshift ;;\n-\t\t*,*)\n-\t\t\txx=$(git rev-parse --sq-quote \"$1\") ;;\n-\t\tesac\n-\t\tmerge_args=\"$merge_args$xx \"\n+\t--no-strategy)\n+\t\tstrategy_args=\n+\t\t;;\n+\t-X*|--strategy-option=*)\n+\t\tmerge_args=\"$merge_args $(git rev-parse --sq-quote \"$1\")\"\n+\t\t;;\n+\t--no-strategy-option)\n+\t\tmerge_args=\n \t\t;;\n-\t-r=*|--r=*|--re=*|--reb=*|--reba=*|--rebas=*|--rebase=*)\n+\t-r*|--rebase=*)\n \t\trebase=\"${1#*=}\"\n \t\t;;\n-\t-r|--r|--re|--reb|--reba|--rebas|--rebase)\n+\t--rebase)\n \t\trebase=true\n \t\t;;\n-\t--no-r|--no-re|--no-reb|--no-reba|--no-rebas|--no-rebase)\n+\t--no-rebase)\n \t\trebase=false\n \t\t;;\n \t--recurse-submodules)\n@@ -160,9 +186,15 @@ do\n \t-S*)\n \t\tgpg_sign_args=$(git rev-parse --sq-quote \"$1\")\n \t\t;;\n-\t--d|--dr|--dry|--dry-|--dry-r|--dry-ru|--dry-run)\n+\t--no-gpg-sign)\n+\t\tgpg_sign_args=--no-gpg-sign\n+\t\t;;\n+\t--dry-run)\n \t\tdry_run=--dry-run\n \t\t;;\n+\t--no-dry-run)\n+\t\tdry_run=\n+\t\t;;\n \t--all|--no-all)\n \t\tall=$1 ;;\n \t-a|--append|--no-append)\n@@ -192,11 +224,8 @@ do\n \t\tshift\n \t\tbreak\n \t\t;;\n-\t-*)\n-\t\tusage\n-\t\t;;\n \t*)\n-\t\tbreak\n+\t\tusage\n \t\t;;\n \tesac\n \tshift\n-- \n2.1.4\n"},{"id":"261448","messageId":"389cf040410c499b2f382bfbcdb526d4@www.dscho.org","threadId":"39356","inReplyTo":"1431957296-31534-3-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH 2/2] pull: use git-rev-parse --parseopt for option parsing","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-05-18T14:43:49Z","receivedAt":"2015-05-18T14:43:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Paul,\n\nOn 2015-05-18 15:54, Paul Tan wrote:\n\n> diff --git a/git-pull.sh b/git-pull.sh\n> index 633c385..67f825c 100755\n> --- a/git-pull.sh\n> +++ b/git-pull.sh\n> @@ -4,13 +4,53 @@\n>  #\n>  # Fetch one or more remote refs and merge it/them into the current HEAD.\n>  \n> -USAGE='[-n | --no-stat] [--[no-]commit] [--[no-]squash]\n> [--[no-]ff|--ff-only] [--[no-]rebase|--rebase=preserve] [-s\n> strategy]... [<fetch-options>] <repo> <head>...'\n> -LONG_USAGE='Fetch one or more remote refs and integrate it/them with\n> the current HEAD.'\n>  SUBDIRECTORY_OK=Yes\n> -OPTIONS_SPEC=\n> +OPTIONS_KEEPDASHDASH=Yes\n\nI have to admit that I was puzzled by this at first. But it seems that the intention is to handle a dashdash argument as an error, therefore we have to keep it. Is my understanding correct?\n\nCiao,\nDscho\n"},{"id":"261764","messageId":"CACRoPnS2AwxT+0By7esubp8ctF9=gcwnr=coPDF9PEvzyyji+g@mail.gmail.com","threadId":"39356","inReplyTo":"389cf040410c499b2f382bfbcdb526d4@www.dscho.org","subject":"Re: [PATCH 2/2] pull: use git-rev-parse --parseopt for option parsing","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-21T08:41:46Z","receivedAt":"2015-05-21T08:41:46Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Hi,\n\nOn Mon, May 18, 2015 at 10:43 PM, Johannes Schindelin\n<johannes.schindelin@gmx.de> wrote:\n> On 2015-05-18 15:54, Paul Tan wrote:\n>\n>> diff --git a/git-pull.sh b/git-pull.sh\n>> index 633c385..67f825c 100755\n>> --- a/git-pull.sh\n>> +++ b/git-pull.sh\n>> @@ -4,13 +4,53 @@\n>>  #\n>>  # Fetch one or more remote refs and merge it/them into the current HEAD.\n>>\n>> -USAGE='[-n | --no-stat] [--[no-]commit] [--[no-]squash]\n>> [--[no-]ff|--ff-only] [--[no-]rebase|--rebase=preserve] [-s\n>> strategy]... [<fetch-options>] <repo> <head>...'\n>> -LONG_USAGE='Fetch one or more remote refs and integrate it/them with\n>> the current HEAD.'\n>>  SUBDIRECTORY_OK=Yes\n>> -OPTIONS_SPEC=\n>> +OPTIONS_KEEPDASHDASH=Yes\n>\n> I have to admit that I was puzzled by this at first. But it seems that the intention is to handle a dashdash argument as an error, therefore we have to keep it. Is my understanding correct?\n\nUgh, I thought we had to keep the dashdash so that we could\ndisambiguate the arguments from the options, but it turns out that\nrev-parse would add its own dashdash. So, no, we don't need to keep\nthe dashdash.\n\nThanks for catching this. Fixed.\n\nRegards,\nPaul\n"}]}