On 25/02/27 01:35AM, Karthik Nayak wrote:
> > 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
> #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.
>
> 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
> > + 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
