Re: [PATCH] contrib/subtree: ensure only one rev is provided

[Junio C Hamano <gitster@pobox.com>]

Denton Liu <liu.denton@gmail.com> writes:

> @@ -185,6 +191,7 @@ if test "$command" != "pull" &&
>  then
>  	revs=$(git rev-parse $default --revs-only "$@") || exit $?
>  	dirs=$(git rev-parse --no-revs --no-flags "$@") || exit $?
> +	ensure_single_rev $revs

This applies to anything other than pull, add and push, so certainly
'split' is covered here.

> @@ -716,9 +723,8 @@ cmd_add_repository () {
>  }
>  
>  cmd_add_commit () {
> -	revs=$(git rev-parse $default --revs-only "$@") || exit $?
> -	set -- $revs
> -	rev="$1"
> +	rev=$(git rev-parse $default --revs-only "$@") || exit $?
> +	ensure_single_rev $rev

There are two callers of this helper.  cmd_add passes "$@" but it
does so only after making sure there is only one argument that is a
commit, so this conversion is not incorrect.

I am not sure if the other caller is OK, though.  cmd_add_repository
can get more than one revs, and uses the first one as $rev to read
the tree from, expecting that this helper to ignore other ones that
are emitted from 'git rev-parse --revs-only "$@"'.

For that matter, one of the early things cmd_split does is to call
the find_existing_splits helper with $revs, and it seems to be
prepared to be red multiple $revs (it is passed to "git log", so I
would expect that incoming $revs is allowed to specify bottom to
limit the traversal, e.g. "git log maint..master").  The addition of
"ensure_single_rev" we saw in an earlier hunk near ll.191 makes such
call impossible.  I am not a user of subtree, so I do not know if
it is a good change (i.e. making something nonsensical impossible to
do is good, making something useful impossible to do is bad).

> @@ -817,16 +823,10 @@ cmd_split () {
>  }
>  
>  cmd_merge () {
> -	revs=$(git rev-parse $default --revs-only "$@") || exit $?
> +	rev=$(git rev-parse $default --revs-only "$@") || exit $?
> +	ensure_single_rev $rev
>  	ensure_clean
>  
> -	set -- $revs
> -	if test $# -ne 1
> -	then
> -		die "You must provide exactly one revision.  Got: '$revs'"
> -	fi
> -	rev="$1"
> -

This one already was insisting on a single version, so it clearly is
a correct no-op conversion, but wouldn't this have been already
caught upfront where anything other than pull, add and push are
handled?  I do understand if the new call to ensure_single is made
to the other caller of cmd_merge in cmd_pull, though.

>  	if test -n "$squash"
>  	then
>  		first_split="$(find_latest_squash "$dir")"

In any case, I do not use subtree, and the last time I looked at
this script is a long time ago, so take all of the above with a
large grain of salt.

Thanks.