{"thread":{"id":"18034","subject":"[PATCH] clone: ignore --depth when cloning locally (implicitly --local)","startedAt":"2009-02-26T18:20:19Z","lastAt":"2009-02-27T16:54:01Z","messageCount":13,"participants":["Johannes Schindelin","Junio C Hamano","Jeff King","Daniel Barkalow"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"106348","messageId":"1f63785d96d243ff19a063f684abbbe46cafc075.1235672273u.git.johannes.schindelin@gmx.de","threadId":"18034","inReplyTo":"cover.1235672273u.git.johannes.schindelin@gmx.de","subject":"[PATCH] clone: ignore --depth when cloning locally (implicitly --local)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-26T18:20:19Z","receivedAt":"2009-02-26T18:20:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When cloning locally, we default to --local, as it makes the whole\noperation fast and efficient.\n\nAs the most common intent of cloning with a --depth parameter is to\nsave space, and --local saves space more than --depth ever can,\nwarn the user and ignore the --depth parameter when cloning locally.\n\nShould --depth be desired, the user can always force proper cloning\nby using a file:// URL.\n\nSigned-off-by: Johannes Schindelin \n---\n\n\tBefore making git-clone a builtin, we had code to force a clone \n\twhen a --depth parameter was passed. However, it is dubitable \n\tthat a user wants --depth rather than --local, and as in the \n\t--local case, we should cater for the common case.\n\n Documentation/git-clone.txt | 4 ++++\n builtin-clone.c | 5 ++++-\n t/t5701-clone-local.sh | 7 +++++++\n 3 files changed, 15 insertions(+), 1 deletions(-)\n\ndiff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\nindex 95f08b9..9b8b389 100644\n--- a/Documentation/git-clone.txt\n+++ b/Documentation/git-clone.txt\n@@ -138,6 +138,10 @@ then the cloned repository will become corrupt.\n \tare only interested in the recent history of a large project\n \twith a long history, and would want to send in fixes\n \tas patches.\n++\n+This option is ignored when cloning locally; to force a shallow\n+clone even locally, use the `--no-hardlinks` option, or a\n+'file://' location.\n \n ::\n \tThe (possibly remote) repository to clone from. See the\ndiff --git a/builtin-clone.c b/builtin-clone.c\nindex c338910..5831034 100644\n--- a/builtin-clone.c\n+++ b/builtin-clone.c\n@@ -511,8 +511,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \trefspec.src = src_ref_prefix;\n \trefspec.dst = branch_top.buf;\n \n-\tif (path && !is_bundle)\n+\tif (path && !is_bundle) {\n+\t\tif (option_depth)\n+\t\t\twarning(\"Ignoring --depth for local clone\");\n \t\trefs = clone_local(path, git_dir);\n+\t}\n \telse {\n \t\tstruct remote *remote = remote_get(argv[0]);\n \t\ttransport = transport_get(remote, remote->url[0]);\ndiff --git a/t/t5701-clone-local.sh b/t/t5701-clone-local.sh\nindex 3559d17..2938c02 100755\n--- a/t/t5701-clone-local.sh\n+++ b/t/t5701-clone-local.sh\n@@ -132,4 +132,11 @@ test_expect_success 'clone empty repository' '\n \t test $actual = $expected)\n '\n \n+test_expect_success 'clone --depth locally ignores --depth' '\n+\ttest_commit meredith chivers &&\n+\tgit clone --depth 1 . depth 2> out.err &&\n+\tgrep \"warning: Ignoring --depth for local clone\" out.err &&\n+\ttest 1 -lt $(cd depth && git rev-list master | wc -l)\n+'\n+\n test_done\n-- \n1.6.2.rc1.350.g6caf6\n"},{"id":"106371","messageId":"7v8wnsrf55.fsf@gitster.siamese.dyndns.org","threadId":"18034","inReplyTo":"1f63785d96d243ff19a063f684abbbe46cafc075.1235672273u.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] clone: ignore --depth when cloning locally (implicitly --local)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-26T22:45:42Z","receivedAt":"2009-02-26T22:45:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> When cloning locally, we default to --local, as it makes the whole\n> operation fast and efficient.\n>\n> As the most common intent of cloning with a --depth parameter is to\n> save space, and --local saves space more than --depth ever can,\n> warn the user and ignore the --depth parameter when cloning locally.\n>\n> Should --depth be desired, the user can always force proper cloning\n> by using a file:// URL.\n>\n> Signed-off-by: Johannes Schindelin \n\nMakes sense. I'm inclined to apply this as a \"fix\" before 1.6.2, but I\ncannot exactly say what we are fixing. User's expectations?\n"},{"id":"106372","messageId":"alpine.DEB.1.00.0902262352380.10279@pacific.mpi-cbg.de","threadId":"18034","inReplyTo":"7v8wnsrf55.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] clone: ignore --depth when cloning locally (implicitly --local)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-26T22:55:41Z","receivedAt":"2009-02-26T22:55:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 26 Feb 2009, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > When cloning locally, we default to --local, as it makes the whole\n> > operation fast and efficient.\n> >\n> > As the most common intent of cloning with a --depth parameter is to\n> > save space, and --local saves space more than --depth ever can,\n> > warn the user and ignore the --depth parameter when cloning locally.\n> >\n> > Should --depth be desired, the user can always force proper cloning\n> > by using a file:// URL.\n> >\n> > Signed-off-by: Johannes Schindelin \n> \n> Makes sense. I'm inclined to apply this as a \"fix\" before 1.6.2, but I\n> cannot exactly say what we are fixing. User's expectations?\n\nIt seems somebody mentioned on IRC that --depth with --no-local will not \nwork as expected, but was not willing to write to the list, or for that \nmatter, do anything else.\n\nSo given that level of enthusiasm for _supporting_ this feature, I am \ninclined to postpone the patch, or drop it altogether.\n\nI mean, I tried to be a good netizen and not let a patch I started go \nstale, but it is not my itch, and if those whose itch it is do not care, \nmaybe I shouldn't, either.\n\nCiao,\nDscho\n"},{"id":"106376","messageId":"cover.1235690903u.git.johannes.schindelin@gmx.de","threadId":"18034","inReplyTo":"alpine.DEB.1.00.0902262352380.10279@pacific.mpi-cbg.de","subject":"[PATCH 0/2] shallow clone stuff","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-26T23:31:17Z","receivedAt":"2009-02-26T23:31:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The first patch fixes the current bug that --no-hardlinks is not heeded, \nand the second patch introduces the warning when cloning locally with a \n--depth parameter.\n\nJohannes Schindelin (2):\n clone: do not ignore --no-hardlinks\n clone: ignore --depth when cloning locally (implicitly --local)\n\n Documentation/git-clone.txt | 4 ++++\n builtin-clone.c | 5 ++++-\n t/t5701-clone-local.sh | 14 ++++++++++++++\n 3 files changed, 22 insertions(+), 1 deletions(-)\n"},{"id":"106375","messageId":"da2c9173f403c4fb509609fb823b6be0728d94f3.1235690903u.git.johannes.schindelin@gmx.de","threadId":"18034","inReplyTo":"alpine.DEB.1.00.0902262352380.10279@pacific.mpi-cbg.de","subject":"[PATCH 1/2] clone: do not ignore --no-hardlinks","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-26T23:31:22Z","receivedAt":"2009-02-26T23:31:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Somehow --no-hardlinks got broken by making clone a builtin. This\nwas discovered during my work on --depth being ignored for local\nclones.\n\nThere will be a test case that tests for --no-hardlinks in conjunction\nwith --depth, so this patch is not accompanied by a separate test.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin-clone.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-clone.c b/builtin-clone.c\nindex c338910..5540372 100644\n--- a/builtin-clone.c\n+++ b/builtin-clone.c\n@@ -511,7 +511,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \trefspec.src = src_ref_prefix;\n \trefspec.dst = branch_top.buf;\n \n-\tif (path && !is_bundle)\n+\tif (path && !is_bundle && use_local_hardlinks)\n \t\trefs = clone_local(path, git_dir);\n \telse {\n \t\tstruct remote *remote = remote_get(argv[0]);\n-- \n1.6.2.rc2.404.g8394e\n"},{"id":"106374","messageId":"5adb270495925f887a1101c6f63828883e1b9b2c.1235690903u.git.johannes.schindelin@gmx.de","threadId":"18034","inReplyTo":"alpine.DEB.1.00.0902262352380.10279@pacific.mpi-cbg.de","subject":"[PATCH 2/2] clone: ignore --depth when cloning locally (implicitly --local)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-26T23:31:25Z","receivedAt":"2009-02-26T23:31:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When cloning locally, we default to --local, as it makes the whole\noperation fast and efficient.\n\nAs the most common intent of cloning with a --depth parameter is to\nsave space, and --local saves more space than --depth ever can,\nwarn the user and ignore the --depth parameter when cloning locally.\n\nShould --depth be desired, the user can always force proper cloning\nby passing the --no-hardlinks parameter, or by using a file:// URL.\n\nSigned-off-by: Johannes Schindelin \n---\n Documentation/git-clone.txt | 4 ++++\n builtin-clone.c | 5 ++++-\n t/t5701-clone-local.sh | 14 ++++++++++++++\n 3 files changed, 22 insertions(+), 1 deletions(-)\n\ndiff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\nindex 95f08b9..9b8b389 100644\n--- a/Documentation/git-clone.txt\n+++ b/Documentation/git-clone.txt\n@@ -138,6 +138,10 @@ then the cloned repository will become corrupt.\n \tare only interested in the recent history of a large project\n \twith a long history, and would want to send in fixes\n \tas patches.\n++\n+This option is ignored when cloning locally; to force a shallow\n+clone even locally, use the `--no-hardlinks` option, or a\n+'file://' location.\n \n ::\n \tThe (possibly remote) repository to clone from. See the\ndiff --git a/builtin-clone.c b/builtin-clone.c\nindex 5540372..73d5a76 100644\n--- a/builtin-clone.c\n+++ b/builtin-clone.c\n@@ -511,8 +511,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \trefspec.src = src_ref_prefix;\n \trefspec.dst = branch_top.buf;\n \n-\tif (path && !is_bundle && use_local_hardlinks)\n+\tif (path && !is_bundle && use_local_hardlinks) {\n+\t\tif (option_depth)\n+\t\t\twarning(\"Ignoring --depth for local clone\");\n \t\trefs = clone_local(path, git_dir);\n+\t}\n \telse {\n \t\tstruct remote *remote = remote_get(argv[0]);\n \t\ttransport = transport_get(remote, remote->url[0]);\ndiff --git a/t/t5701-clone-local.sh b/t/t5701-clone-local.sh\nindex 3559d17..8600539 100755\n--- a/t/t5701-clone-local.sh\n+++ b/t/t5701-clone-local.sh\n@@ -132,4 +132,18 @@ test_expect_success 'clone empty repository' '\n \t test $actual = $expected)\n '\n \n+test_expect_success 'clone --depth locally ignores --depth' '\n+\ttest_commit meredith chivers && git tag -d meredith &&\n+\ttest_commit lisa diamond && git tag -d lisa &&\n+\tgit clone --depth 1 . depth 2> out.err &&\n+\tgrep \"warning: Ignoring --depth for local clone\" out.err &&\n+\ttest 2 -lt $(cd depth && git rev-list master | wc -l)\n+'\n+\n+test_expect_success '--depth is not ignored with --no-hardlinks' '\n+\tgit clone --depth 1 --no-hardlinks . depth2 2> out.err &&\n+\t! grep \"warning: Ignoring --depth for local clone\" out.err &&\n+\ttest 2 = $(cd depth2 && git rev-list master | wc -l)\n+'\n+\n test_done\n-- \n1.6.2.rc2.404.g8394e\n"},{"id":"106384","messageId":"7vmyc8pvhn.fsf@gitster.siamese.dyndns.org","threadId":"18034","inReplyTo":"cover.1235690903u.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 0/2] shallow clone stuff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-27T00:35:32Z","receivedAt":"2009-02-27T00:35:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> The first patch fixes the current bug that --no-hardlinks is not heeded, \n> and the second patch introduces the warning when cloning locally with a \n> --depth parameter.\n\nThanks.\n"},{"id":"106390","messageId":"20090227025829.GA22060@coredump.intra.peff.net","threadId":"18034","inReplyTo":"da2c9173f403c4fb509609fb823b6be0728d94f3.1235690903u.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 1/2] clone: do not ignore --no-hardlinks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-27T02:58:29Z","receivedAt":"2009-02-27T02:58:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 27, 2009 at 12:31:22AM +0100, Johannes Schindelin wrote:\n\n> Somehow --no-hardlinks got broken by making clone a builtin. This\n> was discovered during my work on --depth being ignored for local\n> clones.\n> \n> There will be a test case that tests for --no-hardlinks in conjunction\n> with --depth, so this patch is not accompanied by a separate test.\n\nHmm. But --no-hardlinks has an effect later, in copy_or_link_directory,\nmaking it just do a copy. So it _does_ work, just not in the way you\nexpect.\n\nI think to turn off local shortcuts entirely, --no-local would probably\nmake more sense. IOW, something like this:\n\n---\ndiff --git a/builtin-clone.c b/builtin-clone.c\nindex c338910..7c6f59f 100644\n--- a/builtin-clone.c\n+++ b/builtin-clone.c\n@@ -35,7 +35,8 @@ static const char * const builtin_clone_usage[] = {\n };\n \n static int option_quiet, option_no_checkout, option_bare, option_mirror;\n-static int option_local, option_no_hardlinks, option_shared;\n+static int option_local = -1;\n+static int option_no_hardlinks, option_shared;\n static char *option_template, *option_reference, *option_depth;\n static char *option_origin = NULL;\n static char *option_upload_pack = \"git-upload-pack\";\n@@ -229,7 +230,7 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest)\n \t\tif (!option_no_hardlinks) {\n \t\t\tif (!link(src->buf, dest->buf))\n \t\t\t\tcontinue;\n-\t\t\tif (option_local)\n+\t\t\tif (option_local == 1)\n \t\t\t\tdie(\"failed to create link %s\", dest->buf);\n \t\t\toption_no_hardlinks = 1;\n \t\t}\n@@ -511,7 +512,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \trefspec.src = src_ref_prefix;\n \trefspec.dst = branch_top.buf;\n \n-\tif (path && !is_bundle)\n+\tif (option_local != 0 && path && !is_bundle)\n \t\trefs = clone_local(path, git_dir);\n \telse {\n \t\tstruct remote *remote = remote_get(argv[0]);\n"},{"id":"106404","messageId":"7vy6vsmjeo.fsf@gitster.siamese.dyndns.org","threadId":"18034","inReplyTo":"20090227025829.GA22060@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] clone: do not ignore --no-hardlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-27T07:24:47Z","receivedAt":"2009-02-27T07:24:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Feb 27, 2009 at 12:31:22AM +0100, Johannes Schindelin wrote:\n>\n>> Somehow --no-hardlinks got broken by making clone a builtin. This\n>> was discovered during my work on --depth being ignored for local\n>> clones.\n>> \n>> There will be a test case that tests for --no-hardlinks in conjunction\n>> with --depth, so this patch is not accompanied by a separate test.\n>\n> Hmm. But --no-hardlinks has an effect later, in copy_or_link_directory,\n> making it just do a copy. So it _does_ work, just not in the way you\n> expect.\n\nYup, I agree.\n\n> I think to turn off local shortcuts entirely, --no-local would probably\n> make more sense. IOW, something like this:\n\nSo across filesystems:\n\n - \"git clone /p/a/t/h\" falls back to copying;\n\n - \"git clone --local /p/a/t/h\" should fail without falling back to\n copying; and\n\n - \"git clone --no-local /p/a/t/h\" should work as if file:///p/a/t/h\n was given.\n\nThat is much more sensible than making \"git clone --no-hardlinks /p/a/t/h\"\nimply more than what the option really means: we are making a local copy\nbut do not cheat with hardlinking.\n"},{"id":"106406","messageId":"7vr61kmht9.fsf@gitster.siamese.dyndns.org","threadId":"18034","inReplyTo":"7vy6vsmjeo.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] clone: do not ignore --no-hardlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-27T07:59:14Z","receivedAt":"2009-02-27T07:59:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n> So across filesystems:\n>\n> - \"git clone /p/a/t/h\" falls back to copying;\n>\n> - \"git clone --local /p/a/t/h\" should fail without falling back to\n> copying; and\n>\n> - \"git clone --no-local /p/a/t/h\" should work as if file:///p/a/t/h\n> was given.\n>\n> That is much more sensible than making \"git clone --no-hardlinks /p/a/t/h\"\n> imply more than what the option really means: we are making a local copy\n> but do not cheat with hardlinking.\n\nI liked it so much that I wrote a commit message for it:\n\n commit c255d7bbf4dd567ca60a8991c3ac052c9b2d1593\n Author: Jeff King \n Date: Thu Feb 26 23:31:49 2009 -0800\n\n clone: do not ignore --no-local option\n\n With a variable initialized to false, we couldn't tell if the user didn't\n give --local or if the user explicitly told us --no-local after we get control\n back from parse_options().\n\n This fixes and makes \"git clone --no-local /p/a/t/h\" always use non-local\n transport (aka \"git native protocol over pipe\").\n\n Signed-off-by: Junio C Hamano \n\nExcept that it does not work.\n\nInitializing option_local to -1 and then feeding it to parse_options() via\nOPT_BOOLEAN() is a cute idea, but when parse_options() sees a command line\nlike this:\n\n\t$ git clone -l /p/a/t/h\n\nIt triggers this codepath in get_value():\n\n\tcase OPTION_BOOLEAN:\n\t\t*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;\n\t\treturn 0;\n\nand ends up incrementing it to zero.\n\nI wonder what would break if we simply change this to:\n\n\tcase OPTION_BOOLEAN:\n\t\t*(int *)opt->value = !unset;\n\t\treturn 0;\n\nDamn it, it is called BOOLEAN, and naïvely I would think --option would\nset it to 1, --no-option would set it to 0, and not having --option nor\n--no-option would leave the associated variable alone, but apparently that\nis not what is happening.\n\nPierre, do you remember why this code is implemented this way? The\n\"increment if we have --option\" semantics seems to date back to 4a59fd1\n(Add a simple option parser., 2007-10-15) which is the day one of the\nhistory of parse-options.\n"},{"id":"106407","messageId":"7vmyc8mh56.fsf@gitster.siamese.dyndns.org","threadId":"18034","inReplyTo":"7vr61kmht9.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] clone: do not ignore --no-hardlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-27T08:13:41Z","receivedAt":"2009-02-27T08:13:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> It triggers this codepath in get_value():\n>\n> \tcase OPTION_BOOLEAN:\n> \t\t*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;\n> \t\treturn 0;\n>\n> and ends up incrementing it to zero.\n>\n> I wonder what would break if we simply change this to:\n>\n> \tcase OPTION_BOOLEAN:\n> \t\t*(int *)opt->value = !unset;\n> \t\treturn 0;\n>\n> Damn it, it is called BOOLEAN, and naïvely I would think --option would\n> set it to 1, --no-option would set it to 0, and not having --option nor\n> --no-option would leave the associated variable alone, but apparently that\n> is not what is happening.\n>\n> Pierre, do you remember why this code is implemented this way? The\n> \"increment if we have --option\" semantics seems to date back to 4a59fd1\n> (Add a simple option parser., 2007-10-15) which is the day one of the\n> history of parse-options.\n\nI think that this came from a misguided attempt to do:\n\n\t-v\n -v -v\n -v -v -v\n\nto cumulatively record the desired verbosity levels.\n\nOriginally we did not have OPT__VERBOSITY() nor OPT_VERBOSE() so it is\nunderstandable that OPT_VERBOSE() was implemented in terms of this\nOPT_BOOLEAN() construct.\n\nI think all parse_options() users that do support the verbosity level is\neither using OPT__VERBOSE() or OPT_VERBOSITY() these days, and we could\nprobably fix this by doing something like:\n\n (1) Introduce OPTION_CUMULATIVE_LEVEL whose behaviour is \"--no-option to\n reset to zero, --option to raise by one\", and fix OPTION_BOOLEAN to\n \"--no-option to zero, --option to one\":\n\n\tcase OPTION_BOOLEAN:\n \t\t*(int *)opt->value = !unset;\n\t\treturn 0;\n\tcase OPTION_CUMULATIVE_LEVEL:\n \t\t*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;\n \t\treturn 0;\n\n (2) Use OPT_CUMULATIVE_LEVEL() instead of OPT_BOOLEAN() to implement\n OPT__VERBOSE() and OPT__QUIET().\n\nBut I am inclined to postpone such a change til after 1.6.2.\n"},{"id":"106426","messageId":"20090227113558.GA19756@coredump.intra.peff.net","threadId":"18034","inReplyTo":"7vr61kmht9.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] clone: do not ignore --no-hardlinks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-27T11:35:59Z","receivedAt":"2009-02-27T11:35:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 26, 2009 at 11:59:14PM -0800, Junio C Hamano wrote:\n\n> Except that it does not work.\n> \n> Initializing option_local to -1 and then feeding it to parse_options() via\n> OPT_BOOLEAN() is a cute idea, but when parse_options() sees a command line\n> like this:\n\nAh, sorry. I didn't really test it thoroughly (though I did try\n--no-local, I obviously did not test --local with a hardlink failure).\n\n> Damn it, it is called BOOLEAN, and naïvely I would think --option would\n> set it to 1, --no-option would set it to 0, and not having --option nor\n> --no-option would leave the associated variable alone, but apparently that\n> is not what is happening.\n\nThat is exactly what I expected as a caller (and it sounds like you did,\ntoo), which perhaps argues in favor of changing it.\n\nUpon seeing this, I guessed (as you did in the next email) that it was\nabout bumping up levels of a boolean for things like verbosity. And I\nagree that sould not be OPT_BOOLEAN, but rather OPT_INCREMENT or\nwhatever.\n\nHowever, I did some grepping and it doesn't look like anybody is\nactually making use of the incrementing behavior (I was just looking for\n\"verbose\", though, so it's possible some unrelated boolean is using it).\nSo it may be that the behavior can be changed now while no callers care,\nand we can get away without OPT_INCREMENT until somebody actually needs\nit.\n\n-Peff\n"},{"id":"106445","messageId":"alpine.LNX.1.00.0902271146060.19665@iabervon.org","threadId":"18034","inReplyTo":"7vmyc8mh56.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] clone: do not ignore --no-hardlinks","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2009-02-27T16:54:01Z","receivedAt":"2009-02-27T16:54:01Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Fri, 27 Feb 2009, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > It triggers this codepath in get_value():\n> >\n> > \tcase OPTION_BOOLEAN:\n> > \t\t*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;\n> > \t\treturn 0;\n> >\n> > and ends up incrementing it to zero.\n> >\n> > I wonder what would break if we simply change this to:\n> >\n> > \tcase OPTION_BOOLEAN:\n> > \t\t*(int *)opt->value = !unset;\n> > \t\treturn 0;\n> >\n> > Damn it, it is called BOOLEAN, and naïvely I would think --option would\n> > set it to 1, --no-option would set it to 0, and not having --option nor\n> > --no-option would leave the associated variable alone, but apparently that\n> > is not what is happening.\n> >\n> > Pierre, do you remember why this code is implemented this way? The\n> > \"increment if we have --option\" semantics seems to date back to 4a59fd1\n> > (Add a simple option parser., 2007-10-15) which is the day one of the\n> > history of parse-options.\n> \n> I think that this came from a misguided attempt to do:\n> \n> \t-v\n> -v -v\n> -v -v -v\n> \n> to cumulatively record the desired verbosity levels.\n> \n> Originally we did not have OPT__VERBOSITY() nor OPT_VERBOSE() so it is\n> understandable that OPT_VERBOSE() was implemented in terms of this\n> OPT_BOOLEAN() construct.\n> \n> I think all parse_options() users that do support the verbosity level is\n> either using OPT__VERBOSE() or OPT_VERBOSITY() these days, and we could\n> probably fix this by doing something like:\n> \n> (1) Introduce OPTION_CUMULATIVE_LEVEL whose behaviour is \"--no-option to\n> reset to zero, --option to raise by one\", and fix OPTION_BOOLEAN to\n> \"--no-option to zero, --option to one\":\n> \n> \tcase OPTION_BOOLEAN:\n> \t\t*(int *)opt->value = !unset;\n> \t\treturn 0;\n> \tcase OPTION_CUMULATIVE_LEVEL:\n> \t\t*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;\n> \t\treturn 0;\n> \n> (2) Use OPT_CUMULATIVE_LEVEL() instead of OPT_BOOLEAN() to implement\n> OPT__VERBOSE() and OPT__QUIET().\n\nI think there are a few other options that work like that, such as -C for \ndiff and the like; I don't know if there are any others that go through \nparse_options, but I wouldn't count on there not being any. So I think the \nconversion of OPT_BOOLEAN needs to go with an audit of current users.\n\n\t-Daniel\n*This .sig left intentionally blank*"}]}