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

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

 



Hi Eric,

On Thu, 3 May 2018, Eric Sunshine wrote:

> On Thu, May 3, 2018 at 11:30 AM, Johannes Schindelin
> <johannes.schindelin@xxxxxx> wrote:
> > 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!

> > 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 <johannes.schindelin@xxxxxx>
> > ---
> > 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.

> > +               } 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!

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



[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