git pull: Remove option handling done by fetch

PATCH In review Jens Lehmann 4 participants 11 messages Jun 15, 2016 Documentation

Share & tools

Jens Lehmann

author

2016-06-15 22:50 UTC #0 lore raw

PATCH 1/1 git pull: Remove option handling done by fetch

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 <[email protected]>

---

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

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

git-pull.sh +2 -8

index eb87f49..20a3bbe 100755
@@ -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)

Signature · 1 line

1.7.4.31.g5ae186

Johannes Sixt

2016-06-15 22:50 UTC #1 lore raw

Quoted 46 lines · Jens Lehmann

Signed-off-by Jens Lehmann <[email protected]>

---

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

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

git-pull.sh +1 -8

index eb87f49..20a3bbe 100755
@@ -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

Quoted 1 line

+git fetch $verbosity $progress $dry_run

Quoted 3 lines

--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

Jens Lehmann

author

2016-06-15 22:50 UTC #2 lore raw

Am 04.02.2011 23:26, schrieb Johannes Sixt:

Quoted 49 lines · Jens Lehmann

Quoted 48 lines
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 <[email protected]>
---
I noticed this while implementing the on-demand recursive fetch.
 git-pull.sh |   10 ++--------
 1 files changed, 2 insertions(+), 8 deletions(-)
git-pull.sh +1 -8
index eb87f49..20a3bbe 100755
@@ -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
Quoted 1 line
+git fetch $verbosity $progress $dry_run
--update-head-ok "$@" || exit 1 test -z "$dry_run" || exit 0

Quoted 8 lines

Quoted 1 line
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!

Junio C Hamano

2016-06-15 22:50 UTC #3 lore raw

Quoted 3 lines · Jens Lehmann

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?

Jens Lehmann

author

2016-06-15 22:50 UTC #4 lore raw

Am 06.02.2011 21:45, schrieb Junio C Hamano:

Quoted 9 lines

Quoted 3 lines · Jens Lehmann
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)

Jonathan Nieder

2016-06-15 22:50 UTC #5 lore raw

Quoted 9 lines · Jens Lehmann

Am 06.02.2011 21:45, schrieb Junio C Hamano:
Quoted 8 lines
Quoted 3 lines · Jens Lehmann
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".

[...]

Quoted 1 line

(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?

Jens Lehmann

author

2016-06-15 22:50 UTC #6 lore raw

Am 06.02.2011 23:09, schrieb Jonathan Nieder:

Quoted 8 lines

Quoted 7 lines · Jens Lehmann
Am 06.02.2011 21:45, schrieb Junio C Hamano:
Quoted 6 lines
Quoted 3 lines · Jens Lehmann
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."

Quoted 7 lines

Quoted 2 lines
Quoted 2 lines
Yes, it says that, and I think that was a weasely way to say "the command line parser in git-pull is broken".
[...]
Quoted 1 line
(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" ... ;-)

Quoted 8 lines

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 <[email protected]>

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

Documentation/fetch-options.txt +1 -1

index 695696d..ab0dbfc 100644
@@ -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>::

Documentation/git-pull.txt +4 -0

index 3046691..7d0f601 100644
@@ -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
 ~~~~~~~~~~~~~~~~~~~~~~~~~~

Signature · 1 line

1.7.4.31.g5ae186.dirty

Jonathan Nieder

2016-06-15 22:50 UTC #7 lore raw

Quoted 4 lines · Jens Lehmann

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. :)

[...]

Documentation/git-pull.txt must be given before the options meant for 'git fetch'. · line 90

Quoted 10 lines

Documentation/git-pull.txt +4 -0

@@ -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?

Jens Lehmann

author

2016-06-15 22:50 UTC #8 lore raw

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

Signed-off-by Jens Lehmann <[email protected]>

---

Am 07.02.2011 08:41, schrieb Jonathan Nieder:

Quoted 7 lines

Quoted 6 lines · Jens Lehmann

Documentation/git-pull.txt +0 -0

@@ -84,6 +84,10 @@ must be given before the options meant for 'git fetch'.
 --verbose::
 	Pass --verbose to git-fetch and git-merge.

Quoted 9 lines

Documentation/git-pull.txt must be given before the options meant for 'git fetch'. · line 90
Quoted 4 lines
Documentation/git-pull.txt +4 -0
+--[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(-)

Documentation/fetch-options.txt +1 -1

index 695696d..ab0dbfc 100644
@@ -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>::

Documentation/git-pull.txt +9 -0

index 3046691..b33e6be 100644
@@ -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
 ~~~~~~~~~~~~~~~~~~~~~~~~~~

Signature · 1 line

1.7.4.47.g87a200

Junio C Hamano

2016-06-15 22:50 UTC #9 lore raw

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

Quoted 7 lines · Jens Lehmann

Documentation/fetch-options.txt +0 -0

index 695696d..ab0dbfc 100644
@@ -64,11 +64,11 @@ ifndef::git-pull[]
 	downloaded. The default behavior for a remote may be
 	specified with the remote.<name>.tagopt setting. See

Documentation/fetch-options.txt ifndef::git-pull[] · line 74

Quoted 10 lines

Documentation/fetch-options.txt +1 -1

 	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?

Quoted 6 lines

Documentation/git-pull.txt +0 -0

index 3046691..b33e6be 100644
@@ -84,6 +84,15 @@ must be given before the options meant for 'git fetch'.
 --verbose::

Documentation/git-pull.txt must be given before the options meant for 'git fetch'. · line 94

Quoted 10 lines

Documentation/git-pull.txt +8 -0

 	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.

Jens Lehmann

author

2016-06-15 22:50 UTC #10 lore raw

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

Signed-off-by Jens Lehmann <[email protected]>

---

Am 07.02.2011 22:42, schrieb Junio C Hamano:

Quoted 1 line

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(-)

Documentation/fetch-options.txt +0 -2

index 695696d..f37276e 100644
@@ -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

Documentation/git-pull.txt +9 -0

index 3046691..b33e6be 100644
@@ -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
 ~~~~~~~~~~~~~~~~~~~~~~~~~~

Signature · 1 line

1.7.4.47.g87a200