Re: [PATCH RFC v3 1/3] last-modified: new subcommand to show when files were last modified

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 2, 2025, 11:51 UTC
Message-ID: <xmqqa55mg6a7.fsf@gitster.g>
In-Reply-To: <20250630-toon-new-blame-tree-v3-1-3516025dc3bc@iotcl.com>

Toon Claes <toon@iotcl.com> writes:

Show 14 quoted lines

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

Show 19 quoted lines

> +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.