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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Thomas,

On Sun, 29 Jul 2018, Thomas Gummerer wrote:

> On 07/21, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@xxxxxx>
> > 
> > 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@xxxxxx>
> > ---
> >  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.

Yes, I had the same reaction as you did. But still, I think it is the best
we can do, and I don't think it is worth spending a lot of thought about
ways to fix this, up and until the day when somebody experiences a real
problem there (and that it will be *their* responsibility to fix it).

> > +
> > +	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.

Right...

> 
> > +	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);

I guess so. I shied away from the pretty-printing machinery because last
time I tried to use it in a libified manner I had to put in a major fight
to get the code into git.git. But I guess that was because of a user
format (which uses global state, something I still would like to fix, but
that fight just cost me too much time), which is not the case here.

> Not sure if it's worth changing this at this stage of the series
> though, or if there is something in the above that I'm missing, that
> would make the shorter version not workable.

I think your version is not only shorter, but also possibly safer.

Ciao,
Dscho



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux