list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: Junio C Hamano <>
To: Denton Liu <>
Subject: Re: [PATCH] contrib/subtree: ensure only one rev is provided
Date: Thu, 07 Feb 2019 10:54:46 -0800
Message-ID: <> (raw)
In-Reply-To: <> (Denton Liu's message of "Thu, 7 Feb 2019 03:20:46 -0800")

Denton Liu <> 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.


  reply	other threads:[~2019-02-07 18:54 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-07 11:20 Denton Liu
2019-02-07 18:54 ` Junio C Hamano [this message]
2019-02-07 22:34   ` Avery Pennarun
2019-02-12 10:00     ` Denton Liu
2019-03-11  9:47       ` Denton Liu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ \
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
 note: .onion URLs require Tor:

code repositories for the project(s) associated with this inbox:

AGPL code for this site: git clone