git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
* [PATCH] contrib/subtree: ensure only one rev is provided
@ 2019-02-07 11:20 Denton Liu
  2019-02-07 18:54 ` Junio C Hamano
  0 siblings, 1 reply; 5+ messages in thread
From: Denton Liu @ 2019-02-07 11:20 UTC (permalink / raw)
  To: apenwarr; +Cc: git

While looking at the inline help for git-subtree.sh, I noticed that

	git subtree split --prefix=<prefix> <commit...>

was given as an option. However, it only really makes sense to provide
one revision because of the way the commits are forwarded to rev-parse
so this commit changes "<commit...>" to "<commit>" to reflect this. In
addition, it checks the arguments to ensure that only one rev is
provided for all subcommands that accept a commit.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 contrib/subtree/git-subtree.sh | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 147201dc6c..868e18b9a1 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -14,7 +14,7 @@ git subtree add   --prefix=<prefix> <repository> <ref>
 git subtree merge --prefix=<prefix> <commit>
 git subtree pull  --prefix=<prefix> <repository> <ref>
 git subtree push  --prefix=<prefix> <repository> <ref>
-git subtree split --prefix=<prefix> <commit...>
+git subtree split --prefix=<prefix> <commit>
 --
 h,help        show the help
 q             quiet
@@ -77,6 +77,12 @@ assert () {
 	fi
 }
 
+ensure_single_rev () {
+	if test $# -ne 1
+	then
+		die "You must provide exactly one revision.  Got: '$@'"
+	fi
+}
 
 while test $# -gt 0
 do
@@ -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
 	if test -n "$dirs"
 	then
 		die "Error: Use --prefix instead of bare filenames."
@@ -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
 
 	debug "Adding $dir as '$rev'..."
 	git read-tree --prefix="$dir" $rev || exit $?
@@ -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"
-
 	if test -n "$squash"
 	then
 		first_split="$(find_latest_squash "$dir")"
-- 
2.20.1.522.g5f42c252e9


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] contrib/subtree: ensure only one rev is provided
  2019-02-07 11:20 [PATCH] contrib/subtree: ensure only one rev is provided Denton Liu
@ 2019-02-07 18:54 ` Junio C Hamano
  2019-02-07 22:34   ` Avery Pennarun
  0 siblings, 1 reply; 5+ messages in thread
From: Junio C Hamano @ 2019-02-07 18:54 UTC (permalink / raw)
  To: Denton Liu; +Cc: apenwarr, git

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.


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] contrib/subtree: ensure only one rev is provided
  2019-02-07 18:54 ` Junio C Hamano
@ 2019-02-07 22:34   ` Avery Pennarun
  2019-02-12 10:00     ` Denton Liu
  0 siblings, 1 reply; 5+ messages in thread
From: Avery Pennarun @ 2019-02-07 22:34 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Denton Liu, Git Mailing List

 ]0;joe - On Thu, Feb 7, 2019 at 1:54 PM Junio C Hamano
<gitster@pobox.com> wrote:
> 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).

I think this generality is probably not useful and it will probably confuse
people less if we prevent it.  It was just one of those "if you don't have
any better ideas, just let people do whatever complicated thing they want"
approaches I used when I was first writing it and didn't know how people
would end up using it.

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

I don't use it very often either.  To be honest, I've noticed weird
behaviour in the version installed with git 2.11.0 in Debian, so I went back
to my own version at https://github.com/apenwarr/git-subtree.  I've been
meaning to investigate further to see what patch might have happened that
caused it to act weird; maybe it's since been fixed.

But I don't see any major problems with the patch in this thread.

Thanks!

Avery

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] contrib/subtree: ensure only one rev is provided
  2019-02-07 22:34   ` Avery Pennarun
@ 2019-02-12 10:00     ` Denton Liu
  2019-03-11  9:47       ` Denton Liu
  0 siblings, 1 reply; 5+ messages in thread
From: Denton Liu @ 2019-02-12 10:00 UTC (permalink / raw)
  To: gitster; +Cc: apenwarr, Git Mailing List

On Thu, Feb 07, 2019 at 05:34:38PM -0500, Avery Pennarun wrote:
> But I don't see any major problems with the patch in this thread.
> 
> Thanks!
> 
> Avery

Hi Junio,

If there are no other comments, I think that this patch is ready to be
queued.

Thanks,

Denton

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] contrib/subtree: ensure only one rev is provided
  2019-02-12 10:00     ` Denton Liu
@ 2019-03-11  9:47       ` Denton Liu
  0 siblings, 0 replies; 5+ messages in thread
From: Denton Liu @ 2019-03-11  9:47 UTC (permalink / raw)
  To: gitster; +Cc: apenwarr, Git Mailing List

On Tue, Feb 12, 2019 at 02:00:02AM -0800, Denton Liu wrote:
> On Thu, Feb 07, 2019 at 05:34:38PM -0500, Avery Pennarun wrote:
> > But I don't see any major problems with the patch in this thread.
> > 
> > Thanks!
> > 
> > Avery
> 
> Hi Junio,
> 
> If there are no other comments, I think that this patch is ready to be
> queued.
> 
> Thanks,
> 
> Denton

Hi Junio,

Sorry for the spam but it seems like this patch was dropped. If there
aren't any other comments on the patch, then I think it's ready to be
queued. Patch below for your convenience.

Thanks,

Denton

-- >8 --
Subject: [PATCH] contrib/subtree: ensure only one rev is provided

While looking at the inline help for git-subtree.sh, I noticed that

	git subtree split --prefix=<prefix> <commit...>

was given as an option. However, it only really makes sense to provide
one revision because of the way the commits are forwarded to rev-parse
so change "<commit...>" to "<commit>" to reflect this. In addition,
check the arguments to ensure that only one rev is provided for all
subcommands that accept a commit.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 contrib/subtree/git-subtree.sh | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 147201dc6c..868e18b9a1 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -14,7 +14,7 @@ git subtree add   --prefix=<prefix> <repository> <ref>
 git subtree merge --prefix=<prefix> <commit>
 git subtree pull  --prefix=<prefix> <repository> <ref>
 git subtree push  --prefix=<prefix> <repository> <ref>
-git subtree split --prefix=<prefix> <commit...>
+git subtree split --prefix=<prefix> <commit>
 --
 h,help        show the help
 q             quiet
@@ -77,6 +77,12 @@ assert () {
 	fi
 }
 
+ensure_single_rev () {
+	if test $# -ne 1
+	then
+		die "You must provide exactly one revision.  Got: '$@'"
+	fi
+}
 
 while test $# -gt 0
 do
@@ -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
 	if test -n "$dirs"
 	then
 		die "Error: Use --prefix instead of bare filenames."
@@ -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
 
 	debug "Adding $dir as '$rev'..."
 	git read-tree --prefix="$dir" $rev || exit $?
@@ -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"
-
 	if test -n "$squash"
 	then
 		first_split="$(find_latest_squash "$dir")"
-- 
2.20.1.522.g5f42c252e9


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-03-11  9:47 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-07 11:20 [PATCH] contrib/subtree: ensure only one rev is provided Denton Liu
2019-02-07 18:54 ` Junio C Hamano
2019-02-07 22:34   ` Avery Pennarun
2019-02-12 10:00     ` Denton Liu
2019-03-11  9:47       ` Denton Liu

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