git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Denton Liu <liu.denton@gmail.com>
Cc: apenwarr@gmail.com, git@vger.kernel.org
Subject: Re: [PATCH] contrib/subtree: ensure only one rev is provided
Date: Thu, 07 Feb 2019 10:54:46 -0800
Message-ID: <xmqqftszpgy1.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <cfd86853cce8a2cd5fae9e6fb9a84f1e3d6daaf4.1549538392.git.liu.denton@gmail.com> (Denton Liu's message of "Thu, 7 Feb 2019 03:20:46 -0800")

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.


  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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=xmqqftszpgy1.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=apenwarr@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=liu.denton@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	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/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

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

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git