Re: [PATCH 03/18] branch-diff: first rudimentary implementation

Hi Eric,

On Thu, 3 May 2018, Eric Sunshine wrote:

On Thu, May 3, 2018 at 11:30 AM, Johannes Schindelin
[off-list ref] wrote:

quoted

At this stage, `git branch-diff` can determine corresponding commits of
two related commit ranges. This makes use of the recently introduced
implementation of the Hungarian algorithm.

The core of this patch is a straight port of the ideas of tbdiff, the
seemingly dormant project at https://github.com/trast/tbdiff.

The output does not at all match `tbdiff`'s output yet, as this patch
really concentrates on getting the patch matching part right.

Note: due to differences in the diff algorithm (`tbdiff` uses the
Pythong module `difflib`, Git uses its xdiff fork), the cost matrix

s/Pythong/Python/

Yep!

quoted

calculated by `branch-diff` is different (but very similar) to the one
calculated by `tbdiff`. Therefore, it is possible that they find
different matching commits in corner cases (e.g. when a patch was split
into two patches of roughly equal length).

Signed-off-by: Johannes Schindelin <redacted>
---

diff --git a/builtin/branch-diff.c b/builtin/branch-diff.c

@@ -19,6 +23,279 @@ static int parse_creation_weight(const struct option *opt, const char *arg,
+static int read_patches(const char *range, struct string_list *list)
+{
+       [...]
+       struct strbuf buf = STRBUF_INIT, line = STRBUF_INIT;
+       [...]
+                       } else if (starts_with(line.buf, "    ")) {
+                               strbuf_addbuf(&buf, &line);
+                               strbuf_addch(&buf, '\n');
+                       }
+
+                       continue;

Unnecessary blank line above 'continue'?

Sure.

quoted

+               } else if (starts_with(line.buf, "@@ "))
+                       strbuf_addstr(&buf, "@@");
+               [...]
+       }
+       fclose(in);
+
+       if (util)
+               string_list_append(list, buf.buf)->util = util;
+       strbuf_release(&buf);

strbuf_release(&line);

Yes!

quoted

+       if (finish_command(&cp))
+               return -1;
+
+       return 0;
+}

@@ -32,9 +309,63 @@ int cmd_branch_diff(int argc, const char **argv, const char *prefix)
+       if (argc == 2) {
+               if (!strstr(argv[0], ".."))
+                       warning(_("no .. in range: '%s'"), argv[0]);
+               strbuf_addstr(&range1, argv[0]);
+
+               if (!strstr(argv[1], ".."))
+                       warning(_("no .. in range: '%s'"), argv[1]);
+               strbuf_addstr(&range2, argv[1]);
+       } else if (argc == 1) {
+               if (!b)
+                       die(_("single arg format requires a symmetric range"));
+       } else {
+               error("Need two commit ranges");

Other warning/error messages emitted by this function are not
capitalized: s/Need/need/

Right. And it is also not translated. Fixed both.

Thank you for helping me make this patch series better!

Ciao,
Dscho