Our output methods translate ANSI, so the strbufs only hold the ANSI
sequences.
I'll always trust two Johannes's on Windows matters ;-)
I have no idea why "submodule --summary" uses --first-parent, but
personally, I would _hate_ it not to see the merged commits in the diff.
For a summary, you might get away with seeing
> Merge bla
> Merge blub
> Merge this
> Merge that
but in a diff that does not cut it at all.
As long as bla/blub/this/that are descriptive enough, I do not see at all
why you think "summary" is Ok and "diff" is not. If your response were
"it is just a matter of taste; to some people (or project) --first-parent
is useful and for others it is not", I would understand it, and it would
make sense to use (or not use) --first-parent consistently between this
codepath and "submodule --summary", though.
In any case, just to safe-guard against sick minds, I can add a check that
says that left, right, and all the merge bases _cannot_ have any flags
set, otherwise we output "(you should visit a psychiatrist)" or some such.
I wouldn't suggest adding such a kludge. Being insulting to the user when
we hit a corner case _we_ cannot handle does not help anybody, does it?
I see two saner options. Doing this list walking in a subprocess so that
you wouldn't have to worry about object flags at all in this case would
certainly be easier; the other option obviously is to have a separate
object pool ala libgit2, but that would be a much larger change.
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:47:29
Hi,
On Mon, 5 Oct 2009, Junio C Hamano wrote:
Johannes Schindelin [off-list ref] writes:
quoted
quoted
quoted
+ if (prepare_revision_walk(&rev))
+ message = "(revision walker failed)";
If prepare_revision_walk() failed for whatever reason, can we trust
fast_forward/fast_backward at this point?
No, but it is not used in that case, either, because message is not NULL
anymore.
It is used in that case a few lines below to decide if you add the third
dot. That's why I asked.
Well, fair enough.
The answer is: yes, we can still trust fast_forward/fast_backward, as
there is no question that if the first merge base (which must be the only
merge base by definition, in this case) is either "left" or "right", it is
fast_forward or fast_backward, respectively.
So: no worries.
quoted
I have no idea why "submodule --summary" uses --first-parent, but
personally, I would _hate_ it not to see the merged commits in the
diff.
For a summary, you might get away with seeing
> Merge bla
> Merge blub
> Merge this
> Merge that
but in a diff that does not cut it at all.
As long as bla/blub/this/that are descriptive enough, I do not see at all
why you think "summary" is Ok and "diff" is not. If your response were
"it is just a matter of taste; to some people (or project) --first-parent
is useful and for others it is not", I would understand it, and it would
make sense to use (or not use) --first-parent consistently between this
codepath and "submodule --summary", though.
You may be used to git.git's quality of naming the branches you merge.
Sadly, this is not the common case.
quoted
In any case, just to safe-guard against sick minds, I can add a check that
says that left, right, and all the merge bases _cannot_ have any flags
set, otherwise we output "(you should visit a psychiatrist)" or some such.
I wouldn't suggest adding such a kludge. Being insulting to the user when
we hit a corner case _we_ cannot handle does not help anybody, does it?
Well, I was a little exasperated when I wrote that that you want to handle
that case.
But of course, I should heed Postel's law, and handle the case. Maybe say
something like "(uses superproject's commits)".
I see two saner options. Doing this list walking in a subprocess so that
you wouldn't have to worry about object flags at all in this case would
certainly be easier; the other option obviously is to have a separate
object pool ala libgit2, but that would be a much larger change.
The reason why I insist avoiding a subprocess is performance. The same
reason holds for a separate object pool: it would just impede the speed,
AFAICT.
Besides, I vividly remember what happened to a patch I posted to be able
to just clear the current object pool. And I cannot imagine a patch
introducing a second pool to be any less complicated.
If you really want the case I illustrated (that the submodule actually
contains commits that already have been shown in the superproject) to be
handled showing the correct submodule summary (and with --first-parent, I
think you will agree that it is a summary, even if it is embedded in a
diff), I could imagine calling a subprocess (for simplicity reasons) _iff_
left, right, or any of the merge bases has a flag set.
But I really, really, really want to avoid a fork() in the common case. I
do have some users on Windows, and I do have a few submodules in that
project. Having too many fork() calls there would just give Git a bad
reputation. And it has enough of that, it does not need more.
Ciao,
Dscho
I have no idea why "submodule --summary" uses --first-parent, but
personally, I would _hate_ it not to see the merged commits in the
diff.
For a summary, you might get away with seeing
> Merge bla
> Merge blub
> Merge this
> Merge that
but in a diff that does not cut it at all.
As long as bla/blub/this/that are descriptive enough, I do not see at all
why you think "summary" is Ok and "diff" is not. If your response were
"it is just a matter of taste; to some people (or project) --first-parent
is useful and for others it is not", I would understand it, and it would
make sense to use (or not use) --first-parent consistently between this
codepath and "submodule --summary", though.
You may be used to git.git's quality of naming the branches you merge.
Sadly, this is not the common case.
IMHO both arguments are valid, using --first-parent really is a matter of
taste *and* it is dependent on the quality of branch naming whether it is
useful or not.
But when both commands shall produce the same output, i think we have to
use --first-parent as default, no? And maybe we could add another option
to diff which can change that behaviour according to users taste?
But I really, really, really want to avoid a fork() in the common case. I
do have some users on Windows, and I do have a few submodules in that
project. Having too many fork() calls there would just give Git a bad
reputation. And it has enough of that, it does not need more.
Me too thinks performance matters here. We do have a repo at my dayjob
with more than a handful of submodules and its main target platform is
windows ... so having that perform nicely is a win for us.
Jens
From: Junio C Hamano <hidden> Date: 2016-06-15 22:47:29
Jens Lehmann [off-list ref] writes:
quoted
But I really, really, really want to avoid a fork() in the common case. I
do have some users on Windows, and I do have a few submodules in that
project. Having too many fork() calls there would just give Git a bad
reputation. And it has enough of that, it does not need more.
Me too thinks performance matters here. We do have a repo at my dayjob
with more than a handful of submodules and its main target platform is
windows ... so having that perform nicely is a win for us.
Numbers?
I'd prefer to avoid kludges that favors unsubstantiated performance
argument over correctness.
Thanks.
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:47:29
Hi,
On Tue, 6 Oct 2009, Junio C Hamano wrote:
Jens Lehmann [off-list ref] writes:
quoted
quoted
But I really, really, really want to avoid a fork() in the common case. I
do have some users on Windows, and I do have a few submodules in that
project. Having too many fork() calls there would just give Git a bad
reputation. And it has enough of that, it does not need more.
Me too thinks performance matters here. We do have a repo at my dayjob
with more than a handful of submodules and its main target platform is
windows ... so having that perform nicely is a win for us.
Numbers?
I'd prefer to avoid kludges that favors unsubstantiated performance
argument over correctness.
Well, having worked with msysGit for such a long time, I just _know_ that
a subprocess costs a substantial amount of time.
But as you don't trust my words, maybe Jens could be so kind as to perform
some benchmarks? I am short on Git time budget, but I will make a commit
on my submodule-summary branch that allows to start a subprocess always.
Ciao,
Dscho
But I really, really, really want to avoid a fork() in the common case. I
do have some users on Windows, and I do have a few submodules in that
project. Having too many fork() calls there would just give Git a bad
reputation. And it has enough of that, it does not need more.
Me too thinks performance matters here. We do have a repo at my dayjob
with more than a handful of submodules and its main target platform is
windows ... so having that perform nicely is a win for us.
Numbers?
I'd prefer to avoid kludges that favors unsubstantiated performance
argument over correctness.
Well, having worked with msysGit for such a long time, I just _know_ that
a subprocess costs a substantial amount of time.
But as you don't trust my words, maybe Jens could be so kind as to perform
some benchmarks? I am short on Git time budget, but I will make a commit
on my submodule-summary branch that allows to start a subprocess always.
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:47:29
Hi,
On Tue, 6 Oct 2009, Jens Lehmann wrote:
Johannes Schindelin schrieb:
quoted
Hi,
On Tue, 6 Oct 2009, Junio C Hamano wrote:
quoted
Jens Lehmann [off-list ref] writes:
quoted
quoted
But I really, really, really want to avoid a fork() in the common case. I
do have some users on Windows, and I do have a few submodules in that
project. Having too many fork() calls there would just give Git a bad
reputation. And it has enough of that, it does not need more.
Me too thinks performance matters here. We do have a repo at my dayjob
with more than a handful of submodules and its main target platform is
windows ... so having that perform nicely is a win for us.
Numbers?
I'd prefer to avoid kludges that favors unsubstantiated performance
argument over correctness.
Well, having worked with msysGit for such a long time, I just _know_ that
a subprocess costs a substantial amount of time.
But as you don't trust my words, maybe Jens could be so kind as to perform
some benchmarks? I am short on Git time budget, but I will make a commit
on my submodule-summary branch that allows to start a subprocess always.
Sure, will do.
Okay, it is there. It is quick and dirty, so you don't even want to look
at the commit message.
Could you please run something like "time git diff --submodule-summary
--all" with and without this patch?
Thanks,
Dscho
But I really, really, really want to avoid a fork() in the common case. I
do have some users on Windows, and I do have a few submodules in that
project. Having too many fork() calls there would just give Git a bad
reputation. And it has enough of that, it does not need more.
Me too thinks performance matters here. We do have a repo at my dayjob
with more than a handful of submodules and its main target platform is
windows ... so having that perform nicely is a win for us.
Numbers?
Here they are:
First i did them with the repo at hand, current msysgit master with
Dscho's git-repo checked out:
without fork : real 0m0.672s
with fork : real 0m0.781s
So here it's a about 16% slower when using fork() (and both are
generating about 7270 shortlog entries).
But i thought this to be a rather unusual situation, having only one
submodule being changed by 7270 commits ...
So i took a live repo from my dayjob containing 8 submodules. In each
submodule i did a "git checkout HEAD^" to simulate one change. And
then i got:
without fork : real 0m0.203s
with fork : real 0m0.453s
This is a degradation of more than 120% because of the fork()s. And
just for fun i ran the scripted submodule summary too:
scripted : real 0m3.437s
So the forked version outperforms the scripted version by a factor of
7, while the speedup from Dscho's original proposal is almost 17fold.
(If i did my computations right, the extra costs for each changed
submodule are a bit more than 30ms when fork()ing. Dscho's version
doesn't seem to suffer from changed submodules at all, i measured
0.203s for both versions before i did the submodule init and update).
(Best of three, "time git diff --submodule-summary". My system is an
Athlon64x2 4600+ with WindowsXP)
Jens