Toon Claes <toon@xxxxxxxxx> 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.
