[PATCH v2] test-chmtime: use parse_options() to parse command-line flags

["Shashank S via GitGitGadget" <gitgitgadget@gmail.com>]

From: Shashank S Mogaveera <shashu8660@gmail.com>

cmd__chmtime() currently parses the '-v'/'--verbose' and '-g'/'--get'
flags manually using strcmp() checks and manual index tracking.

Migrate cmd__chmtime() to use parse_options() and usage_with_options()
instead. Pass PARSE_OPT_KEEP_UNKNOWN_OPT so that negative relative time
offsets (such as '-86400') are not rejected as unknown short options
and remain in argv for timespec_arg() to parse.

Additionally, strictly prevent combining mutually exclusive options
('--get' and '--verbose') by utilizing die_for_incompatible_opt2(),
and introduce a new test script to verify both this restriction and
the standard usage.

Signed-off-by: Shashank S Mogaveera <shashu8660@gmail.com>
---
    [Outreachy]test-chmtime: use parse_options() to parse command-line flags

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2280%2Fshashu8660%2Foutreachy3-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2280/shashu8660/outreachy3-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2280

Range-diff vs v1:

 1:  4f702e3f0b ! 1:  43c96de13e test-chmtime: use parse_options() to parse command-line flags
     @@ Commit message
          cmd__chmtime() currently parses the '-v'/'--verbose' and '-g'/'--get'
          flags manually using strcmp() checks and manual index tracking.
      
     -    Refactor cmd__chmtime() to use parse_options() and usage_with_options()
     +    Migrate cmd__chmtime() to use parse_options() and usage_with_options()
          instead. Pass PARSE_OPT_KEEP_UNKNOWN_OPT so that negative relative time
          offsets (such as '-86400') are not rejected as unknown short options
          and remain in argv for timespec_arg() to parse.
      
     +    Additionally, strictly prevent combining mutually exclusive options
     +    ('--get' and '--verbose') by utilizing die_for_incompatible_opt2(),
     +    and introduce a new test script to verify both this restriction and
     +    the standard usage.
     +
          Signed-off-by: Shashank S Mogaveera <shashu8660@gmail.com>
      
       ## t/helper/test-chmtime.c ##
      @@
     +  *
     +  * The mtime can be changed to an absolute value:
     +  *
     +- *	test-tool chmtime =<seconds> file...
     ++ *  test-tool chmtime =<seconds> file...
     +  *
     +  * Relative to the current time as returned by time(3):
     +  *
     +- *	test-tool chmtime =+<seconds> (or =-<seconds>) file...
     ++ *  test-tool chmtime =+<seconds> (or =-<seconds>) file...
     +  *
     +  * Or relative to the current mtime of the file:
     +  *
     +- *	test-tool chmtime <seconds> file...
     +- *	test-tool chmtime +<seconds> (or -<seconds>) file...
     ++ *  test-tool chmtime <seconds> file...
     ++ *  test-tool chmtime +<seconds> (or -<seconds>) file...
     +  *
     +  * Examples:
     +  *
     +  * To print the mtime and the file name use --verbose and set
     +  * the file mtime offset to 0:
     +  *
     +- *	test-tool chmtime -v +0 file
     ++ *  test-tool chmtime -v +0 file
     +  *
     +  * To print only the mtime use --get:
     +  *
     +- *	test-tool chmtime --get file
     ++ *  test-tool chmtime --get file
     +  *
     +  * To set the mtime to current time:
     +  *
     +- *	test-tool chmtime =+0 file
     ++ *  test-tool chmtime =+0 file
     +  *
     +  * To set the file mtime offset to +1 and print the new value:
     +  *
     +- *	test-tool chmtime --get +1 file
     ++ *  test-tool chmtime --get +1 file
     +  *
        */
       #include "test-tool.h"
       #include "git-compat-util.h"
     @@ t/helper/test-chmtime.c
      -static const char usage_str[] =
      -	"(-v|--verbose|-g|--get) (+|=|=+|=-|-)<seconds> <file>...";
      +static const char * const chmtime_usage[] = {
     -+	"test-tool chmtime (-v|--verbose|-g|--get) (+|=|=+|=-|-)<seconds> <file>...",
     ++	"test-tool chmtime [<options>] (+|=|=+|=-|-)<seconds> <file>...",
      +	NULL
      +};
       
       static int timespec_arg(const char *arg, long int *set_time, int *set_eq)
       {
     -@@ t/helper/test-chmtime.c: static int timespec_arg(const char *arg, long int *set_time, int *set_eq)
     - 
     - int cmd__chmtime(int argc, const char **argv)
     - {
     --	static int verbose;
     --	static int get;
     -+	int verbose = 0;
     -+	int get = 0;
     +@@ t/helper/test-chmtime.c: int cmd__chmtime(int argc, const char **argv)
     + 	static int verbose;
     + 	static int get;
       
      -	int i = 1;
      +	int i = 0;
     @@ t/helper/test-chmtime.c: static int timespec_arg(const char *arg, long int *set_
      -	if (i == argc) {
      -		goto usage;
      -	}
     -+	if (argc < 1 || (!get && !verbose && argc < 2))
     ++	die_for_incompatible_opt2(get, "--get", verbose, "--verbose");
     ++
     ++	if (argc < 1)
      +		usage_with_options(chmtime_usage, options);
       
       	if (timespec_arg(argv[i], &set_time, &set_eq)) {
     - 		++i;
     - 	} else {
     - 		if (get == 0 && verbose == 0) {
     - 			fprintf(stderr, "Not a base-10 integer: %s\n", argv[i] + 1);
     +-		++i;
     +-	} else {
     +-		if (get == 0 && verbose == 0) {
     +-			fprintf(stderr, "Not a base-10 integer: %s\n", argv[i] + 1);
      -			goto usage;
     -+			usage_with_options(chmtime_usage, options);
     - 		}
     +-		}
     ++		i++;
     ++	} else if (!get && !verbose) {
     ++		fprintf(stderr, "Not a base-10 integer: %s\n", argv[i] + 1);
     ++		usage_with_options(chmtime_usage, options);
       	}
       
       	if (i == argc)
     @@ t/helper/test-chmtime.c: int cmd__chmtime(int argc, const char **argv)
       			continue;
       		}
       
     + #ifdef GIT_WINDOWS_NATIVE
     + 		if (!(sb.st_mode & S_IWUSR) &&
     +-				chmod(argv[i], sb.st_mode | S_IWUSR)) {
     ++		    chmod(argv[i], sb.st_mode | S_IWUSR)) {
     + 			fprintf(stderr, "Could not make user-writable %s: %s",
     + 				argv[i], strerror(errno));
     + 			return 1;
     +@@ t/helper/test-chmtime.c: int cmd__chmtime(int argc, const char **argv)
     + 		utb.actime = sb.st_atime;
     + 		utb.modtime = set_eq ? set_time : sb.st_mtime + set_time;
     + 
     +-		mtime = utb.modtime < 0 ? 0: utb.modtime;
     ++		mtime = utb.modtime < 0 ? 0 : utb.modtime;
     + 		if (get) {
     + 			printf("%"PRIuMAX"\n", mtime);
     + 		} else if (verbose) {
      @@ t/helper/test-chmtime.c: int cmd__chmtime(int argc, const char **argv)
       			}
       #endif
     @@ t/helper/test-chmtime.c: int cmd__chmtime(int argc, const char **argv)
      -	fprintf(stderr, "usage: %s %s\n", argv[0], usage_str);
      -	return 1;
       }
     +
     + ## t/meson.build ##
     +@@ t/meson.build: integration_tests = [
     +   't0071-sort.sh',
     +   't0080-unit-test-output.sh',
     +   't0081-find-pack.sh',
     ++  't0082-test-chmtime.sh',
     +   't0090-cache-tree.sh',
     +   't0091-bugreport.sh',
     +   't0092-diagnose.sh',
     +
     + ## t/t0082-test-chmtime.sh (new) ##
     +@@
     ++#!/bin/sh
     ++
     ++test_description='tests for test-tool chmtime'
     ++
     ++TEST_PASSES_SANITIZE_LEAK=true
     ++. ./test-lib.sh
     ++
     ++test_expect_success 'setup' '
     ++	echo hello >file
     ++'
     ++
     ++test_expect_success 'mutually exclusive options are rejected' '
     ++	test_must_fail test-tool chmtime --get --verbose +0 file 2>err &&
     ++	test_grep "cannot be used together" err
     ++'
     ++
     ++test_expect_success 'missing timespec without -g or -v is rejected' '
     ++	test_must_fail test-tool chmtime file 2>err &&
     ++	test_grep "usage: test-tool chmtime" err
     ++'
     ++
     ++test_done


 t/helper/test-chmtime.c | 72 +++++++++++++++++++----------------------
 t/meson.build           |  1 +
 t/t0082-test-chmtime.sh | 22 +++++++++++++
 3 files changed, 57 insertions(+), 38 deletions(-)
 create mode 100755 t/t0082-test-chmtime.sh

diff --git a/t/helper/test-chmtime.c b/t/helper/test-chmtime.c
index 0e5538833a..cfcbca75a4 100644
--- a/t/helper/test-chmtime.c
+++ b/t/helper/test-chmtime.c
@@ -5,43 +5,46 @@
  *
  * The mtime can be changed to an absolute value:
  *
- *	test-tool chmtime =<seconds> file...
+ *  test-tool chmtime =<seconds> file...
  *
  * Relative to the current time as returned by time(3):
  *
- *	test-tool chmtime =+<seconds> (or =-<seconds>) file...
+ *  test-tool chmtime =+<seconds> (or =-<seconds>) file...
  *
  * Or relative to the current mtime of the file:
  *
- *	test-tool chmtime <seconds> file...
- *	test-tool chmtime +<seconds> (or -<seconds>) file...
+ *  test-tool chmtime <seconds> file...
+ *  test-tool chmtime +<seconds> (or -<seconds>) file...
  *
  * Examples:
  *
  * To print the mtime and the file name use --verbose and set
  * the file mtime offset to 0:
  *
- *	test-tool chmtime -v +0 file
+ *  test-tool chmtime -v +0 file
  *
  * To print only the mtime use --get:
  *
- *	test-tool chmtime --get file
+ *  test-tool chmtime --get file
  *
  * To set the mtime to current time:
  *
- *	test-tool chmtime =+0 file
+ *  test-tool chmtime =+0 file
  *
  * To set the file mtime offset to +1 and print the new value:
  *
- *	test-tool chmtime --get +1 file
+ *  test-tool chmtime --get +1 file
  *
  */
 #include "test-tool.h"
 #include "git-compat-util.h"
+#include "parse-options.h"
 #include <utime.h>
 
-static const char usage_str[] =
-	"(-v|--verbose|-g|--get) (+|=|=+|=-|-)<seconds> <file>...";
+static const char * const chmtime_usage[] = {
+	"test-tool chmtime [<options>] (+|=|=+|=-|-)<seconds> <file>...",
+	NULL
+};
 
 static int timespec_arg(const char *arg, long int *set_time, int *set_eq)
 {
@@ -71,37 +74,34 @@ int cmd__chmtime(int argc, const char **argv)
 	static int verbose;
 	static int get;
 
-	int i = 1;
+	int i = 0;
 	/* no mtime change by default */
 	int set_eq = 0;
 	long int set_time = 0;
 
-	if (argc < 3)
-		goto usage;
+	struct option options[] = {
+		OPT_BOOL('v', "verbose", &verbose, "print the mtime and the file name"),
+		OPT_BOOL('g', "get", &get, "print only the mtime"),
+		OPT_END()
+	};
 
-	if (strcmp(argv[i], "--get") == 0 || strcmp(argv[i], "-g") == 0) {
-		get = 1;
-		++i;
-	} else if (strcmp(argv[i], "--verbose") == 0 || strcmp(argv[i], "-v") == 0) {
-		verbose = 1;
-		++i;
-	}
+	argc = parse_options(argc, argv, NULL, options, chmtime_usage,
+			     PARSE_OPT_KEEP_UNKNOWN_OPT);
 
-	if (i == argc) {
-		goto usage;
-	}
+	die_for_incompatible_opt2(get, "--get", verbose, "--verbose");
+
+	if (argc < 1)
+		usage_with_options(chmtime_usage, options);
 
 	if (timespec_arg(argv[i], &set_time, &set_eq)) {
-		++i;
-	} else {
-		if (get == 0 && verbose == 0) {
-			fprintf(stderr, "Not a base-10 integer: %s\n", argv[i] + 1);
-			goto usage;
-		}
+		i++;
+	} else if (!get && !verbose) {
+		fprintf(stderr, "Not a base-10 integer: %s\n", argv[i] + 1);
+		usage_with_options(chmtime_usage, options);
 	}
 
 	if (i == argc)
-		goto usage;
+		usage_with_options(chmtime_usage, options);
 
 	for (; i < argc; i++) {
 		struct stat sb;
@@ -110,13 +110,13 @@ int cmd__chmtime(int argc, const char **argv)
 
 		if (stat(argv[i], &sb) < 0) {
 			fprintf(stderr, "Failed to stat %s: %s. Skipping\n",
-			        argv[i], strerror(errno));
+				argv[i], strerror(errno));
 			continue;
 		}
 
 #ifdef GIT_WINDOWS_NATIVE
 		if (!(sb.st_mode & S_IWUSR) &&
-				chmod(argv[i], sb.st_mode | S_IWUSR)) {
+		    chmod(argv[i], sb.st_mode | S_IWUSR)) {
 			fprintf(stderr, "Could not make user-writable %s: %s",
 				argv[i], strerror(errno));
 			return 1;
@@ -126,7 +126,7 @@ int cmd__chmtime(int argc, const char **argv)
 		utb.actime = sb.st_atime;
 		utb.modtime = set_eq ? set_time : sb.st_mtime + set_time;
 
-		mtime = utb.modtime < 0 ? 0: utb.modtime;
+		mtime = utb.modtime < 0 ? 0 : utb.modtime;
 		if (get) {
 			printf("%"PRIuMAX"\n", mtime);
 		} else if (verbose) {
@@ -150,14 +150,10 @@ int cmd__chmtime(int argc, const char **argv)
 			}
 #endif
 			fprintf(stderr, "Failed to modify time on %s: %s\n",
-			        argv[i], strerror(errno));
+				argv[i], strerror(errno));
 			return 1;
 		}
 	}
 
 	return 0;
-
-usage:
-	fprintf(stderr, "usage: %s %s\n", argv[0], usage_str);
-	return 1;
 }
diff --git a/t/meson.build b/t/meson.build
index 11b3752f00..004bda52c1 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -128,6 +128,7 @@ integration_tests = [
   't0071-sort.sh',
   't0080-unit-test-output.sh',
   't0081-find-pack.sh',
+  't0082-test-chmtime.sh',
   't0090-cache-tree.sh',
   't0091-bugreport.sh',
   't0092-diagnose.sh',
diff --git a/t/t0082-test-chmtime.sh b/t/t0082-test-chmtime.sh
new file mode 100755
index 0000000000..277df4ff1a
--- /dev/null
+++ b/t/t0082-test-chmtime.sh
@@ -0,0 +1,22 @@
+#!/bin/sh
+
+test_description='tests for test-tool chmtime'
+
+TEST_PASSES_SANITIZE_LEAK=true
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	echo hello >file
+'
+
+test_expect_success 'mutually exclusive options are rejected' '
+	test_must_fail test-tool chmtime --get --verbose +0 file 2>err &&
+	test_grep "cannot be used together" err
+'
+
+test_expect_success 'missing timespec without -g or -v is rejected' '
+	test_must_fail test-tool chmtime file 2>err &&
+	test_grep "usage: test-tool chmtime" err
+'
+
+test_done

base-commit: 7e34d8174dcda3435d1404c9a30bef46afe6c13e
-- 
gitgitgadget