{"thread":{"id":"50426","subject":"[PATCH] contrib/subtree: ensure only one rev is provided","startedAt":"2019-02-07T11:20:51Z","lastAt":"2019-03-11T09:47:22Z","messageCount":5,"participants":["Denton Liu","Junio C Hamano","Avery Pennarun"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"368704","messageId":"cfd86853cce8a2cd5fae9e6fb9a84f1e3d6daaf4.1549538392.git.liu.denton@gmail.com","threadId":"50426","inReplyTo":null,"subject":"[PATCH] contrib/subtree: ensure only one rev is provided","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-02-07T11:20:46Z","receivedAt":"2019-02-07T11:20:51Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"While looking at the inline help for git-subtree.sh, I noticed that\n\n\tgit subtree split --prefix=<prefix> <commit...>\n\nwas given as an option. However, it only really makes sense to provide\none revision because of the way the commits are forwarded to rev-parse\nso this commit changes \"<commit...>\" to \"<commit>\" to reflect this. In\naddition, it checks the arguments to ensure that only one rev is\nprovided for all subcommands that accept a commit.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n contrib/subtree/git-subtree.sh | 24 ++++++++++++------------\n 1 file changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex 147201dc6c..868e18b9a1 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -14,7 +14,7 @@ git subtree add   --prefix=<prefix> <repository> <ref>\n git subtree merge --prefix=<prefix> <commit>\n git subtree pull  --prefix=<prefix> <repository> <ref>\n git subtree push  --prefix=<prefix> <repository> <ref>\n-git subtree split --prefix=<prefix> <commit...>\n+git subtree split --prefix=<prefix> <commit>\n --\n h,help        show the help\n q             quiet\n@@ -77,6 +77,12 @@ assert () {\n \tfi\n }\n \n+ensure_single_rev () {\n+\tif test $# -ne 1\n+\tthen\n+\t\tdie \"You must provide exactly one revision.  Got: '$@'\"\n+\tfi\n+}\n \n while test $# -gt 0\n do\n@@ -185,6 +191,7 @@ if test \"$command\" != \"pull\" &&\n then\n \trevs=$(git rev-parse $default --revs-only \"$@\") || exit $?\n \tdirs=$(git rev-parse --no-revs --no-flags \"$@\") || exit $?\n+\tensure_single_rev $revs\n \tif test -n \"$dirs\"\n \tthen\n \t\tdie \"Error: Use --prefix instead of bare filenames.\"\n@@ -716,9 +723,8 @@ cmd_add_repository () {\n }\n \n cmd_add_commit () {\n-\trevs=$(git rev-parse $default --revs-only \"$@\") || exit $?\n-\tset -- $revs\n-\trev=\"$1\"\n+\trev=$(git rev-parse $default --revs-only \"$@\") || exit $?\n+\tensure_single_rev $rev\n \n \tdebug \"Adding $dir as '$rev'...\"\n \tgit read-tree --prefix=\"$dir\" $rev || exit $?\n@@ -817,16 +823,10 @@ cmd_split () {\n }\n \n cmd_merge () {\n-\trevs=$(git rev-parse $default --revs-only \"$@\") || exit $?\n+\trev=$(git rev-parse $default --revs-only \"$@\") || exit $?\n+\tensure_single_rev $rev\n \tensure_clean\n \n-\tset -- $revs\n-\tif test $# -ne 1\n-\tthen\n-\t\tdie \"You must provide exactly one revision.  Got: '$revs'\"\n-\tfi\n-\trev=\"$1\"\n-\n \tif test -n \"$squash\"\n \tthen\n \t\tfirst_split=\"$(find_latest_squash \"$dir\")\"\n-- \n2.20.1.522.g5f42c252e9\n\n"},{"id":"368718","messageId":"xmqqftszpgy1.fsf@gitster-ct.c.googlers.com","threadId":"50426","inReplyTo":"cfd86853cce8a2cd5fae9e6fb9a84f1e3d6daaf4.1549538392.git.liu.denton@gmail.com","subject":"Re: [PATCH] contrib/subtree: ensure only one rev is provided","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-07T18:54:46Z","receivedAt":"2019-02-07T18:54:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu <liu.denton@gmail.com> writes:\n\n> @@ -185,6 +191,7 @@ if test \"$command\" != \"pull\" &&\n>  then\n>  \trevs=$(git rev-parse $default --revs-only \"$@\") || exit $?\n>  \tdirs=$(git rev-parse --no-revs --no-flags \"$@\") || exit $?\n> +\tensure_single_rev $revs\n\nThis applies to anything other than pull, add and push, so certainly\n'split' is covered here.\n\n> @@ -716,9 +723,8 @@ cmd_add_repository () {\n>  }\n>  \n>  cmd_add_commit () {\n> -\trevs=$(git rev-parse $default --revs-only \"$@\") || exit $?\n> -\tset -- $revs\n> -\trev=\"$1\"\n> +\trev=$(git rev-parse $default --revs-only \"$@\") || exit $?\n> +\tensure_single_rev $rev\n\nThere are two callers of this helper.  cmd_add passes \"$@\" but it\ndoes so only after making sure there is only one argument that is a\ncommit, so this conversion is not incorrect.\n\nI am not sure if the other caller is OK, though.  cmd_add_repository\ncan get more than one revs, and uses the first one as $rev to read\nthe tree from, expecting that this helper to ignore other ones that\nare emitted from 'git rev-parse --revs-only \"$@\"'.\n\nFor that matter, one of the early things cmd_split does is to call\nthe find_existing_splits helper with $revs, and it seems to be\nprepared to be red multiple $revs (it is passed to \"git log\", so I\nwould expect that incoming $revs is allowed to specify bottom to\nlimit the traversal, e.g. \"git log maint..master\").  The addition of\n\"ensure_single_rev\" we saw in an earlier hunk near ll.191 makes such\ncall impossible.  I am not a user of subtree, so I do not know if\nit is a good change (i.e. making something nonsensical impossible to\ndo is good, making something useful impossible to do is bad).\n\n> @@ -817,16 +823,10 @@ cmd_split () {\n>  }\n>  \n>  cmd_merge () {\n> -\trevs=$(git rev-parse $default --revs-only \"$@\") || exit $?\n> +\trev=$(git rev-parse $default --revs-only \"$@\") || exit $?\n> +\tensure_single_rev $rev\n>  \tensure_clean\n>  \n> -\tset -- $revs\n> -\tif test $# -ne 1\n> -\tthen\n> -\t\tdie \"You must provide exactly one revision.  Got: '$revs'\"\n> -\tfi\n> -\trev=\"$1\"\n> -\n\nThis one already was insisting on a single version, so it clearly is\na correct no-op conversion, but wouldn't this have been already\ncaught upfront where anything other than pull, add and push are\nhandled?  I do understand if the new call to ensure_single is made\nto the other caller of cmd_merge in cmd_pull, though.\n\n>  \tif test -n \"$squash\"\n>  \tthen\n>  \t\tfirst_split=\"$(find_latest_squash \"$dir\")\"\n\nIn any case, I do not use subtree, and the last time I looked at\nthis script is a long time ago, so take all of the above with a\nlarge grain of salt.\n\nThanks.\n\n"},{"id":"368750","messageId":"CAHqTa-3bDnAm=49uBDLWxLrpOMd6sh1ve1fmmnf5kCbVxHsawg@mail.gmail.com","threadId":"50426","inReplyTo":"xmqqftszpgy1.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] contrib/subtree: ensure only one rev is provided","fromName":"Avery Pennarun","fromEmail":"apenwarr@gmail.com","sentAt":"2019-02-07T22:34:38Z","receivedAt":"2019-02-07T22:34:53Z","isPatch":true,"sender":{"key":"apenwarr@gmail.com","avatar":"https://avatars.githubusercontent.com/u/20592?v=4"},"body":" ]0;joe - On Thu, Feb 7, 2019 at 1:54 PM Junio C Hamano\n<gitster@pobox.com> wrote:\n> I am not sure if the other caller is OK, though.  cmd_add_repository\n> can get more than one revs, and uses the first one as $rev to read\n> the tree from, expecting that this helper to ignore other ones that\n> are emitted from 'git rev-parse --revs-only \"$@\"'.\n>\n> For that matter, one of the early things cmd_split does is to call\n> the find_existing_splits helper with $revs, and it seems to be\n> prepared to be red multiple $revs (it is passed to \"git log\", so I\n> would expect that incoming $revs is allowed to specify bottom to\n> limit the traversal, e.g. \"git log maint..master\").  The addition of\n> \"ensure_single_rev\" we saw in an earlier hunk near ll.191 makes such\n> call impossible.  I am not a user of subtree, so I do not know if\n> it is a good change (i.e. making something nonsensical impossible to\n> do is good, making something useful impossible to do is bad).\n\nI think this generality is probably not useful and it will probably confuse\npeople less if we prevent it.  It was just one of those \"if you don't have\nany better ideas, just let people do whatever complicated thing they want\"\napproaches I used when I was first writing it and didn't know how people\nwould end up using it.\n\n> In any case, I do not use subtree, and the last time I looked at\n> this script is a long time ago, so take all of the above with a\n> large grain of salt.\n\nI don't use it very often either.  To be honest, I've noticed weird\nbehaviour in the version installed with git 2.11.0 in Debian, so I went back\nto my own version at https://github.com/apenwarr/git-subtree.  I've been\nmeaning to investigate further to see what patch might have happened that\ncaused it to act weird; maybe it's since been fixed.\n\nBut I don't see any major problems with the patch in this thread.\n\nThanks!\n\nAvery\n"},{"id":"369140","messageId":"20190212100002.GA28167@archbookpro.localdomain","threadId":"50426","inReplyTo":"CAHqTa-3bDnAm=49uBDLWxLrpOMd6sh1ve1fmmnf5kCbVxHsawg@mail.gmail.com","subject":"Re: [PATCH] contrib/subtree: ensure only one rev is provided","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-02-12T10:00:02Z","receivedAt":"2019-02-12T10:00:07Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Thu, Feb 07, 2019 at 05:34:38PM -0500, Avery Pennarun wrote:\n> But I don't see any major problems with the patch in this thread.\n> \n> Thanks!\n> \n> Avery\n\nHi Junio,\n\nIf there are no other comments, I think that this patch is ready to be\nqueued.\n\nThanks,\n\nDenton\n"},{"id":"371137","messageId":"20190311094717.GB31092@archbookpro.localdomain","threadId":"50426","inReplyTo":"20190212100002.GA28167@archbookpro.localdomain","subject":"Re: [PATCH] contrib/subtree: ensure only one rev is provided","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-03-11T09:47:17Z","receivedAt":"2019-03-11T09:47:22Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Tue, Feb 12, 2019 at 02:00:02AM -0800, Denton Liu wrote:\n> On Thu, Feb 07, 2019 at 05:34:38PM -0500, Avery Pennarun wrote:\n> > But I don't see any major problems with the patch in this thread.\n> > \n> > Thanks!\n> > \n> > Avery\n> \n> Hi Junio,\n> \n> If there are no other comments, I think that this patch is ready to be\n> queued.\n> \n> Thanks,\n> \n> Denton\n\nHi Junio,\n\nSorry for the spam but it seems like this patch was dropped. If there\naren't any other comments on the patch, then I think it's ready to be\nqueued. Patch below for your convenience.\n\nThanks,\n\nDenton\n\n-- >8 --\nSubject: [PATCH] contrib/subtree: ensure only one rev is provided\n\nWhile looking at the inline help for git-subtree.sh, I noticed that\n\n\tgit subtree split --prefix=<prefix> <commit...>\n\nwas given as an option. However, it only really makes sense to provide\none revision because of the way the commits are forwarded to rev-parse\nso change \"<commit...>\" to \"<commit>\" to reflect this. In addition,\ncheck the arguments to ensure that only one rev is provided for all\nsubcommands that accept a commit.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n contrib/subtree/git-subtree.sh | 24 ++++++++++++------------\n 1 file changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex 147201dc6c..868e18b9a1 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -14,7 +14,7 @@ git subtree add   --prefix=<prefix> <repository> <ref>\n git subtree merge --prefix=<prefix> <commit>\n git subtree pull  --prefix=<prefix> <repository> <ref>\n git subtree push  --prefix=<prefix> <repository> <ref>\n-git subtree split --prefix=<prefix> <commit...>\n+git subtree split --prefix=<prefix> <commit>\n --\n h,help        show the help\n q             quiet\n@@ -77,6 +77,12 @@ assert () {\n \tfi\n }\n \n+ensure_single_rev () {\n+\tif test $# -ne 1\n+\tthen\n+\t\tdie \"You must provide exactly one revision.  Got: '$@'\"\n+\tfi\n+}\n \n while test $# -gt 0\n do\n@@ -185,6 +191,7 @@ if test \"$command\" != \"pull\" &&\n then\n \trevs=$(git rev-parse $default --revs-only \"$@\") || exit $?\n \tdirs=$(git rev-parse --no-revs --no-flags \"$@\") || exit $?\n+\tensure_single_rev $revs\n \tif test -n \"$dirs\"\n \tthen\n \t\tdie \"Error: Use --prefix instead of bare filenames.\"\n@@ -716,9 +723,8 @@ cmd_add_repository () {\n }\n \n cmd_add_commit () {\n-\trevs=$(git rev-parse $default --revs-only \"$@\") || exit $?\n-\tset -- $revs\n-\trev=\"$1\"\n+\trev=$(git rev-parse $default --revs-only \"$@\") || exit $?\n+\tensure_single_rev $rev\n \n \tdebug \"Adding $dir as '$rev'...\"\n \tgit read-tree --prefix=\"$dir\" $rev || exit $?\n@@ -817,16 +823,10 @@ cmd_split () {\n }\n \n cmd_merge () {\n-\trevs=$(git rev-parse $default --revs-only \"$@\") || exit $?\n+\trev=$(git rev-parse $default --revs-only \"$@\") || exit $?\n+\tensure_single_rev $rev\n \tensure_clean\n \n-\tset -- $revs\n-\tif test $# -ne 1\n-\tthen\n-\t\tdie \"You must provide exactly one revision.  Got: '$revs'\"\n-\tfi\n-\trev=\"$1\"\n-\n \tif test -n \"$squash\"\n \tthen\n \t\tfirst_split=\"$(find_latest_squash \"$dir\")\"\n-- \n2.20.1.522.g5f42c252e9\n\n"}]}