Re: [PATCH RFC v3 1/3] last-modified: new subcommand to show when files were last modified
From: Junio C Hamano <hidden>
Date: 2025-07-02 11:51:14
Toon Claes [off-list ref] writes:
quoted hunk
diff --git a/builtin/last-modified.c b/builtin/last-modified.c new file mode 100644 index 0000000000..4ff058c302 --- /dev/null +++ b/builtin/last-modified.c@@ -0,0 +1,44 @@ +#include "git-compat-util.h" +#include "last-modified.h" +#include "hex.h" +#include "quote.h" +#include "config.h" +#include "object-name.h" +#include "parse-options.h" +#include "builtin.h"
Apparently "parse-options.h" is included but is never used. How much of these include do you truly use in this step? I was looking at the code, since I was wondering why you forgot to handle "-h", which comes absolutely for free when you use the parse-options API in the most natural way.
+int cmd_last_modified(int argc,
+ const char **argv,
+ const char *prefix,
+ struct repository *repo)
+{
+ struct last_modified lm;
+
+ repo_config(repo, git_default_config, NULL);
+
+ if (last_modified_init(&lm, repo, prefix, argc, argv))
+ die(_("error setting up last-modified traversal"));
+
+ if (last_modified_run(&lm, show_entry, &lm) < 0)
+ die(_("error running last-modified traversal"));
+
+ last_modified_release(&lm);
+
+ return 0;
+}It is a bit unusual for the top-legvel cmd_foo() to totally give up the responsibility of command line parsing, and let a helper function take over everything. Is the idea that the family of last_modified_foo() functions wants to form a library-ish API? I think the primary reason I find the arrangement a bit unusual is that such a library interface would not deal with end-user interactions like command line parsing. Even commands that let setup_revisions() slurp the command line arguments typically does necessary set-up (like discoverying the git directory and reading the configuration files) on the side of the caller.