Re: [PATCH v4 09/21] range-diff: adjust the output of the commit pairs

> On 07/21, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > 
> > This change brings `git range-diff` yet another step closer to
> > feature parity with tbdiff: it now shows the oneline, too, and indicates
> > with `=` when the commits have identical diffs.
> > 
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  range-diff.c | 64 ++++++++++++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 55 insertions(+), 9 deletions(-)
> > 
> > diff --git a/range-diff.c b/range-diff.c
> > index 1ecee2c09..8329f52e7 100644
> > --- a/range-diff.c
> > +++ b/range-diff.c
> > @@ -7,6 +7,8 @@
> >  #include "xdiff-interface.h"
> >  #include "linear-assignment.h"
> >  #include "diffcore.h"
> > +#include "commit.h"
> > +#include "pretty.h"
> >  
> >  struct patch_util {
> >  	/* For the search for an exact match */
> > @@ -255,9 +257,54 @@ static void get_correspondences(struct string_list *a, struct string_list *b,
> >  	free(b2a);
> >  }
> >  
> > -static const char *short_oid(struct patch_util *util)
> > +static void output_pair_header(struct strbuf *buf,
> > +			       struct strbuf *dashes,
> > +			       struct patch_util *a_util,
> > +			       struct patch_util *b_util)
> >  {
> > -	return find_unique_abbrev(&util->oid, DEFAULT_ABBREV);
> > +	struct object_id *oid = a_util ? &a_util->oid : &b_util->oid;
> > +	struct commit *commit;
> > +
> > +	if (!dashes->len)
> > +		strbuf_addchars(dashes, '-',
> > +				strlen(find_unique_abbrev(oid,
> > +							  DEFAULT_ABBREV)));
> 
> We're doing this only once, which makes sense.  What's a bit
> unfortunate here I guess is that if the first commit we're dealing
> with in the range-diff has a longer unique abbreviation, the dashes
> will be longer for all commits, even if all the others have a shorter
> abbreviation.
> 
> Tbh I don't really know what the right thing to do here is, so this is
> probably as good a heuristic as any.  It would probably be worse to
> have different length dashes lines, than guessing based on the first
> commit.

> > +
> > +	strbuf_reset(buf);
> > +	if (!a_util)
> > +		strbuf_addf(buf, "-:  %s ", dashes->buf);
> > +	else
> > +		strbuf_addf(buf, "%d:  %s ", a_util->i + 1,
> > +			    find_unique_abbrev(&a_util->oid, DEFAULT_ABBREV));
> > +
> > +	if (!a_util)
> > +		strbuf_addch(buf, '>');
> > +	else if (!b_util)
> > +		strbuf_addch(buf, '<');
> > +	else if (strcmp(a_util->patch, b_util->patch))
> > +		strbuf_addch(buf, '!');
> > +	else
> > +		strbuf_addch(buf, '=');
> > +
> > +	if (!b_util)
> > +		strbuf_addf(buf, " -:  %s", dashes->buf);
> > +	else
> > +		strbuf_addf(buf, " %d:  %s", b_util->i + 1,
> > +			    find_unique_abbrev(&b_util->oid, DEFAULT_ABBREV));
> > +
> > +	commit = lookup_commit_reference(oid);
> 
> This bit surprised me slightly.  May be worth mentioning that we now
> also show the first line of the commit message here.

> 
> > +	if (commit) {
> > +		const char *commit_buffer = get_commit_buffer(commit, NULL);
> > +		const char *subject;
> > +
> > +		find_commit_subject(commit_buffer, &subject);
> > +		strbuf_addch(buf, ' ');
> > +		format_subject(buf, subject, " ");
> > +		unuse_commit_buffer(commit, commit_buffer);
> 
> I think the above could be written slightly shorter as
> 
>     strbuf_addch(buf, ' ');
>     pp_commit_easy(CMIT_FMT_ONELINE, commit, &buf);