revisions.c: put promisor option in specialized struct

Commit Message

Matthew DeVore Dec. 3, 2018, 7:23 p.m. UTC

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 <[e-mail]>
---
 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(-)

Comments

Jeff King Dec. 3, 2018, 9:24 p.m. UTC | #1

On Mon, Dec 03, 2018 at 11:23:56AM -0800, Matthew DeVore wrote:

> 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 <[e-mail]>
> ---
>  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:

> @@ -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 Dec. 3, 2018, 10:01 p.m. UTC | #2

On 12/03/2018 01:24 PM, Jeff King wrote:
>> @@ -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).
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

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

Patch

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 24bba8147f..b22c99f540 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -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);
diff --git a/builtin/prune.c b/builtin/prune.c
index e42653b99c..1ec9ddd751 100644
--- a/builtin/prune.c
+++ b/builtin/prune.c
@@ -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);
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 3a2c0c23b6..c3095c6fed 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -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;
diff --git a/revision.c b/revision.c
index 13e0519c02..f6b32e6a42 100644
--- a/revision.c
+++ b/revision.c
@@ -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;
diff --git a/revision.h b/revision.h
index 7987bfcd2e..7d6e050569 100644
--- a/revision.h
+++ b/revision.h
@@ -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;
 };