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

[Junio C Hamano <gitster@pobox.com>]

Toon Claes <toon@iotcl.com> writes:

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