[PATCH] git pull: Remove option handling done by fetch

[PATCH] git pull: Remove option handling done by fetch

[Jens Lehmann]

In commits be254a0ea9 and 7dce19d374 the handling of the new fetch options
"--[no-]recurse-submodules" had been added to git-pull.sh. This was not
necessary because all options to "git fetch" are passed to it and handled
there, so lets remove them.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---

I noticed this while implementing the on-demand recursive fetch.

 git-pull.sh |   10 ++--------
 1 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/git-pull.sh b/git-pull.sh
index eb87f49..20a3bbe 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -38,7 +38,7 @@ test -z "$(git ls-files -u)" || die_conflict
 test -f "$GIT_DIR/MERGE_HEAD" && die_merge

 strategy_args= diffstat= no_commit= squash= no_ff= ff_only=
-log_arg= verbosity= progress= recurse_submodules=
+log_arg= verbosity= progress=
 merge_args=
 curr_branch=$(git symbolic-ref -q HEAD)
 curr_branch_short="${curr_branch#refs/heads/}"
@@ -105,12 +105,6 @@ do
 	--no-r|--no-re|--no-reb|--no-reba|--no-rebas|--no-rebase)
 		rebase=false
 		;;
-	--recurse-submodules)
-		recurse_submodules=--recurse-submodules
-		;;
-	--no-recurse-submodules)
-		recurse_submodules=--no-recurse-submodules
-		;;
 	--d|--dr|--dry|--dry-|--dry-r|--dry-ru|--dry-run)
 		dry_run=--dry-run
 		;;
@@ -223,7 +217,7 @@ test true = "$rebase" && {
 	done
 }
 orig_head=$(git rev-parse -q --verify HEAD)
-git fetch $verbosity $progress $dry_run $recurse_submodules --update-head-ok "$@" || exit 1
+git fetch $verbosity $progress $dry_run --update-head-ok "$@" || exit 1
 test -z "$dry_run" || exit 0

 curr_head=$(git rev-parse -q --verify HEAD)
-- 
1.7.4.31.g5ae186

Re: [PATCH] git pull: Remove option handling done by fetch

[Johannes Sixt]

On Freitag, 4. Februar 2011, Jens Lehmann wrote:
> In commits be254a0ea9 and 7dce19d374 the handling of the new fetch options
> "--[no-]recurse-submodules" had been added to git-pull.sh. This was not
> necessary because all options to "git fetch" are passed to it and handled
> there, so lets remove them.
>
> Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
> ---
>
> I noticed this while implementing the on-demand recursive fetch.
>
>  git-pull.sh |   10 ++--------
>  1 files changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/git-pull.sh b/git-pull.sh
> index eb87f49..20a3bbe 100755
> --- a/git-pull.sh
> +++ b/git-pull.sh
> @@ -38,7 +38,7 @@ test -z "$(git ls-files -u)" || die_conflict
>  test -f "$GIT_DIR/MERGE_HEAD" && die_merge
>
>  strategy_args= diffstat= no_commit= squash= no_ff= ff_only=
> -log_arg= verbosity= progress= recurse_submodules=
> +log_arg= verbosity= progress=
>  merge_args=
>  curr_branch=$(git symbolic-ref -q HEAD)
>  curr_branch_short="${curr_branch#refs/heads/}"
> @@ -105,12 +105,6 @@ do
>  	--no-r|--no-re|--no-reb|--no-reba|--no-rebas|--no-rebase)
>  		rebase=false
>  		;;
> -	--recurse-submodules)
> -		recurse_submodules=--recurse-submodules
> -		;;
> -	--no-recurse-submodules)
> -		recurse_submodules=--no-recurse-submodules
> -		;;
>  	--d|--dr|--dry|--dry-|--dry-r|--dry-ru|--dry-run)
>  		dry_run=--dry-run
>  		;;
> @@ -223,7 +217,7 @@ test true = "$rebase" && {
>  	done
>  }
>  orig_head=$(git rev-parse -q --verify HEAD)
>  -git fetch $verbosity $progress $dry_run $recurse_submodules 
> --update-head-ok "$@" || exit 1
>> +git fetch $verbosity $progress $dry_run 
> --update-head-ok "$@" || exit 1 test -z "$dry_run" || exit 0
>
>  curr_head=$(git rev-parse -q --verify HEAD)

Huh? What will, for example,

   git pull --recurse-submodules --no-ff origin

do before and after your patch? Doesn't your patch force users to 
write --[no-]recurse-submodules last?

-- Hannes

Re: [PATCH] git pull: Remove option handling done by fetch

[Jens Lehmann]

Am 04.02.2011 23:26, schrieb Johannes Sixt:
> On Freitag, 4. Februar 2011, Jens Lehmann wrote:
>> In commits be254a0ea9 and 7dce19d374 the handling of the new fetch options
>> "--[no-]recurse-submodules" had been added to git-pull.sh. This was not
>> necessary because all options to "git fetch" are passed to it and handled
>> there, so lets remove them.
>>
>> Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
>> ---
>>
>> I noticed this while implementing the on-demand recursive fetch.
>>
>>  git-pull.sh |   10 ++--------
>>  1 files changed, 2 insertions(+), 8 deletions(-)
>>
>> diff --git a/git-pull.sh b/git-pull.sh
>> index eb87f49..20a3bbe 100755
>> --- a/git-pull.sh
>> +++ b/git-pull.sh
>> @@ -38,7 +38,7 @@ test -z "$(git ls-files -u)" || die_conflict
>>  test -f "$GIT_DIR/MERGE_HEAD" && die_merge
>>
>>  strategy_args= diffstat= no_commit= squash= no_ff= ff_only=
>> -log_arg= verbosity= progress= recurse_submodules=
>> +log_arg= verbosity= progress=
>>  merge_args=
>>  curr_branch=$(git symbolic-ref -q HEAD)
>>  curr_branch_short="${curr_branch#refs/heads/}"
>> @@ -105,12 +105,6 @@ do
>>  	--no-r|--no-re|--no-reb|--no-reba|--no-rebas|--no-rebase)
>>  		rebase=false
>>  		;;
>> -	--recurse-submodules)
>> -		recurse_submodules=--recurse-submodules
>> -		;;
>> -	--no-recurse-submodules)
>> -		recurse_submodules=--no-recurse-submodules
>> -		;;
>>  	--d|--dr|--dry|--dry-|--dry-r|--dry-ru|--dry-run)
>>  		dry_run=--dry-run
>>  		;;
>> @@ -223,7 +217,7 @@ test true = "$rebase" && {
>>  	done
>>  }
>>  orig_head=$(git rev-parse -q --verify HEAD)
>>  -git fetch $verbosity $progress $dry_run $recurse_submodules 
>> --update-head-ok "$@" || exit 1
>>> +git fetch $verbosity $progress $dry_run 
>> --update-head-ok "$@" || exit 1 test -z "$dry_run" || exit 0
>>
>>  curr_head=$(git rev-parse -q --verify HEAD)
> 
> Huh? What will, for example,
> 
>    git pull --recurse-submodules --no-ff origin
> 
> do before and after your patch? Doesn't your patch force users to 
> write --[no-]recurse-submodules last?

Yes, but isn't that exactly what the pull man-page says? Quote:
"Options meant for git pull itself and the underlying git merge
must be given before the options meant for git fetch."
(The reason I added this option handling in the first place was
that I hadn't been aware of this at that time either ... ;-)

Thanks for your review!

Re: [PATCH] git pull: Remove option handling done by fetch

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

> Yes, but isn't that exactly what the pull man-page says? Quote:
> "Options meant for git pull itself and the underlying git merge
> must be given before the options meant for git fetch."

Yes, it says that, and I think that was a weasely way to say "the command
line parser in git-pull is broken".  The lines you are removing was from
the patch that fixed that breakage, aren't they?

Re: [PATCH] git pull: Remove option handling done by fetch

[Jens Lehmann]

Am 06.02.2011 21:45, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
> 
>> Yes, but isn't that exactly what the pull man-page says? Quote:
>> "Options meant for git pull itself and the underlying git merge
>> must be given before the options meant for git fetch."
> 
> Yes, it says that, and I think that was a weasely way to say "the command
> line parser in git-pull is broken".  The lines you are removing was from
> the patch that fixed that breakage, aren't they?

Nope, they were from the patch where I taught "git fetch" the
"--recurse-submodules" option and was not aware at that time that
"git pull" should just pass on almost all fetch options (the only
exceptions to that rule being -q, -v, -n and --progress). The thing
I had in mind was to later pass the same "--recurse-submodules"
option to the merge command too (when I finished implementing that
option). But when I understood later that pull handles the fetch
options in an interesting way I noticed that it would depend on the
order of options given if the "--recurse-submodules" would then be
passed to both fetch and merge or just to fetch, which will lead to
an interesting and unintuitive behavior I was not eager to expose.

So yes, I hit the strangeness of the "git pull" option parsing, but
decided to not mess it up further by adding another option to the
ones it does handle differently but play by the rules which are
used now (The other possibility would have been to document it
as a new option to "git pull", but that would have lead to the
problem I described earlier when merge will learn that option too).

So I have no strong feelings about this patch but believe it is the
right thing to do as long as "git pull" handles its options the way
it does. But looking at the confusion that option handling caused
I think it might be a worthwhile idea to overhaul it.

(CCed Jonathan, as he is the author of the lines I quoted)

Re: [PATCH] git pull: Remove option handling done by fetch

[Jonathan Nieder]

Jens Lehmann wrote:
> Am 06.02.2011 21:45, schrieb Junio C Hamano:
>> Jens Lehmann <Jens.Lehmann@web.de> writes:

>>> Yes, but isn't that exactly what the pull man-page says? Quote:
>>> "Options meant for git pull itself and the underlying git merge
>>> must be given before the options meant for git fetch."
>> 
>> Yes, it says that, and I think that was a weasely way to say "the command
>> line parser in git-pull is broken".
[...]
> (CCed Jonathan, as he is the author of the lines I quoted)

They're from Junio. :)  See v1.5.4.5~26 (git-pull documentation: warn
about the option order, 2008-03-10).

I also agree that (1) it would be very nice to find a way to fix this
and (2) until then, it seems best as a general principle not to
regress in those odd cases where we sort of fixed it.

In other words, if we were adding --recurse-submodules today, I'd
agree that it should be treated like other fetch options, but we
already added the option and some people/scripts might be used to
the added flexibility, no?

Re: [PATCH] git pull: Remove option handling done by fetch

[Jens Lehmann]

Am 06.02.2011 23:09, schrieb Jonathan Nieder:
> Jens Lehmann wrote:
>> Am 06.02.2011 21:45, schrieb Junio C Hamano:
>>> Jens Lehmann <Jens.Lehmann@web.de> writes:
> 
>>>> Yes, but isn't that exactly what the pull man-page says? Quote:
>>>> "Options meant for git pull itself and the underlying git merge
>>>> must be given before the options meant for git fetch."
>>>
>>> Yes, it says that, and I think that was a weasely way to say "the command
>>> line parser in git-pull is broken".
> [...]
>> (CCed Jonathan, as he is the author of the lines I quoted)
> 
> They're from Junio. :)  See v1.5.4.5~26 (git-pull documentation: warn
> about the option order, 2008-03-10).

And I thought I could trust "git blame -M -C" ... ;-)

> I also agree that (1) it would be very nice to find a way to fix this
> and (2) until then, it seems best as a general principle not to
> regress in those odd cases where we sort of fixed it.
> 
> In other words, if we were adding --recurse-submodules today, I'd
> agree that it should be treated like other fetch options, but we
> already added the option and some people/scripts might be used to
> the added flexibility, no?

Ok, even though that option was added only two months ago, somebody
could already use it. So that leaves option two:

1) Drop the patch I proposed

2) Document "--[no-]recurse-submodules" as " as "git pull" options

(And then I can later pass the same option to "git merge", which is
much better than the solutions I came up with ;-)

Maybe something like this?

-------------8<--------------
Subject: [PATCH] pull: Document the "--[no-]recurse-submodules" options

In commits be254a0ea9 and 7dce19d374 the handling of the new fetch options
"--[no-]recurse-submodules" had been added to git-pull.sh. But they were
not documented as the pull options they now are, so let's fix that.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
 Documentation/fetch-options.txt |    2 +-
 Documentation/git-pull.txt      |    4 ++++
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 695696d..ab0dbfc 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -64,11 +64,11 @@ ifndef::git-pull[]
 	downloaded. The default behavior for a remote may be
 	specified with the remote.<name>.tagopt setting. See
 	linkgit:git-config[1].
-endif::git-pull[]

 --[no-]recurse-submodules::
 	This option controls if new commits of all populated submodules should
 	be fetched too (see linkgit:git-config[1] and linkgit:gitmodules[5]).
+endif::git-pull[]

 ifndef::git-pull[]
 --submodule-prefix=<path>::
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index 3046691..7d0f601 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -84,6 +84,10 @@ must be given before the options meant for 'git fetch'.
 --verbose::
 	Pass --verbose to git-fetch and git-merge.

+--[no-]recurse-submodules::
+	This option controls if new commits of all populated submodules should
+	be fetched too (see linkgit:git-config[1] and linkgit:gitmodules[5]).
+
 Options related to merging
 ~~~~~~~~~~~~~~~~~~~~~~~~~~

-- 
1.7.4.31.g5ae186.dirty

Re: [PATCH] git pull: Remove option handling done by fetch

[Jonathan Nieder]

Jens Lehmann wrote:

> 2) Document "--[no-]recurse-submodules" as " as "git pull" options
>
> (And then I can later pass the same option to "git merge", which is
> much better than the solutions I came up with ;-)

Hmm. :)

[...]
> --- a/Documentation/git-pull.txt
> +++ b/Documentation/git-pull.txt
> @@ -84,6 +84,10 @@ must be given before the options meant for 'git fetch'.
>  --verbose::
>  	Pass --verbose to git-fetch and git-merge.
> 
> +--[no-]recurse-submodules::
> +	This option controls if new commits of all populated submodules should
> +	be fetched too (see linkgit:git-config[1] and linkgit:gitmodules[5]).
> +

Is it worth mentioning that this does not (yet) automatically check
out the new commits in submodules after a merge, or would such
documentation be too likely to be forgotten and left stale in the
future?

[PATCH v2] pull: Document the "--[no-]recurse-submodules" options

[Jens Lehmann]

In commits be254a0ea9 and 7dce19d374 the handling of the new fetch options
"--[no-]recurse-submodules" had been added to git-pull.sh. But they were
not documented as the pull options they now are, so let's fix that.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---

Am 07.02.2011 08:41, schrieb Jonathan Nieder:
> Jens Lehmann wrote:
>> --- a/Documentation/git-pull.txt
>> +++ b/Documentation/git-pull.txt
>> @@ -84,6 +84,10 @@ must be given before the options meant for 'git fetch'.
>>  --verbose::
>>  	Pass --verbose to git-fetch and git-merge.
>>
>> +--[no-]recurse-submodules::
>> +	This option controls if new commits of all populated submodules should
>> +	be fetched too (see linkgit:git-config[1] and linkgit:gitmodules[5]).
>> +
> 
> Is it worth mentioning that this does not (yet) automatically check
> out the new commits in submodules after a merge, or would such
> documentation be too likely to be forgotten and left stale in the
> future?

Good point, here is v2! (And I will add an updated description to the
relevant commits in my github repo, so we won't forget that later)

 Documentation/fetch-options.txt |    2 +-
 Documentation/git-pull.txt      |    9 +++++++++
 2 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 695696d..ab0dbfc 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -64,11 +64,11 @@ ifndef::git-pull[]
 	downloaded. The default behavior for a remote may be
 	specified with the remote.<name>.tagopt setting. See
 	linkgit:git-config[1].
-endif::git-pull[]

 --[no-]recurse-submodules::
 	This option controls if new commits of all populated submodules should
 	be fetched too (see linkgit:git-config[1] and linkgit:gitmodules[5]).
+endif::git-pull[]

 ifndef::git-pull[]
 --submodule-prefix=<path>::
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index 3046691..b33e6be 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -84,6 +84,15 @@ must be given before the options meant for 'git fetch'.
 --verbose::
 	Pass --verbose to git-fetch and git-merge.

+--[no-]recurse-submodules::
+	This option controls if new commits of all populated submodules should
+	be fetched too (see linkgit:git-config[1] and linkgit:gitmodules[5]).
+	That might be necessary to get the data needed for merging submodule
+	commits, a feature git learned in 1.7.3. Notice that the result of a
+	merge will not be checked out in the submodule, "git submodule update"
+	has to be called afterwards to bring the work tree up to date with the
+	merge result.
+
 Options related to merging
 ~~~~~~~~~~~~~~~~~~~~~~~~~~

-- 
1.7.4.47.g87a200

Re: [PATCH v2] pull: Document the "--[no-]recurse-submodules" options

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
> index 695696d..ab0dbfc 100644
> --- a/Documentation/fetch-options.txt
> +++ b/Documentation/fetch-options.txt
> @@ -64,11 +64,11 @@ ifndef::git-pull[]
>  	downloaded. The default behavior for a remote may be
>  	specified with the remote.<name>.tagopt setting. See
>  	linkgit:git-config[1].
> -endif::git-pull[]
>
>  --[no-]recurse-submodules::
>  	This option controls if new commits of all populated submodules should
>  	be fetched too (see linkgit:git-config[1] and linkgit:gitmodules[5]).
> +endif::git-pull[]
>
>  ifndef::git-pull[]
>  --submodule-prefix=<path>::

Hmph, why not enclose the three of them inside a single ifndef here?

> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
> index 3046691..b33e6be 100644
> --- a/Documentation/git-pull.txt
> +++ b/Documentation/git-pull.txt
> @@ -84,6 +84,15 @@ must be given before the options meant for 'git fetch'.
>  --verbose::
>  	Pass --verbose to git-fetch and git-merge.
>
> +--[no-]recurse-submodules::
> +	This option controls if new commits of all populated submodules should
> +	be fetched too (see linkgit:git-config[1] and linkgit:gitmodules[5]).
> +	That might be necessary to get the data needed for merging submodule
> +	commits, a feature git learned in 1.7.3. Notice that the result of a
> +	merge will not be checked out in the submodule, "git submodule update"
> +	has to be called afterwards to bring the work tree up to date with the
> +	merge result.

Ok.

[PATCH v3] pull: Document the "--[no-]recurse-submodules" options

[Jens Lehmann]

In commits be254a0ea9 and 7dce19d374 the handling of the new fetch options
"--[no-]recurse-submodules" had been added to git-pull.sh. But they were
not documented as the pull options they now are, so let's fix that.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---

Am 07.02.2011 22:42, schrieb Junio C Hamano:
> Hmph, why not enclose the three of them inside a single ifndef here?

Sure!

 Documentation/fetch-options.txt |    2 --
 Documentation/git-pull.txt      |    9 +++++++++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 695696d..f37276e 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -64,13 +64,11 @@ ifndef::git-pull[]
 	downloaded. The default behavior for a remote may be
 	specified with the remote.<name>.tagopt setting. See
 	linkgit:git-config[1].
-endif::git-pull[]

 --[no-]recurse-submodules::
 	This option controls if new commits of all populated submodules should
 	be fetched too (see linkgit:git-config[1] and linkgit:gitmodules[5]).

-ifndef::git-pull[]
 --submodule-prefix=<path>::
 	Prepend <path> to paths printed in informative messages
 	such as "Fetching submodule foo".  This option is used
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index 3046691..b33e6be 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -84,6 +84,15 @@ must be given before the options meant for 'git fetch'.
 --verbose::
 	Pass --verbose to git-fetch and git-merge.

+--[no-]recurse-submodules::
+	This option controls if new commits of all populated submodules should
+	be fetched too (see linkgit:git-config[1] and linkgit:gitmodules[5]).
+	That might be necessary to get the data needed for merging submodule
+	commits, a feature git learned in 1.7.3. Notice that the result of a
+	merge will not be checked out in the submodule, "git submodule update"
+	has to be called afterwards to bring the work tree up to date with the
+	merge result.
+
 Options related to merging
 ~~~~~~~~~~~~~~~~~~~~~~~~~~

-- 
1.7.4.47.g87a200