Re: [PATCH] contrib/subtree: ensure only one rev is provided
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 7, 2019, 18:54 UTC
- Message-ID
- <xmqqftszpgy1.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <cfd86853cce8a2cd5fae9e6fb9a84f1e3d6daaf4.1549538392.git.liu.denton@gmail.com>
Denton Liu <liu.denton@gmail.com> writes:
Show 5 quoted lines
> @@ -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.
Show 9 quoted lines
> @@ -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 $revThere 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).
Show 16 quoted lines
> @@ -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.