From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 21 Oct 2025 11:43:44 GMT
Subject: Re: [PATCH v4 06/12] builtin/history: implement "reword" subcommand
Message-ID: <aPdx8Pv-9UQDpe7I@pks.im>
In-Reply-To: <CAOLa=ZSU8yr9Gn0EZ7x705qPyVM-qiMjgMCNCb8p8SMGTToxqQ@mail.gmail.com>

On Tue, Oct 14, 2025 at 07:04:06AM -0400, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > diff --git a/builtin/history.c b/builtin/history.c
> > index f6fe32610b..7b2a0023e8 100644
> > --- a/builtin/history.c
> > +++ b/builtin/history.c
> > @@ -1,22 +1,389 @@
> > +#define USE_THE_REPOSITORY_VARIABLE
> > +
> >  #include "builtin.h"
> > +#include "commit-reach.h"
> > +#include "commit.h"
> > +#include "config.h"
> > +#include "editor.h"
> > +#include "environment.h"
> >  #include "gettext.h"
> > +#include "hex.h"
> > +#include "oidmap.h"
> 
> Nit: This can be dropped, perhaps needed in a future patch?

Yeah, it's indeed needed in a subsequent patch. Let me move the import
around.

> >  #include "parse-options.h"
> > +#include "refs.h"
> > +#include "replay.h"
> > +#include "reset.h"
> > +#include "revision.h"
> > +#include "sequencer.h"
> > +#include "strvec.h"
> > +#include "tree.h"
> > +#include "wt-status.h"
> > +
> > +static int collect_commits(struct repository *repo,
> > +			   struct commit *old_commit,
> > +			   struct commit *new_commit,
> > +			   struct strvec *out)
> > +{
> > +	struct setup_revision_opt revision_opts = {
> > +		.assume_dashdash = 1,
> > +	};
> > +	struct strvec revisions = STRVEC_INIT;
> > +	struct commit_list *from_list = NULL;
> > +	struct commit *child;
> > +	struct rev_info rev = { 0 };
> > +	int ret;
> > +
> > +	/*
> > +	 * Check that the old commit actually is an ancestor of HEAD. If not
> > +	 * the whole request becomes nonsensical.
> > +	*/
> 
> Missing space here

Good eyes.

> > +	if (old_commit) {
> > +		commit_list_insert(old_commit, &from_list);
> > +		if (!repo_is_descendant_of(repo, new_commit, from_list)) {
> > +			ret = error(_("commit must be reachable from current HEAD commit"));
> > +			goto out;
> > +		}
> > +	}
> 
> Makes sense. There is an inherent assumption using the 'git history'
> command that you want to modify the history of the current reference.
> 
> One question, wouldn't it make sense to parse and check that the commit
> to be reworded should be checked to be a descendant of HEAD earlier on
> in `cmd_history_reword()`?
> 
> This would ensure this function `collect_commits()` doesn't worry about
> how it is meant to be used, and simply worries about collecting commits.

The reason why I opted to move this into `collect_commits()` is so that
we don't have to reimplement that check for every single subcommand, as
they also have the same restriction.

[snip]
> > +static int fill_commit_message(struct repository *repo,
> > +			       const struct object_id *old_tree,
> > +			       const struct object_id *new_tree,
> > +			       const char *default_message,
> > +			       const char *provided_message,
> > +			       const char *action,
> > +			       struct strbuf *out)
> > +{
> > +	if (!provided_message) {
> > +		const char *path = git_path_commit_editmsg();
> > +		const char *hint =
> > +			_("Please enter the commit message for the %s changes. Lines starting\n"
> > +     "with '%s' will be kept; you may remove them yourself if you want to.\n");
> 
> Shouldn't this be s/kept/removed? Also this line needs to be aligned.

Huh, yes, indeed.

[snip]
> > +	if (repo_parse_commit(repo, original_commit)) {
> > +		ret = error(_("unable to parse commit %s"),
> > +			    oid_to_hex(&original_commit->object.oid));
> > +		goto out;
> > +	}
> 
> Isn't this already done as part of
> `lookup_commit_reference_by_name_gently()` which is called by
> `lookup_commit_reference_by_name()` ?

Yes, you're right.

[snip]
> > +	ret = commit_tree(final_message.buf, final_message.len,
> > +			  &repo_get_commit_tree(repo, original_commit)->object.oid,
> 
> Can't we use original_commit_tree_oid here?

Yup, indeed.

> >  		const char **argv,
> >  		const char *prefix,
> > -		struct repository *repo UNUSED)
> > +		struct repository *repo)
> >  {
> >  	const char * const usage[] = {
> >  		N_("git history [<options>]"),
> > +		N_("git history reword [<options>] <commit>"),
> 
> This string is used twice, perhaps we move it to a macro?

Yeah, why not.

Patrick