diff mbox series

[2/4] missing: support rejecting --missing=print

Message ID 20240418184043.2900955-3-christian.couder@gmail.com (mailing list archive)
State Superseded
Headers show
Series upload-pack: support a missing-action | expand

Commit Message

Christian Couder April 18, 2024, 6:40 p.m. UTC
`git pack-objects` supports the `--missing=<missing-action>` option in
the same way as `git rev-list` except when '<missing-action>' is
"print", which `git pack-objects` doesn't support.

As we want to refactor `git pack-objects` to use the same code from
"missing.{c,h}" as `git rev-list` for the `--missing=...` feature, let's
make it possible for that code to reject `--missing=print`.

`git pack-objects` will then use that code in a following commit.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/rev-list.c | 2 +-
 missing.c          | 4 ++--
 missing.h          | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

Comments

Junio C Hamano April 18, 2024, 9:47 p.m. UTC | #1
Christian Couder <christian.couder@gmail.com> writes:

> `git pack-objects` supports the `--missing=<missing-action>` option in
> the same way as `git rev-list` except when '<missing-action>' is
> "print", which `git pack-objects` doesn't support.
>
> As we want to refactor `git pack-objects` to use the same code from
> "missing.{c,h}" as `git rev-list` for the `--missing=...` feature, let's
> make it possible for that code to reject `--missing=print`.
>
> `git pack-objects` will then use that code in a following commit.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  builtin/rev-list.c | 2 +-
>  missing.c          | 4 ++--
>  missing.h          | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index f71cc5ebe1..a712a6fd62 100644
> --- a/builtin/rev-list.c
> +++ b/builtin/rev-list.c
> @@ -539,7 +539,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
>  			int res;
>  			if (revs.exclude_promisor_objects)
>  				die(_("options '%s' and '%s' cannot be used together"), "--exclude-promisor-objects", "--missing");
> -			res = parse_missing_action_value(arg);
> +			res = parse_missing_action_value(arg, 1);

Hmph, this smells like a horribly unscalable design, as we make the
vocabulary of missing-action richer, you'd end up piling on "this
choice is allowed in this call" parameters, wouldn't you?  The first
person who adds such an ad-hoc parameter would say "hey, what's just
one extra parameter print_ok between friends", but the next person
would say the same thing for their new choice and adds frotz_ok, and
we'd be in chaos.

Rather, shouldn't the _caller_ decide if the parsed value is
something it does not like and barf?

Alternatively, add a _single_ "reject" bitset and do something like

	int parse_missing_action_value(const char *value, unsigned reject)
	{
		...
		if (!(reject & (1<<MA_ERROR) && !strcmp(value, "error")))
			return MA_ERROR;
		if (!(reject & (1<<MA_PRINT) && !strcmp(value, "print")))
			return MA_PRINT;
		...

which would scale better (but still my preference is to have the
caller deal with only the values it recognises---do not make the
caller say "if (res >= 0 && res != MA_PRINT)" as that will not scale
when new choices that are accepted elsewhere are added.
diff mbox series

Patch

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index f71cc5ebe1..a712a6fd62 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -539,7 +539,7 @@  int cmd_rev_list(int argc, const char **argv, const char *prefix)
 			int res;
 			if (revs.exclude_promisor_objects)
 				die(_("options '%s' and '%s' cannot be used together"), "--exclude-promisor-objects", "--missing");
-			res = parse_missing_action_value(arg);
+			res = parse_missing_action_value(arg, 1);
 			if (res >= 0) {
 				arg_missing_action = res;
 				break;
diff --git a/missing.c b/missing.c
index 83e0c5e584..d306dea2d9 100644
--- a/missing.c
+++ b/missing.c
@@ -2,7 +2,7 @@ 
 #include "missing.h"
 #include "object-file.h"
 
-int parse_missing_action_value(const char *value)
+int parse_missing_action_value(const char *value, int print_ok)
 {
 	if (!strcmp(value, "error"))
 		return MA_ERROR;
@@ -12,7 +12,7 @@  int parse_missing_action_value(const char *value)
 		return MA_ALLOW_ANY;
 	}
 
-	if (!strcmp(value, "print")) {
+	if (!strcmp(value, "print") && print_ok) {
 		fetch_if_missing = 0;
 		return MA_PRINT;
 	}
diff --git a/missing.h b/missing.h
index c8261dfe55..77906691e7 100644
--- a/missing.h
+++ b/missing.h
@@ -13,6 +13,6 @@  enum missing_action {
   if parsing failed. Also sets the fetch_if_missing global variable
   from "object-file.h".
  */
-int parse_missing_action_value(const char *value);
+int parse_missing_action_value(const char *value, int print_ok);
 
 #endif /* MISSING_H */