* [PATCH 1/2] pull: pass --strategy along to to rebase @ 2008-02-23 0:52 Jay Soffian 2008-02-23 0:52 ` [PATCH 2/2] pull: document usage via OPTIONS_SPEC Jay Soffian 0 siblings, 1 reply; 7+ messages in thread From: Jay Soffian @ 2008-02-23 0:52 UTC (permalink / raw) To: git; +Cc: Jay Soffian, Johannes Schindelin, Junio C Hamano rebase supports --strategy, so pull should pass the option along to it. Signed-off-by: Jay Soffian <jaysoffian@gmail.com> --- I noticed the other day that pull didn't pass --strategy along to rebase, but forgot I'd noticed, even though Johannes replied w/a patch. Anyway, my next patch documents pull's usage via its OPTIONS_SPEC and I noticed the problem again. I fix it here so that I can document its use in the next patch. :-) git-pull.sh | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/git-pull.sh b/git-pull.sh index 46da0f4..3ce32b5 100755 --- a/git-pull.sh +++ b/git-pull.sh @@ -174,6 +174,7 @@ fi merge_name=$(git fmt-merge-msg <"$GIT_DIR/FETCH_HEAD") || exit test true = "$rebase" && - exec git-rebase --onto $merge_head ${oldremoteref:-$merge_head} + exec git-rebase $strategy_args --onto $merge_head \ + ${oldremoteref:-$merge_head} exec git-merge $no_summary $no_commit $squash $no_ff $strategy_args \ "$merge_name" HEAD $merge_head -- 1.5.4.2.285.g1e9f ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] pull: document usage via OPTIONS_SPEC 2008-02-23 0:52 [PATCH 1/2] pull: pass --strategy along to to rebase Jay Soffian @ 2008-02-23 0:52 ` Jay Soffian 2008-02-23 1:15 ` Junio C Hamano 0 siblings, 1 reply; 7+ messages in thread From: Jay Soffian @ 2008-02-23 0:52 UTC (permalink / raw) To: git; +Cc: Jay Soffian, Johannes Schindelin, Junio C Hamano document usage via OPTIONS_SPEC Signed-off-by: Jay Soffian <jaysoffian@gmail.com> --- git-pull.sh | 32 +++++++++++++++++++++++++++++--- 1 files changed, 29 insertions(+), 3 deletions(-) diff --git a/git-pull.sh b/git-pull.sh index 3ce32b5..194c1d0 100755 --- a/git-pull.sh +++ b/git-pull.sh @@ -4,10 +4,36 @@ # # Fetch one or more remote refs and merge it/them into the current HEAD. -USAGE='[-n | --no-summary] [--[no-]commit] [--[no-]squash] [--[no-]ff] [-s strategy]... [<fetch-options>] <repo> <head>...' -LONG_USAGE='Fetch one or more remote refs and merge it/them into the current HEAD.' SUBDIRECTORY_OK=Yes -OPTIONS_SPEC= +OPTIONS_SPEC="\ +git pull [options] [<repo>] [<refspec>] +-- + fetch options +q,quiet make the fetch process less verbose +v,verbose make the fetch process more verbose +a,append append to FETCH_HEAD instead of overwritting +upload-pack=, specify path to git-fetch-pack on remote end +f,force force local branch to be updated by remote branch +t,tags fetch all remote tags +no-tags do not fetch any remote tags +k,keep keep downloaded pack +u,update-head-ok used internally between git pull and git fetch +depth= deep shallow history by specified number of commits + merge options +summary show diffstat at end of merge +n,no-summary do not show diff stat at end of merge +commit commit the result after merging +no-commit do not commit the result after merging +squash update the index and working tree only +no-squash override --squash, perform a normal merge and commit +ff only update branch pointer if merge is a fast-forward +no-ff do a merge commit even if the merge is a fast-forward +s,strategy= use given merge strategy + rebase options +rebase rebase after fetching +no-rebase merge after fetching +s,strategy= use given merge strategy; implies -m to rebase +" . git-sh-setup set_reflog_action "pull $*" require_work_tree -- 1.5.4.2.285.g1e9f ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] pull: document usage via OPTIONS_SPEC 2008-02-23 0:52 ` [PATCH 2/2] pull: document usage via OPTIONS_SPEC Jay Soffian @ 2008-02-23 1:15 ` Junio C Hamano 2008-02-23 1:40 ` Jay Soffian 2008-02-26 20:42 ` [PATCH v2] " Jay Soffian 0 siblings, 2 replies; 7+ messages in thread From: Junio C Hamano @ 2008-02-23 1:15 UTC (permalink / raw) To: Jay Soffian; +Cc: git, Johannes Schindelin OPTIONS_SPEC is not just for documentation but it actively mucks with the incoming options list. Have you checked if you did not break the actual parsing? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] pull: document usage via OPTIONS_SPEC 2008-02-23 1:15 ` Junio C Hamano @ 2008-02-23 1:40 ` Jay Soffian 2008-02-26 20:42 ` [PATCH v2] " Jay Soffian 1 sibling, 0 replies; 7+ messages in thread From: Jay Soffian @ 2008-02-23 1:40 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Johannes Schindelin On Fri, Feb 22, 2008 at 8:15 PM, Junio C Hamano <gitster@pobox.com> wrote: > OPTIONS_SPEC is not just for documentation but it actively mucks > with the incoming options list. > > Have you checked if you did not break the actual parsing? Duh, no. Sorry. Will do. j. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] pull: document usage via OPTIONS_SPEC 2008-02-23 1:15 ` Junio C Hamano 2008-02-23 1:40 ` Jay Soffian @ 2008-02-26 20:42 ` Jay Soffian 2008-02-26 21:23 ` Junio C Hamano 1 sibling, 1 reply; 7+ messages in thread From: Jay Soffian @ 2008-02-26 20:42 UTC (permalink / raw) To: git; +Cc: Jay Soffian, Johannes Schindelin, Junio C Hamano Replaced USAGE and LONG_USAGE with OPTIONS_SPEC Signed-off-by: Jay Soffian <jaysoffian@gmail.com> --- Okay, I'm pretty sure this is correct now. make test passes, and it also passes my "it looks right" test after having gone over it a few times. This isn't quite ready for inclusion yet though because I want some feedback: * There is one semantic change. You can't use "-s=<strategy>" anymore. That's not really proper usage of a short option (it's either "-s<strategy>" or "-s <strategy>"). Is it okay to not accept the "-s=<strategy>" form? * Is it worth doing this at all? If the plan is to rewrite everything as builtins I kinda feel like I'm wasting my time. If not, I'll work on doing this for all the other scripts which are missing an OPTIONS_SPEC. Then maybe they can all be merged at once? * You can see I added the GIT_PULL_DEBUG_ARGS blob. Frankly, I can't really think of a good way to test this change other than to do something like that and then add a test for each individual option. But that's going to really bloat the test suite. Is it worth it to do this for every script? Or is passing the existing test suite and enough eyeballs looking at it in next good enough? Can you think of another way to check for regressions? j. git-pull.sh | 94 ++++++++++++++++++++++++++++++++++++++-------------------- 1 files changed, 61 insertions(+), 33 deletions(-) diff --git a/git-pull.sh b/git-pull.sh index 3ce32b5..8b1d732 100755 --- a/git-pull.sh +++ b/git-pull.sh @@ -4,10 +4,36 @@ # # Fetch one or more remote refs and merge it/them into the current HEAD. -USAGE='[-n | --no-summary] [--[no-]commit] [--[no-]squash] [--[no-]ff] [-s strategy]... [<fetch-options>] <repo> <head>...' -LONG_USAGE='Fetch one or more remote refs and merge it/them into the current HEAD.' SUBDIRECTORY_OK=Yes -OPTIONS_SPEC= +OPTIONS_SPEC="\ +git pull [options] [<repo>] [<refspec>] +-- + fetch options +q,quiet make the fetch process less verbose +v,verbose make the fetch process more verbose +a,append append to FETCH_HEAD instead of overwritting +upload-pack=, specify path to git-fetch-pack on remote end +f,force force local branch to be updated by remote branch +t,tags fetch all remote tags +no-tags do not fetch any remote tags +k,keep keep downloaded pack +u,update-head-ok used internally between git pull and git fetch +depth= deep shallow history by specified number of commits + merge options +summary show diffstat at end of merge +n,no-summary do not show diff stat at end of merge +commit commit the result after merging +no-commit do not commit the result after merging +squash update the index and working tree only +no-squash override --squash, perform a normal merge and commit +ff only update branch pointer if merge is a fast-forward +no-ff do a merge commit even if the merge is a fast-forward +s,strategy= use given merge strategy + rebase options +rebase rebase after fetching +no-rebase merge after fetching +s,strategy= use given merge strategy; implies -m to rebase +" . git-sh-setup set_reflog_action "pull $*" require_work_tree @@ -23,46 +49,33 @@ rebase=$(git config --bool branch.$curr_branch_short.rebase) while : do case "$1" in - -n|--n|--no|--no-|--no-s|--no-su|--no-sum|--no-summ|\ - --no-summa|--no-summar|--no-summary) + -n|--no-summary) no_summary=-n ;; --summary) - no_summary=$1 - ;; - --no-c|--no-co|--no-com|--no-comm|--no-commi|--no-commit) + no_summary=$1 ;; + --no-commit) no_commit=--no-commit ;; - --c|--co|--com|--comm|--commi|--commit) + --commit) no_commit=--commit ;; - --sq|--squ|--squa|--squas|--squash) + --squash) squash=--squash ;; - --no-sq|--no-squ|--no-squa|--no-squas|--no-squash) + --no-squash) squash=--no-squash ;; --ff) no_ff=--ff ;; --no-ff) no_ff=--no-ff ;; - -s=*|--s=*|--st=*|--str=*|--stra=*|--strat=*|--strate=*|\ - --strateg=*|--strategy=*|\ - -s|--s|--st|--str|--stra|--strat|--strate|--strateg|--strategy) - case "$#,$1" in - *,*=*) - strategy=`expr "z$1" : 'z-[^=]*=\(.*\)'` ;; - 1,*) - usage ;; - *) - strategy="$2" - shift ;; - esac - strategy_args="${strategy_args}-s $strategy " - ;; - -r|--r|--re|--reb|--reba|--rebas|--rebase) - rebase=true - ;; - --no-r|--no-re|--no-reb|--no-reba|--no-rebas|--no-rebase) - rebase=false + -s|--strategy) + shift + strategy_args="${strategy_args}-s $1 " ;; - -h|--h|--he|--hel|--help) - usage + -r|--rebase) + rebase=true ;; + --no-rebase) + rebase=false ;; + --) + shift + break ;; *) # Pass thru anything that may be meant for fetch. @@ -72,12 +85,27 @@ do shift done +test '' != "$GIT_PULL_DEBUG_ARGS" && { + exec >&2 + for var in strategy_args no_summary no_commit squash no_ff rebase + do + echo "$var='$(eval echo "\$$var")'" + done + argnum=1 + for opt + do + echo "$argnum='$opt'" + argnum=$(($argnum + 1)) + done + exit 0 +} + error_on_no_merge_candidates () { exec >&2 for opt do case "$opt" in - -t|--t|--ta|--tag|--tags) + -t|--tags) echo "Fetching tags only, you probably meant:" echo " git fetch --tags" exit 1 -- 1.5.4.3.345.g651f8 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] pull: document usage via OPTIONS_SPEC 2008-02-26 20:42 ` [PATCH v2] " Jay Soffian @ 2008-02-26 21:23 ` Junio C Hamano 2008-02-26 21:37 ` Jay Soffian 0 siblings, 1 reply; 7+ messages in thread From: Junio C Hamano @ 2008-02-26 21:23 UTC (permalink / raw) To: Jay Soffian; +Cc: git, Johannes Schindelin Jay Soffian <jaysoffian@gmail.com> writes: > * There is one semantic change. You can't use "-s=<strategy>" anymore. That's not > really proper usage of a short option (it's either "-s<strategy>" or > "-s <strategy>"). Is it okay to not accept the "-s=<strategy>" form? Well, with my maintainer hat on, I must resist _any_ change ;-). Personally I would not mind this. It is a borderline between regression and making the option parameters more consistent. > * Is it worth doing this at all? If the plan is to rewrite everything as > builtins I kinda feel like I'm wasting my time. If a contributor feels wasting his time, what should reviewers feel reviewing such patches ;-)? > * You can see I added the GIT_PULL_DEBUG_ARGS blob. Frankly, I can't really > think of a good way to test this change other than to do > something like that and then add a test for each individual option. I see this would be useful while you are developing. But the testsuite is meant to check the behaviour that is externally observable. It does not matter if you change the way you internally represent --verbose option from settting $verbose to resetting $quiet, but tests based on GIT_PULL_DEBUG_ARGS would notice the difference in the implementation detail, so I do not see much point leaving this in; it would not be useful for test scripts, I suspect. > +git pull [options] [<repo>] [<refspec>] > +-- > + fetch options > +q,quiet make the fetch process less verbose > +v,verbose make the fetch process more verbose > +a,append append to FETCH_HEAD instead of overwritting > +upload-pack=, specify path to git-fetch-pack on remote end > +f,force force local branch to be updated by remote branch > +t,tags fetch all remote tags While it is technically correct that you _can_ feed any option meant for git-fetch to this command, some options do not make any sense in the context of git-pull, and we should not advertise them, or better yet, actively reject them if you can. I think --tags is one of them. > . git-sh-setup > set_reflog_action "pull $*" > require_work_tree > @@ -23,46 +49,33 @@ rebase=$(git config --bool branch.$curr_branch_short.rebase) > while : > do > case "$1" in > ... > ;; > *) > # Pass thru anything that may be meant for fetch. > break Because the loop breaks here, the option description above should mention that options meant for fetch should come after all the options you want to give to git-pull itself. For example, I do not think "git pull -q -s stupid $there $that" is meant to work. A better long-term alternative would be to lift that restriction. I do not recall offhand but does the parse-options reorder the options in any way? If that is the case, it makes the above not a long-term thing but a must-be-done in a patch that starts to use parse-options. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] pull: document usage via OPTIONS_SPEC 2008-02-26 21:23 ` Junio C Hamano @ 2008-02-26 21:37 ` Jay Soffian 0 siblings, 0 replies; 7+ messages in thread From: Jay Soffian @ 2008-02-26 21:37 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Johannes Schindelin On Tue, Feb 26, 2008 at 4:23 PM, Junio C Hamano <gitster@pobox.com> wrote: > Jay Soffian <jaysoffian@gmail.com> writes: > > > * There is one semantic change. You can't use "-s=<strategy>" anymore. That's not > > really proper usage of a short option (it's either "-s<strategy>" or > > "-s <strategy>"). Is it okay to not accept the "-s=<strategy>" form? > > Well, with my maintainer hat on, I must resist _any_ change ;-). > Personally I would not mind this. It is a borderline between > regression and making the option parameters more consistent. My reasoning was that were the script converted to a built-in, the -s= behavior would not likely be maintained (or even noticed...). And that form certainly isn't documented. I think it was an accident that it ever worked. > If a contributor feels wasting his time, what should reviewers > feel reviewing such patches ;-)? I get the smiley, but let me rephrase: is there a long term goal to replace all shell scripts with builtins? And to answer your rhetorical question, reviewers should feel appreciated, because they are. > While it is technically correct that you _can_ feed any option > meant for git-fetch to this command, some options do not make > any sense in the context of git-pull, and we should not > advertise them, or better yet, actively reject them if you can. Well then we have a documentation issue then, because it was from the git pull docs that I wrote the option spec. So I suppose this patch should include a documentation cleanup at the same time. > Because the loop breaks here, the option description above > should mention that options meant for fetch should come after > all the options you want to give to git-pull itself. For > example, I do not think "git pull -q -s stupid $there $that" is > meant to work. > > A better long-term alternative would be to lift that restriction. Sigh, nothing is ever as simple as it seems. And here I was just trying to improve the usage statement. :-( > I do not recall offhand but does the parse-options reorder the > options in any way? If that is the case, it makes the above not > a long-term thing but a must-be-done in a patch that starts to > use parse-options. I don't think it does. Back to the drawing board... j. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2008-02-26 21:38 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-02-23 0:52 [PATCH 1/2] pull: pass --strategy along to to rebase Jay Soffian 2008-02-23 0:52 ` [PATCH 2/2] pull: document usage via OPTIONS_SPEC Jay Soffian 2008-02-23 1:15 ` Junio C Hamano 2008-02-23 1:40 ` Jay Soffian 2008-02-26 20:42 ` [PATCH v2] " Jay Soffian 2008-02-26 21:23 ` Junio C Hamano 2008-02-26 21:37 ` Jay Soffian
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).