explicitly support or not support --exclude-promisor-objects

RFC In review Matthew DeVore 4 participants 19 messages Oct 23, 2018 – Dec 4, 2018 Documentation

lore

Share & tools

lore

Matthew DeVore

author

2018-10-23 01:13 UTC #0 lore raw

cover explicitly support or not support --exclude-promisor-objects

This patch set fixes incorrect parsing of the --exclude-promisor-objects option that I found while working on:

https://public-inbox.org/git/[email protected]/

Thank you,

Matthew DeVore (2):
Documentation/git-log.txt: do not show --exclude-promisor-objects
exclude-promisor-objects: declare when option is allowed

 Documentation/rev-list-options.txt | 2 +-
 builtin/pack-objects.c             | 1 +
 builtin/prune.c                    | 1 +
 builtin/rev-list.c                 | 1 +
 revision.c                         | 3 ++-
 revision.h                         | 1 +
 t/t4202-log.sh                     | 4 ++++
 t/t8002-blame.sh                   | 4 ++++
 8 files changed, 15 insertions(+), 2 deletions(-)

Signature · 1 line

2.19.1.568.g152ad8e336-goog

Matthew DeVore

author

2018-10-23 01:13 UTC #1 lore raw

RFC 1/2 Documentation/git-log.txt: do not show --exclude-promisor-objects

Do not suggest that --exclude-promisor-objects is supported by git-log, since it currently BUG-crashes and it's not necessary to support it. Options that control behavior for promisor objects should be limited to a small number of commands.

Signed-off-by Matthew DeVore <[email protected]>

---
 Documentation/rev-list-options.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Documentation/rev-list-options.txt +1 -1

index 5f1672913b..bab5f50b17 100644
@@ -761,7 +761,6 @@ Unexpected missing objects will raise an error.
 +
 The form '--missing=print' is like 'allow-any', but will also print a
 list of the missing objects.  Object IDs are prefixed with a ``?'' character.
-endif::git-rev-list[]
 
 --exclude-promisor-objects::
 	(For internal use only.)  Prefilter object traversal at

@@ -769,6 +768,7 @@ endif::git-rev-list[]
 	stronger than `--missing=allow-promisor` because it limits the
 	traversal, rather than just silencing errors about missing
 	objects.
+endif::git-rev-list[]
 
 --no-walk[=(sorted|unsorted)]::
 	Only show the given commits, but do not traverse their ancestors.

Signature · 1 line

2.19.1.568.g152ad8e336-goog

Matthew DeVore

author

2018-10-23 01:14 UTC #2 lore raw

RFC 2/2 exclude-promisor-objects: declare when option is allowed

In reply to Matthew DeVore

The --exclude-promisor-objects option causes some funny behavior in at least two commands: log and blame. It causes a BUG crash:

	$ git log --exclude-promisor-objects
	BUG: revision.c:2143: exclude_promisor_objects can only be used
	when fetch_if_missing is 0
	Aborted
	[134]

Fix this such that the option is treated like any other unknown option. The commands that must support it are limited, so declare in those commands that the flag is supported. In particular:

pack-objects
prune
rev-list

The commands were found by searching for logic which parses --exclude-promisor-objects outside of revision.c. Extra logic outside of revision.c is needed because fetch_if_missing must be turned on before revision.c sees the option or it will BUG-crash. The above list is supported by the fact that no other command is introspectively invoked by another command passing --exclude-promisor-object.

Signed-off-by Matthew DeVore <[email protected]>

---
 builtin/pack-objects.c | 1 +
 builtin/prune.c        | 1 +
 builtin/rev-list.c     | 1 +
 revision.c             | 3 ++-
 revision.h             | 1 +
 t/t4202-log.sh         | 4 ++++
 t/t8002-blame.sh       | 4 ++++
 7 files changed, 14 insertions(+), 1 deletion(-)

builtin/pack-objects.c +1 -0

index b059b86aee..c409fa25d6 100644
@@ -3108,6 +3108,7 @@ static void get_object_list(int ac, const char **av)
 
 	repo_init_revisions(the_repository, &revs, NULL);
 	save_commit_buffer = 0;
+	revs.allow_exclude_promisor_objects_opt = 1;
 	setup_revisions(ac, av, &revs, NULL);
 
 	/* make sure shallows are read */

builtin/prune.c +1 -0

index 41230f8215..11284d0bf3 100644
@@ -120,6 +120,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
 	save_commit_buffer = 0;
 	read_replace_refs = 0;
 	ref_paranoia = 1;
+	revs.allow_exclude_promisor_objects_opt = 1;
 	repo_init_revisions(the_repository, &revs, prefix);
 
 	argc = parse_options(argc, argv, prefix, options, prune_usage, 0);

builtin/rev-list.c +1 -0

index 5064d08e1b..2880ed37e3 100644
@@ -374,6 +374,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 	repo_init_revisions(the_repository, &revs, prefix);
 	revs.abbrev = DEFAULT_ABBREV;
+	revs.allow_exclude_promisor_objects_opt = 1;
 	revs.commit_format = CMIT_FMT_UNSPECIFIED;
 	revs.do_not_die_on_missing_tree = 1;

revision.c +2 -1

index a1ddb9e11c..28fb2a70cd 100644
@@ -2138,7 +2138,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->limited = 1;
 	} else if (!strcmp(arg, "--ignore-missing")) {
 		revs->ignore_missing = 1;
-	} else if (!strcmp(arg, "--exclude-promisor-objects")) {
+	} else if (revs->allow_exclude_promisor_objects_opt &&
+		   !strcmp(arg, "--exclude-promisor-objects")) {
 		if (fetch_if_missing)
 			BUG("exclude_promisor_objects can only be used when fetch_if_missing is 0");
 		revs->exclude_promisor_objects = 1;

revision.h +1 -0

index 1cd0c4b200..0d2abc2d36 100644
@@ -156,6 +156,7 @@ struct rev_info {
 			do_not_die_on_missing_tree:1,
 
 			/* for internal use only */
+			allow_exclude_promisor_objects_opt:1,
 			exclude_promisor_objects:1;
 
 	/* Diff flags */

t/t4202-log.sh +4 -0

index 153a506151..819c24d10e 100755
@@ -1703,4 +1703,8 @@ test_expect_success 'log --source paints symmetric ranges' '
 	test_cmp expect actual
 '
 
+test_expect_success '--exclude-promisor-objects does not BUG-crash' '
+	test_must_fail git log --exclude-promisor-objects source-a
+'
+
 test_done

t/t8002-blame.sh +4 -0

index 380e1c1054..eea048e52c 100755
@@ -118,4 +118,8 @@ test_expect_success '--no-abbrev works like --abbrev=40' '
 	check_abbrev 40 --no-abbrev
 '
 
+test_expect_success '--exclude-promisor-objects does not BUG-crash' '
+	test_must_fail git blame --exclude-promisor-objects one
+'
+
 test_done

Signature · 1 line

2.19.1.568.g152ad8e336-goog

Matthew DeVore

2018-10-23 01:18 UTC #16 lore raw

In reply to Matthew DeVore

Quoted 3 lines · Matthew DeVore

This patch set fixes incorrect parsing of the --exclude-promisor-objects option that I found while working on:

Somehow I sent two copies of every message in the patchset. I'm sorry for the mess.

Junio C Hamano

2018-10-23 04:48 UTC #17 lore raw

In reply to Matthew DeVore

Quoted 5 lines · Matthew DeVore

This patch set fixes incorrect parsing of the --exclude-promisor-objects option that I found while working on:
https://public-inbox.org/git/[email protected]/

Thanks; both patches make sense.

As the problematic feature appeared in 2.17.x track, I'll see if I can easily make it ready to be merged down to maint-2.17 track later when somebody wants to.

Quoted 13 lines

Matthew DeVore (2):
Documentation/git-log.txt: do not show --exclude-promisor-objects
exclude-promisor-objects: declare when option is allowed

 Documentation/rev-list-options.txt | 2 +-
 builtin/pack-objects.c             | 1 +
 builtin/prune.c                    | 1 +
 builtin/rev-list.c                 | 1 +
 revision.c                         | 3 ++-
 revision.h                         | 1 +
 t/t4202-log.sh                     | 4 ++++
 t/t8002-blame.sh                   | 4 ++++
 8 files changed, 15 insertions(+), 2 deletions(-)

Junio C Hamano

2018-10-23 05:09 UTC #3 lore raw

Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed

In reply to Matthew DeVore

Quoted 22 lines · Matthew DeVore

 t/t4202-log.sh         | 4 ++++
 t/t8002-blame.sh       | 4 ++++
 7 files changed, 14 insertions(+), 1 deletion(-)

...

t/t4202-log.sh +4 -0

index 153a506151..819c24d10e 100755
@@ -1703,4 +1703,8 @@ test_expect_success 'log --source paints symmetric ranges' '
 	test_cmp expect actual
 '
 
+test_expect_success '--exclude-promisor-objects does not BUG-crash' '
+	test_must_fail git log --exclude-promisor-objects source-a
+'
+
 test_done

t/t8002-blame.sh +0 -0

index 380e1c1054..eea048e52c 100755

t/t8002-blame.sh test_expect_success '--no-abbrev works like --abbrev=40' ' · line 125

Quoted 8 lines

t/t8002-blame.sh +4 -0

@@ -118,4 +118,8 @@ test_expect_success '--no-abbrev works like --abbrev=40' '
 	check_abbrev 40 --no-abbrev
 '
 
+test_expect_success '--exclude-promisor-objects does not BUG-crash' '
+	test_must_fail git blame --exclude-promisor-objects one
+'
+
 test_done

OK. We used to be hitting BUG() which is an abort() in disguise, so must-fail would have caught it without the fix in this patch. Now we would see a more controlled failure.

... goes and makes sure that is the case ...

Not really. We were already doing a controlled failure via die(), so these two tests would not have caught the problem in the code before the fix in this patch.

But nevertheless this is a good change; I do not think it is worth grepping for "unrecognized option" to differentiate the two cases.

Thanks.

Matthew DeVore

2018-10-23 17:09 UTC #18 lore raw

In reply to Junio C Hamano

Quoted 6 lines · Junio C Hamano

Thanks; both patches make sense.
As the problematic feature appeared in 2.17.x track, I'll see if I can easily make it ready to be merged down to maint-2.17 track later when somebody wants to.

Great. Thank you for the review.

Matthew DeVore

2018-10-23 18:04 UTC #4 lore raw

Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed

In reply to Junio C Hamano

Quoted 4 lines · Junio C Hamano

Not really. We were already doing a controlled failure via die(), so these two tests would not have caught the problem in the code before the fix in this patch.

BUG is apparently considered a "wrong" failure and not a controlled one by test_must_fail. I just double-checked that the tests fail without this patch.

not ok 119 - --exclude-promisor-objects does not BUG-crash
# # test_must_fail git blame --exclude-promisor-objects one
# # failed 1 among 119 test(s)
1..119

Junio C Hamano

2018-10-24 03:32 UTC #5 lore raw

Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed

Quoted 10 lines · Matthew DeVore

Quoted 4 lines · Junio C Hamano
Not really. We were already doing a controlled failure via die(), so these two tests would not have caught the problem in the code before the fix in this patch.
BUG is apparently considered a "wrong" failure and not a controlled one by test_must_fail. I just double-checked that the tests fail without this patch.

Ah, I was testing a wrong codepath. Yes, it does call BUG("..."), which is a prettier-looking abort(), but I somehow thought it was doing die("BUG: ...").

In any case, thanks for the fix.

Jeff King

2018-11-21 16:40 UTC #6 lore raw

Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed

In reply to Matthew DeVore

builtin/prune.c int cmd_prune(int argc, const char **argv, const char *prefix) · line 126

Quoted 12 lines · Matthew DeVore

builtin/prune.c +1 -0

index 41230f8215..11284d0bf3 100644
@@ -120,6 +120,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
 	save_commit_buffer = 0;
 	read_replace_refs = 0;
 	ref_paranoia = 1;
+	revs.allow_exclude_promisor_objects_opt = 1;
 	repo_init_revisions(the_repository, &revs, prefix);
 
 	argc = parse_options(argc, argv, prefix, options, prune_usage, 0);

I think this line is in the wrong place. The very first thing repo_init_revisions() will do is memset() the revs struct to all-zeroes, so it cannot possibly be doing anything.

Normally it would need to go after init_revisions() but before setup_revisions(), but we don't seem to call the latter at all in builtin/prune.c. Which makes sense, because you cannot pass options to influence the reachability traversal. So I don't think we need to care about this flag at all here.

Speaking of which, would this flag work better as a field in setup_revision_opt, which is passed to setup_revisions()? The intent seem to be to influence how we parse command-line arguments, and that's where other similar flags are (e.g., assume_dashdash).

-Peff

Matthew DeVore

2018-12-01 01:41 UTC #7 lore raw

Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed

Quoted 13 lines · Jeff King

Quoted 11 lines · Matthew DeVore

builtin/prune.c +1 -0

index 41230f8215..11284d0bf3 100644
@@ -120,6 +120,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
  	save_commit_buffer = 0;
  	read_replace_refs = 0;
  	ref_paranoia = 1;
+	revs.allow_exclude_promisor_objects_opt = 1;
  	repo_init_revisions(the_repository, &revs, prefix);

Quoted 5 lines

builtin/prune.c int cmd_prune(int argc, const char **argv, const char *prefix) · line 126
Quoted 1 line
builtin/prune.c +0 -0
  	argc = parse_options(argc, argv, prefix, options, prune_usage, 0);
I think this line is in the wrong place. The very first thing repo_init_revisions() will do is memset() the revs struct to all-zeroes, so it cannot possibly be doing anything.

Ah of course :)

Quoted 6 lines

Normally it would need to go after init_revisions() but before setup_revisions(), but we don't seem to call the latter at all in builtin/prune.c. Which makes sense, because you cannot pass options to influence the reachability traversal. So I don't think we need to care about this flag at all here.

Agreed, prune.c doesn't use setup_revisions() even transitively, so we don't care about this flag.

Quoted 5 lines

Speaking of which, would this flag work better as a field in setup_revision_opt, which is passed to setup_revisions()? The intent seem to be to influence how we parse command-line arguments, and that's where other similar flags are (e.g., assume_dashdash).

Good idea. This would solve the problem of mistakenly believing the flag matters when it doesn't, since it is in the struct which is used as the arguments of the exact function that cares about it. Here's a new patch - I'm tweaking e-mail client settings so hopefully this makes it to the list without mangling - if not I'll resend it with git-send-email later.

From 941c89fe1e226ed4d210ce35d0d906526b8277ed Mon Sep 17 00:00:00 2001
From: Matthew DeVore <[email protected]>
Date: Fri, 30 Nov 2018 16:43:32 -0800
Subject: [PATCH] revisions.c: put promisor option in specialized struct

Put the allow_exclude_promisor_objects flag in setup_revision_opt. When it was in rev_info, it was unclear when it was used, since rev_info is passed to functions that don't use the flag. This resulted in unnecessary setting of the flag in prune.c, so fix that as well.

Signed-off-by Matthew DeVore <[email protected]>

---
 builtin/pack-objects.c |  7 +++++--
 builtin/prune.c        |  1 -
 builtin/rev-list.c     |  6 ++++--
 revision.c             | 17 ++++++++++++-----
 revision.h             |  4 ++--
 5 files changed, 23 insertions(+), 12 deletions(-)

builtin/pack-objects.c +5 -2

index 24bba8147f..b22c99f540 100644
@@ -3084,14 +3084,17 @@ static void record_recent_commit(struct commit *commit, void *data)
 static void get_object_list(int ac, const char **av)
 {
 	struct rev_info revs;
+	struct setup_revision_opt s_r_opt;
 	char line[1000];
 	int flags = 0;
 	int save_warning;
 
 	repo_init_revisions(the_repository, &revs, NULL);
 	save_commit_buffer = 0;
-	revs.allow_exclude_promisor_objects_opt = 1;
-	setup_revisions(ac, av, &revs, NULL);
+
+	memset(&s_r_opt, 0, sizeof(s_r_opt));
+	s_r_opt.allow_exclude_promisor_objects = 1;
+	setup_revisions(ac, av, &revs, &s_r_opt);
 
 	/* make sure shallows are read */
 	is_repository_shallow(the_repository);

builtin/prune.c +0 -1

index e42653b99c..1ec9ddd751 100644
@@ -120,7 +120,6 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
 	save_commit_buffer = 0;
 	read_replace_refs = 0;
 	ref_paranoia = 1;
-	revs.allow_exclude_promisor_objects_opt = 1;
 	repo_init_revisions(the_repository, &revs, prefix);
 
 	argc = parse_options(argc, argv, prefix, options, prune_usage, 0);

builtin/rev-list.c +4 -2

index 3a2c0c23b6..c3095c6fed 100644
@@ -362,6 +362,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 {
 	struct rev_info revs;
 	struct rev_list_info info;
+	struct setup_revision_opt s_r_opt;
 	int i;
 	int bisect_list = 0;
 	int bisect_show_vars = 0;

@@ -375,7 +376,6 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 	repo_init_revisions(the_repository, &revs, prefix);
 	revs.abbrev = DEFAULT_ABBREV;
-	revs.allow_exclude_promisor_objects_opt = 1;
 	revs.commit_format = CMIT_FMT_UNSPECIFIED;
 	revs.do_not_die_on_missing_tree = 1;

@@ -407,7 +407,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	argc = setup_revisions(argc, argv, &revs, NULL);
+	memset(&s_r_opt, 0, sizeof(s_r_opt));
+	s_r_opt.allow_exclude_promisor_objects = 1;
+	argc = setup_revisions(argc, argv, &revs, &s_r_opt);
 
 	memset(&info, 0, sizeof(info));
 	info.revs = &revs;

revision.c +0 -0

index 13e0519c02..221ba79594 100644
@@ -1791,7 +1791,8 @@ static void add_message_grep(struct rev_info *revs, const char *pattern)
 }
 
 static int handle_revision_opt(struct rev_info *revs, int argc, const

char **argv,

-			       int *unkc, const char **unkv)
+			       int *unkc, const char **unkv,
+			       int allow_exclude_promisor_objects)

{
const char *arg = argv[0];
const char *optarg;
@@ -2151,7 +2152,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
revs->limited = 1;
} else if (!strcmp(arg, "--ignore-missing")) {
revs->ignore_missing = 1;
- } else if (revs->allow_exclude_promisor_objects_opt &&
+ } else if (allow_exclude_promisor_objects &&
!strcmp(arg, "--exclude-promisor-objects")) {
if (fetch_if_missing)
BUG("exclude_promisor_objects can only be used when
fetch_if_missing is 0");
@@ -2173,7 +2174,8 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
const char * const usagestr[])
{
int n = handle_revision_opt(revs, ctx->argc, ctx->argv,

-				    &ctx->cpidx, ctx->out);
+				    &ctx->cpidx, ctx->out,
+				    /*allow_exclude_promisor_objects=*/0);

if (n <= 0) {
error("unknown option `%s'", ctx->argv[0]); usage_with_options(usagestr, options);
@@ -2340,9 +2342,12 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
int i, flags, left, seen_dashdash, got_rev_arg = 0, revarg_opt;
struct argv_array prune_data = ARGV_ARRAY_INIT;
const char *submodule = NULL;
+ int allow_exclude_prom_objs = 0;

- if (opt)
+ if (opt) {
submodule = opt->submodule;
+ allow_exclude_prom_objs = opt->allow_exclude_promisor_objects;
+ }

/* First, search for "--" */
if (opt && opt->assume_dashdash) {
@@ -2391,7 +2396,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
continue;
}

- opts = handle_revision_opt(revs, argc - i, argv + i, &left, argv);
+ opts = handle_revision_opt(revs, argc - i, argv + i,
+ &left, argv,
+ allow_exclude_prom_objs);
if (opts > 0) {
i += opts - 1;
continue;

revision.h +2 -2

index 7987bfcd2e..7d6e050569 100644
@@ -161,7 +161,6 @@ struct rev_info {
 			do_not_die_on_missing_tree:1,
 
 			/* for internal use only */
-			allow_exclude_promisor_objects_opt:1,
 			exclude_promisor_objects:1;
 
 	/* Diff flags */

@@ -297,7 +296,8 @@ struct setup_revision_opt {
 	const char *def;
 	void (*tweak)(struct rev_info *, struct setup_revision_opt *);
 	const char *submodule;	/* TODO: drop this and use rev_info->repo */
-	int assume_dashdash;
+	int assume_dashdash : 1;
+	int allow_exclude_promisor_objects : 1;
 	unsigned revarg_opt;
 };

Signature · 1 line

2.20.0.rc1.387.gf8505762e3-goog

Jeff King

2018-12-01 19:44 UTC #8 lore raw

Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed

Quoted 16 lines · Matthew DeVore

Quoted 4 lines
Speaking of which, would this flag work better as a field in setup_revision_opt, which is passed to setup_revisions()? The intent seem to be to influence how we parse command-line arguments, and that's where other similar flags are (e.g., assume_dashdash).
Good idea. This would solve the problem of mistakenly believing the flag matters when it doesn't, since it is in the struct which is used as the arguments of the exact function that cares about it. Here's a new patch - I'm tweaking e-mail client settings so hopefully this makes it to the list without mangling - if not I'll resend it with git-send-email later.
From 941c89fe1e226ed4d210ce35d0d906526b8277ed Mon Sep 17 00:00:00 2001
From: Matthew DeVore <[email protected]>
Date: Fri, 30 Nov 2018 16:43:32 -0800
Subject: [PATCH] revisions.c: put promisor option in specialized struct

Quoted 4 lines

Put the allow_exclude_promisor_objects flag in setup_revision_opt. When it was in rev_info, it was unclear when it was used, since rev_info is passed to functions that don't use the flag. This resulted in unnecessary setting of the flag in prune.c, so fix that as well.

Thanks, this looks pretty reasonable overall. Two comments:

builtin/pack-objects.c static void record_recent_commit(struct commit *commit, void *data) · line 3097

Quoted 8 lines

builtin/pack-objects.c +4 -2

 	repo_init_revisions(the_repository, &revs, NULL);
 	save_commit_buffer = 0;
-	revs.allow_exclude_promisor_objects_opt = 1;
-	setup_revisions(ac, av, &revs, NULL);
+
+	memset(&s_r_opt, 0, sizeof(s_r_opt));
+	s_r_opt.allow_exclude_promisor_objects = 1;
+	setup_revisions(ac, av, &revs, &s_r_opt);

I wonder if a static initializer for setup_revision_opt is worth it. It would remove the need for this memset. Probably not a big deal either way, though.

Review

Quoted 5 lines

static int handle_revision_opt(struct rev_info *revs, int argc, const char
**argv,
(quoted diff) +2 -1
-			       int *unkc, const char **unkv)
+			       int *unkc, const char **unkv,
+			       int allow_exclude_promisor_objects)

Why not pass in the whole setup_revision_opt struct? We don't need anything else from it yet, but it seems like the point of that struct is to pass around preferences like this.

-Peff

Matthew DeVore

2018-12-03 19:11 UTC #9 lore raw

Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed

Quoted 4 lines · Jeff King

Quoted 4 lines

  	repo_init_revisions(the_repository, &revs, NULL);
  	save_commit_buffer = 0;

builtin/pack-objects.c +0 -2

-	revs.allow_exclude_promisor_objects_opt = 1;
-	setup_revisions(ac, av, &revs, NULL);

Quoted 8 lines

builtin/pack-objects.c static void record_recent_commit(struct commit *commit, void *data) · line 3097
Quoted 4 lines
builtin/pack-objects.c +4 -0
+
+	memset(&s_r_opt, 0, sizeof(s_r_opt));
+	s_r_opt.allow_exclude_promisor_objects = 1;
+	setup_revisions(ac, av, &revs, &s_r_opt);
I wonder if a static initializer for setup_revision_opt is worth it. It would remove the need for this memset. Probably not a big deal either way, though.

I think you mean something like this:

static struct setup_revision_opt s_r_opt = {NULL, NULL, NULL, 0, 1, 0};

This is a bit cryptic (I have to read the struct declaration in order to know what is being set to 1) and if the struct ever gets a new field before allow_exclude_promisor_objects, this initializer has to be updated.

Quoted 10 lines

Review
Quoted 6 lines
static int handle_revision_opt(struct rev_info *revs, int argc, const char
**argv,
(quoted diff) +2 -1
-			       int *unkc, const char **unkv)
+			       int *unkc, const char **unkv,
+			       int allow_exclude_promisor_objects)
Why not pass in the whole setup_revision_opt struct? We don't need anything else from it yet, but it seems like the point of that struct is to pass around preferences like this.

OK, the code reads better if I do that, so I agree.

Quoted 3 lines

-Peff

Matthew DeVore

author

2018-12-03 19:24 UTC #13 lore raw

PATCH 1/1 revisions.c: put promisor option in specialized struct

In reply to Jeff King

Signed-off-by Matthew DeVore <[email protected]>

---
 builtin/pack-objects.c |  7 +++++--
 builtin/prune.c        |  1 -
 builtin/rev-list.c     |  6 ++++--
 revision.c             | 10 ++++++----
 revision.h             |  4 ++--
 5 files changed, 17 insertions(+), 11 deletions(-)

builtin/pack-objects.c +5 -2

index 24bba8147f..b22c99f540 100644
@@ -3084,14 +3084,17 @@ static void record_recent_commit(struct commit *commit, void *data)
 static void get_object_list(int ac, const char **av)
 {
 	struct rev_info revs;
+	struct setup_revision_opt s_r_opt;
 	char line[1000];
 	int flags = 0;
 	int save_warning;
 
 	repo_init_revisions(the_repository, &revs, NULL);
 	save_commit_buffer = 0;
-	revs.allow_exclude_promisor_objects_opt = 1;
-	setup_revisions(ac, av, &revs, NULL);
+
+	memset(&s_r_opt, 0, sizeof(s_r_opt));
+	s_r_opt.allow_exclude_promisor_objects = 1;
+	setup_revisions(ac, av, &revs, &s_r_opt);
 
 	/* make sure shallows are read */
 	is_repository_shallow(the_repository);

builtin/prune.c +0 -1

index e42653b99c..1ec9ddd751 100644
@@ -120,7 +120,6 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
 	save_commit_buffer = 0;
 	read_replace_refs = 0;
 	ref_paranoia = 1;
-	revs.allow_exclude_promisor_objects_opt = 1;
 	repo_init_revisions(the_repository, &revs, prefix);
 
 	argc = parse_options(argc, argv, prefix, options, prune_usage, 0);

builtin/rev-list.c +4 -2

index 3a2c0c23b6..c3095c6fed 100644
@@ -362,6 +362,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 {
 	struct rev_info revs;
 	struct rev_list_info info;
+	struct setup_revision_opt s_r_opt;
 	int i;
 	int bisect_list = 0;
 	int bisect_show_vars = 0;

@@ -375,7 +376,6 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 	repo_init_revisions(the_repository, &revs, prefix);
 	revs.abbrev = DEFAULT_ABBREV;
-	revs.allow_exclude_promisor_objects_opt = 1;
 	revs.commit_format = CMIT_FMT_UNSPECIFIED;
 	revs.do_not_die_on_missing_tree = 1;

@@ -407,7 +407,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	argc = setup_revisions(argc, argv, &revs, NULL);
+	memset(&s_r_opt, 0, sizeof(s_r_opt));
+	s_r_opt.allow_exclude_promisor_objects = 1;
+	argc = setup_revisions(argc, argv, &revs, &s_r_opt);
 
 	memset(&info, 0, sizeof(info));
 	info.revs = &revs;

revision.c +6 -4

index 13e0519c02..f6b32e6a42 100644
@@ -1791,7 +1791,8 @@ static void add_message_grep(struct rev_info *revs, const char *pattern)
 }
 
 static int handle_revision_opt(struct rev_info *revs, int argc, const char **argv,
-			       int *unkc, const char **unkv)
+			       int *unkc, const char **unkv,
+			       const struct setup_revision_opt* opt)
 {
 	const char *arg = argv[0];
 	const char *optarg;

@@ -2151,7 +2152,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->limited = 1;
 	} else if (!strcmp(arg, "--ignore-missing")) {
 		revs->ignore_missing = 1;
-	} else if (revs->allow_exclude_promisor_objects_opt &&
+	} else if (opt && opt->allow_exclude_promisor_objects &&
 		   !strcmp(arg, "--exclude-promisor-objects")) {
 		if (fetch_if_missing)
 			BUG("exclude_promisor_objects can only be used when fetch_if_missing is 0");

@@ -2173,7 +2174,7 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
 			const char * const usagestr[])
 {
 	int n = handle_revision_opt(revs, ctx->argc, ctx->argv,
-				    &ctx->cpidx, ctx->out);
+				    &ctx->cpidx, ctx->out, NULL);
 	if (n <= 0) {
 		error("unknown option `%s'", ctx->argv[0]);
 		usage_with_options(usagestr, options);

@@ -2391,7 +2392,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 				continue;
 			}
 
-			opts = handle_revision_opt(revs, argc - i, argv + i, &left, argv);
+			opts = handle_revision_opt(revs, argc - i, argv + i,
+						   &left, argv, opt);
 			if (opts > 0) {
 				i += opts - 1;
 				continue;

revision.h +2 -2

index 7987bfcd2e..7d6e050569 100644
@@ -161,7 +161,6 @@ struct rev_info {
 			do_not_die_on_missing_tree:1,
 
 			/* for internal use only */
-			allow_exclude_promisor_objects_opt:1,
 			exclude_promisor_objects:1;
 
 	/* Diff flags */

@@ -297,7 +296,8 @@ struct setup_revision_opt {
 	const char *def;
 	void (*tweak)(struct rev_info *, struct setup_revision_opt *);
 	const char *submodule;	/* TODO: drop this and use rev_info->repo */
-	int assume_dashdash;
+	int assume_dashdash : 1;
+	int allow_exclude_promisor_objects : 1;
 	unsigned revarg_opt;
 };

Signature · 1 line

2.20.0.rc1.387.gf8505762e3-goog

Jeff King

2018-12-03 21:15 UTC #10 lore raw

Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed

In reply to Matthew DeVore

Quoted 7 lines · Matthew DeVore

Quoted 7 lines
builtin/pack-objects.c static void record_recent_commit(struct commit *commit, void *data) · line 3097
Quoted 3 lines
builtin/pack-objects.c +3 -0
+	memset(&s_r_opt, 0, sizeof(s_r_opt));
+	s_r_opt.allow_exclude_promisor_objects = 1;
+	setup_revisions(ac, av, &revs, &s_r_opt);
I wonder if a static initializer for setup_revision_opt is worth it. It would remove the need for this memset. Probably not a big deal either way, though.

Quoted 7 lines

I think you mean something like this:
static struct setup_revision_opt s_r_opt = {NULL, NULL, NULL, 0, 1, 0};
This is a bit cryptic (I have to read the struct declaration in order to know what is being set to 1) and if the struct ever gets a new field before allow_exclude_promisor_objects, this initializer has to be updated.

I agree that's pretty awful. I meant something like this:

  struct setup_revision_opt s_r_opt = { NULL };
  ...

  s_r_opt.allow_exclude_promisor_objects = 1;
  setup_revisions(...);

It's functionally equivalent to the memset(), but you don't have to wonder about whether we peek at the uninitialized state in between.

That said, our C99 designated initializer weather-balloons haven't gotten any complaints yet. So I think you could actually do:

struct setup_revision_opt s_r_opt = {
.allow_exclude_promisor_objects = 1,
};
...
setup_revisions(...);

which is pretty nice.

-Peff

Jeff King

2018-12-03 21:24 UTC #14 lore raw

Re: [PATCH] revisions.c: put promisor option in specialized struct

In reply to Matthew DeVore

Quoted 5 lines · Matthew DeVore

Put the allow_exclude_promisor_objects flag in setup_revision_opt. When it was in rev_info, it was unclear when it was used, since rev_info is passed to functions that don't use the flag. This resulted in unnecessary setting of the flag in prune.c, so fix that as well.

Quoted 8 lines

Signed-off-by Matthew DeVore <[email protected]>

---
 builtin/pack-objects.c |  7 +++++--
 builtin/prune.c        |  1 -
 builtin/rev-list.c     |  6 ++++--
 revision.c             | 10 ++++++----
 revision.h             |  4 ++--
 5 files changed, 17 insertions(+), 11 deletions(-)

Thanks, this mostly looks good to me (with or without tweaking the initializers as discussed in the other part of the thread).

One thing I noticed:

revision.h struct setup_revision_opt { · line 302

Quoted 9 lines

revision.h +2 -1

@@ -297,7 +296,8 @@ struct setup_revision_opt {
 	const char *def;
 	void (*tweak)(struct rev_info *, struct setup_revision_opt *);
 	const char *submodule;	/* TODO: drop this and use rev_info->repo */
-	int assume_dashdash;
+	int assume_dashdash : 1;
+	int allow_exclude_promisor_objects : 1;
 	unsigned revarg_opt;
 };

I don't know that we need to penny-pinch bytes in this struct, but in general it shouldn't hurt either awy. However, a signed bit-field with 1 bit is funny. I'm not even sure what the standard has to say, but in twos-complement that would store "-1" and "0" (gcc -Wpedantic also complains about overflow in assigning "1" to it).

So this probably ought to be "unsigned".

-Peff

Matthew DeVore

2018-12-03 21:54 UTC #11 lore raw

Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed

In reply to Jeff King

Quoted 6 lines · Jeff King

That said, our C99 designated initializer weather-balloons haven't gotten any complaints yet. So I think you could actually do:
   struct setup_revision_opt s_r_opt = {
	.allow_exclude_promisor_objects = 1,
   };

I like this way best, so I'll use it. Thank you.

Matthew DeVore

2018-12-03 22:09 UTC #15 lore raw

Re: [PATCH] revisions.c: put promisor option in specialized struct

In reply to Jeff King

Quoted 7 lines · Jeff King

Quoted 7 lines

revision.h +2 -1

@@ -297,7 +296,8 @@ struct setup_revision_opt {
  	const char *def;
  	void (*tweak)(struct rev_info *, struct setup_revision_opt *);
  	const char *submodule;	/* TODO: drop this and use rev_info->repo */
-	int assume_dashdash;
+	int assume_dashdash : 1;
+	int allow_exclude_promisor_objects : 1;

Quoted 8 lines

revision.h struct setup_revision_opt { · line 302
Quoted 2 lines
revision.h +0 -0
  	unsigned revarg_opt;
  };
I don't know that we need to penny-pinch bytes in this struct, but in general it shouldn't hurt either awy. However, a signed bit-field with 1 bit is funny. I'm not even sure what the standard has to say, but in twos-complement that would store "-1" and "0" (gcc -Wpedantic also complains about overflow in assigning "1" to it).

Interesting. I hadn't suspected this. But I confirmed it with this:

#include <stdio.h>

struct x {
int y : 1;
int z : 1;
};

int main() {
struct x x;
x.y = 1;
x.z = 1;
printf("%d %d\n", (int) x.y, (int) x.z);
return 0;
}

-- Output --
-1 -1

Quoted 2 lines

So this probably ought to be "unsigned".

Earlier in this file we define bit fields this way:

	/* Traversal flags */
	unsigned int	dense:1,
			prune:1,

... using \t to align the field names, so I'll mimic that style.

Junio C Hamano

2018-12-04 02:20 UTC #12 lore raw

Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed

In reply to Jeff King

Quoted 10 lines · Jeff King

That said, our C99 designated initializer weather-balloons haven't gotten any complaints yet. So I think you could actually do:
struct setup_revision_opt s_r_opt = {
.allow_exclude_promisor_objects = 1,
};
...
setup_revisions(...);
which is pretty nice.

Yup. The output from

    $ git grep -n ' \.[a-z0-9_]* =' -- \*.[ch]

with a bit of "git blame" tells us that cbc0f81d ("strbuf: use designated initializers in STRBUF_INIT", 2017-07-10) is the balloon for this exact feature. The same for array was done in 512f41cf ("clean.c: use designated initializer", 2017-07-14)

[I am writing it down so that I do not have to dig for it every time and instead can ask the list archive]