Skip to contentSkip to conversation

test-chmtime: use parse_options() to parse command-line flags

PATCH Superseded Shashank S via GitGitGadget 4 participants 10 messages Oct 27 – Nov 9 Arm/Amlogic Meson SoC support

lore
Share & tools
lore

Shashank S via GitGitGadget

author
#0 lore raw

PATCH 1/1 test-chmtime: use parse_options() to parse command-line flags

From: Shashank S Mogaveera <[email protected]>

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

Signed-off-by Shashank S Mogaveera <[email protected]>

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

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

 t/helper/test-chmtime.c | 46 +++++++++++++++++++----------------------
 1 file changed, 21 insertions(+), 25 deletions(-)
t/helper/test-chmtime.c +21 -25
index 0e5538833a..dc6281b1d6 100644
@@ -38,10 +38,13 @@
  */
 #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 (-v|--verbose|-g|--get) (+|=|=+|=-|-)<seconds> <file>...",
+	NULL
+};
 
 static int timespec_arg(const char *arg, long int *set_time, int *set_eq)
 {

@@ -68,40 +71,37 @@ 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;
 
-	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;
-	}
+	if (argc < 1 || (!get && !verbose && argc < 2))
+		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;
+			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,7 +110,7 @@ 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;
 		}
 

@@ -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;
 }

base-commit: 7e34d8174dcda3435d1404c9a30bef46afe6c13e

Signature · 1 line
gitgitgadget
#1 lore raw
Quoted 9 lines · Shashank S via GitGitGadget

From: Shashank S Mogaveera <[email protected]>

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

I somehow find the verb "refactor" used to describe a change like this slightly awkward, but it is excellent to reminder readers why it is necessary to use PARSE_OPT_KEEP_UNKNOWN_OPT.

Quoted 10 lines
 t/helper/test-chmtime.c | 46 +++++++++++++++++++----------------------
 1 file changed, 21 insertions(+), 25 deletions(-)
t/helper/test-chmtime.c +0 -0
index 0e5538833a..dc6281b1d6 100644
@@ -38,10 +38,13 @@
  */
 #include "test-tool.h"

t/helper/test-chmtime.c line 47

Quoted 10 lines
t/helper/test-chmtime.c +5 -2
 #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 (-v|--verbose|-g|--get) (+|=|=+|=-|-)<seconds> <file>...",
+	NULL
+};

Is it necessary to spell out the options like verbose and get, once you switch to the parse-options API? What does "test-tool chmtime -h" say with this patch applied? I think it is common to say [<options>] instead and let the list of options produced by the parse-options API to take it from there.

t/helper/test-chmtime.c static int timespec_arg(const char *arg, long int *set_time, int *set_eq) · line 75

Quoted 10 lines
t/helper/test-chmtime.c +2 -2
 static int timespec_arg(const char *arg, long int *set_time, int *set_eq)
 {

@@ -68,40 +71,37 @@ 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;

There is no reason why these two need to be static, but as a "main()" function for a run-once-and-exit program, there is no reason why they should not be static, either. With this change, you are forced to explicitly initialize them to 0.

Quoted 24 lines
t/helper/test-chmtime.c +0 -13
-	int i = 1;
 	/* no mtime change by default */
 	int set_eq = 0;
 	long int set_time = 0;
 
-	if (argc < 3)
-		goto usage;
 
-	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;
-	}
 
-	if (i == argc) {
-		goto usage;
-	}
 
 	if (timespec_arg(argv[i], &set_time, &set_eq)) {
 		++i;
 	} else {
 		if (get == 0 && verbose == 0) {

t/helper/test-chmtime.c static int timespec_arg(const char *arg, long int *set_time, int *set_eq) · line 107

Quoted 10 lines
t/helper/test-chmtime.c +0 -2
 			fprintf(stderr, "Not a base-10 integer: %s\n", argv[i] + 1);
-			goto usage;
 		}
 	}
 
 	if (i == argc)
-		goto usage;
 
 	for (; i < argc; i++) {
 		struct stat sb;

I agree that the original was horrible in that it did not allow -v to come before -g etc.

Now, let's read the updated code.

Quoted 7 lines
t/helper/test-chmtime.c +2 -0
+	int i = 0;
 
 	/* no mtime change by default */
 	int set_eq = 0;
 	long int set_time = 0;
 
+	struct option options[] = {

t/helper/test-chmtime.c static int timespec_arg(const char *arg, long int *set_time, int *set_eq) · line 92

Quoted 10 lines
t/helper/test-chmtime.c +8 -0
+		OPT_BOOL('v', "verbose", &verbose, "print the mtime and the file name"),
+		OPT_BOOL('g', "get", &get, "print only the mtime"),
+		OPT_END()
+	};
 
+	argc = parse_options(argc, argv, NULL, options, chmtime_usage,
+			     PARSE_OPT_KEEP_UNKNOWN_OPT);
 
+	if (argc < 1 || (!get && !verbose && argc < 2))
+		usage_with_options(chmtime_usage, options);

I am not sure what is going on here. The command, when parsing out the -g and -v options, expects to have <seconds> and 1 or more <file> arguments. Why should it reject argc==2 when neither -g nor -v is given, but allow argc==2 when -g and/or -v is?

OK, the usage of this command is a bit opaque. The command works in two ways.

* When -v/-g is given, it is a request to show the timestamp of the file, so <seconds> becomes optional (i.e., when given, it sets and then shows). We still want at least one <file>.

* Otherwise, it is a request to set the timestamp, so <seconds> is required, and we want at least one <file>.

Not the best way to design an end-user UI, but this is merely a test helper, so let's not complain too much ;-).

t/helper/test-chmtime.c static int timespec_arg(const char *arg, long int *set_time, int *set_eq) · line 101

Quoted 8 lines
t/helper/test-chmtime.c +1 -0
 	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);
+			usage_with_options(chmtime_usage, options);
 		}
 	}

This is largely the fault of the original, but I wonder if we can avoid the "without -v/-g, timespec is required" logic in two separate places like this (and the earlier one that puzzled me)?

t/helper/test-chmtime.c static int timespec_arg(const char *arg, long int *set_time, int *set_eq) · line 104

Quoted 2 lines
t/helper/test-chmtime.c +1 -0
 	if (i == argc)
+		usage_with_options(chmtime_usage, options);

And we want at least one <file> in any case.

t/helper/test-chmtime.c static int timespec_arg(const char *arg, long int *set_time, int *set_eq) · line 107

Quoted 2 lines
t/helper/test-chmtime.c +0 -0
 	for (; i < argc; i++) {
 		struct stat sb;

OK. We will iterate over the <file> arguments given.

t/helper/test-chmtime.c int cmd__chmtime(int argc, const char **argv) · line 115

Quoted 8 lines
t/helper/test-chmtime.c +1 -1
@@ -110,7 +110,7 @@ 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;
 		}

This fixes a whitespace breakage, which is an unrelated change, but I'll let it pass.

t/helper/test-chmtime.c int cmd__chmtime(int argc, const char **argv) · line 156

Quoted 9 lines
t/helper/test-chmtime.c +1 -1
@@ -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;
 		}
 	}

Likewise.

t/helper/test-chmtime.c int cmd__chmtime(int argc, const char **argv) · line 159

Quoted 7 lines
t/helper/test-chmtime.c +0 -4
 	return 0;
-
-usage:
-	fprintf(stderr, "usage: %s %s\n", argv[0], usage_str);
-	return 1;
 }

We exit via usage_with_options() with 129 now, but used to exit(1) upon a command line error.

Looking good. Thanks.

#2 lore raw

On Wed, 28 Oct 2026 at 00:08, Junio C Hamano <[email protected]> wrote:

Thank you for the detailed review and for pointing these out!

Quoted 6 lines
Quoted 1 line

and remain in argv for timespec_arg() to parse.

I somehow find the verb "refactor" used to describe a change like this slightly awkward, but it is excellent to reminder readers why it is necessary to use PARSE_OPT_KEEP_UNKNOWN_OPT.

Understood! In my v2, I will change the commit message to use "update" instead of "refactor".

Quoted 14 lines
Quoted 14 lines
 t/helper/test-chmtime.c | 46 +++++++++++++++++++----------------------
 1 file changed, 21 insertions(+), 25 deletions(-)
t/helper/test-chmtime.c +1 -0
index 0e5538833a..dc6281b1d6 100644
@@ -38,10 +38,13 @@
  */
 #include "test-tool.h"
 #include "git-compat-util.h"
+#include "parse-options.h"
 #include <utime.h>
Quoted 13 lines

t/helper/test-chmtime.c line 47

Quoted 6 lines
t/helper/test-chmtime.c +4 -2
-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>...",
+     NULL
+};

Is it necessary to spell out the options like verbose and get, once you switch to the parse-options API? What does "test-tool chmtime -h" say with this patch applied? I think it is common to say [<options>] instead and let the list of options produced by the parse-options API to take it from there.

You are completely right. Hardcoding the flags in the string is redundant now that parse_options() handles the -h output. I will update the usage string to [<options>] in v2.

Quoted 4 lines
Quoted 4 lines
t/helper/test-chmtime.c +0 -0
 static int timespec_arg(const char *arg, long int *set_time, int *set_eq)
 {

@@ -68,40 +71,37 @@ static int timespec_arg(const char *arg, long int *set_time, int *set_eq)
Quoted 12 lines

t/helper/test-chmtime.c static int timespec_arg(const char *arg, long int *set_time, int *set_eq) · line 75

Quoted 6 lines
t/helper/test-chmtime.c +2 -2
 int cmd__chmtime(int argc, const char **argv)
 {
-     static int verbose;
-     static int get;
+     int verbose = 0;
+     int get = 0;

There is no reason why these two need to be static, but as a "main()" function for a run-once-and-exit program, there is no reason why they should not be static, either. With this change, you are forced to explicitly initialize them to 0.

That is a great point about the automatic initialization of static variables. Since it isn't strictly necessary to change them for parse_options() to work, I will revert this specific change in v2 to keep the diff as small and focused as possible.

Quoted 70 lines
Quoted 24 lines
t/helper/test-chmtime.c +0 -13
-     int i = 1;
      /* no mtime change by default */
      int set_eq = 0;
      long int set_time = 0;
 
-     if (argc < 3)
-             goto usage;
 
-     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;
-     }
 
-     if (i == argc) {
-             goto usage;
-     }
 
      if (timespec_arg(argv[i], &set_time, &set_eq)) {
              ++i;
      } else {
              if (get == 0 && verbose == 0) {

t/helper/test-chmtime.c static int timespec_arg(const char *arg, long int *set_time, int *set_eq) · line 107

Quoted 10 lines
t/helper/test-chmtime.c +0 -2
                      fprintf(stderr, "Not a base-10 integer: %s\n", argv[i] + 1);
-                     goto usage;
              }
      }
 
      if (i == argc)
-             goto usage;
 
      for (; i < argc; i++) {
              struct stat sb;

I agree that the original was horrible in that it did not allow -v to come before -g etc.

Now, let's read the updated code.

Quoted 7 lines
t/helper/test-chmtime.c +2 -0
+     int i = 0;
 
      /* no mtime change by default */
      int set_eq = 0;
      long int set_time = 0;
 
+     struct option options[] = {

t/helper/test-chmtime.c static int timespec_arg(const char *arg, long int *set_time, int *set_eq) · line 92

Quoted 10 lines
t/helper/test-chmtime.c +8 -0
+             OPT_BOOL('v', "verbose", &verbose, "print the mtime and the file name"),
+             OPT_BOOL('g', "get", &get, "print only the mtime"),
+             OPT_END()
+     };
 
+     argc = parse_options(argc, argv, NULL, options, chmtime_usage,
+                          PARSE_OPT_KEEP_UNKNOWN_OPT);
 
+     if (argc < 1 || (!get && !verbose && argc < 2))
+             usage_with_options(chmtime_usage, options);

I am not sure what is going on here. The command, when parsing out the -g and -v options, expects to have <seconds> and 1 or more <file> arguments. Why should it reject argc==2 when neither -g nor -v is given, but allow argc==2 when -g and/or -v is?

OK, the usage of this command is a bit opaque. The command works in two ways.

* When -v/-g is given, it is a request to show the timestamp of the file, so <seconds> becomes optional (i.e., when given, it sets and then shows). We still want at least one <file>.

Quoted 6 lines

* Otherwise, it is a request to set the timestamp, so <seconds> is required, and we want at least one <file>.

Not the best way to design an end-user UI, but this is merely a test helper, so let's not complain too much ;-).

I completely agree, the original logic was very opaque. By removing the initial (!get && !verbose && argc < 2) check, we can rely entirely on the timespec_arg() check to validate the <seconds> argument, and then simply use if (i == argc) at the end to ensure <file> arguments were provided. I will clean this flow up significantly in v2.

Quoted 49 lines
Quoted 8 lines
      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);
+                     usage_with_options(chmtime_usage, options);
              }
      }

This is largely the fault of the original, but I wonder if we can avoid the "without -v/-g, timespec is required" logic in two separate places like this (and the earlier one that puzzled me)?

Quoted 2 lines
      if (i == argc)

+ usage_with_options(chmtime_usage, options);

And we want at least one <file> in any case.

Quoted 2 lines
      for (; i < argc; i++) {
              struct stat sb;

OK. We will iterate over the <file> arguments given.

t/helper/test-chmtime.c int cmd__chmtime(int argc, const char **argv) · line 115

Quoted 8 lines
t/helper/test-chmtime.c +1 -1
@@ -110,7 +110,7 @@ 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;
              }

This fixes a whitespace breakage, which is an unrelated change, but I'll let it pass.

t/helper/test-chmtime.c int cmd__chmtime(int argc, const char **argv) · line 156

Quoted 9 lines
t/helper/test-chmtime.c +1 -1
@@ -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;
              }
      }

Likewise.

Quoted 1 line
Quoted 10 lines

t/helper/test-chmtime.c int cmd__chmtime(int argc, const char **argv) · line 159

Quoted 6 lines
      return 0;
t/helper/test-chmtime.c +0 -4
-
-usage:
-     fprintf(stderr, "usage: %s %s\n", argv[0], usage_str);
-     return 1;
 }

We exit via usage_with_options() with 129 now, but used to exit(1) upon a command line error.

which is a nice bonus of migrating to the parse-options API.

Quoted 1 line

Looking good. Thanks.

Thanks
Shashank S Mogaveera

#3 lore raw
In reply to Shashank S via GitGitGadget
Quoted 3 lines · Shashank S via GitGitGadget
 t/helper/test-chmtime.c | 46 +++++++++++++++++++----------------------
 1 file changed, 21 insertions(+), 25 deletions(-)

Forgot to mention one thing. Do we have any tests to cover the behaviour of this test helper? If not, should we add some (not a rhetorical question)?

Also, one thing I missed is that the original ignored --verbose when --get was given, since they contradict with each other. Shouldn't we make them explicitly incompatible with each other, perhaps before where we call timespec_arg()?

t/helper/test-chmtime.c static int timespec_arg(const char *arg, long int *set_time, int *set_eq) · line 92

Quoted 10 lines
t/helper/test-chmtime.c +4 -0
@@ -68,40 +71,37 @@ static int timespec_arg(const char *arg, long int *set_time, int *set_eq)
 
 int cmd__chmtime(int argc, const char **argv)
 {
...
+	argc = parse_options(argc, argv, NULL, options, chmtime_usage,
+			     PARSE_OPT_KEEP_UNKNOWN_OPT);
 
+	if (argc < 1 || (!get && !verbose && argc < 2))
+		usage_with_options(chmtime_usage, options);

	die_for_incompatible_opt2(get, "--get", verbose, "--verbose");
Quoted 6 lines
t/helper/test-chmtime.c +0 -0
 	if (timespec_arg(argv[i], &set_time, &set_eq)) {
 		++i;
...
 }

base-commit: 7e34d8174dcda3435d1404c9a30bef46afe6c13e

#4 lore raw
Quoted 7 lines · Junio C Hamano
Quoted 3 lines · Shashank S via GitGitGadget
 t/helper/test-chmtime.c | 46 +++++++++++++++++++----------------------
 1 file changed, 21 insertions(+), 25 deletions(-)
Quoted 9 lines

Forgot to mention one thing. Do we have any tests to cover the behaviour of this test helper? If not, should we add some (not a rhetorical question)?

Also, one thing I missed is that the original ignored --verbose when --get was given, since they contradict with each other. Shouldn't we make them explicitly incompatible with each other, perhaps before where we call timespec_arg()?

This is a great catch. While test-chmtime is heavily utilized in scripts like t5304-prune.sh, there are no tests covering the helper's actual argument parsing.

In my v2, I will add die_for_incompatible_opt2() immediately after parse_options(), and I will include a second patch in the series that adds a dedicated t/t00xx-test-chmtime.sh script to explicitly test the --get/--verbose incompatibility and the standard usage.

Thanks again for the excellent guidance

Shashank s mogaveera

author
#5 lore raw

PATCH v2 1/1 test-chmtime: use parse_options() to parse command-line flags

In reply to Shashank S via GitGitGadget

From: Shashank S Mogaveera <[email protected]>

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 <[email protected]>

---
[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 <[email protected]>

## 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
t/helper/test-chmtime.c +34 -38
index 0e5538833a..cfcbca75a4 100644
@@ -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;
 }
t/meson.build +1 -0
index 11b3752f00..004bda52c1 100644
@@ -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',
t/t0082-test-chmtime.sh new file +22 -0
new file mode 100755
index 0000000000..277df4ff1a
@@ -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

Signature · 1 line
gitgitgadget
#6 lore raw

Hi Shashank,

I built v2 (43c96de13e) with NO_RUST=1 and compared it with a build of the base commit (7e34d8174d). t0082 passes 3 of 3. Thanks for taking Junio's three suggestions from v1.

Quoted 3 lines

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.

No test covers this. With that argument set to 0, t0082 still passed 3 of 3, while "test-tool chmtime --get -86400 file" failed with "unknown switch `8'" (exit 129). Could you add a test like this one? It passes with v2 and fails without the flag:

test_expect_success 'negative offset is not taken as an option' '
test-tool chmtime =1000000000 file &&
test-tool chmtime --get -86400 file >actual && echo 999913600 >expect &&
test_cmp expect actual
'

Compared with the base build, I also saw three changes that the commit message does not mention:

- "+0 --get f": base tried to stat "--get"; v2 accepts the flag after the timespec and prints the mtime.
- "-g -v f": base tried to stat "-v" [and still printed the mtime]

    and exited 0; v2 dies (exit 128).

- usage errors exit with 129 instead of 1.

Could you mention them in the commit message, or let me know if any of them is not intended?

Cynthia

#7 lore raw

On Mon, 2 Nov 2026 at 08:17, Cynthia Uche <[email protected]> wrote:

Hi Cynthia,

Thank you so much for taking the time to test and review this

Quoted 7 lines

Hi Shashank
No test covers this. With that argument set to 0, t0082 still passed 3 of 3, while "test-tool chmtime --get -86400 file" failed with "unknown switch `8'" (exit 129). Could you add a test like this one? It passes with v2 and fails without the flag:

Quoted 7 lines

test_expect_success 'negative offset is not taken as an option' '

        test-tool chmtime =1000000000 file &&
        test-tool chmtime --get -86400 file >actual &&
        echo 999913600 >expect &&
        test_cmp expect actual

'

This is an excellent catch. Adding a dedicated test to prevent a regression here is a great idea, and I appreciate you providing the snippet. I will include this test in the v3 iteration.

Quoted 12 lines

Compared with the base build, I also saw three changes that the commit message does not mention:

- "+0 --get f": base tried to stat "--get"; v2 accepts the flag after the timespec and prints the mtime.
- "-g -v f": base tried to stat "-v" [and still printed the mtime]

    and exited 0; v2 dies (exit 128).

- usage errors exit with 129 instead of 1.

Could you mention them in the commit message, or let me know if any of them is not intended?

All of these behavioral shifts are intended! Since parse_options() handles arguments independent of their position and uses standard error codes (129 for usage, 128 for die), the tool now behaves much more like a standard Git command. You are completely right that I should document these side-effects explicitly in the commit message.

I will add a breakdown of these behavioral improvements to the commit message in v3. Thanks again for the meticulous review!

Shashank S Mogaveera

author
#8 lore raw

PATCH v3 1/1 test-chmtime: use parse_options() to parse command-line flags

In reply to Shashank S via GitGitGadget

From: Shashank S Mogaveera <[email protected]>

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.

This migration also introduces the following intended behavioral improvements over the original manual parsing:
- Position-independent flags: Flags can now be safely passed after the timespec (e.g., +0 --get file now works, whereas it previously tried to stat a file literally named --get).
- Incompatible flags strictly abort: Passing both -g and -v now triggers a fatal error (exit 128), whereas it previously tried to stat -v and exited 0.
- Standardized error codes: Usage errors now properly exit with 129 instead of 1.

Signed-off-by Shashank S Mogaveera <[email protected]>

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

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

Range-diff vs v2:

1: 43c96de13e ! 1: 512b17b39a test-chmtime: use parse_options() to parse command-line flags
@@ Commit message
and introduce a new test script to verify both this restriction and the standard usage.

+ This migration also introduces the following intended behavioral
+ improvements over the original manual parsing:
+ - Position-independent flags: Flags can now be safely passed after the
+ timespec (e.g., +0 --get file now works, whereas it previously tried
+ to stat a file literally named --get).
+ - Incompatible flags strictly abort: Passing both -g and -v now
+ triggers a fatal error (exit 128), whereas it previously tried to stat
+ -v and exited 0.
+ - Standardized error codes: Usage errors now properly exit with 129
+ instead of 1.
+

Signed-off-by Shashank S Mogaveera <[email protected]>

## t/helper/test-chmtime.c ##
@@ t/t0082-test-chmtime.sh (new)
+ test_grep "usage: test-tool chmtime" err
+'
+
++test_expect_success 'negative offset is not taken as an option' '
++ test-tool chmtime =1000000000 file &&
++ test-tool chmtime --get -86400 file >actual &&
++ echo 999913600 >expect &&
++ test_cmp expect actual
++'
++
+test_done

 t/helper/test-chmtime.c | 72 +++++++++++++++++++----------------------
 t/meson.build           |  1 +
 t/t0082-test-chmtime.sh | 29 +++++++++++++++++
 3 files changed, 64 insertions(+), 38 deletions(-)
 create mode 100755 t/t0082-test-chmtime.sh
t/helper/test-chmtime.c +34 -38
index 0e5538833a..cfcbca75a4 100644
@@ -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;
 }
t/meson.build +1 -0
index 11b3752f00..004bda52c1 100644
@@ -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',
t/t0082-test-chmtime.sh new file +29 -0
new file mode 100755
index 0000000000..f1a9d6c466
@@ -0,0 +1,29 @@
+#!/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_expect_success 'negative offset is not taken as an option' '
+	test-tool chmtime =1000000000 file &&
+	test-tool chmtime --get -86400 file >actual &&
+	echo 999913600 >expect &&
+	test_cmp expect actual
+'
+
+test_done

base-commit: 7e34d8174dcda3435d1404c9a30bef46afe6c13e

Signature · 1 line
gitgitgadget
#9 lore raw

Hi Shashank,

I applied v3 on master (fb255012ac). t0082 passes 4 of 4, and the negative-offset test fails if I set the PARSE_OPT_KEEP_UNKNOWN_OPT argument to 0, so it guards that flag. The behaviour changes listed in the commit message match what I see against a build of master.

Looks good. Thanks for adding the test and the commit message text.

Signature · 1 line
Cynthia