{"thread":{"id":"49642","subject":"[RFC 0/2] explicitly support or not support --exclude-promisor-objects","startedAt":"2018-10-23T01:13:56Z","lastAt":"2018-12-04T02:20:21Z","messageCount":19,"participants":["Matthew DeVore","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"361239","messageId":"cover.1540256910.git.matvore@google.com","threadId":"49642","inReplyTo":null,"subject":"[RFC 0/2] explicitly support or not support --exclude-promisor-objects","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2018-10-23T01:13:40Z","receivedAt":"2018-10-23T01:13:56Z","isPatch":false,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"This patch set fixes incorrect parsing of the --exclude-promisor-objects\noption that I found while working on:\n\n https://public-inbox.org/git/cover.1539298957.git.matvore@google.com/\n\nThank you,\n\nMatthew DeVore (2):\n Documentation/git-log.txt: do not show --exclude-promisor-objects\n exclude-promisor-objects: declare when option is allowed\n\n Documentation/rev-list-options.txt | 2 +-\n builtin/pack-objects.c | 1 +\n builtin/prune.c | 1 +\n builtin/rev-list.c | 1 +\n revision.c | 3 ++-\n revision.h | 1 +\n t/t4202-log.sh | 4 ++++\n t/t8002-blame.sh | 4 ++++\n 8 files changed, 15 insertions(+), 2 deletions(-)\n\n-- \n2.19.1.568.g152ad8e336-goog\n\n"},{"id":"361240","messageId":"06bb0dd35081f950cd9196f1af0949565bce2f73.1540256910.git.matvore@google.com","threadId":"49642","inReplyTo":"cover.1540256910.git.matvore@google.com","subject":"[RFC 1/2] Documentation/git-log.txt: do not show --exclude-promisor-objects","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2018-10-23T01:13:41Z","receivedAt":"2018-10-23T01:13:58Z","isPatch":false,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"Do not suggest that --exclude-promisor-objects is supported by git-log,\nsince it currently BUG-crashes and it's not necessary to support it.\nOptions that control behavior for promisor objects should be limited to\na small number of commands.\n\nSigned-off-by: Matthew DeVore \n---\n Documentation/rev-list-options.txt | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 5f1672913b..bab5f50b17 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -761,7 +761,6 @@ Unexpected missing objects will raise an error.\n +\n The form '--missing=print' is like 'allow-any', but will also print a\n list of the missing objects. Object IDs are prefixed with a ``?'' character.\n-endif::git-rev-list[]\n \n --exclude-promisor-objects::\n \t(For internal use only.) Prefilter object traversal at\n@@ -769,6 +768,7 @@ endif::git-rev-list[]\n \tstronger than `--missing=allow-promisor` because it limits the\n \ttraversal, rather than just silencing errors about missing\n \tobjects.\n+endif::git-rev-list[]\n \n --no-walk[=(sorted|unsorted)]::\n \tOnly show the given commits, but do not traverse their ancestors.\n-- \n2.19.1.568.g152ad8e336-goog\n\n"},{"id":"361241","messageId":"931421945c040ba4518d91f7af9f386d0136bd2f.1540256910.git.matvore@google.com","threadId":"49642","inReplyTo":"cover.1540256910.git.matvore@google.com","subject":"[RFC 2/2] exclude-promisor-objects: declare when option is allowed","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2018-10-23T01:13:42Z","receivedAt":"2018-10-23T01:14:00Z","isPatch":false,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"The --exclude-promisor-objects option causes some funny behavior in at\nleast two commands: log and blame. It causes a BUG crash:\n\n\t$ git log --exclude-promisor-objects\n\tBUG: revision.c:2143: exclude_promisor_objects can only be used\n\twhen fetch_if_missing is 0\n\tAborted\n\t[134]\n\nFix this such that the option is treated like any other unknown option.\nThe commands that must support it are limited, so declare in those\ncommands that the flag is supported. In particular:\n\n\tpack-objects\n\tprune\n\trev-list\n\nThe commands were found by searching for logic which parses\n--exclude-promisor-objects outside of revision.c. Extra logic outside of\nrevision.c is needed because fetch_if_missing must be turned on before\nrevision.c sees the option or it will BUG-crash. The above list is\nsupported by the fact that no other command is introspectively invoked\nby another command passing --exclude-promisor-object.\n\nSigned-off-by: Matthew DeVore \n---\n builtin/pack-objects.c | 1 +\n builtin/prune.c | 1 +\n builtin/rev-list.c | 1 +\n revision.c | 3 ++-\n revision.h | 1 +\n t/t4202-log.sh | 4 ++++\n t/t8002-blame.sh | 4 ++++\n 7 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex b059b86aee..c409fa25d6 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3108,6 +3108,7 @@ static void get_object_list(int ac, const char **av)\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n \tsave_commit_buffer = 0;\n+\trevs.allow_exclude_promisor_objects_opt = 1;\n \tsetup_revisions(ac, av, &revs, NULL);\n \n \t/* make sure shallows are read */\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex 41230f8215..11284d0bf3 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -120,6 +120,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \tsave_commit_buffer = 0;\n \tread_replace_refs = 0;\n \tref_paranoia = 1;\n+\trevs.allow_exclude_promisor_objects_opt = 1;\n \trepo_init_revisions(the_repository, &revs, prefix);\n \n \targc = parse_options(argc, argv, prefix, options, prune_usage, 0);\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 5064d08e1b..2880ed37e3 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -374,6 +374,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \tgit_config(git_default_config, NULL);\n \trepo_init_revisions(the_repository, &revs, prefix);\n \trevs.abbrev = DEFAULT_ABBREV;\n+\trevs.allow_exclude_promisor_objects_opt = 1;\n \trevs.commit_format = CMIT_FMT_UNSPECIFIED;\n \trevs.do_not_die_on_missing_tree = 1;\n \ndiff --git a/revision.c b/revision.c\nindex a1ddb9e11c..28fb2a70cd 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2138,7 +2138,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->limited = 1;\n \t} else if (!strcmp(arg, \"--ignore-missing\")) {\n \t\trevs->ignore_missing = 1;\n-\t} else if (!strcmp(arg, \"--exclude-promisor-objects\")) {\n+\t} else if (revs->allow_exclude_promisor_objects_opt &&\n+\t\t !strcmp(arg, \"--exclude-promisor-objects\")) {\n \t\tif (fetch_if_missing)\n \t\t\tBUG(\"exclude_promisor_objects can only be used when fetch_if_missing is 0\");\n \t\trevs->exclude_promisor_objects = 1;\ndiff --git a/revision.h b/revision.h\nindex 1cd0c4b200..0d2abc2d36 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -156,6 +156,7 @@ struct rev_info {\n \t\t\tdo_not_die_on_missing_tree:1,\n \n \t\t\t/* for internal use only */\n+\t\t\tallow_exclude_promisor_objects_opt:1,\n \t\t\texclude_promisor_objects:1;\n \n \t/* Diff flags */\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 153a506151..819c24d10e 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -1703,4 +1703,8 @@ test_expect_success 'log --source paints symmetric ranges' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--exclude-promisor-objects does not BUG-crash' '\n+\ttest_must_fail git log --exclude-promisor-objects source-a\n+'\n+\n test_done\ndiff --git a/t/t8002-blame.sh b/t/t8002-blame.sh\nindex 380e1c1054..eea048e52c 100755\n--- a/t/t8002-blame.sh\n+++ b/t/t8002-blame.sh\n@@ -118,4 +118,8 @@ test_expect_success '--no-abbrev works like --abbrev=40' '\n \tcheck_abbrev 40 --no-abbrev\n '\n \n+test_expect_success '--exclude-promisor-objects does not BUG-crash' '\n+\ttest_must_fail git blame --exclude-promisor-objects one\n+'\n+\n test_done\n-- \n2.19.1.568.g152ad8e336-goog\n\n"},{"id":"361242","messageId":"alpine.DEB.2.21.1810221816480.38032@matvore-linux.svl.corp.google.com","threadId":"49642","inReplyTo":"cover.1540256910.git.matvore@google.com","subject":"Re: [RFC 0/2] explicitly support or not support --exclude-promisor-objects","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2018-10-23T01:18:16Z","receivedAt":"2018-10-23T01:18:47Z","isPatch":false,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"\n\nOn Mon, 22 Oct 2018, Matthew DeVore wrote:\n\n> This patch set fixes incorrect parsing of the --exclude-promisor-objects\n> option that I found while working on:\n>\n\nSomehow I sent two copies of every message in the patchset. I'm sorry\nfor the mess.\n"},{"id":"361258","messageId":"xmqqmur5s1y9.fsf@gitster-ct.c.googlers.com","threadId":"49642","inReplyTo":"cover.1540256910.git.matvore@google.com","subject":"Re: [RFC 0/2] explicitly support or not support --exclude-promisor-objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-23T04:48:30Z","receivedAt":"2018-10-23T04:48:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthew DeVore writes:\n\n> This patch set fixes incorrect parsing of the --exclude-promisor-objects\n> option that I found while working on:\n>\n> https://public-inbox.org/git/cover.1539298957.git.matvore@google.com/\n>\n\nThanks; both patches make sense. \n\nAs the problematic feature appeared in 2.17.x track, I'll see if I\ncan easily make it ready to be merged down to maint-2.17 track later\nwhen somebody wants to.\n\n> Matthew DeVore (2):\n> Documentation/git-log.txt: do not show --exclude-promisor-objects\n> exclude-promisor-objects: declare when option is allowed\n>\n> Documentation/rev-list-options.txt | 2 +-\n> builtin/pack-objects.c | 1 +\n> builtin/prune.c | 1 +\n> builtin/rev-list.c | 1 +\n> revision.c | 3 ++-\n> revision.h | 1 +\n> t/t4202-log.sh | 4 ++++\n> t/t8002-blame.sh | 4 ++++\n> 8 files changed, 15 insertions(+), 2 deletions(-)\n"},{"id":"361259","messageId":"xmqqin1ts11h.fsf@gitster-ct.c.googlers.com","threadId":"49642","inReplyTo":"931421945c040ba4518d91f7af9f386d0136bd2f.1540256910.git.matvore@google.com","subject":"Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-23T05:08:10Z","receivedAt":"2018-10-23T05:09:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthew DeVore writes:\n\n> t/t4202-log.sh | 4 ++++\n> t/t8002-blame.sh | 4 ++++\n> 7 files changed, 14 insertions(+), 1 deletion(-)\n> ...\n> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n> index 153a506151..819c24d10e 100755\n> --- a/t/t4202-log.sh\n> +++ b/t/t4202-log.sh\n> @@ -1703,4 +1703,8 @@ test_expect_success 'log --source paints symmetric ranges' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success '--exclude-promisor-objects does not BUG-crash' '\n> +\ttest_must_fail git log --exclude-promisor-objects source-a\n> +'\n> +\n> test_done\n> diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh\n> index 380e1c1054..eea048e52c 100755\n> --- a/t/t8002-blame.sh\n> +++ b/t/t8002-blame.sh\n> @@ -118,4 +118,8 @@ test_expect_success '--no-abbrev works like --abbrev=40' '\n> \tcheck_abbrev 40 --no-abbrev\n> '\n> \n> +test_expect_success '--exclude-promisor-objects does not BUG-crash' '\n> +\ttest_must_fail git blame --exclude-promisor-objects one\n> +'\n> +\n> test_done\n\nOK. We used to be hitting BUG() which is an abort() in disguise, so\nmust-fail would have caught it without the fix in this patch. Now\nwe would see a more controlled failure.\n\n ... goes and makes sure that is the case ...\n\nNot really. We were already doing a controlled failure via die(),\nso these two tests would not have caught the problem in the code\nbefore the fix in this patch.\n\nBut nevertheless this is a good change; I do not think it is worth\ngrepping for \"unrecognized option\" to differentiate the two cases.\n\nThanks.\n"},{"id":"361301","messageId":"alpine.DEB.2.21.1810231008050.28795@matvore-linux.svl.corp.google.com","threadId":"49642","inReplyTo":"xmqqmur5s1y9.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC 0/2] explicitly support or not support --exclude-promisor-objects","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2018-10-23T17:09:10Z","receivedAt":"2018-10-23T17:09:47Z","isPatch":false,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"\n\nOn Tue, 23 Oct 2018, Junio C Hamano wrote:\n\n> Thanks; both patches make sense.\n>\n> As the problematic feature appeared in 2.17.x track, I'll see if I\n> can easily make it ready to be merged down to maint-2.17 track later\n> when somebody wants to.\n>\n\nGreat. Thank you for the review.\n"},{"id":"361306","messageId":"alpine.DEB.2.21.1810231053550.56374@matvore-linux.svl.corp.google.com","threadId":"49642","inReplyTo":"xmqqin1ts11h.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2018-10-23T17:55:49Z","receivedAt":"2018-10-23T18:04:22Z","isPatch":false,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"\n\nOn Tue, 23 Oct 2018, Junio C Hamano wrote:\n\n> Not really. We were already doing a controlled failure via die(),\n> so these two tests would not have caught the problem in the code\n> before the fix in this patch.\n>\n\nBUG is apparently considered a \"wrong\" failure and not a controlled one\nby test_must_fail. I just double-checked that the tests fail without\nthis patch.\n\nnot ok 119 - --exclude-promisor-objects does not BUG-crash\n# \n#\t\ttest_must_fail git blame --exclude-promisor-objects one\n# \n# failed 1 among 119 test(s)\n1..119\n"},{"id":"361355","messageId":"xmqqva5sqgeb.fsf@gitster-ct.c.googlers.com","threadId":"49642","inReplyTo":"alpine.DEB.2.21.1810231053550.56374@matvore-linux.svl.corp.google.com","subject":"Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-24T01:31:40Z","receivedAt":"2018-10-24T03:32:58Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthew DeVore writes:\n\n> On Tue, 23 Oct 2018, Junio C Hamano wrote:\n>\n>> Not really. We were already doing a controlled failure via die(),\n>> so these two tests would not have caught the problem in the code\n>> before the fix in this patch.\n>>\n>\n> BUG is apparently considered a \"wrong\" failure and not a controlled one\n> by test_must_fail. I just double-checked that the tests fail without\n> this patch.\n\nAh, I was testing a wrong codepath. Yes, it does call BUG(\"...\"),\nwhich is a prettier-looking abort(), but I somehow thought it was\ndoing die(\"BUG: ...\").\n\nIn any case, thanks for the fix.\n"},{"id":"363841","messageId":"20181121164019.GA24621@sigill.intra.peff.net","threadId":"49642","inReplyTo":"931421945c040ba4518d91f7af9f386d0136bd2f.1540256910.git.matvore@google.com","subject":"Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-21T16:40:19Z","receivedAt":"2018-11-21T16:40:22Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 22, 2018 at 06:13:42PM -0700, Matthew DeVore wrote:\n\n> diff --git a/builtin/prune.c b/builtin/prune.c\n> index 41230f8215..11284d0bf3 100644\n> --- a/builtin/prune.c\n> +++ b/builtin/prune.c\n> @@ -120,6 +120,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n> \tsave_commit_buffer = 0;\n> \tread_replace_refs = 0;\n> \tref_paranoia = 1;\n> +\trevs.allow_exclude_promisor_objects_opt = 1;\n> \trepo_init_revisions(the_repository, &revs, prefix);\n> \n> \targc = parse_options(argc, argv, prefix, options, prune_usage, 0);\n\nI think this line is in the wrong place. The very first thing\nrepo_init_revisions() will do is memset() the revs struct to all-zeroes,\nso it cannot possibly be doing anything.\n\nNormally it would need to go after init_revisions() but before\nsetup_revisions(), but we don't seem to call the latter at all in\nbuiltin/prune.c. Which makes sense, because you cannot pass options to\ninfluence the reachability traversal. So I don't think we need to care\nabout this flag at all here.\n\nSpeaking of which, would this flag work better as a field in\nsetup_revision_opt, which is passed to setup_revisions()? The intent\nseem to be to influence how we parse command-line arguments, and that's\nwhere other similar flags are (e.g., assume_dashdash).\n\n-Peff\n"},{"id":"364408","messageId":"19c82fb0-e0d6-0b15-06ab-cfba4d699d94@comcast.net","threadId":"49642","inReplyTo":"20181121164019.GA24621@sigill.intra.peff.net","subject":"Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2018-12-01T01:32:47Z","receivedAt":"2018-12-01T01:41:21Z","isPatch":false,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"\n\nOn 11/21/2018 08:40 AM, Jeff King wrote:\n> On Mon, Oct 22, 2018 at 06:13:42PM -0700, Matthew DeVore wrote:\n> \n>> diff --git a/builtin/prune.c b/builtin/prune.c\n>> index 41230f8215..11284d0bf3 100644\n>> --- a/builtin/prune.c\n>> +++ b/builtin/prune.c\n>> @@ -120,6 +120,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n>> \tsave_commit_buffer = 0;\n>> \tread_replace_refs = 0;\n>> \tref_paranoia = 1;\n>> +\trevs.allow_exclude_promisor_objects_opt = 1;\n>> \trepo_init_revisions(the_repository, &revs, prefix);\n>> \n>> \targc = parse_options(argc, argv, prefix, options, prune_usage, 0);\n> \n> I think this line is in the wrong place. The very first thing\n> repo_init_revisions() will do is memset() the revs struct to all-zeroes,\n> so it cannot possibly be doing anything.\n\nAh of course :)\n\n> \n> Normally it would need to go after init_revisions() but before\n> setup_revisions(), but we don't seem to call the latter at all in\n> builtin/prune.c. Which makes sense, because you cannot pass options to\n> influence the reachability traversal. So I don't think we need to care\n> about this flag at all here.\nAgreed, prune.c doesn't use setup_revisions() even transitively, so we \ndon't care about this flag.\n\n> \n> Speaking of which, would this flag work better as a field in\n> setup_revision_opt, which is passed to setup_revisions()? The intent\n> seem to be to influence how we parse command-line arguments, and that's\n> where other similar flags are (e.g., assume_dashdash).\n\nGood idea. This would solve the problem of mistakenly believing the flag \nmatters when it doesn't, since it is in the struct which is used as the \narguments of the exact function that cares about it. Here's a new patch \n- I'm tweaking e-mail client settings so hopefully this makes it to the \nlist without mangling - if not I'll resend it with `git-send-email` later.\n\n From 941c89fe1e226ed4d210ce35d0d906526b8277ed Mon Sep 17 00:00:00 2001\nFrom: Matthew DeVore \nDate: Fri, 30 Nov 2018 16:43:32 -0800\nSubject: [PATCH] revisions.c: put promisor option in specialized struct\n\nPut the allow_exclude_promisor_objects flag in setup_revision_opt. When\nit was in rev_info, it was unclear when it was used, since rev_info is\npassed to functions that don't use the flag. This resulted in\nunnecessary setting of the flag in prune.c, so fix that as well.\n\nSigned-off-by: Matthew DeVore \n---\n builtin/pack-objects.c | 7 +++++--\n builtin/prune.c | 1 -\n builtin/rev-list.c | 6 ++++--\n revision.c | 17 ++++++++++++-----\n revision.h | 4 ++--\n 5 files changed, 23 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 24bba8147f..b22c99f540 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3084,14 +3084,17 @@ static void record_recent_commit(struct commit \n*commit, void *data)\n static void get_object_list(int ac, const char **av)\n {\n \tstruct rev_info revs;\n+\tstruct setup_revision_opt s_r_opt;\n \tchar line[1000];\n \tint flags = 0;\n \tint save_warning;\n\n \trepo_init_revisions(the_repository, &revs, NULL);\n \tsave_commit_buffer = 0;\n-\trevs.allow_exclude_promisor_objects_opt = 1;\n-\tsetup_revisions(ac, av, &revs, NULL);\n+\n+\tmemset(&s_r_opt, 0, sizeof(s_r_opt));\n+\ts_r_opt.allow_exclude_promisor_objects = 1;\n+\tsetup_revisions(ac, av, &revs, &s_r_opt);\n\n \t/* make sure shallows are read */\n \tis_repository_shallow(the_repository);\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex e42653b99c..1ec9ddd751 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -120,7 +120,6 @@ int cmd_prune(int argc, const char **argv, const \nchar *prefix)\n \tsave_commit_buffer = 0;\n \tread_replace_refs = 0;\n \tref_paranoia = 1;\n-\trevs.allow_exclude_promisor_objects_opt = 1;\n \trepo_init_revisions(the_repository, &revs, prefix);\n\n \targc = parse_options(argc, argv, prefix, options, prune_usage, 0);\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 3a2c0c23b6..c3095c6fed 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -362,6 +362,7 @@ int cmd_rev_list(int argc, const char **argv, const \nchar *prefix)\n {\n \tstruct rev_info revs;\n \tstruct rev_list_info info;\n+\tstruct setup_revision_opt s_r_opt;\n \tint i;\n \tint bisect_list = 0;\n \tint bisect_show_vars = 0;\n@@ -375,7 +376,6 @@ int cmd_rev_list(int argc, const char **argv, const \nchar *prefix)\n \tgit_config(git_default_config, NULL);\n \trepo_init_revisions(the_repository, &revs, prefix);\n \trevs.abbrev = DEFAULT_ABBREV;\n-\trevs.allow_exclude_promisor_objects_opt = 1;\n \trevs.commit_format = CMIT_FMT_UNSPECIFIED;\n \trevs.do_not_die_on_missing_tree = 1;\n\n@@ -407,7 +407,9 @@ int cmd_rev_list(int argc, const char **argv, const \nchar *prefix)\n \t\t}\n \t}\n\n-\targc = setup_revisions(argc, argv, &revs, NULL);\n+\tmemset(&s_r_opt, 0, sizeof(s_r_opt));\n+\ts_r_opt.allow_exclude_promisor_objects = 1;\n+\targc = setup_revisions(argc, argv, &revs, &s_r_opt);\n\n \tmemset(&info, 0, sizeof(info));\n \tinfo.revs = &revs;\ndiff --git a/revision.c b/revision.c\nindex 13e0519c02..221ba79594 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1791,7 +1791,8 @@ static void add_message_grep(struct rev_info \n*revs, const char *pattern)\n }\n\n static int handle_revision_opt(struct rev_info *revs, int argc, const \nchar **argv,\n-\t\t\t int *unkc, const char **unkv)\n+\t\t\t int *unkc, const char **unkv,\n+\t\t\t int allow_exclude_promisor_objects)\n {\n \tconst char *arg = argv[0];\n \tconst char *optarg;\n@@ -2151,7 +2152,7 @@ static int handle_revision_opt(struct rev_info \n*revs, int argc, const char **arg\n \t\trevs->limited = 1;\n \t} else if (!strcmp(arg, \"--ignore-missing\")) {\n \t\trevs->ignore_missing = 1;\n-\t} else if (revs->allow_exclude_promisor_objects_opt &&\n+\t} else if (allow_exclude_promisor_objects &&\n \t\t !strcmp(arg, \"--exclude-promisor-objects\")) {\n \t\tif (fetch_if_missing)\n \t\t\tBUG(\"exclude_promisor_objects can only be used when \nfetch_if_missing is 0\");\n@@ -2173,7 +2174,8 @@ void parse_revision_opt(struct rev_info *revs, \nstruct parse_opt_ctx_t *ctx,\n \t\t\tconst char * const usagestr[])\n {\n \tint n = handle_revision_opt(revs, ctx->argc, ctx->argv,\n-\t\t\t\t &ctx->cpidx, ctx->out);\n+\t\t\t\t &ctx->cpidx, ctx->out,\n+\t\t\t\t /*allow_exclude_promisor_objects=*/0);\n \tif (n <= 0) {\n \t\terror(\"unknown option `%s'\", ctx->argv[0]);\n \t\tusage_with_options(usagestr, options);\n@@ -2340,9 +2342,12 @@ int setup_revisions(int argc, const char **argv, \nstruct rev_info *revs, struct s\n \tint i, flags, left, seen_dashdash, got_rev_arg = 0, revarg_opt;\n \tstruct argv_array prune_data = ARGV_ARRAY_INIT;\n \tconst char *submodule = NULL;\n+\tint allow_exclude_prom_objs = 0;\n\n-\tif (opt)\n+\tif (opt) {\n \t\tsubmodule = opt->submodule;\n+\t\tallow_exclude_prom_objs = opt->allow_exclude_promisor_objects;\n+\t}\n\n \t/* First, search for \"--\" */\n \tif (opt && opt->assume_dashdash) {\n@@ -2391,7 +2396,9 @@ int setup_revisions(int argc, const char **argv, \nstruct rev_info *revs, struct s\n \t\t\t\tcontinue;\n \t\t\t}\n\n-\t\t\topts = handle_revision_opt(revs, argc - i, argv + i, &left, argv);\n+\t\t\topts = handle_revision_opt(revs, argc - i, argv + i,\n+\t\t\t\t\t\t &left, argv,\n+\t\t\t\t\t\t allow_exclude_prom_objs);\n \t\t\tif (opts > 0) {\n \t\t\t\ti += opts - 1;\n \t\t\t\tcontinue;\ndiff --git a/revision.h b/revision.h\nindex 7987bfcd2e..7d6e050569 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -161,7 +161,6 @@ struct rev_info {\n \t\t\tdo_not_die_on_missing_tree:1,\n\n \t\t\t/* for internal use only */\n-\t\t\tallow_exclude_promisor_objects_opt:1,\n \t\t\texclude_promisor_objects:1;\n\n \t/* Diff flags */\n@@ -297,7 +296,8 @@ struct setup_revision_opt {\n \tconst char *def;\n \tvoid (*tweak)(struct rev_info *, struct setup_revision_opt *);\n \tconst char *submodule;\t/* TODO: drop this and use rev_info->repo */\n-\tint assume_dashdash;\n+\tint assume_dashdash : 1;\n+\tint allow_exclude_promisor_objects : 1;\n \tunsigned revarg_opt;\n };\n\n-- \n2.20.0.rc1.387.gf8505762e3-goog\n"},{"id":"364420","messageId":"20181201194424.GB28918@sigill.intra.peff.net","threadId":"49642","inReplyTo":"19c82fb0-e0d6-0b15-06ab-cfba4d699d94@comcast.net","subject":"Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-01T19:44:24Z","receivedAt":"2018-12-01T19:44:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 30, 2018 at 05:32:47PM -0800, Matthew DeVore wrote:\n\n> > Speaking of which, would this flag work better as a field in\n> > setup_revision_opt, which is passed to setup_revisions()? The intent\n> > seem to be to influence how we parse command-line arguments, and that's\n> > where other similar flags are (e.g., assume_dashdash).\n> \n> Good idea. This would solve the problem of mistakenly believing the flag\n> matters when it doesn't, since it is in the struct which is used as the\n> arguments of the exact function that cares about it. Here's a new patch -\n> I'm tweaking e-mail client settings so hopefully this makes it to the list\n> without mangling - if not I'll resend it with `git-send-email` later.\n> \n> From 941c89fe1e226ed4d210ce35d0d906526b8277ed Mon Sep 17 00:00:00 2001\n> From: Matthew DeVore \n> Date: Fri, 30 Nov 2018 16:43:32 -0800\n> Subject: [PATCH] revisions.c: put promisor option in specialized struct\n> \n> Put the allow_exclude_promisor_objects flag in setup_revision_opt. When\n> it was in rev_info, it was unclear when it was used, since rev_info is\n> passed to functions that don't use the flag. This resulted in\n> unnecessary setting of the flag in prune.c, so fix that as well.\n\nThanks, this looks pretty reasonable overall. Two comments:\n\n> \trepo_init_revisions(the_repository, &revs, NULL);\n> \tsave_commit_buffer = 0;\n> -\trevs.allow_exclude_promisor_objects_opt = 1;\n> -\tsetup_revisions(ac, av, &revs, NULL);\n> +\n> +\tmemset(&s_r_opt, 0, sizeof(s_r_opt));\n> +\ts_r_opt.allow_exclude_promisor_objects = 1;\n> +\tsetup_revisions(ac, av, &revs, &s_r_opt);\n\nI wonder if a static initializer for setup_revision_opt is worth it. It\nwould remove the need for this memset. Probably not a big deal either\nway, though.\n\n> static int handle_revision_opt(struct rev_info *revs, int argc, const char\n> **argv,\n> -\t\t\t int *unkc, const char **unkv)\n> +\t\t\t int *unkc, const char **unkv,\n> +\t\t\t int allow_exclude_promisor_objects)\n\nWhy not pass in the whole setup_revision_opt struct? We don't need\nanything else from it yet, but it seems like the point of that struct is\nto pass around preferences like this.\n\n-Peff\n"},{"id":"364472","messageId":"80a08b99-14cb-e398-e6c2-2aa94a5fdda3@comcast.net","threadId":"49642","inReplyTo":"20181201194424.GB28918@sigill.intra.peff.net","subject":"Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2018-12-03T19:10:49Z","receivedAt":"2018-12-03T19:11:15Z","isPatch":false,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"On 12/01/2018 11:44 AM, Jeff King wrote:\n>> \trepo_init_revisions(the_repository, &revs, NULL);\n>> \tsave_commit_buffer = 0;\n>> -\trevs.allow_exclude_promisor_objects_opt = 1;\n>> -\tsetup_revisions(ac, av, &revs, NULL);\n>> +\n>> +\tmemset(&s_r_opt, 0, sizeof(s_r_opt));\n>> +\ts_r_opt.allow_exclude_promisor_objects = 1;\n>> +\tsetup_revisions(ac, av, &revs, &s_r_opt);\n> \n> I wonder if a static initializer for setup_revision_opt is worth it. It\n> would remove the need for this memset. Probably not a big deal either\n> way, though.\nI think you mean something like this:\n\nstatic struct setup_revision_opt s_r_opt = {NULL, NULL, NULL, 0, 1, 0};\n\nThis is a bit cryptic (I have to read the struct declaration in order to \nknow what is being set to 1) and if the struct ever gets a new field \nbefore allow_exclude_promisor_objects, this initializer has to be updated.\n\n> \n>> static int handle_revision_opt(struct rev_info *revs, int argc, const char\n>> **argv,\n>> -\t\t\t int *unkc, const char **unkv)\n>> +\t\t\t int *unkc, const char **unkv,\n>> +\t\t\t int allow_exclude_promisor_objects)\n> \n> Why not pass in the whole setup_revision_opt struct? We don't need\n> anything else from it yet, but it seems like the point of that struct is\n> to pass around preferences like this.\nOK, the code reads better if I do that, so I agree.\n\n> \n> -Peff\n> \n"},{"id":"364473","messageId":"20181203192356.51432-1-matvore@google.com","threadId":"49642","inReplyTo":"20181201194424.GB28918@sigill.intra.peff.net","subject":"[PATCH] revisions.c: put promisor option in specialized struct","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2018-12-03T19:23:56Z","receivedAt":"2018-12-03T19:24:34Z","isPatch":true,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"Put the allow_exclude_promisor_objects flag in setup_revision_opt. When\nit was in rev_info, it was unclear when it was used, since rev_info is\npassed to functions that don't use the flag. This resulted in\nunnecessary setting of the flag in prune.c, so fix that as well.\n\nSigned-off-by: Matthew DeVore \n---\n builtin/pack-objects.c | 7 +++++--\n builtin/prune.c | 1 -\n builtin/rev-list.c | 6 ++++--\n revision.c | 10 ++++++----\n revision.h | 4 ++--\n 5 files changed, 17 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 24bba8147f..b22c99f540 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3084,14 +3084,17 @@ static void record_recent_commit(struct commit *commit, void *data)\n static void get_object_list(int ac, const char **av)\n {\n \tstruct rev_info revs;\n+\tstruct setup_revision_opt s_r_opt;\n \tchar line[1000];\n \tint flags = 0;\n \tint save_warning;\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n \tsave_commit_buffer = 0;\n-\trevs.allow_exclude_promisor_objects_opt = 1;\n-\tsetup_revisions(ac, av, &revs, NULL);\n+\n+\tmemset(&s_r_opt, 0, sizeof(s_r_opt));\n+\ts_r_opt.allow_exclude_promisor_objects = 1;\n+\tsetup_revisions(ac, av, &revs, &s_r_opt);\n \n \t/* make sure shallows are read */\n \tis_repository_shallow(the_repository);\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex e42653b99c..1ec9ddd751 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -120,7 +120,6 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \tsave_commit_buffer = 0;\n \tread_replace_refs = 0;\n \tref_paranoia = 1;\n-\trevs.allow_exclude_promisor_objects_opt = 1;\n \trepo_init_revisions(the_repository, &revs, prefix);\n \n \targc = parse_options(argc, argv, prefix, options, prune_usage, 0);\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 3a2c0c23b6..c3095c6fed 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -362,6 +362,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n {\n \tstruct rev_info revs;\n \tstruct rev_list_info info;\n+\tstruct setup_revision_opt s_r_opt;\n \tint i;\n \tint bisect_list = 0;\n \tint bisect_show_vars = 0;\n@@ -375,7 +376,6 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \tgit_config(git_default_config, NULL);\n \trepo_init_revisions(the_repository, &revs, prefix);\n \trevs.abbrev = DEFAULT_ABBREV;\n-\trevs.allow_exclude_promisor_objects_opt = 1;\n \trevs.commit_format = CMIT_FMT_UNSPECIFIED;\n \trevs.do_not_die_on_missing_tree = 1;\n \n@@ -407,7 +407,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\targc = setup_revisions(argc, argv, &revs, NULL);\n+\tmemset(&s_r_opt, 0, sizeof(s_r_opt));\n+\ts_r_opt.allow_exclude_promisor_objects = 1;\n+\targc = setup_revisions(argc, argv, &revs, &s_r_opt);\n \n \tmemset(&info, 0, sizeof(info));\n \tinfo.revs = &revs;\ndiff --git a/revision.c b/revision.c\nindex 13e0519c02..f6b32e6a42 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1791,7 +1791,8 @@ static void add_message_grep(struct rev_info *revs, const char *pattern)\n }\n \n static int handle_revision_opt(struct rev_info *revs, int argc, const char **argv,\n-\t\t\t int *unkc, const char **unkv)\n+\t\t\t int *unkc, const char **unkv,\n+\t\t\t const struct setup_revision_opt* opt)\n {\n \tconst char *arg = argv[0];\n \tconst char *optarg;\n@@ -2151,7 +2152,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->limited = 1;\n \t} else if (!strcmp(arg, \"--ignore-missing\")) {\n \t\trevs->ignore_missing = 1;\n-\t} else if (revs->allow_exclude_promisor_objects_opt &&\n+\t} else if (opt && opt->allow_exclude_promisor_objects &&\n \t\t !strcmp(arg, \"--exclude-promisor-objects\")) {\n \t\tif (fetch_if_missing)\n \t\t\tBUG(\"exclude_promisor_objects can only be used when fetch_if_missing is 0\");\n@@ -2173,7 +2174,7 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,\n \t\t\tconst char * const usagestr[])\n {\n \tint n = handle_revision_opt(revs, ctx->argc, ctx->argv,\n-\t\t\t\t &ctx->cpidx, ctx->out);\n+\t\t\t\t &ctx->cpidx, ctx->out, NULL);\n \tif (n <= 0) {\n \t\terror(\"unknown option `%s'\", ctx->argv[0]);\n \t\tusage_with_options(usagestr, options);\n@@ -2391,7 +2392,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\t\tcontinue;\n \t\t\t}\n \n-\t\t\topts = handle_revision_opt(revs, argc - i, argv + i, &left, argv);\n+\t\t\topts = handle_revision_opt(revs, argc - i, argv + i,\n+\t\t\t\t\t\t &left, argv, opt);\n \t\t\tif (opts > 0) {\n \t\t\t\ti += opts - 1;\n \t\t\t\tcontinue;\ndiff --git a/revision.h b/revision.h\nindex 7987bfcd2e..7d6e050569 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -161,7 +161,6 @@ struct rev_info {\n \t\t\tdo_not_die_on_missing_tree:1,\n \n \t\t\t/* for internal use only */\n-\t\t\tallow_exclude_promisor_objects_opt:1,\n \t\t\texclude_promisor_objects:1;\n \n \t/* Diff flags */\n@@ -297,7 +296,8 @@ struct setup_revision_opt {\n \tconst char *def;\n \tvoid (*tweak)(struct rev_info *, struct setup_revision_opt *);\n \tconst char *submodule;\t/* TODO: drop this and use rev_info->repo */\n-\tint assume_dashdash;\n+\tint assume_dashdash : 1;\n+\tint allow_exclude_promisor_objects : 1;\n \tunsigned revarg_opt;\n };\n \n-- \n2.20.0.rc1.387.gf8505762e3-goog\n\n"},{"id":"364484","messageId":"20181203211555.GA8700@sigill.intra.peff.net","threadId":"49642","inReplyTo":"80a08b99-14cb-e398-e6c2-2aa94a5fdda3@comcast.net","subject":"Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-03T21:15:55Z","receivedAt":"2018-12-03T21:15:59Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 03, 2018 at 11:10:49AM -0800, Matthew DeVore wrote:\n\n> > > +\tmemset(&s_r_opt, 0, sizeof(s_r_opt));\n> > > +\ts_r_opt.allow_exclude_promisor_objects = 1;\n> > > +\tsetup_revisions(ac, av, &revs, &s_r_opt);\n> > \n> > I wonder if a static initializer for setup_revision_opt is worth it. It\n> > would remove the need for this memset. Probably not a big deal either\n> > way, though.\n> I think you mean something like this:\n> \n> static struct setup_revision_opt s_r_opt = {NULL, NULL, NULL, 0, 1, 0};\n> \n> This is a bit cryptic (I have to read the struct declaration in order to\n> know what is being set to 1) and if the struct ever gets a new field before\n> allow_exclude_promisor_objects, this initializer has to be updated.\n\nI agree that's pretty awful. I meant something like this:\n\n struct setup_revision_opt s_r_opt = { NULL };\n ...\n\n s_r_opt.allow_exclude_promisor_objects = 1;\n setup_revisions(...);\n\nIt's functionally equivalent to the memset(), but you don't have to\nwonder about whether we peek at the uninitialized state in between.\n\nThat said, our C99 designated initializer weather-balloons haven't\ngotten any complaints yet. So I think you could actually do:\n\n struct setup_revision_opt s_r_opt = {\n\t.allow_exclude_promisor_objects = 1,\n };\n ...\n setup_revisions(...);\n\nwhich is pretty nice.\n\n-Peff\n"},{"id":"364486","messageId":"20181203212431.GB8700@sigill.intra.peff.net","threadId":"49642","inReplyTo":"20181203192356.51432-1-matvore@google.com","subject":"Re: [PATCH] revisions.c: put promisor option in specialized struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-03T21:24:31Z","receivedAt":"2018-12-03T21:24:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 03, 2018 at 11:23:56AM -0800, Matthew DeVore wrote:\n\n> Put the allow_exclude_promisor_objects flag in setup_revision_opt. When\n> it was in rev_info, it was unclear when it was used, since rev_info is\n> passed to functions that don't use the flag. This resulted in\n> unnecessary setting of the flag in prune.c, so fix that as well.\n> \n> Signed-off-by: Matthew DeVore \n> ---\n> builtin/pack-objects.c | 7 +++++--\n> builtin/prune.c | 1 -\n> builtin/rev-list.c | 6 ++++--\n> revision.c | 10 ++++++----\n> revision.h | 4 ++--\n> 5 files changed, 17 insertions(+), 11 deletions(-)\n\nThanks, this mostly looks good to me (with or without tweaking the\ninitializers as discussed in the other part of the thread).\n\nOne thing I noticed:\n\n> @@ -297,7 +296,8 @@ struct setup_revision_opt {\n> \tconst char *def;\n> \tvoid (*tweak)(struct rev_info *, struct setup_revision_opt *);\n> \tconst char *submodule;\t/* TODO: drop this and use rev_info->repo */\n> -\tint assume_dashdash;\n> +\tint assume_dashdash : 1;\n> +\tint allow_exclude_promisor_objects : 1;\n> \tunsigned revarg_opt;\n> };\n\nI don't know that we need to penny-pinch bytes in this struct, but in\ngeneral it shouldn't hurt either awy. However, a signed bit-field with 1\nbit is funny. I'm not even sure what the standard has to say, but in\ntwos-complement that would store \"-1\" and \"0\" (gcc -Wpedantic also\ncomplains about overflow in assigning \"1\" to it).\n\nSo this probably ought to be \"unsigned\".\n\n-Peff\n"},{"id":"364491","messageId":"4c32ed9f-0882-c454-f136-bf98840a44da@comcast.net","threadId":"49642","inReplyTo":"20181203211555.GA8700@sigill.intra.peff.net","subject":"Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2018-12-03T21:54:13Z","receivedAt":"2018-12-03T21:54:40Z","isPatch":false,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"On 12/03/2018 01:15 PM, Jeff King wrote:\n> That said, our C99 designated initializer weather-balloons haven't\n> gotten any complaints yet. So I think you could actually do:\n> \n> struct setup_revision_opt s_r_opt = {\n> \t.allow_exclude_promisor_objects = 1,\n> };\n\nI like this way best, so I'll use it. Thank you.\n"},{"id":"364493","messageId":"0809e859-b3a6-79ef-031c-9ff3326da31d@comcast.net","threadId":"49642","inReplyTo":"20181203212431.GB8700@sigill.intra.peff.net","subject":"Re: [PATCH] revisions.c: put promisor option in specialized struct","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2018-12-03T22:01:12Z","receivedAt":"2018-12-03T22:09:45Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"On 12/03/2018 01:24 PM, Jeff King wrote:\n>> @@ -297,7 +296,8 @@ struct setup_revision_opt {\n>> \tconst char *def;\n>> \tvoid (*tweak)(struct rev_info *, struct setup_revision_opt *);\n>> \tconst char *submodule;\t/* TODO: drop this and use rev_info->repo */\n>> -\tint assume_dashdash;\n>> +\tint assume_dashdash : 1;\n>> +\tint allow_exclude_promisor_objects : 1;\n>> \tunsigned revarg_opt;\n>> };\n> \n> I don't know that we need to penny-pinch bytes in this struct, but in\n> general it shouldn't hurt either awy. However, a signed bit-field with 1\n> bit is funny. I'm not even sure what the standard has to say, but in\n> twos-complement that would store \"-1\" and \"0\" (gcc -Wpedantic also\n> complains about overflow in assigning \"1\" to it).\nInteresting. I hadn't suspected this. But I confirmed it with this:\n\n#include \n\nstruct x {\n int y : 1;\n int z : 1;\n};\n\nint main() {\n struct x x;\n x.y = 1;\n x.z = 1;\n printf(\"%d %d\\n\", (int) x.y, (int) x.z);\n return 0;\n}\n\n-- Output --\n-1 -1\n\n> \n> So this probably ought to be \"unsigned\".\n\n\nEarlier in this file we define bit fields this way:\n\t/* Traversal flags */\n\tunsigned int\tdense:1,\n\t\t\tprune:1,\n\n... using \\t to align the field names, so I'll mimic that style.\n"},{"id":"364518","messageId":"xmqq7egqghi7.fsf@gitster-ct.c.googlers.com","threadId":"49642","inReplyTo":"20181203211555.GA8700@sigill.intra.peff.net","subject":"Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-04T02:20:16Z","receivedAt":"2018-12-04T02:20:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> That said, our C99 designated initializer weather-balloons haven't\n> gotten any complaints yet. So I think you could actually do:\n>\n> struct setup_revision_opt s_r_opt = {\n> \t.allow_exclude_promisor_objects = 1,\n> };\n> ...\n> setup_revisions(...);\n>\n> which is pretty nice.\n\nYup. The output from \n\n $ git grep -n ' \\.[a-z0-9_]* =' -- \\*.[ch]\n\nwith a bit of \"git blame\" tells us that cbc0f81d (\"strbuf: use\ndesignated initializers in STRBUF_INIT\", 2017-07-10) is the balloon\nfor this exact feature. The same for array was done in 512f41cf\n(\"clean.c: use designated initializer\", 2017-07-14)\n\n[I am writing it down so that I do not have to dig for it every time\nand instead can ask the list archive]\n\n\n"}]}