Re: [PATCH v3 2/3] builtin: introduce diff-pairs command

From: Justin Tobler <jltobler@gmail.com>
Date: Feb 27, 2025, 22:36 UTC
Message-ID: <z5mea66lqrhcjm2da5qyg7e4n3wvxsilhtta45qie5m6hub5o4@ftdgftsmaeul>
In-Reply-To: <CAOLa=ZS6yqquxAL_vCUf0FRrMbA6OLeY1VjqMv=pnxAo_17zLA@mail.gmail.com>

On 25/02/27 01:35AM, Karthik Nayak wrote:

Show 29 quoted lines

> > diff --git a/builtin/diff-pairs.c b/builtin/diff-pairs.c
> > new file mode 100644
> > index 0000000000..9472b10461
> > --- /dev/null
> > +++ b/builtin/diff-pairs.c
> > @@ -0,0 +1,193 @@
> > +#include "builtin.h"
> > +#include "commit.h"
> > +#include "config.h"
> > +#include "diff.h"
> > +#include "diffcore.h"
> > +#include "gettext.h"
> > +#include "hex.h"
> > +#include "object.h"
> > +#include "parse-options.h"
> > +#include "revision.h"
> > +#include "strbuf.h"
> > +
> 
> Nit: I could also compile without some of these headers, do we still
> need them all?
> 
>     diff --git a/builtin/diff-pairs.c b/builtin/diff-pairs.c
>     index 86e59a7e3a..1aea2ee726 100644
>     --- a/builtin/diff-pairs.c
>     +++ b/builtin/diff-pairs.c
>     @@ -1,14 +1,9 @@
>      #include "builtin.h"
>     -#include "commit.h"

Looks like this one is unneeded. Will remove

Show 9 quoted lines

>      #include "config.h"
>     -#include "diff.h"
>      #include "diffcore.h"
>     -#include "gettext.h"
>      #include "hex.h"
>     -#include "object.h"
>      #include "parse-options.h"
>      #include "revision.h"
>     -#include "strbuf.h"

The others are directly referenced. I think it would be preferable to explicitly state them instead of relying on them being included transitively.

Show 20 quoted lines

> 
>      static unsigned parse_mode_or_die(const char *mode, const char **endp)
>      {
> 
> > +static unsigned parse_mode_or_die(const char *mode, const char **endp)
> > +{
> > +	uint16_t ret;
> > +
> > +	*endp = parse_mode(mode, &ret);
> > +	if (!*endp)
> > +		die(_("unable to parse mode: %s"), mode);
> > +	return ret;
> > +}
> > +
> > +static void parse_oid_or_die(const char *p, struct object_id *oid,
> > +			     const char **endp, const struct git_hash_algo *algop)
> >
> 
> Nit: without double checking, I couldn't tell what 'p' was, can we
> rename the variables here to be consistent with `parse_oid_hex_algop()`?

Will update

Show 6 quoted lines

> > +		case DIFF_STATUS_RENAMED:
> > +		case DIFF_STATUS_COPIED:
> > +			{
> 
> style: The general rule followed is to open the braces in the same line
> as the case statement. So `case DIFF_STATUS_COPIED: {`

Will fix in the next version.

Thanks -Justin