From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 21 Oct 2025 11:43:39 GMT
Subject: Re: [PATCH v4 05/12] builtin: add new "history" command
Message-ID: <aPdx68-hxXBBKOc6@pks.im>
In-Reply-To: <CAOLa=ZQSc0jHv9gK5T06GA_mprknucwCj5H_M2djZ-UWfRMbJQ@mail.gmail.com>

On Tue, Oct 14, 2025 at 05:07:03AM -0400, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > new file mode 100644
> > index 0000000000..1537960374
> > --- /dev/null
> > +++ b/Documentation/git-history.adoc
> > @@ -0,0 +1,45 @@
> > +git-history(1)
> > +==============
> > +
> > +NAME
> > +----
> > +git-history - EXPERIMENTAL: Rewrite history of the current branch
> > +
> > +SYNOPSIS
> > +--------
> > +[synopsis]
> > +git history [<options>]
> > +
> > +DESCRIPTION
> > +-----------
> > +
> > +Rewrite history by rearranging or modifying specific commits in the
> > +history.
> > +
> > +This command is similar to linkgit:git-rebase[1] and uses the same
> > +underlying machinery. You should use rebases if you either want to
> > +reapply a range of commits onto a different base, or interactive rebases
> > +if you want to edit a range of commits.
> > +
> >
> 
> The either..or in the last sentence is a bit confusing; as it is not an
> either between 'want to reapply a range of commit onto a different base'
> & 'interactive rebases'.
> 
> Perhaps we can simply s/either//

Fair.

> > diff --git a/builtin/history.c b/builtin/history.c
> > new file mode 100644
> > index 0000000000..f6fe32610b
> > --- /dev/null
> > +++ b/builtin/history.c
> > @@ -0,0 +1,22 @@
> > +#include "builtin.h"
> > +#include "gettext.h"
> > +#include "parse-options.h"
> > +
> > +int cmd_history(int argc,
> > +		const char **argv,
> > +		const char *prefix,
> > +		struct repository *repo UNUSED)
> > +{
> > +	const char * const usage[] = {
> > +		N_("git history [<options>]"),
> > +		NULL,
> > +	};
> 
> Nit: We have pointer alignment set to 'Right' in our styling guide and
> also mentioned in our 'Documentation/CodingGuidelines'
> 
>    When declaring pointers, the star sides with the variable
>    name, i.e. "char *string", not "char* string" or
>    "char * string".  This makes it easier to understand code
>    like "char *string, c;".
> 
> The rest of the patch looks good!

This is one of the common exceptions though:

    $ git grep 'const char \* const' | wc -l
    186
    $ git grep 'const char \*const' | wc -l
    108

So when there is another keyword following the asterisk we tend to have
an additional space inbetween. We tend to only drop the space when the
next token is the variable name.

Patrick