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

[Patrick Steinhardt <ps@pks.im>]

On Fri, May 23, 2025 at 11:33:48AM +0200, Toon Claes wrote:
> diff --git a/Documentation/git-last-modified.adoc b/Documentation/git-last-modified.adoc
> new file mode 100644
> index 0000000000..1af38f402e
> --- /dev/null
> +++ b/Documentation/git-last-modified.adoc
> @@ -0,0 +1,49 @@
> +git-last-modified(1)
> +====================
> +
> +NAME
> +----
> +git-last-modified - EXPERIMENTAL: Show when files were last modified

Nit: we don't have the EXPERIMENTAL label here for git-switch(1) or
git-restore(1).

> +
> +
> +SYNOPSIS
> +--------
> +[synopsis]
> +git last-modified [-r] [<revision-range>] [[--] <path>...]
> +
> +DESCRIPTION
> +-----------
> +
> +Shows which commit last modified each of the relevant files and subdirectories.
> +
> +THIS COMMAND IS EXPERIMENTAL. THE BEHAVIOR MAY CHANGE.
> +
> +OPTIONS
> +-------
> +
> +-r::
> +	Recurse into subtrees.
> +
> +-t::
> +	Show tree entry itself as well as subtrees.  Implies `-r`.

These flags aren't yet supported in this version, are they?

> diff --git a/builtin/last-modified.c b/builtin/last-modified.c
> new file mode 100644
> index 0000000000..0d4733f666
> --- /dev/null
> +++ b/builtin/last-modified.c
> @@ -0,0 +1,43 @@
> +#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"
> +
> +static void show_entry(const char *path, const struct commit *commit, void *d)
> +{
> +	struct last_modified *lm = d;
> +
> +	if (commit->object.flags & BOUNDARY)
> +		putchar('^');
> +	printf("%s\t", oid_to_hex(&commit->object.oid));
> +
> +	if (lm->rev.diffopt.line_termination)
> +		write_name_quoted(path, stdout, '\n');
> +	else
> +		printf("%s%c", path, '\0');
> +
> +	fflush(stdout);
> +}
> +
> +int cmd_last_modified(int argc,
> +		   const char **argv,
> +		   const char *prefix,
> +		   struct repository *repo)
> +{
> +	int ret = 0;

`ret` is basically unused here, we only use it to return 0.

> diff --git a/last-modified.c b/last-modified.c
> new file mode 100644
> index 0000000000..9283f8fcae
> --- /dev/null
> +++ b/last-modified.c
> @@ -0,0 +1,213 @@
> +#include "git-compat-util.h"
> +#include "last-modified.h"
> +#include "commit.h"
> +#include "diffcore.h"
> +#include "diff.h"
> +#include "object.h"
> +#include "revision.h"
> +#include "repository.h"
> +#include "log-tree.h"
> +
> +struct last_modified_entry {
> +	struct hashmap_entry hashent;
> +	struct object_id oid;
> +	struct commit *commit;
> +	const char path[FLEX_ARRAY];
> +};
> +
> +static void add_from_diff(struct diff_queue_struct *q,
> +			  struct diff_options *opt UNUSED,
> +			  void *data)
> +{
> +	struct last_modified *lm = data;
> +
> +	for (int i = 0; i < q->nr; i++) {
> +		struct diff_filepair *p = q->queue[i];
> +		struct last_modified_entry *ent;
> +		const char *path = p->two->path;
> +
> +		FLEX_ALLOC_STR(ent, path, path);
> +		oidcpy(&ent->oid, &p->two->oid);
> +		hashmap_entry_init(&ent->hashent, strhash(ent->path));
> +		hashmap_add(&lm->paths, &ent->hashent);
> +	}
> +}
> 
> +static int add_from_revs(struct last_modified *lm)
> +{
> +	size_t count = 0;
> +	struct diff_options diffopt;
> +
> +	memcpy(&diffopt, &lm->rev.diffopt, sizeof(diffopt));
> +	copy_pathspec(&diffopt.pathspec, &lm->rev.diffopt.pathspec);
> +	diffopt.output_format = DIFF_FORMAT_CALLBACK;
> +	diffopt.format_callback = add_from_diff;
> +	diffopt.format_callback_data = lm;

As far as I understand we populate `paths` from the diff here, and
`paths` later on acts as a filter of paths we're interested in? Might be
nice to add a comment explaining the intent of this.

> +	for (size_t i = 0; i < lm->rev.pending.nr; i++) {
> +		struct object_array_entry *obj = lm->rev.pending.objects + i;
> +
> +		if (obj->item->flags & UNINTERESTING)
> +			continue;
> +
> +		if (count++)
> +			return error(_("can only get last-modified one tree at a time"));

It's a bit funny that `count` is pretending to be a counter even though
it ultimately is only a boolean flag whether we have already seen an
interesting item.

> +		diff_tree_oid(lm->rev.repo->hash_algo->empty_tree,
> +			      &obj->item->oid, "", &diffopt);
> +		diff_flush(&diffopt);
> +	}
> +	clear_pathspec(&diffopt.pathspec);

Shouldn't we call `diff_free()` instead of `clear_pathspec` to clear the
whole `struct diff_options`?

> +
> +	return 0;
> +}
> +
> +static int last_modified_entry_hashcmp(const void *unused UNUSED,
> +				    const struct hashmap_entry *hent1,
> +				    const struct hashmap_entry *hent2,
> +				    const void *path)
> +{
> +	const struct last_modified_entry *ent1 =
> +		container_of(hent1, const struct last_modified_entry, hashent);
> +	const struct last_modified_entry *ent2 =
> +		container_of(hent2, const struct last_modified_entry, hashent);
> +	return strcmp(ent1->path, path ? path : ent2->path);
> +}
> +
> +void last_modified_init(struct last_modified *lm,
> +		     struct repository *r,
> +		     const char *prefix,
> +		     int argc, const char **argv)
> +{
> +	memset(lm, 0, sizeof(*lm));
> +	hashmap_init(&lm->paths, last_modified_entry_hashcmp, NULL, 0);
> +
> +	repo_init_revisions(r, &lm->rev, prefix);
> +	lm->rev.def = "HEAD";
> +	lm->rev.combine_merges = 1;
> +	lm->rev.show_root_diff = 1;
> +	lm->rev.boundary = 1;
> +	lm->rev.no_commit_id = 1;
> +	lm->rev.diff = 1;
> +	if (setup_revisions(argc, argv, &lm->rev, NULL) > 1)
> +		die(_("unknown last-modified argument: %s"), argv[1]);
> +
> +	if (add_from_revs(lm) < 0)
> +		die(_("unable to setup last-modified"));

Given that this is library code, do we rather want to have
`last_modified_init()` return an error code and let the caller die?

> +}
> +
> +void last_modified_release(struct last_modified *lm)
> +{
> +	hashmap_clear_and_free(&lm->paths, struct last_modified_entry, hashent);
> +	release_revisions(&lm->rev);
> +}
> +
> +struct last_modified_callback_data {
> +	struct commit *commit;
> +	struct hashmap *paths;
> +
> +	last_modified_callback callback;
> +	void *callback_data;
> +};
> +
> +static void mark_path(const char *path, const struct object_id *oid,
> +		      struct last_modified_callback_data *data)
> +{
> +	struct last_modified_entry *ent;
> +
> +	/* Is it even a path that we are interested in? */
> +	ent = hashmap_get_entry_from_hash(data->paths, strhash(path), path,
> +					  struct last_modified_entry, hashent);
> +	if (!ent)
> +		return;

Yup, so this here is the filter to figure out whether we care for a
path, which uses the `paths` map we have populated at the beginning.

> +	/* Have we already found a commit? */
> +	if (ent->commit)
> +		return;

Can this case even be hit? We remove the entry from the map once we have
seen it, so I'd expect that we never hit the same commit map entry
twice. If so, can this be converted to a `BUG()` or am I missing the
obvious?

> +	/*
> +	 * Is it arriving at a version of interest, or is it from a side branch
> +	 * which did not contribute to the final state?
> +	 */
> +	if (!oideq(oid, &ent->oid))
> +		return;
> +
> +	ent->commit = data->commit;
> +	if (data->callback)
> +		data->callback(path, data->commit, data->callback_data);
> +
> +	hashmap_remove(data->paths, &ent->hashent, path);

And we end up removing that entry from paths so that we don't revisit it
in the future. After all, we're only interested in a single commit per
path.

> +	free(ent);
> +}
> +
> +static void last_modified_diff(struct diff_queue_struct *q,
> +		       struct diff_options *opt UNUSED, void *cbdata)
> +{
> +	struct last_modified_callback_data *data = cbdata;
> +
> +	for (int i = 0; i < q->nr; i++) {
> +		struct diff_filepair *p = q->queue[i];
> +		switch (p->status) {
> +		case DIFF_STATUS_DELETED:
> +			/*
> +			 * There's no point in feeding a deletion, as it could
> +			 * not have resulted in our current state, which
> +			 * actually has the file.
> +			 */
> +			break;
> +
> +		default:
> +			/*
> +			 * Otherwise, we care only that we somehow arrived at
> +			 * a final path/sha1 state. Note that this covers some
> +			 * potentially controversial areas, including:
> +			 *
> +			 *  1. A rename or copy will be found, as it is the
> +			 *     first time the content has arrived at the given
> +			 *     path.
> +			 *
> +			 *  2. Even a non-content modification like a mode or
> +			 *     type change will trigger it.

Curious, but sensible. We're looking for the last time a specific tree
entry was changed, and that of course includes modifications. I could
totally see that we may eventually want to add a flag that ignores such
mode changes and only presents content changes. But for now I agree that
this is sensible.

> +			 * We take the inclusive approach for now, and find
> +			 * anything which impacts the path. Options to tweak
> +			 * the behavior (e.g., to "--follow" the content across
> +			 * renames) can come later.
> +			 */
> +			mark_path(p->two->path, &p->two->oid, data);
> +			break;
> +		}
> +	}
> +}
> +
> +int last_modified_run(struct last_modified *lm, last_modified_callback cb, void *cbdata)
> +{
> +	struct last_modified_callback_data data;
> +
> +	data.paths = &lm->paths;
> +	data.callback = cb;
> +	data.callback_data = cbdata;
> +
> +	lm->rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
> +	lm->rev.diffopt.format_callback = last_modified_diff;
> +	lm->rev.diffopt.format_callback_data = &data;
> +
> +	prepare_revision_walk(&lm->rev);
> +
> +	while (hashmap_get_size(&lm->paths)) {

Okay, and this is the core of our logic: we continue walking the tree
until there are no more paths that we care about.

> diff --git a/last-modified.h b/last-modified.h
> new file mode 100644
> index 0000000000..42a819d979
> --- /dev/null
> +++ b/last-modified.h
> @@ -0,0 +1,27 @@
> +#ifndef LAST_MODIFIED_H
> +#define LAST_MODIFIED_H
> +
> +#include "commit.h"
> +#include "revision.h"
> +#include "hashmap.h"
> +
> +struct last_modified {
> +	struct hashmap paths;
> +	struct rev_info rev;
> +};
> +
> +void last_modified_init(struct last_modified *lm,
> +		     struct repository *r,
> +		     const char *prefix,
> +		     int argc, const char **argv);
> +
> +void last_modified_release(struct last_modified *);
> +
> +typedef void (*last_modified_callback)(const char *path,
> +				    const struct commit *commit,
> +				    void *data);
> +int last_modified_run(struct last_modified *lm,
> +		   last_modified_callback cb,
> +		   void *cbdata);
> +#endif /* LAST_MODIFIED_H */

It would be nice to have some documentation for each of these functions
as well as a bit of a higher-level conceptual info.