Re: [PATCH 02/18] Add a new builtin: branch-diff

Hi Ramsay,

On Fri, 4 May 2018, Ramsay Jones wrote:

On 03/05/18 21:25, Johannes Schindelin wrote:

quoted

On Thu, 3 May 2018, Ramsay Jones wrote:

quoted

quoted

On 03/05/18 16:30, Johannes Schindelin wrote:

[snip]

quoted

quoted

quoted

diff --git a/builtin/branch-diff.c b/builtin/branch-diff.c
new file mode 100644
index 00000000000..97266cd326d
--- /dev/null
+++ b/builtin/branch-diff.c

@@ -0,0 +1,40 @@
+#include "cache.h"
+#include "parse-options.h"
+
+static const char * const builtin_branch_diff_usage[] = {
+	N_("git rebase--helper [<options>] ( A..B C..D | A...B | base A B )"),

s/rebase--helper/branch-diff/

Whoops!

BTW funny side note: when I saw that you replied, I instinctively thought
"oh no, I forgot to mark a function as `static`!" ;-)

Heh, but I hadn't got around to applying the patches and building
git yet! ;-)

;-)

Sparse has two complaints:

  >     SP builtin/branch-diff.c
  > builtin/branch-diff.c:433:41: warning: Using plain integer as NULL pointer
  > builtin/branch-diff.c:431:5: warning: symbol 'cmd_branch_diff' was not declared. Should it be static?

I suppressed those warnings with the following patch (on top
of these patches):

  $ git diff
  diff --git a/builtin/branch-diff.c b/builtin/branch-diff.c
  index edf80ecb7..1373c22f4 100644
  --- a/builtin/branch-diff.c
  +++ b/builtin/branch-diff.c
  @@ -1,4 +1,5 @@
   #include "cache.h"
  +#include "builtin.h"
   #include "parse-options.h"
   #include "string-list.h"
   #include "run-command.h"
  @@ -430,7 +431,7 @@ static void output(struct string_list *a, struct string_list *b,
 
   int cmd_branch_diff(int argc, const char **argv, const char *prefix)
   {
  -       struct diff_options diffopt = { 0 };
  +       struct diff_options diffopt = { NULL };
          struct strbuf four_spaces = STRBUF_INIT;
          int dual_color = 0;
          double creation_weight = 0.6;
  $ 

Thanks!

The first hunk applies to patch 02/18 (ie this very patch) and
the second hunk should be applied to patch 05/18 (ie, "branch-diff:
also show the diff between patches").

I actually have a hacky script to fixup commits in a patch series. It lets
me stage part of the current changes, then figures out which of the
commits' changes overlap with the staged changed. If there is only one
commit, it automatically commits with --fixup, otherwise it lets me choose
which one I want to fixup (giving me the list of candidates).

BTW I ran `make sparse` for the first time, and it spits out tons of
stuff. And I notice that they are all non-fatal warnings, but so were the
ones you pointed out above. This is a bit sad, as I would *love* to
install a VSTS build job to run `make sparse` automatically. Examples of
warnings *after* applying your patch:

connect.c:481:40: warning: incorrect type in argument 2 (invalid types)
connect.c:481:40:    expected union __CONST_SOCKADDR_ARG [usertype] __addr
connect.c:481:40:    got struct sockaddr *ai_addr

or

pack-revindex.c:65:23: warning: memset with byte count of 262144

What gives?

Ciao,
Dscho