git pull --upload-pack reversion in git 2.5.0

git pull --upload-pack reversion in git 2.5.0

[Joey Hess]

[-- Attachment #1: Type: text/plain, Size: 401 bytes --]

In git 2.1.4, I can run: git pull --upload-pack 'echo --foo'

This also seems to work in 2.4.6, but in 2.5.0, the option parser
does something weird, apparently looking inside the quoted parameter
and parsing parameters in there:

error: unknown option `foo'
usage: git fetch [<options>] [<repository> [<refspec>...]]

Needless to say, this broke my use of --upload-pack.

-- 
see shy jo

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: git pull --upload-pack reversion in git 2.5.0

[Matthieu Moy]

Joey Hess <id@joeyh.name> writes:

> In git 2.1.4, I can run: git pull --upload-pack 'echo --foo'
>
> This also seems to work in 2.4.6, but in 2.5.0, the option parser
> does something weird, apparently looking inside the quoted parameter
> and parsing parameters in there:
>
> error: unknown option `foo'
> usage: git fetch [<options>] [<repository> [<refspec>...]]

This bisects down to:

commit e3b601da2af53cbb9a63e59113d524a8d946ea12 (junio/pt/pull-optparse)
Author: Paul Tan <pyokagan@gmail.com>
Date:   Tue Jun 2 22:22:53 2015 +0800

    pull: use git-rev-parse --parseopt for option parsing
    
    To enable unambiguous parsing of abbreviated options, bundled short
    options, separate form options and to provide consistent usage help, use
    git-rev-parse --parseopt for option parsing. With this, simplify the
    option parsing code.
    
    Signed-off-by: Paul Tan <pyokagan@gmail.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

=> Cc-ing Paul.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: git pull --upload-pack reversion in git 2.5.0

[Joey Hess]

[-- Attachment #1: Type: text/plain, Size: 256 bytes --]

I think this comes down to a lack of quoting where git-pull runs
git-fetch. Before eb2a8d9ed3fca2ba2f617b704992d483605f3bb6,
"$@" was passed through to git-fetch, but now there is a $upload_pack
which is passed without being quoted.

-- 
see shy jo

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: git pull --upload-pack reversion in git 2.5.0

[Junio C Hamano]

On Thu, Jul 30, 2015 at 11:31 AM, Joey Hess <id@joeyh.name> wrote:
> I think this comes down to a lack of quoting where git-pull runs
> git-fetch. Before eb2a8d9ed3fca2ba2f617b704992d483605f3bb6,
> "$@" was passed through to git-fetch, but now there is a $upload_pack
> which is passed without being quoted.

Yes, it is not just the matter of using "$upload_pack", though ;-)

${upload_pack+"$upload_pack"} or something.

[PATCH] pull.sh: quote $upload_pack when passing it to git-fetch

[Matthieu Moy]

The previous code broke for example

  git pull --upload-pack 'echo --foo'

Reported-by: Joey Hess <id@joeyh.name>
Fix-suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
Junio wrote:
> ${upload_pack+"$upload_pack"} or something.

Indeed, we need to pass nothing, not the empty string if $upload_pack
is not defined.

This should fix it.

 git-pull.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-pull.sh b/git-pull.sh
index a814bf6..26c5e9f 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -295,7 +295,7 @@ test true = "$rebase" && {
 }
 orig_head=$(git rev-parse -q --verify HEAD)
 git fetch $verbosity $progress $dry_run $recurse_submodules $all $append \
-$upload_pack $force $tags $prune $keep $depth $unshallow $update_shallow \
+${upload_pack+"$upload_pack"} $force $tags $prune $keep $depth $unshallow $update_shallow \
 $refmap --update-head-ok "$@" || exit 1
 test -z "$dry_run" || exit 0
 
-- 
2.5.0.rc0.7.ge1edd74.dirty

Re: [PATCH] pull.sh: quote $upload_pack when passing it to git-fetch

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> The previous code broke for example
>
>   git pull --upload-pack 'echo --foo'
>
> Reported-by: Joey Hess <id@joeyh.name>
> Fix-suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
> Junio wrote:
>> ${upload_pack+"$upload_pack"} or something.
>
> Indeed, we need to pass nothing, not the empty string if $upload_pack
> is not defined.
>
> This should fix it.

The problematic commit touches a lot more than upload-pack, but all
others that take user-supplied strings are meant for "git merge" or
"git rebase" that are properly quoted and then eval'ed, so this
should be sufficient.

Thanks.

>
>  git-pull.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-pull.sh b/git-pull.sh
> index a814bf6..26c5e9f 100755
> --- a/git-pull.sh
> +++ b/git-pull.sh
> @@ -295,7 +295,7 @@ test true = "$rebase" && {
>  }
>  orig_head=$(git rev-parse -q --verify HEAD)
>  git fetch $verbosity $progress $dry_run $recurse_submodules $all $append \
> -$upload_pack $force $tags $prune $keep $depth $unshallow $update_shallow \
> +${upload_pack+"$upload_pack"} $force $tags $prune $keep $depth
> $unshallow $update_shallow \
>  $refmap --update-head-ok "$@" || exit 1
>  test -z "$dry_run" || exit 0

Re: [PATCH] pull.sh: quote $upload_pack when passing it to git-fetch

[Paul Tan]

On Fri, Jul 31, 2015 at 4:40 AM, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
> The previous code broke for example
>
>   git pull --upload-pack 'echo --foo'
>
> Reported-by: Joey Hess <id@joeyh.name>
> Fix-suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>

Thanks for cleaning up my mess! ><

Regards,
Paul

Re: git pull --upload-pack reversion in git 2.5.0

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> On Thu, Jul 30, 2015 at 11:31 AM, Joey Hess <id@joeyh.name> wrote:
>> I think this comes down to a lack of quoting where git-pull runs
>> git-fetch. Before eb2a8d9ed3fca2ba2f617b704992d483605f3bb6,
>> "$@" was passed through to git-fetch, but now there is a $upload_pack
>> which is passed without being quoted.
>
> Yes, it is not just the matter of using "$upload_pack", though ;-)
>
> ${upload_pack+"$upload_pack"} or something.

Thanks for a bug report.

The next time, I'd appreciate if these regressions are caught before
it hits the tagged release, during the pre-release -rc period at the
latest, but preferrably earlier.  I'm greedy and expect far more
from those who are known to be competent heavy Git users than from
"I update every few releases" casual folks ;-)

Thanks, anyway.