Re: [RFC/PATCH 2/4] replace: use OPT_CMDMODE to handle modes

[Jeff King]

On Thu, Mar 06, 2014 at 07:35:19PM +0100, Christian Couder wrote:

> > +   if (!cmdmode)
> > +   cmdmode = argc ? MODE_REPLACE : MODE_DELETE;
> 
> Shouldn't it be MODE_LIST instead of MODE_DELETE?

Argh, yes, thank you for catching. My original iteration used chars like
'd' and 'l' (similar to other uses of OPT_CMDMODE). I switched it at the
last minute to an enum, and somehow thinko'd that completely (and
obviously did not run the tests again afterwards).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC/PATCH 2/4] replace: use OPT_CMDMODE to handle modes

[Jeff King]

By using OPT_CMDMODE, the mutual exclusion between modes is
taken care of for us. It also makes it easy for us to
maintain a single variable with the mode, which makes its
intent more clear. We can use a single switch() to make sure
we have covered all of the modes.

This ends up breaking even in code size, but the win will be
much bigger when we start adding more modes.

Signed-off-by: Jeff King 
---
 builtin/replace.c | 49 +
 1 file changed, 25 insertions(+), 24 deletions(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index 6a0e8bd..0b5cb17 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -168,11 +168,17 @@ static int replace_object(const char *object_ref, const 
char *replace_ref,
 
 int cmd_replace(int argc, const char **argv, const char *prefix)
 {
-   int list = 0, delete = 0, force = 0;
+   int force = 0;
const char *format = NULL;
+   enum {
+   MODE_UNSPECIFIED = 0,
+   MODE_LIST,
+   MODE_DELETE,
+   MODE_REPLACE
+   } cmdmode = MODE_UNSPECIFIED;
struct option options[] = {
-   OPT_BOOL('l', "list", &list, N_("list replace refs")),
-   OPT_BOOL('d', "delete", &delete, N_("delete replace refs")),
+   OPT_CMDMODE('l', "list", &cmdmode, N_("list replace refs"), 
MODE_LIST),
+   OPT_CMDMODE('d', "delete", &cmdmode, N_("delete replace refs"), 
MODE_DELETE),
OPT_BOOL('f', "force", &force, N_("replace the ref if it 
exists")),
OPT_STRING(0, "format", &format, N_("format"), N_("use this 
format")),
OPT_END()
@@ -182,42 +188,37 @@ int cmd_replace(int argc, const char **argv, const char 
*prefix)
 
argc = parse_options(argc, argv, prefix, options, git_replace_usage, 0);
 
-   if (!list && !delete)
-   if (!argc)
-   list = 1;
+   if (!cmdmode)
+   cmdmode = argc ? MODE_REPLACE : MODE_DELETE;
 
-   if (list && delete)
-   usage_msg_opt("-l and -d cannot be used together",
- git_replace_usage, options);
-
-   if (format && !list)
+   if (format && cmdmode != MODE_LIST)
usage_msg_opt("--format cannot be used when not listing",
  git_replace_usage, options);
 
-   if (force && (list || delete))
-   usage_msg_opt("-f cannot be used with -d or -l",
+   if (force && cmdmode != MODE_REPLACE)
+   usage_msg_opt("-f only makes sense when writing a replacement",
  git_replace_usage, options);
 
-   /* Delete refs */
-   if (delete) {
+   switch (cmdmode) {
+   case MODE_DELETE:
if (argc < 1)
usage_msg_opt("-d needs at least one argument",
  git_replace_usage, options);
return for_each_replace_name(argv, delete_replace_ref);
-   }
 
-   /* Replace object */
-   if (!list && argc) {
+   case MODE_REPLACE:
if (argc != 2)
usage_msg_opt("bad number of arguments",
  git_replace_usage, options);
return replace_object(argv[0], argv[1], force);
-   }
 
-   /* List refs, even if "list" is not set */
-   if (argc > 1)
-   usage_msg_opt("only one pattern can be given with -l",
- git_replace_usage, options);
+   case MODE_LIST:
+   if (argc > 1)
+   usage_msg_opt("only one pattern can be given with -l",
+ git_replace_usage, options);
+   return list_replace_refs(argv[0], format);
 
-   return list_replace_refs(argv[0], format);
+   default:
+   die("BUG: invalid cmdmode %d", (int)cmdmode);
+   }
 }
-- 
1.8.5.2.500.g8060133

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html