* [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).