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
