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

[Johannes Schindelin]

Hi Ramsay,

On Wed, 9 May 2018, Ramsay Jones wrote:

> On 05/05/18 20:41, Johannes Schindelin wrote:
> [snip]
> 
> [Sorry for the late reply - still catching up after (long weekend)
> UK public holiday]
> 
> > Well, what I would want to do is let the cloud work for me. By adding an
> > automated build to my Visual Studio Team Services (VSTS) account, of
> > course, as I have "cloud privilege" (i.e. I work in the organization
> > providing the service, so I get to play with all of it for free).
> > 
> > So I really don't want to build sparse every time a new revision needs to
> > be tested (whether that be from one of my branches, an internal PR for
> > pre-review of patches to be sent to the mailing list, or maybe even `pu`
> > or the personalized branches on https://github.com/gitster/git).
> > 
> > I really would need a ready-to-install sparse, preferably as light-weight
> > as possible (by not requiring any dependencies outside what is available
> > in VSTS' hosted Linux build agents.
> > 
> > Maybe there is a specific apt source for sparse?
> 
> Not that I'm aware of, sorry! :(
> 
> [release _source_ tar-balls are available, but that's not
> what you are after, right?]

No, that's not what I am after, because my goal is not to build sparse
every time somebody pushes a new commit.

I want to use the Hosted Agents of Visual Studio Team Services (because I
have cloud privilege, as part of the team working on VSTS, I can use them
for free, as much as I want, within reason of course). And yes, I want to
use the Hosted Linux Agents for the sparse job.

So I cannot compile sparse and then install it into an agent. Those agents
are recycled after every build, so that every new build starts from a
clean slate.

If you have anything in the way of providing some easily-consumable
package, that would do the job. I guess I could build sparse.deb via
checkinstall in one VSTS build, offer it as artifact, and consume that
from the VSTS job that uses it on the Git branches.

Could you point me to a robus, yet current revision of sparse (and ideally
provide me with precise instructions how to build it so that I do not have
to hunt for that information)?

Ciao,
Dscho

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

[Ramsay Jones]

On 05/05/18 20:41, Johannes Schindelin wrote:
[snip]

[Sorry for the late reply - still catching up after (long weekend)
UK public holiday]

> Well, what I would want to do is let the cloud work for me. By adding an
> automated build to my Visual Studio Team Services (VSTS) account, of
> course, as I have "cloud privilege" (i.e. I work in the organization
> providing the service, so I get to play with all of it for free).
> 
> So I really don't want to build sparse every time a new revision needs to
> be tested (whether that be from one of my branches, an internal PR for
> pre-review of patches to be sent to the mailing list, or maybe even `pu`
> or the personalized branches on https://github.com/gitster/git).
> 
> I really would need a ready-to-install sparse, preferably as light-weight
> as possible (by not requiring any dependencies outside what is available
> in VSTS' hosted Linux build agents.
> 
> Maybe there is a specific apt source for sparse?

Not that I'm aware of, sorry! :(

[release _source_ tar-balls are available, but that's not
what you are after, right?]

I don't know what is involved in setting up a 'ppa repo' for
Ubuntu, which I suspect is the kind of thing you want, but it
would have helped me several times in the past (so that I could
have something to point people to) ... ;-)

ATB,
Ramsay Jones

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

[Johannes Schindelin]

Hi Elijah,

On Fri, 4 May 2018, Elijah Newren wrote:

> On Thu, May 3, 2018 at 11:40 PM, Johannes Schindelin
>  wrote:
> > 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).
> 
> Ooh, interesting.  Are you willing to share said hacky script by chance?

It is part of a real huge hacky script of pretty much all things I add as
aliases, so I extracted the relevant part for you:

-- snip --
#!/bin/sh

fixup () { # [--upstream=] [--not=]
upstream=
not=
while case "$1" in
--upstream) shift; upstream="$1";;
--upstream=*) upstream="${1#*=}";;
--not) shift; not="$not $1";;
--not=*) not="$not ${1#*=}";;
-*) die "Unknown option: $1";;
*) break;;
esac; do shift; done

test $# -le 1 ||
die "Need 0 or 1 commit"

! git diff-index --cached --quiet --ignore-submodules HEAD -- || {
git update-index --ignore-submodules --refresh
! git diff-files --quiet --ignore-submodules -- ||
die "No changes"

git add -p ||
exit
}
! git diff-index --cached --quiet --ignore-submodules HEAD -- ||
die "No staged changes"

test $# = 1 || {
if test -z "$upstream"
then
upstream="$(git rev-parse --symbolic-full-name \
HEAD@{upstream} 2> /dev/null)" &&
test "$(git rev-parse HEAD)" != \
"$(git rev-parse $upstream)" ||
upstream=origin/master
fi

revs="$(git rev-list $upstream.. --not $not --)" ||
die "Could not get commits between $upstream and HEAD"

test -n "$revs" ||
die "No commits between $upstream and HEAD"

while count=$(test -z "$revs" && echo 0 || echo "$revs" | wc -l 
| tr -dc 0-9) &&
test $count -gt 1
do
printf '\nMultiple candidates:\n'
echo $revs | xargs git log --no-walk --oneline | cat -n

read input
case "$input" in
[0-9]|[0-9][0-9]|[0-9][0-9][0-9])
revs="$(echo "$revs" | sed -n "${input}p")"
count=1
break
;;
h|hh)
revs=$(history_of_staged_changes $upstream..)
continue
;;
)
history_of_staged_changes -p $upstream..
continue
;;
p)
git log -p --no-walk $revs
continue
;;

[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f])
revs=$input
continue
esac
revs="$(git rev-list --no-walk --grep="$input" $revs)"
done

test $count = 1 ||
die "No commit given"

set $revs
}

git commit --fixup "$1"
message="$(git show -s --format=%s%n%n%b)"
case "$message" in
'fixup! fixup! '*)
message="${message#fixup! }"
message="${message#fixup! }"
message="${message#fixup! }"
git commit --amend -m "fixup! $message"
;;
esac
}

history_of_staged_changes () { # 
pretty=
if test "a-p" = "a$1"
then
pretty=-p
shift
fi

test $# -le 1 ||
die "Usage: $0 "

test $# = 1 || {
upstream=$(git rev-parse --verify HEAD@{u} 2>/dev/null) ||
upstream=origin/master
set $upstream..
}

args=$(for file in $(git diff --no-color --cached --name-only)
do
for hunk in $(get_hunks --cached -- "$file")
do

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

[Johannes Schindelin]

Hi Ramsay,

On Fri, 4 May 2018, Ramsay Jones wrote:

> On 04/05/18 07:40, Johannes Schindelin wrote:
> [snip] 
> > 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?
> 
> My stock answer, until recently, was that you are using a very
> old version of sparse.

Sure. I did this in an Ubuntu 16.04 LTS VM, via `sudo apt-get install
sparse`.

> Which is probably still true here - but I recently noticed that more
> up-to-date platforms/gcc versions also have many problems. (The main
> sparse contributors tend to stick with conservative distros and/or don't
> use sparse on any software that uses system headers - thus they tend not
> to notice the problems caused by new gcc/glibc versions! ;-) )
> 
> Since I am on Linux Mint 18.3 (based on the last Ubuntu LTS) and build
> sparse from source, the current 'master', 'next' and 'pu' branches are
> all 'sparse-clean' for me. (On cygwin, which is built with NO_REGEX, I
> have a single sparse warning).
> 
> I was just about to say that, unusually for me, I was using a sparse
> built from a release tag, but then remembered that I have some
> additional patches which fixes a problem on fedora 27!  Using sparse on
> fedora 27 is otherwise useless. (There are still many warnings spewed on
> f27 - but they are caused by incorrect system headers :( ).
> 
> The current release of sparse is v0.5.2, which probably hasn't been
> included in any distro yet (I think the previous release v0.5.1, which
> should also work for you, is in Debian unstable).  If you wanted to try
> building a more up-to-date sparse, the repo is at:
> git://git.kernel.org/pub/scm/devel/sparse/sparse.git.

Well, what I would want to do is let the cloud work for me. By adding an
automated build to my Visual Studio Team Services (VSTS) account, of
course, as I have "cloud privilege" (i.e. I work in the organization
providing the service, so I get to play with all of it for free).

So I really don't want to build sparse every time a new revision needs to
be tested (whether that be from one of my branches, an internal PR for
pre-review of patches to be sent to the mailing list, or maybe even `pu`
or the personalized branches on https://github.com/gitster/git).

I really would need a ready-to-install sparse, preferably as light-weight
as possible (by not requiring any dependencies outside what is available
in VSTS' hosted Linux build agents.

Maybe there is a specific apt source for sparse?

> Linux Mint 19, which will be released in about a month, will be
> using the Ubuntu 18.04 LTS as a base, so I guess it is possible
> that I will need to debug sparse again ...

:-)

> BTW, I spent some time last night playing with 'git-branch-diff'.

Great!

> First of all - Good Job! This tool will be very useful (thanks
> also go to Thomas, of course).

Both Thomases. Thomas Rast and Thomas Gummerer.

> I noticed that there seemed to be an occasional 'whitespace error'
> indicator (red background) directly after an +/- change character
> which I suspect is an error (I haven't actually checked). However,
> this indicator disappears if you add the --dual-color option.

Indeed. This is a quirk of the whitespace error paired with diffing diffs:
the whitespace error correctly treats the leading space as marker for a
context line, but if you diff diffs, the next character may still be a
marker for a context line (but this time the "inner" diff). And our
whitespace error detection mechanism cannot guess that it looks at a diff
of diffs.

However, in dual-color mode, we know that we will almost certainly look at
diffs of diffs (except if the change is in the commit message, in which
case I don't care about whitespace errors at all, anyway).

So I have this hack in 16/18:
https://public-inbox.org/git/b99ab186c4f11239a10950b9902d9c87d0e60513.1525448066.git.johannes.schinde...@gmx.de/T/#u

Essentially, I extend the dual-color mode to detect where such a bogus
whitespace error would be detected, and simply *skip the space*! I can get
away with that because dual-color is meant for human consumption, and if a
horizontal tab follows, it would not matter whether there was a space: it
would find the same tab stop. Likewise, if the space comes before a CR or
LF, or even just before the final NUL, the space can be safely omitted
from the output, too.

Ciao,
Dscho

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

[Elijah Newren]

On Thu, May 3, 2018 at 11:40 PM, Johannes Schindelin
 wrote:
> 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).

Ooh, interesting.  Are you willing to share said hacky script by chance?

(And as a total aside, I found your apply-from-public-inbox.sh script
and really like it.  Thanks for making it public.)

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

[Ramsay Jones]

On 04/05/18 07:40, Johannes Schindelin wrote:
[snip] 
> 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?

My stock answer, until recently, was that you are using a very
old version of sparse. Which is probably still true here - but
I recently noticed that more up-to-date platforms/gcc versions
also have many problems. (The main sparse contributors tend to
stick with conservative distros and/or don't use sparse on any
software that uses system headers - thus they tend not to notice
the problems caused by new gcc/glibc versions! ;-) )

Since I am on Linux Mint 18.3 (based on the last Ubuntu LTS) and
build sparse from source, the current 'master', 'next' and 'pu'
branches are all 'sparse-clean' for me. (On cygwin, which is
built with NO_REGEX, I have a single sparse warning).

I was just about to say that, unusually for me, I was using a
sparse built from a release tag, but then remembered that I have
some additional patches which fixes a problem on fedora 27!
Using sparse on fedora 27 is otherwise useless. (There are still
many warnings spewed on f27 - but they are caused by incorrect
system headers :( ).

The current release of sparse is v0.5.2, which probably hasn't
been included in any distro yet (I think the previous release
v0.5.1, which should also work for you, is in Debian unstable).
If you wanted to try building a more up-to-date sparse, the repo
is at: git://git.kernel.org/pub/scm/devel/sparse/sparse.git.

Linux Mint 19, which will be released in about a month, will be
using the Ubuntu 18.04 LTS as a base, so I guess it is possible
that I will need to debug sparse again ...

BTW, I spent some time last night playing with 'git-branch-diff'.

First of all - Good Job! This tool will be very useful (thanks
also go to Thomas, of course).

I noticed that there seemed to be an occasional 'whitespace error'
indicator (red background) directly after an +/- change character
which I suspect is an error (I haven't actually checked). However,
this indicator disappears if you add the --dual-color option.

Thanks!

ATB,
Ramsay Jones

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

[Duy Nguyen]

On Fri, May 4, 2018 at 5:23 PM, Johannes Schindelin
 wrote:
>> > So that's what `info` does: it influences whether/where
>> > the command is listed in `git help`'s output... Interesting. I thought the
>> > lines here were trying to automate parts of the tab completion or
>> > something.
>>
>> Oh it does many things. The completion part is coming (so yeah you
>> don't need to update git-completion.bash at all, as long as you have a
>> line here and use parse_options() ;-), but I think it's mainly for
>> "git help" and command listing in "git help git" (this command for
>> example should show up under the "Main porcelain commands" in that man
>> page when you put a line here)
>
> I have a hard time believing that anything automated can infer from the
> source code that branch-diff can accept the non-options in three different
> formats, and what those formats look like...

Yeah. For now it can only do options which is machine readable. But I
have a big plan, so who knows :D
-- 
Duy

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

[Johannes Schindelin]

Hi Duy,

On Fri, 4 May 2018, Duy Nguyen wrote:

> On Fri, May 4, 2018 at 9:23 AM, Johannes Schindelin
>  wrote:
> > Oh, okay. It was not at all clear to me what the exact format and role of
> > these lines are...
> 
> Noted. I'm making more updates in this file in another topic and will
> add some explanation so the next guy will be less confused.

Thank you!

> > So that's what `info` does: it influences whether/where
> > the command is listed in `git help`'s output... Interesting. I thought the
> > lines here were trying to automate parts of the tab completion or
> > something.
> 
> Oh it does many things. The completion part is coming (so yeah you
> don't need to update git-completion.bash at all, as long as you have a
> line here and use parse_options() ;-), but I think it's mainly for
> "git help" and command listing in "git help git" (this command for
> example should show up under the "Main porcelain commands" in that man
> page when you put a line here)

I have a hard time believing that anything automated can infer from the
source code that branch-diff can accept the non-options in three different
formats, and what those formats look like...

Ciao,
Dscho

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

[Duy Nguyen]

On Fri, May 04, 2018 at 04:44:49PM +0200, Duy Nguyen wrote:
> On Fri, May 4, 2018 at 9:23 AM, Johannes Schindelin
>  wrote:
> > Oh, okay. It was not at all clear to me what the exact format and role of
> > these lines are...
> 
> Noted. I'm making more updates in this file in another topic and will
> add some explanation so the next guy will be less confused.

This is what I will include (but in a separate topic). I will not CC
you there to keep your inbox slightly less full. I hope this helps
understand what command-list.txt is for.

diff --git a/command-list.txt b/command-list.txt
index 40776b9587..929d8f0ea0 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -1,3 +1,47 @@
+# Command classification list
+# ---
+# All supported commands, builtin or external, must be described in
+# here. This info is used to list commands in various places. Each
+# command is on one line followed by one or more attributes.
+#
+# The first attribute group is mandatory and indicates the command
+# type. This group includes:
+#
+#   mainporcelain
+#   ancillarymanipulators
+#   ancillaryinterrogators
+#   foreignscminterface
+#   plumbingmanipulators
+#   plumbinginterrogators
+#   synchingrepositories
+#   synchelpers
+#   purehelpers
+#
+# the type names are self explanatory. But if you want to see what
+# command belongs to what group to get a better idea, have a look at
+# "git" man page, "GIT COMMANDS" section.
+#
+# Commands of type mainporcelain can also optionally have one of these
+# attributes:
+#
+#   init
+#   worktree
+#   info
+#   history
+#   remote
+#
+# These commands are considered "common" and will show up in "git
+# help" output in groups. Uncommon porcelain commands must not
+# specify any of these attributes.
+#
+# "complete" attribute is used to mark that the command should be
+# completable by git-completion.bash. Note that by default,
+# mainporcelain commands are completable and you don't need this
+# attribute.
+#
+# While not true commands, guides are also specified here, which can
+# only have "guide" attribute and nothing else.
+#
 ### command list (do not change this line, also do not change alignment)
 # command name  category [category] [category]
 git-add mainporcelain   worktree

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

[Duy Nguyen]

On Fri, May 4, 2018 at 9:23 AM, Johannes Schindelin
 wrote:
> Oh, okay. It was not at all clear to me what the exact format and role of
> these lines are...

Noted. I'm making more updates in this file in another topic and will
add some explanation so the next guy will be less confused.

> So that's what `info` does: it influences whether/where
> the command is listed in `git help`'s output... Interesting. I thought the
> lines here were trying to automate parts of the tab completion or
> something.

Oh it does many things. The completion part is coming (so yeah you
don't need to update git-completion.bash at all, as long as you have a
line here and use parse_options() ;-), but I think it's mainly for
"git help" and command listing in "git help git" (this command for
example should show up under the "Main porcelain commands" in that man
page when you put a line here)
-- 
Duy

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

[Eric Sunshine]

On Fri, May 4, 2018 at 2:52 AM, Johannes Schindelin
 wrote:
> On Thu, 3 May 2018, Eric Sunshine wrote:
>> On Thu, May 3, 2018 at 11:30 AM, Johannes Schindelin
>>  wrote:
>> > +static const char * const builtin_branch_diff_usage[] = {
>> > +   N_("git rebase--helper [] ( A..B C..D | A...B | base A B 
>> > )"),
>>
>> The formatting of "" vs. "base" confused me into thinking
>> that the latter was a literal keyword, but I see from reading patch
>> 3/18 that it is not a literal at all, thus probably ought to be
>> specified as "".
>
> Good point. Or maybe BASE?

Indeed, that's probably more consistent with 'A', 'B', etc. than .

> Or I should just use the same convention as in the man page. Or not, as
> the usage should be conciser.
>
> This is what I have currently:
>
> static const char * const builtin_branch_diff_usage[] = {
> N_("git branch-diff [] .. .."),
> N_("git branch-diff [] ..."),
> N_("git branch-diff []   "),
> NULL
> };

I can live with this. It's more verbose but more self-explanatory,
thus likely a good choice.

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

[Johannes Schindelin]

Hi Duy,

On Fri, 4 May 2018, Duy Nguyen wrote:

> On Thu, May 3, 2018 at 10:32 PM, Johannes Schindelin
>  wrote:
> >
> > On Thu, 3 May 2018, Johannes Schindelin wrote:
> >
> >> On Thu, 3 May 2018, Duy Nguyen wrote:
> >>
> >> > On Thu, May 3, 2018 at 5:30 PM, Johannes Schindelin
> >> >  wrote:
> >> > > diff --git a/command-list.txt b/command-list.txt
> >> > > index a1fad28fd82..c89ac8f417f 100644
> >> > > --- a/command-list.txt
> >> > > +++ b/command-list.txt
> >> > > @@ -19,6 +19,7 @@ git-archive mainporcelain
> >> > >  git-bisect  mainporcelain   info
> >> > >  git-blame   ancillaryinterrogators
> >> > >  git-branch  mainporcelain   
> >> > > history
> >> > > +git-branch-diff mainporcelain   info
> >> >
> >> > Making it part of "git help" with the info keywords at this stage may
> >> > be premature. "git help" is about _common_ commands and we don't know
> >> > (yet) how popular this will be.
> >>
> >> Makes sense. I removed the `mainporcelain` keyword locally.
> >
> > On second thought, I *think* you meant to imply that I should remove that
> > line altogether. Will do that now.
> 
> Actually I only suggested to remove the last word "info". That was
> what made this command "common". Classifying all commands in this file
> is definitely a good thing, and I think mainporcelain is the right
> choice.

Oh, okay. It was not at all clear to me what the exact format and role of
these lines are... So that's what `info` does: it influences whether/where
the command is listed in `git help`'s output... Interesting. I thought the
lines here were trying to automate parts of the tab completion or
something.

I re-added the line, this time without `info` and verified that
`branch-diff` does not show up in `git help`'s output.

Ciao,
Dscho

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

[Johannes Schindelin]

Hi Eric,

On Thu, 3 May 2018, Eric Sunshine wrote:

> On Thu, May 3, 2018 at 11:30 AM, Johannes Schindelin
>  wrote:
> > This builtin does not do a whole lot so far, apart from showing a usage
> > that is oddly similar to that of `git tbdiff`. And for a good reason:
> > the next commits will turn `branch-diff` into a full-blown replacement
> > for `tbdiff`.
> >
> > At this point, we ignore tbdiff's color options, as they will all be
> > implemented later and require some patches to the diff machinery.
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> > diff --git a/builtin/branch-diff.c b/builtin/branch-diff.c
> > @@ -0,0 +1,40 @@
> > +static const char * const builtin_branch_diff_usage[] = {
> > +   N_("git rebase--helper [] ( A..B C..D | A...B | base A B 
> > )"),
> > +   NULL
> > +};
> 
> The formatting of "" vs. "base" confused me into thinking
> that the latter was a literal keyword, but I see from reading patch
> 3/18 that it is not a literal at all, thus probably ought to be
> specified as "".

Good point. Or maybe BASE?

Or I should just use the same convention as in the man page. Or not, as
the usage should be conciser.

This is what I have currently:

static const char * const builtin_branch_diff_usage[] = {
N_("git branch-diff [] .. .."),
N_("git branch-diff [] ..."),
N_("git branch-diff []   "),
NULL
};

Thanks,
Dscho

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

[Johannes Schindelin]

Hi Ramsay,

On Fri, 4 May 2018, Ramsay Jones wrote:

> On 03/05/18 21:25, Johannes Schindelin wrote:
> 
> > On Thu, 3 May 2018, Ramsay Jones wrote:
> 
> >> On 03/05/18 16:30, Johannes Schindelin wrote:
> [snip]
> 
> >>> diff --git a/builtin/branch-diff.c b/builtin/branch-diff.c
> >>> new file mode 100644
> >>> index 000..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 [] ( 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

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

[Duy Nguyen]

On Thu, May 3, 2018 at 10:32 PM, Johannes Schindelin
 wrote:
> Hi Duy,
>
> On Thu, 3 May 2018, Johannes Schindelin wrote:
>
>> On Thu, 3 May 2018, Duy Nguyen wrote:
>>
>> > On Thu, May 3, 2018 at 5:30 PM, Johannes Schindelin
>> >  wrote:
>> > > diff --git a/command-list.txt b/command-list.txt
>> > > index a1fad28fd82..c89ac8f417f 100644
>> > > --- a/command-list.txt
>> > > +++ b/command-list.txt
>> > > @@ -19,6 +19,7 @@ git-archive mainporcelain
>> > >  git-bisect  mainporcelain   info
>> > >  git-blame   ancillaryinterrogators
>> > >  git-branch  mainporcelain   history
>> > > +git-branch-diff mainporcelain   info
>> >
>> > Making it part of "git help" with the info keywords at this stage may
>> > be premature. "git help" is about _common_ commands and we don't know
>> > (yet) how popular this will be.
>>
>> Makes sense. I removed the `mainporcelain` keyword locally.
>
> On second thought, I *think* you meant to imply that I should remove that
> line altogether. Will do that now.

Actually I only suggested to remove the last word "info". That was
what made this command "common". Classifying all commands in this file
is definitely a good thing, and I think mainporcelain is the right
choice.
-- 
Duy

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

[Junio C Hamano]

Johannes Schindelin  writes:

> So please: as soon as you have a concrete suggestion where to use cyan
> (and preferably even a DIFF_* constant to feed to diff_get_color_opt()), I
> will be more than interested.

I do not think Stefan's comment was that he was keen to use 'cyan'.
It was a color I suggested in a review of his change where he added
new colors to the color.[ch] palette, and I found that reusing an
existing color would have achieved the same distinction between
lines of output from his code, and it would be beneficial to make
the outcome consistent to consider why these existing colors are
used in existing places and trying to align the rationale for new
uses. "cyan" was cited as an example to illustrate that last point,
i.e. we use it to dim out relatively uninteresting part.

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

[Eric Sunshine]

On Thu, May 3, 2018 at 11:30 AM, Johannes Schindelin
 wrote:
> This builtin does not do a whole lot so far, apart from showing a usage
> that is oddly similar to that of `git tbdiff`. And for a good reason:
> the next commits will turn `branch-diff` into a full-blown replacement
> for `tbdiff`.
>
> At this point, we ignore tbdiff's color options, as they will all be
> implemented later and require some patches to the diff machinery.
>
> Signed-off-by: Johannes Schindelin 
> ---
> diff --git a/builtin/branch-diff.c b/builtin/branch-diff.c
> @@ -0,0 +1,40 @@
> +static const char * const builtin_branch_diff_usage[] = {
> +   N_("git rebase--helper [] ( A..B C..D | A...B | base A B )"),
> +   NULL
> +};

The formatting of "" vs. "base" confused me into thinking
that the latter was a literal keyword, but I see from reading patch
3/18 that it is not a literal at all, thus probably ought to be
specified as "".

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

[Ramsay Jones]

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

> On Thu, 3 May 2018, Ramsay Jones wrote:

>> On 03/05/18 16:30, Johannes Schindelin wrote:
[snip]

>>> diff --git a/builtin/branch-diff.c b/builtin/branch-diff.c
>>> new file mode 100644
>>> index 000..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 [] ( 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;
  $ 

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").

ATB,
Ramsay Jones

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

[Johannes Schindelin]

Hi Stefan,

On Thu, 3 May 2018, Stefan Beller wrote:

> On Thu, May 3, 2018 at 1:42 PM, Johannes Schindelin
>  wrote:
> 
> >> Speaking of colors, for origin/sb/blame-color Junio hinted at re-using
> >> cyan for "uninteresting" parts to deliver a consistent color scheme for
> >> Git. Eventually he dreams of having 2 layers of indirection IIUC, with
> >> "uninteresting" -> cyan
> >> "repeated lines in blame" -> uninteresting
> >>
> >> Maybe we can fit the coloring of this tool in this scheme, too?
> >
> > Sure. So you mean I should use cyan for... what part of the colored
> > output? ;-)
> 
> It is just a FYI heads up, not an actionable bikeshed painting plan. ;)

Oh, I did not understand it as bike-shedding at all. I had hoped that you
ran `git branch-diff --dual-color` on something interesting and found e.g.
the yellow color inherited from DIFF_COMMIT to be the wrong color for
unchanged commit pairs.

So please: as soon as you have a concrete suggestion where to use cyan
(and preferably even a DIFF_* constant to feed to diff_get_color_opt()), I
will be more than interested.

> >> Do we need to dynamic of a floating point, or would a rather small range
> >> suffice here? (Also see rename detection settings, that take percents as
> >> integers)
> >
> > I guess you are right, and we do not need floats. It was just very, very
> > convenient to do that instead of using integers because
> >
> > - I already had the Jonker-Volgenant implementation "lying around" from my
> >   previous life as an image processing expert, using doubles (but it was
> >   in Java, not in C, so I quickly converted it for branch-diff).
> >
> > - I was actually not paying attention whether divisions are a thing in the
> >   algorithm. From a cursory glance, it would appear that we are never
> >   dividing in hungarian.c, so theoretically integers should be fine.
> >
> > - using doubles neatly side-steps the overflow problem. If I use integers
> >   instead, I always will have to worry what to do if, say, adding
> >   `INT_MAX` to `INT_MAX`.
> >
> > I am particularly worried about that last thing: it could easily lead to
> > incorrect results if we blindly, say, pretend that `INT_MAX + INT_MAX ==
> > INT_MAX` for the purpose of avoiding overflows.
> >
> > If, however, I misunderstood and you are only concerned about using
> > *double-precision* floating point numbers, and would suggest using `float`
> > typed variables instead, that would be totally cool with me.
> 
> So by being worried about INT_MAX occurring, you are implying that
> we have to worry about a large range of values, so maybe floating points
> are the best choice here.

I am not really worried about a large range of values, I am worried about
a use case where we use the maximal value as an "impossible, must avoid at
all cost" value. See this line in hungarian.c:

u2 = DBL_MAX;

It does not seem as if any arithmetic is done on u2 after that
(theoretically, it should not survive the loop that comes after it and
tries to replace u2 with any smaller value it finds, but what if that loop
does not even run because column_count == 1? Sure, it is a pathological
case, but even those should have correct results).

But actually, yes, there *is* arithmetic performed on u2:

if (u1 < u2)
v[j1] -= u2 - u1;

So in the pathological case where we try to find the best assignment of a
single column to an arbitrary number of rows (where simply the row with
a smallest cost should be picked), we try to subtract from v[j1] an
insanely large value. As a consequence, v[j1] will be close to INT_MIN if
we were to switch to integers, and who is to say that the next time we get
to this part, j1 will be different? If it is the same, and we hit the same
u2, then we might end up subtracting something close to INT_MAX from
INT_MIN, which will definitely overflow and the computation will be
incorrect.

*That* is what I am worried about: overflowing integer arithmetic. IIRC if
you subtract DBL_MAX from -DBL_MAX, you still end up with -DBL_MAX. So in
that respect, using floating point numbers here is safer.

> Looking through that algorithm the costs seem to be integers only
> measuring number of lines, so I would not be too worried about running
> into INT_MAX problems except for the costs that are assigned INT_MAX
> explicitly.
> 
> I was more asking, if floating point is the right tool for the job.

I think I would have to spend some real quality time with the code in
hungarian.c to turn it into using integer costs instead of floating point
numbers, to ensure that the arithmetic is done in a way that is consistent
with the algorithm, even if we cannot represent the arithmetic faithfully
with limited-range integers.

I'll think about the best way forward.

Ciao,
Dscho

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

[Stefan Beller]

On Thu, May 3, 2018 at 1:42 PM, Johannes Schindelin
 wrote:

>> Speaking of colors, for origin/sb/blame-color Junio hinted at re-using
>> cyan for "uninteresting" parts to deliver a consistent color scheme for
>> Git. Eventually he dreams of having 2 layers of indirection IIUC, with
>> "uninteresting" -> cyan
>> "repeated lines in blame" -> uninteresting
>>
>> Maybe we can fit the coloring of this tool in this scheme, too?
>
> Sure. So you mean I should use cyan for... what part of the colored
> output? ;-)
>

It is just a FYI heads up, not an actionable bikeshed painting plan. ;)

>> Do we need to dynamic of a floating point, or would a rather small range
>> suffice here? (Also see rename detection settings, that take percents as
>> integers)
>
> I guess you are right, and we do not need floats. It was just very, very
> convenient to do that instead of using integers because
>
> - I already had the Jonker-Volgenant implementation "lying around" from my
>   previous life as an image processing expert, using doubles (but it was
>   in Java, not in C, so I quickly converted it for branch-diff).
>
> - I was actually not paying attention whether divisions are a thing in the
>   algorithm. From a cursory glance, it would appear that we are never
>   dividing in hungarian.c, so theoretically integers should be fine.
>
> - using doubles neatly side-steps the overflow problem. If I use integers
>   instead, I always will have to worry what to do if, say, adding
>   `INT_MAX` to `INT_MAX`.
>
> I am particularly worried about that last thing: it could easily lead to
> incorrect results if we blindly, say, pretend that `INT_MAX + INT_MAX ==
> INT_MAX` for the purpose of avoiding overflows.
>
> If, however, I misunderstood and you are only concerned about using
> *double-precision* floating point numbers, and would suggest using `float`
> typed variables instead, that would be totally cool with me.

So by being worried about INT_MAX occurring, you are implying that
we have to worry about a large range of values, so maybe floating points
are the best choice here.

Looking through that algorithm the costs seem to be integers only
measuring number of lines, so I would not be too worried about running
into INT_MAX problems except for the costs that are assigned INT_MAX
explicitly.

I was more asking, if floating point is the right tool for the job.

Stefan

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

[Johannes Schindelin]

Hi Stefan,

On Thu, 3 May 2018, Stefan Beller wrote:

> On Thu, May 3, 2018 at 8:30 AM, Johannes Schindelin
>  wrote:
> > This builtin does not do a whole lot so far, apart from showing a
> > usage that is oddly similar to that of `git tbdiff`. And for a good
> > reason: the next commits will turn `branch-diff` into a full-blown
> > replacement for `tbdiff`.
> 
> While I appreciate the 1:1 re-implementation, I'll comment as if this
> was a newly invented tool, questioning design choices. They are probably
> chosen pretty well, and fudge facotrs as below are at tweaked to a
> reasonable factor, but I'll try to look with fresh eyes.

Absolutely. While tbdiff got some testing over time, it has definitely not
gotten as much exposure as branch-diff hopefully will.

BTW I chose a different command name on purpose, so that we are free to
change the design and not harm existing tbdiff users.

> > At this point, we ignore tbdiff's color options, as they will all be
> > implemented later and require some patches to the diff machinery.
> 
> Speaking of colors, for origin/sb/blame-color Junio hinted at re-using
> cyan for "uninteresting" parts to deliver a consistent color scheme for
> Git. Eventually he dreams of having 2 layers of indirection IIUC, with
> "uninteresting" -> cyan
> "repeated lines in blame" -> uninteresting
> 
> Maybe we can fit the coloring of this tool in this scheme, too?

Sure. So you mean I should use cyan for... what part of the colored
output? ;-)

> > +   double creation_weight = 0.6;
> 
> I wondered if we use doubles in Gits code base at all,
> and I found
> 
> khash.h:59:static const double __ac_HASH_UPPER = 0.77;
> pack-bitmap-write.c:248:static const double
> REUSE_BITMAP_THRESHOLD = 0.2;
> pack-bitmap.c:751:  static const double REUSE_PERCENT = 0.9;
> 
> all other occurrences of `git grep double` are mentioning it in other
> contexts (e.g. "double linked list" or comments).
> 
> When implementing diff heuristics in 433860f3d0b (diff: improve
> positioning of add/delete blocks in diffs, 2016-09-05), Michael broke
> it down to fixed integers instead of floating point.
> 
> Do we need to dynamic of a floating point, or would a rather small range
> suffice here? (Also see rename detection settings, that take percents as
> integers)

I guess you are right, and we do not need floats. It was just very, very
convenient to do that instead of using integers because

- I already had the Jonker-Volgenant implementation "lying around" from my
  previous life as an image processing expert, using doubles (but it was
  in Java, not in C, so I quickly converted it for branch-diff).

- I was actually not paying attention whether divisions are a thing in the
  algorithm. From a cursory glance, it would appear that we are never
  dividing in hungarian.c, so theoretically integers should be fine.

- using doubles neatly side-steps the overflow problem. If I use integers
  instead, I always will have to worry what to do if, say, adding
  `INT_MAX` to `INT_MAX`.

I am particularly worried about that last thing: it could easily lead to
incorrect results if we blindly, say, pretend that `INT_MAX + INT_MAX ==
INT_MAX` for the purpose of avoiding overflows.

If, however, I misunderstood and you are only concerned about using
*double-precision* floating point numbers, and would suggest using `float`
typed variables instead, that would be totally cool with me.

Ciao,
Dscho

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

[Johannes Schindelin]

Hi Duy,

On Thu, 3 May 2018, Johannes Schindelin wrote:

> On Thu, 3 May 2018, Duy Nguyen wrote:
> 
> > On Thu, May 3, 2018 at 5:30 PM, Johannes Schindelin
> >  wrote:
> > > diff --git a/command-list.txt b/command-list.txt
> > > index a1fad28fd82..c89ac8f417f 100644
> > > --- a/command-list.txt
> > > +++ b/command-list.txt
> > > @@ -19,6 +19,7 @@ git-archive mainporcelain
> > >  git-bisect  mainporcelain   info
> > >  git-blame   ancillaryinterrogators
> > >  git-branch  mainporcelain   history
> > > +git-branch-diff mainporcelain   info
> > 
> > Making it part of "git help" with the info keywords at this stage may
> > be premature. "git help" is about _common_ commands and we don't know
> > (yet) how popular this will be.
> 
> Makes sense. I removed the `mainporcelain` keyword locally.

On second thought, I *think* you meant to imply that I should remove that
line altogether. Will do that now.

Ciao,
Dscho

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

[Johannes Schindelin]

Hi Duy,

On Thu, 3 May 2018, Duy Nguyen wrote:

> On Thu, May 3, 2018 at 5:30 PM, Johannes Schindelin
>  wrote:
> > diff --git a/command-list.txt b/command-list.txt
> > index a1fad28fd82..c89ac8f417f 100644
> > --- a/command-list.txt
> > +++ b/command-list.txt
> > @@ -19,6 +19,7 @@ git-archive mainporcelain
> >  git-bisect  mainporcelain   info
> >  git-blame   ancillaryinterrogators
> >  git-branch  mainporcelain   history
> > +git-branch-diff mainporcelain   info
> 
> Making it part of "git help" with the info keywords at this stage may
> be premature. "git help" is about _common_ commands and we don't know
> (yet) how popular this will be.

Makes sense. I removed the `mainporcelain` keyword locally.

> (I'm not complaining though, I almost wrote "what witchcraft is this
> and where can I get it" when I see branch-diff output mentioned in
> AEvar's mail)

Yeah, it is pretty neat.

Ciao,
Dscho

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

[Johannes Schindelin]

Hi Ramsay,

On Thu, 3 May 2018, Ramsay Jones wrote:

> On 03/05/18 16:30, Johannes Schindelin wrote:
> > This builtin does not do a whole lot so far, apart from showing a usage
> > that is oddly similar to that of `git tbdiff`. And for a good reason:
> > the next commits will turn `branch-diff` into a full-blown replacement
> > for `tbdiff`.
> > 
> > At this point, we ignore tbdiff's color options, as they will all be
> > implemented later and require some patches to the diff machinery.
> > 
> > Signed-off-by: Johannes Schindelin 
> > ---
> >  .gitignore|  1 +
> >  Makefile  |  1 +
> >  builtin.h |  1 +
> >  builtin/branch-diff.c | 40 
> >  command-list.txt  |  1 +
> >  git.c |  1 +
> >  6 files changed, 45 insertions(+)
> >  create mode 100644 builtin/branch-diff.c
> > 
> > diff --git a/.gitignore b/.gitignore
> > index 833ef3b0b78..1346a64492f 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -20,6 +20,7 @@
> >  /git-bisect--helper
> >  /git-blame
> >  /git-branch
> > +/git-branch-diff
> >  /git-bundle
> >  /git-cat-file
> >  /git-check-attr
> > diff --git a/Makefile b/Makefile
> > index 96f2e76a904..9b1984776d8 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -953,6 +953,7 @@ BUILTIN_OBJS += builtin/archive.o
> >  BUILTIN_OBJS += builtin/bisect--helper.o
> >  BUILTIN_OBJS += builtin/blame.o
> >  BUILTIN_OBJS += builtin/branch.o
> > +BUILTIN_OBJS += builtin/branch-diff.o
> >  BUILTIN_OBJS += builtin/bundle.o
> >  BUILTIN_OBJS += builtin/cat-file.o
> >  BUILTIN_OBJS += builtin/check-attr.o
> > diff --git a/builtin.h b/builtin.h
> > index 42378f3aa47..e1c4d2a529a 100644
> > --- a/builtin.h
> > +++ b/builtin.h
> > @@ -135,6 +135,7 @@ extern int cmd_archive(int argc, const char **argv, 
> > const char *prefix);
> >  extern int cmd_bisect__helper(int argc, const char **argv, const char 
> > *prefix);
> >  extern int cmd_blame(int argc, const char **argv, const char *prefix);
> >  extern int cmd_branch(int argc, const char **argv, const char *prefix);
> > +extern int cmd_branch_diff(int argc, const char **argv, const char 
> > *prefix);
> >  extern int cmd_bundle(int argc, const char **argv, const char *prefix);
> >  extern int cmd_cat_file(int argc, const char **argv, const char *prefix);
> >  extern int cmd_checkout(int argc, const char **argv, const char *prefix);
> > diff --git a/builtin/branch-diff.c b/builtin/branch-diff.c
> > new file mode 100644
> > index 000..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 [] ( 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`!" ;-)

Thank you for helping me improve the patches,
Dscho

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

[Stefan Beller]

Hi Johannes,

On Thu, May 3, 2018 at 8:30 AM, Johannes Schindelin
 wrote:
> This builtin does not do a whole lot so far, apart from showing a usage
> that is oddly similar to that of `git tbdiff`. And for a good reason:
> the next commits will turn `branch-diff` into a full-blown replacement
> for `tbdiff`.

While I appreciate the 1:1 re-implementation, I'll comment as if this
was a newly invented tool, questioning design choices. They are probably
chosen pretty well, and fudge facotrs as below are at tweaked to a reasonable
factor, but I'll try to look with fresh eyes.

>
> At this point, we ignore tbdiff's color options, as they will all be
> implemented later and require some patches to the diff machinery.

Speaking of colors, for origin/sb/blame-color Junio hinted at re-using
cyan for "uninteresting" parts to deliver a consistent color scheme for
Git. Eventually he dreams of having 2 layers of indirection IIUC, with
"uninteresting" -> cyan
"repeated lines in blame" -> uninteresting

Maybe we can fit the coloring of this tool in this scheme, too?

> +   double creation_weight = 0.6;

I wondered if we use doubles in Gits code base at all,
and I found

khash.h:59:static const double __ac_HASH_UPPER = 0.77;
pack-bitmap-write.c:248:static const double
REUSE_BITMAP_THRESHOLD = 0.2;
pack-bitmap.c:751:  static const double REUSE_PERCENT = 0.9;

all other occurrences of `git grep double` are mentioning it in other
contexts (e.g. "double linked list" or comments).

When implementing diff heuristics in 433860f3d0b (diff: improve
positioning of add/delete blocks in diffs, 2016-09-05), Michael broke
it down to fixed integers instead of floating point.

Do we need to dynamic of a floating point, or would a rather small range
suffice here? (Also see rename detection settings, that take percents as
integers)

Thanks,
Stefan

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

[Duy Nguyen]

On Thu, May 3, 2018 at 5:30 PM, Johannes Schindelin
 wrote:
> diff --git a/command-list.txt b/command-list.txt
> index a1fad28fd82..c89ac8f417f 100644
> --- a/command-list.txt
> +++ b/command-list.txt
> @@ -19,6 +19,7 @@ git-archive mainporcelain
>  git-bisect  mainporcelain   info
>  git-blame   ancillaryinterrogators
>  git-branch  mainporcelain   history
> +git-branch-diff mainporcelain   info

Making it part of "git help" with the info keywords at this stage may
be premature. "git help" is about _common_ commands and we don't know
(yet) how popular this will be.

(I'm not complaining though, I almost wrote "what witchcraft is this
and where can I get it" when I see branch-diff output mentioned in
AEvar's mail)
-- 
Duy

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

[Ramsay Jones]

On 03/05/18 16:30, Johannes Schindelin wrote:
> This builtin does not do a whole lot so far, apart from showing a usage
> that is oddly similar to that of `git tbdiff`. And for a good reason:
> the next commits will turn `branch-diff` into a full-blown replacement
> for `tbdiff`.
> 
> At this point, we ignore tbdiff's color options, as they will all be
> implemented later and require some patches to the diff machinery.
> 
> Signed-off-by: Johannes Schindelin 
> ---
>  .gitignore|  1 +
>  Makefile  |  1 +
>  builtin.h |  1 +
>  builtin/branch-diff.c | 40 
>  command-list.txt  |  1 +
>  git.c |  1 +
>  6 files changed, 45 insertions(+)
>  create mode 100644 builtin/branch-diff.c
> 
> diff --git a/.gitignore b/.gitignore
> index 833ef3b0b78..1346a64492f 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -20,6 +20,7 @@
>  /git-bisect--helper
>  /git-blame
>  /git-branch
> +/git-branch-diff
>  /git-bundle
>  /git-cat-file
>  /git-check-attr
> diff --git a/Makefile b/Makefile
> index 96f2e76a904..9b1984776d8 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -953,6 +953,7 @@ BUILTIN_OBJS += builtin/archive.o
>  BUILTIN_OBJS += builtin/bisect--helper.o
>  BUILTIN_OBJS += builtin/blame.o
>  BUILTIN_OBJS += builtin/branch.o
> +BUILTIN_OBJS += builtin/branch-diff.o
>  BUILTIN_OBJS += builtin/bundle.o
>  BUILTIN_OBJS += builtin/cat-file.o
>  BUILTIN_OBJS += builtin/check-attr.o
> diff --git a/builtin.h b/builtin.h
> index 42378f3aa47..e1c4d2a529a 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -135,6 +135,7 @@ extern int cmd_archive(int argc, const char **argv, const 
> char *prefix);
>  extern int cmd_bisect__helper(int argc, const char **argv, const char 
> *prefix);
>  extern int cmd_blame(int argc, const char **argv, const char *prefix);
>  extern int cmd_branch(int argc, const char **argv, const char *prefix);
> +extern int cmd_branch_diff(int argc, const char **argv, const char *prefix);
>  extern int cmd_bundle(int argc, const char **argv, const char *prefix);
>  extern int cmd_cat_file(int argc, const char **argv, const char *prefix);
>  extern int cmd_checkout(int argc, const char **argv, const char *prefix);
> diff --git a/builtin/branch-diff.c b/builtin/branch-diff.c
> new file mode 100644
> index 000..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 [] ( A..B C..D | A...B | base A B )"),

s/rebase--helper/branch-diff/

ATB,
Ramsay Jones