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.
> +
> + 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);
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.
> + }
> + strbuf_addch(buf, '\n');
> +
> + fwrite(buf->buf, buf->len, 1, stdout);
> }
>
> static struct diff_filespec *get_filespec(const char *name, const char *p)
> @@ -286,6 +333,7 @@ static void patch_diff(const char *a, const char *b,
> static void output(struct string_list *a, struct string_list *b,
> struct diff_options *diffopt)
> {
> + struct strbuf buf = STRBUF_INIT, dashes = STRBUF_INIT;
> int i = 0, j = 0;
>
> /*
> @@ -307,25 +355,21 @@ static void output(struct string_list *a, struct string_list *b,
>
> /* Show unmatched LHS commit whose predecessors were shown. */
> if (i < a->nr && a_util->matching < 0) {
> - printf("%d: %s < -: --------\n",
> - i + 1, short_oid(a_util));
> + output_pair_header(&buf, &dashes, a_util, NULL);
> i++;
> continue;
> }
>
> /* Show unmatched RHS commits. */
> while (j < b->nr && b_util->matching < 0) {
> - printf("-: -------- > %d: %s\n",
> - j + 1, short_oid(b_util));
> + output_pair_header(&buf, &dashes, NULL, b_util);
> b_util = ++j < b->nr ? b->items[j].util : NULL;
> }
>
> /* Show matching LHS/RHS pair. */
> if (j < b->nr) {
> a_util = a->items[b_util->matching].util;
> - printf("%d: %s ! %d: %s\n",
> - b_util->matching + 1, short_oid(a_util),
> - j + 1, short_oid(b_util));
> + output_pair_header(&buf, &dashes, a_util, b_util);
> if (!(diffopt->output_format & DIFF_FORMAT_NO_OUTPUT))
> patch_diff(a->items[b_util->matching].string,
> b->items[j].string, diffopt);
> @@ -333,6 +377,8 @@ static void output(struct string_list *a, struct string_list *b,
> j++;
> }
> }
> + strbuf_release(&buf);
> + strbuf_release(&dashes);
> }
>
> int show_range_diff(const char *range1, const char *range2,
> --
> gitgitgadget
>
