# [PATCH] git pull: Remove option handling done by fetch 11 messages from 2011-02-04 to 2011-02-07. Participants: Jens Lehmann, Johannes Sixt, Junio C Hamano, Jonathan Nieder. Thread: https://gitlist.dev/t/26411 ## Jens Lehmann, 2011-02-04 20:17 Subject: [PATCH] git pull: Remove option handling done by fetch Message-ID: <4D4C5EBC.2090100@web.de> URL: https://gitlist.dev/e/4D4C5EBC.2090100%40web.de ``` 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 --- 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 ``` ## Johannes Sixt, 2011-02-04 22:26 Subject: Re: [PATCH] git pull: Remove option handling done by fetch Message-ID: <201102042326.08607.j6t@kdbg.org> URL: https://gitlist.dev/e/201102042326.08607.j6t%40kdbg.org In-Reply-To: <4D4C5EBC.2090100@web.de> ``` 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 > --- > > 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 ``` ## Jens Lehmann, 2011-02-05 11:26 Subject: Re: [PATCH] git pull: Remove option handling done by fetch Message-ID: <4D4D33E7.4000303@web.de> URL: https://gitlist.dev/e/4D4D33E7.4000303%40web.de In-Reply-To: <201102042326.08607.j6t@kdbg.org> ``` 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 >> --- >> >> 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! ``` ## Junio C Hamano, 2011-02-06 20:45 Subject: Re: [PATCH] git pull: Remove option handling done by fetch Message-ID: <7vipwwx56r.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vipwwx56r.fsf%40alter.siamese.dyndns.org In-Reply-To: <4D4D33E7.4000303@web.de> ``` Jens Lehmann 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? ``` ## Jens Lehmann, 2011-02-06 21:59 Subject: Re: [PATCH] git pull: Remove option handling done by fetch Message-ID: <4D4F19D0.2000408@web.de> URL: https://gitlist.dev/e/4D4F19D0.2000408%40web.de In-Reply-To: <7vipwwx56r.fsf@alter.siamese.dyndns.org> ``` Am 06.02.2011 21:45, schrieb Junio C Hamano: > Jens Lehmann 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) ``` ## Jonathan Nieder, 2011-02-06 22:09 Subject: Re: [PATCH] git pull: Remove option handling done by fetch Message-ID: <20110206220939.GC17210@elie> URL: https://gitlist.dev/e/20110206220939.GC17210%40elie In-Reply-To: <4D4F19D0.2000408@web.de> ``` Jens Lehmann wrote: > Am 06.02.2011 21:45, schrieb Junio C Hamano: >> Jens Lehmann 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? ``` ## Jens Lehmann, 2011-02-06 22:57 Subject: Re: [PATCH] git pull: Remove option handling done by fetch Message-ID: <4D4F273C.8030003@web.de> URL: https://gitlist.dev/e/4D4F273C.8030003%40web.de In-Reply-To: <20110206220939.GC17210@elie> ``` Am 06.02.2011 23:09, schrieb Jonathan Nieder: > Jens Lehmann wrote: >> Am 06.02.2011 21:45, schrieb Junio C Hamano: >>> Jens Lehmann 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 --- 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..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=:: 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 ``` ## Jonathan Nieder, 2011-02-07 07:41 Subject: Re: [PATCH] git pull: Remove option handling done by fetch Message-ID: <20110207074157.GA2736@elie> URL: https://gitlist.dev/e/20110207074157.GA2736%40elie In-Reply-To: <4D4F273C.8030003@web.de> ``` 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? ``` ## Jens Lehmann, 2011-02-07 19:27 Subject: [PATCH v2] pull: Document the "--[no-]recurse-submodules" options Message-ID: <4D5047BD.6030304@web.de> URL: https://gitlist.dev/e/4D5047BD.6030304%40web.de In-Reply-To: <20110207074157.GA2736@elie> ``` 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 --- 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..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=:: 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 ``` ## Junio C Hamano, 2011-02-07 21:42 Subject: Re: [PATCH v2] pull: Document the "--[no-]recurse-submodules" options Message-ID: <7vsjvzttbs.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vsjvzttbs.fsf%40alter.siamese.dyndns.org In-Reply-To: <4D5047BD.6030304@web.de> ``` Jens Lehmann 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..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=:: 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. ``` ## Jens Lehmann, 2011-02-07 22:24 Subject: [PATCH v3] pull: Document the "--[no-]recurse-submodules" options Message-ID: <4D507136.2030505@web.de> URL: https://gitlist.dev/e/4D507136.2030505%40web.de In-Reply-To: <7vsjvzttbs.fsf@alter.siamese.dyndns.org> ``` 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 --- 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..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=:: Prepend 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 ```