* Re: [PATCH v2 2/2] submodules: always use a relative path from gitdir to work tree
From: Jens Lehmann @ 2012-02-14 17:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Antony Male, Phil Hord
In-Reply-To: <7vlio6ec7q.fsf@alter.siamese.dyndns.org>
Am 13.02.2012 20:59, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
>> + a=$(cd "$gitdir" && pwd)
>> + b=$(cd "$path" && pwd)
>> + while [ "$b" ] && [ "${a%%/*}" = "${b%%/*}" ]
>> + do
>> + a=${a#*/} b=${b#*/};
>> + done
>> + rel=$(echo $a | sed -e 's|[^/]*|..|g')
>> + (clear_local_git_env; cd "$path" && git config core.worktree "$rel/$b")
>> }
>
>
> The style ([ "$b" ] vs "test -n "$b") aside, it strikes me odd that you
> only check $b; it is unclear what guarantees that "$a" is always longer.
> Maybe there is a reason but then a one-line comment here would not hurt?
I just copied that while loop from a few lines up, but you are right
about the style and logic issues (Will send a cleanup patch for the
other loop too when we agree on how to write this one).
After adding a comment, using test instead of [], testing both $a and
$b and assigning each variable on it's own line I get the following
interdiff. Does that make more sense?
diff --git a/git-submodule.sh b/git-submodule.sh
index 3463d6d..ed76ce2 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -172,9 +172,11 @@ module_clone()
a=$(cd "$gitdir" && pwd)
b=$(cd "$path" && pwd)
- while [ "$b" ] && [ "${a%%/*}" = "${b%%/*}" ]
+ # Remove all common leading directories
+ while test -n "$a" && test -n "$b" && test "${a%%/*}" = "${b%%/*}"
do
- a=${a#*/} b=${b#*/};
+ a=${a#*/}
+ b=${b#*/}
done
rel=$(echo $a | sed -e 's|[^/]*|..|g')
(clear_local_git_env; cd "$path" && git config core.worktree "$rel/$b")
^ permalink raw reply related
* Re: cvs2git migration
From: Holger Hellmuth @ 2012-02-14 20:17 UTC (permalink / raw)
To: supadhyay; +Cc: git
In-Reply-To: <1329220866066-7283631.post@n2.nabble.com>
On 14.02.2012 13:01, supadhyay wrote:
> Hi All,
>
> I want to migrate my version control CVS to GIT. I have few links which
> mentioned about how to migrate CVS repositories to GIT but there is no
> description about how to migrate existing CVS users to GIT?
As far as I know CVS uses its own server called pserver. And the usual
username and password combo for authentication.
if you want to use git with a central repository, you should use
gitolite. And gitolite uses RSA or DSA keys that are not compatible with
CVS passwords. You can't simply migrate the users to gitolite.
Every user has to generate and send the public half of that key
(generated with ssh-keygen) to the administrator of that repository.
If you have problems authenticating all those users sending you public
keys, let them check in the public keys into a specific path in one of
the old CVS repositories. Then the administrator of the git repository
can fetch them from the CVS (and check that the right user added the
patch for his new key).
^ permalink raw reply
* Re: [PATCH v2 2/2] submodules: always use a relative path from gitdir to work tree
From: Junio C Hamano @ 2012-02-14 20:24 UTC (permalink / raw)
To: Jens Lehmann; +Cc: Git Mailing List, Antony Male, Phil Hord
In-Reply-To: <4F3A9B98.6040908@web.de>
Jens Lehmann <Jens.Lehmann@web.de> writes:
> After adding a comment, using test instead of [], testing both $a and
> $b and assigning each variable on it's own line I get the following
> interdiff. Does that make more sense?
My earlier request for comment was to say
# $a is always longer than $b for such and such reasons
to explain why testing $b without testing $a was sufficient.
It is obvious (at least to me) that the loop continues as long as $a and
$b begin with the same string before their first '/' and removes that
common segment from both of them, so I do not think the new comment is
absolutely necessary, but it would not hurt to have it, especially it is
short enough and to the point.
Thanks.
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 3463d6d..ed76ce2 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -172,9 +172,11 @@ module_clone()
>
> a=$(cd "$gitdir" && pwd)
> b=$(cd "$path" && pwd)
> - while [ "$b" ] && [ "${a%%/*}" = "${b%%/*}" ]
> + # Remove all common leading directories
> + while test -n "$a" && test -n "$b" && test "${a%%/*}" = "${b%%/*}"
> do
> - a=${a#*/} b=${b#*/};
> + a=${a#*/}
> + b=${b#*/}
> done
> rel=$(echo $a | sed -e 's|[^/]*|..|g')
> (clear_local_git_env; cd "$path" && git config core.worktree "$rel/$b")
^ permalink raw reply
* Re: [PATCH 2/2] t: mailmap: add simple name translation test
From: Felipe Contreras @ 2012-02-14 20:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Marius Storm-Olsen, Jim Meyering, Jonathan Nieder
In-Reply-To: <7v8vk56us3.fsf@alter.siamese.dyndns.org>
On Tue, Feb 14, 2012 at 10:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>
> It was clear that we didn't have any test for "blame -e" hence it was no
> brainer to judge that the patch 1/2 is good without any description.
>
> But I am scratching my head, deciphering what this patch adds.
>
> It appears to me that the existing tests that map author@example.com from
> the original "A U Thor" to "Repo Guy" and inspect names and mails in
> various output already cover this "Wrong with <right@company.xx> can be
> corrected to Mr. Right" case this patch adds.
Yes, but in the first tests they don't check for 'git blame', and much
less 'git blame -e', and the second tests only check complex mappings.
> What am I missing? Instead of explaining it to me, can it be explained in
> the log message?
If an explanations along the lines of the above make sense, I can resend.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: git-latexdiff: Git and Latexdiff working together
From: Tim Haga @ 2012-02-14 20:19 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git
In-Reply-To: <vpq7gzph7mi.fsf@bauges.imag.fr>
Hi,
your tool seems very useful for LaTeX users, especially in scientific
work. I could not test it in detail until now, but i had two or
three runs with an article draft and it seems to work.
Long story short, i would appreciate it if your tool would be
integrated in git.
T.
Am Tue, 14 Feb 2012 14:22:45 +0100
schrieb Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>:
> Hi,
>
> You may know latexdiff, a neat tool to visualize differences between
> LaTeX files (it annotates your .tex file with colors for removed/added
> parts, producing another compilable .tex file).
>
> I wrote a little shell-script that allows one to use latexdiff on files
> versionned by Git, with e.g.
>
> git latexdiff HEAD^ --main foo.tex --output foo.pdf
>
> Essentially, it does a checkout of the old and new revisions, and calls
> latexdiff + pdflatex for you.
>
> The result is attached in case anyone is interested.
>
> It may be relevant to add this to contrib/ in git.git. If anyone's
> interested, let me know, and I'll resend the code in the form of a
> patch doing that.
>
> Regards,
>
^ permalink raw reply
* Re: diff --stat
From: Jeff King @ 2012-02-14 20:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vfwed6uws.fsf@alter.siamese.dyndns.org>
On Tue, Feb 14, 2012 at 12:07:31PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Hmm. Looking at scale_linear, the formula is:
> >
> > return ((it - 1) * (width - 1) + max_change - 1) / (max_change - 1);
> >
> > I don't see how that can be accurate, since the magnitude of the "-1"
> > tweak will vary based on the value of "it". This code is due to
> > 3ed74e6, but I don't quite follow the logic in the commit message.
>
> Doesn't it need +1 at the end, I wonder? We want to map:
>
> - zero to zero
> - any number to at least 1
>
> so scaling a non-zero "it" so that maximum maps to (width-1) and then
> adding 1 would be the right way for the latter case.
Yeah, that makes more sense to me. I think you could also get by with
simply rounding the above properly to the nearest integer (so a little
bit of error that makes 23.9 into 24.0 would be OK, because we would end
up rounding 30.9 to 31.0, too). This seems like the most obvious
solution to me:
static int divide_and_round(int a, int b)
{
return (2 * a + b) / (2 * b);
}
/*
* We want non-zero changes to have at least 1 marker, so special-case
* zero, then scale to width-1, and add back in 1.
*/
static int scale_linear(int it, int width, int max_change)
{
if (!it)
return 0;
return 1 + divide_and_round(it * (width-1), max_change);
}
Any "must show at least 1" scheme is going to have some error in the
scaling (because we are rounding up all of the low end). I have a
feeling that the scheme from 3ed74e6 was trying to distribute that error
more evenly throughout the scale, and the scheme above is lumping all of
it at the start (i.e., the difference between what constitutes one
marker and two markers is much greater than the difference between two
and three).
But that's just a vague feeling. For some reason my brain is not doing
well with math today, so I'll forego writing a proof.
> Of course, an easy way out without worrying about the correct math is to
> scale the total and the smaller one and then declare that the scaled
> larger one is the difference between the two. That way, both of these two
> files have 109 in total so the length of the entire graph would be the
> same ;-).
It looks like we actually did that, pre-3ed74e6. I think it's a valid
strategy. It is just pushing the error around, but I don't know that
people are counting the +/- markers on the line and comparing them
exactly. A little error there is less of a big deal than error between
two lines which are supposed to have the same number of changes (you'll
note that we don't give the per-file added/deleted numbers exactly, so
they are a good place to hide error. :) ).
-Peff
^ permalink raw reply
* Re: [PATCH v2 2/2] submodules: always use a relative path from gitdir to work tree
From: Junio C Hamano @ 2012-02-14 20:34 UTC (permalink / raw)
To: Jens Lehmann; +Cc: Git Mailing List, Antony Male, Phil Hord
In-Reply-To: <7v4nut6u4s.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
>
>> After adding a comment, using test instead of [], testing both $a and
>> $b and assigning each variable on it's own line I get the following
>> interdiff. Does that make more sense?
>
> My earlier request for comment was to say
>
> # $a is always longer than $b for such and such reasons
>
> to explain why testing $b without testing $a was sufficient.
Heh, after I follow the entire module_clone, $gitdir is defined in earlier
parts of the function to be "$(rev-parse --git-dir)/modules/$path", so it
is clear that it is longer than $path. Unless "cd $there && pwd" does not
result in a funny situation (such as $something/modules is a symbolic link
to another place that is much closer to the root of the filesystem), that
is.
And in such a case, the prefix part of $a and $b would be different from
the very beginning hopefully.
> It is obvious (at least to me) that the loop continues as long as $a and
> $b begin with the same string before their first '/' and removes that
> common segment from both of them, so I do not think the new comment is
> absolutely necessary, but it would not hurt to have it, especially it is
> short enough and to the point.
>
> Thanks.
>
>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index 3463d6d..ed76ce2 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -172,9 +172,11 @@ module_clone()
>>
>> a=$(cd "$gitdir" && pwd)
>> b=$(cd "$path" && pwd)
>> - while [ "$b" ] && [ "${a%%/*}" = "${b%%/*}" ]
>> + # Remove all common leading directories
>> + while test -n "$a" && test -n "$b" && test "${a%%/*}" = "${b%%/*}"
>> do
>> - a=${a#*/} b=${b#*/};
>> + a=${a#*/}
>> + b=${b#*/}
>> done
>> rel=$(echo $a | sed -e 's|[^/]*|..|g')
Perhaps aseert that $a never becomes empty before this line (or set it
explicitly to "." when $a is empty), as otherwise
>> (clear_local_git_env; cd "$path" && git config core.worktree "$rel/$b")
this will refer to "/$b" from the root?
^ permalink raw reply
* [PATCH v3 0/2] test: tests for the "double > from mailmap" bug
From: Jonathan Nieder @ 2012-02-14 20:34 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Jeff King
In-Reply-To: <1329235894-20581-1-git-send-email-felipe.contreras@gmail.com>
Hi again,
Felipe Contreras wrote:
> I sent both the fix and the tests. Another fix was applied, but we are still
> missing the tests.
>
> These are good before, and after the fix.
To summarize the previous discussion[1]: some people had comments, and
you seem to have found value in exactly none of them. OK. CC-ing
Peff, since he at least probably has looked over this code before.
By the way, the address you are using for Marius is out of date.
> Felipe Contreras (2):
> t: mailmap: add 'git blame -e' tests
So that people don't destroy this test in later refactorings, I would
like to collect statements that we want the test to ensure remain
true.
Apparently the fix in f026358e ("mailmap: always return a plain mail
address from map_user()", 2012-02-05) was for the case of the name
changing and email address not changing due to mailmap mapping. Most
callers use a separate buffer for the email address so there is room
to modify the name in place, but "git blame" keeps angle brackets in
the same buffer for no obvious reason I can see. (Callers should
be able to add the brackets themselves instead of relying on
ci.author_mail to contain them, but that's a story for another day.)
Anyway, the existing tests for the returned email missed this since
it does not affect "git shortlog -e" but only "git blame -e".
Therefore this patch reuses the test data for shortlog -e and lets us
use it for blame, too. It is easier to understand after the other
patch, IMHO.
This is _not_ meant as a more general test for the "git blame -e"
format (which would belong somewhere near t8008) as far as I can tell.
It is just checking that mailmap doesn't screw up.
> t: mailmap: add simple name translation test
Before, I thought this might be a straightforward test for the bug
fixed by f026358e. That didn't justify the patch that touches
several different test assertions.
In fact it seems to be intended to test the case addressed by f026358e
(name changing, email not) in various mailmap callers: "git shortlog -e",
"git log --pretty", "git blame".
Here's a reroll.
Enjoy,
Jonathan
Felipe Contreras (2):
test: mailmap can change author name without changing email
test: check that "git blame -e" uses mailmap correctly
t/t4203-mailmap.sh | 28 ++++++++++++++++++++++++++++
1 files changed, 28 insertions(+), 0 deletions(-)
[1] http://thread.gmane.org/gmane.comp.version-control.git/189896
^ permalink raw reply
* [PATCH 1/2] test: mailmap can change author name without changing email
From: Jonathan Nieder @ 2012-02-14 20:35 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Jeff King
In-Reply-To: <20120214203431.GB13210@burratino>
From: Felipe Contreras <felipe.contreras@gmail.com>
A mailmap entry of the format 'A U Thor <email@example.com>' has two
effects: (1) 'A U Thor' becomes the canonical author name for commits
with author address 'email@example.com', and (2) 'email@example.com'
becomes the canonical author email for commits with author name 'A U
Thor'.
We already have tests for the effect (1) in the committer name, but
not in the author name, so the tests do not cover the shortlog and
blame codepaths as they should. Fix that.
[jn: simplified by reusing Committer mailmap entry, clarified
description]
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
t/t4203-mailmap.sh | 12 ++++++++++++
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index 1f182f61..45526395 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -157,6 +157,9 @@ A U Thor <author@example.com> (1):
CTO <cto@company.xx> (1):
seventh
+Committed <committer@example.com> (1):
+ eighth
+
Other Author <other@author.xx> (2):
third
fourth
@@ -196,6 +199,11 @@ test_expect_success 'Shortlog output (complex mapping)' '
test_tick &&
git commit --author "CTO <cto@coompany.xx>" -m seventh &&
+ echo eight >>one &&
+ git add one &&
+ test_tick &&
+ git commit --author "C O Mitter <committer@example.com>" -m eighth &&
+
mkdir -p internal_mailmap &&
echo "Committed <committer@example.com>" > internal_mailmap/.mailmap &&
echo "<cto@company.xx> <cto@coompany.xx>" >> internal_mailmap/.mailmap &&
@@ -212,6 +220,9 @@ test_expect_success 'Shortlog output (complex mapping)' '
# git log with --pretty format which uses the name and email mailmap placemarkers
cat >expect <<\EOF
+Author C O Mitter <committer@example.com> maps to Committed <committer@example.com>
+Committer C O Mitter <committer@example.com> maps to Committed <committer@example.com>
+
Author CTO <cto@coompany.xx> maps to CTO <cto@company.xx>
Committer C O Mitter <committer@example.com> maps to Committed <committer@example.com>
@@ -248,6 +259,7 @@ OBJID (Other Author DATE 4) four
OBJID (Santa Claus DATE 5) five
OBJID (Santa Claus DATE 6) six
OBJID (CTO DATE 7) seven
+OBJID (Committed DATE 8) eight
EOF
test_expect_success 'Blame output (complex mapping)' '
git blame one >actual &&
--
1.7.9
^ permalink raw reply related
* [PATCH 2/2] test: check that "git blame -e" uses mailmap correctly
From: Jonathan Nieder @ 2012-02-14 20:36 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Jeff King
In-Reply-To: <20120214203431.GB13210@burratino>
From: Felipe Contreras <felipe.contreras@gmail.com>
Until f026358e ("mailmap: always return a plain mail address from
map_user()", 2012-02-05), git blame -e would add a spurious '>' after
the unchanged email address with brackets it passed to the mailmap
machinery, resulting in lines with a doubled '>' like this:
620456e6 (<committer@example.com>> 2005-04-07 15:20:13 -0700 8) eight
Add a test to make sure it doesn't happen again. This reuses the test
data for the existing "shortlog -e" test so it also tests other kinds
of mail mapping.
[jn: fixed some cut+paste cruft, added a patch description]
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
t/t4203-mailmap.sh | 16 ++++++++++++++++
1 files changed, 16 insertions(+), 0 deletions(-)
diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index 45526395..ef900d84 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -267,4 +267,20 @@ test_expect_success 'Blame output (complex mapping)' '
test_cmp expect actual.fuzz
'
+cat >expect <<\EOF
+^OBJI (<author@example.com> DATE 1) one
+OBJID (<some@dude.xx> DATE 2) two
+OBJID (<other@author.xx> DATE 3) three
+OBJID (<other@author.xx> DATE 4) four
+OBJID (<santa.claus@northpole.xx> DATE 5) five
+OBJID (<santa.claus@northpole.xx> DATE 6) six
+OBJID (<cto@company.xx> DATE 7) seven
+OBJID (<committer@example.com> DATE 8) eight
+EOF
+test_expect_success 'Blame -e output' '
+ git blame -e one >actual &&
+ fuzz_blame actual >actual.fuzz &&
+ test_cmp expect actual.fuzz
+'
+
test_done
--
1.7.9
^ permalink raw reply related
* Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff
From: Jeff King @ 2012-02-14 20:38 UTC (permalink / raw)
To: Michał Kiedrowicz; +Cc: Jakub Narebski, git
In-Reply-To: <20120214210453.16c6bd2e@gmail.com>
On Tue, Feb 14, 2012 at 09:04:53PM +0100, Michał Kiedrowicz wrote:
> Sure, now it's a simple algorithm, but if we add more code, we will
> have problems with making it consistent in both gitweb and
> diff-highlight (which is nice-to-have IMO).
True. I was thinking they would stay simple enough that porting features
wouldn't be too painful. But that might be overly optimistic.
> Note that my patches to gitweb already support combined diffs (in
> obvious cases) while diff-highlight will fail on them badly (see for
> example 09bb4eb4f14c). I haven't done it in diff-highlight because I
> noticed that problem while working on patches for gitweb.
Hmm. 09bb4eb4f14c looks fine to me, because all of the combined lines
are from the left-hand side. A better example is 4802997, where the
first hunk properly highlights, but the second does not (because we
interpret the lines as context lines).
Even worse is "git show 5de89d3 -- notes-cache.c", where we match a "- "
line with a "++" line, and erroneously think the changes begin in the
second character (even though it is simply a combined-diff marker).
These are reasonably rare, so I don't consider them critical bugs, but
yeah, it would be nice to fix.
-Peff
^ permalink raw reply
* Re: [PATCH v3 0/2] test: tests for the "double > from mailmap" bug
From: Felipe Contreras @ 2012-02-14 21:06 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Jeff King
In-Reply-To: <20120214203431.GB13210@burratino>
Hi,
On Tue, Feb 14, 2012 at 10:34 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> I sent both the fix and the tests. Another fix was applied, but we are still
>> missing the tests.
>>
>> These are good before, and after the fix.
>
> To summarize the previous discussion[1]: some people had comments, and
> you seem to have found value in exactly none of them. OK. CC-ing
> Peff, since he at least probably has looked over this code before.
Just because you have comments doesn't mean I *must* address them. We
have a difference of opinion, nothing wrong with that.
> By the way, the address you are using for Marius is out of date.
>
>> Felipe Contreras (2):
>> t: mailmap: add 'git blame -e' tests
>
> So that people don't destroy this test in later refactorings, I would
> like to collect statements that we want the test to ensure remain
> true.
>
> Apparently the fix in f026358e ("mailmap: always return a plain mail
> address from map_user()", 2012-02-05) was for the case of the name
> changing and email address not changing due to mailmap mapping. Most
> callers use a separate buffer for the email address so there is room
> to modify the name in place, but "git blame" keeps angle brackets in
> the same buffer for no obvious reason I can see. (Callers should
> be able to add the brackets themselves instead of relying on
> ci.author_mail to contain them, but that's a story for another day.)
>
> Anyway, the existing tests for the returned email missed this since
> it does not affect "git shortlog -e" but only "git blame -e".
> Therefore this patch reuses the test data for shortlog -e and lets us
> use it for blame, too. It is easier to understand after the other
> patch, IMHO.
This is not related to f026358e. The test added by this patch would
pass before and after f026358e.
This is what I mean by reading too much into the patch. There's
nothing to it; "add 'git blame -e' tests" just adds tests for 'git
blame -e', it doesn't catch any issues related to f026358e; it just
makes sure there are no further regressions with the tests that are
already passing.
Again, some tests for 'git blame -e' > no tests for 'git blame -e'.
> This is _not_ meant as a more general test for the "git blame -e"
> format (which would belong somewhere near t8008) as far as I can tell.
> It is just checking that mailmap doesn't screw up.
Thus the 't: mailmap: ' prefix.
>> t: mailmap: add simple name translation test
>
> Before, I thought this might be a straightforward test for the bug
> fixed by f026358e. That didn't justify the patch that touches
> several different test assertions.
>
> In fact it seems to be intended to test the case addressed by f026358e
> (name changing, email not) in various mailmap callers: "git shortlog -e",
> "git log --pretty", "git blame".
No. As the summary says, it's intended to add a simple name
translation test, which is missing from all the tests that spawn from
the repository generated in 'Shortlog output (complex mapping)' test.
This is the most minimal patch that can be generated if you add a
commit to this repository, and any further tests that are related to
it would look the same.
As Junio pointed out what is missing from the explanation is that this
simple name translation test is targeted toward the 'git blame'
commands, because such translation is not tested for them currently.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH v3 0/2] test: tests for the "double > from mailmap" bug
From: Jeff King @ 2012-02-14 21:14 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Felipe Contreras, git
In-Reply-To: <20120214203431.GB13210@burratino>
On Tue, Feb 14, 2012 at 02:34:31PM -0600, Jonathan Nieder wrote:
> Felipe Contreras wrote:
>
> > I sent both the fix and the tests. Another fix was applied, but we are still
> > missing the tests.
> >
> > These are good before, and after the fix.
>
> To summarize the previous discussion[1]: some people had comments, and
> you seem to have found value in exactly none of them. OK. CC-ing
> Peff, since he at least probably has looked over this code before.
The short answer is that both patches look OK to me.
Here's a longer digression that you may feel free to ignore.
When I investigated this problem, I looked only at the code (and I think
Junio's fix is the right one). However, having just looked at your two
patches without having previously looked at those tests, I found them
quite painless to review.
Having participated in the bug-hunt, I knew they were probably related
to testing that bug, but it was very unclear to me from the original
series why there were two patches, and not one. Or why, when we have
mailmap tests, they did not catch this bug. And that is _now_, with the
context of having just participated in the discussion; six months from
now I would have even less hope of understanding the context.
My general review process is (in this order):
1. Figure out why a change is needed. This should be explained in the
commit message. And no, just adding tests is not assumed to be
needed. Why did the old tests not cover this case? The answer can
be a simple "nobody bothered to write them", and that's OK. But
some description of the current state can help reviewers understand
the rationale (e.g., "we tested with shortlog, but not mailmap, and
certain code paths are only exposed through mailmap").
2. Figure out what the change should be doing. This should also be in
the commit message, though at a high level. In the case of tests,
obviously we're adding new tests. But are we properly covering all
of the new cases? Does the "what" match the "why" from (1)?
3. Look at the patch. Do the changes match what the commit message
claimed in (2)?
4. Look at the patch for style, implementation, etc. Is the code
quality good enough to be included?
The steps are dependent on each other, and I usually quit if I can't get
past one step. Sometimes I will look at the patch to try to figure out
(1) or (2), especially if the patch is trivial. But in my ideal world,
every patch lets me just walk through those steps.
I won't claim that these steps create error-free reviews. Based on those
steps, your patches look good to me, and it seems from Felipe's response
that there might be some inaccuracies in your description. But it at
least gives us a starting point for discussion.
-Peff
^ permalink raw reply
* Re: [PATCH v3 0/2] test: tests for the "double > from mailmap" bug
From: Jonathan Nieder @ 2012-02-14 21:15 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Jeff King, Junio C Hamano
In-Reply-To: <CAMP44s3YRHgMPX2Hzydm_TLB27OABWETjABMcwrHmDk-=pN2hw@mail.gmail.com>
Felipe Contreras wrote:
> On Tue, Feb 14, 2012 at 10:34 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> To summarize the previous discussion[1]: some people had comments, and
>> you seem to have found value in exactly none of them. OK. CC-ing
>> Peff, since he at least probably has looked over this code before.
>
> Just because you have comments doesn't mean I *must* address them. We
> have a difference of opinion, nothing wrong with that.
I said "OK", didn't I?
[...]
>> In fact it seems to be intended to test the case addressed by f026358e
>> (name changing, email not) in various mailmap callers: "git shortlog -e",
>> "git log --pretty", "git blame".
>
> No. As the summary says, it's intended to add a simple name
> translation test, which is missing from all the tests that spawn from
> the repository generated in 'Shortlog output (complex mapping)' test.
> This is the most minimal patch that can be generated if you add a
> commit to this repository, and any further tests that are related to
> it would look the same.
>
> As Junio pointed out what is missing from the explanation is that this
> simple name translation test is targeted toward the 'git blame'
> commands, because such translation is not tested for them currently.
Um. So this has nothing to do with f026358e at all? Mentioning that
commit and that this test does not pass with an older codebase is not
useful to the humans that will look over this test later?
Adding explanation and rearranging things so people encountering this
later have to spend _less_ time to understand it is something I
consider useful. It means people are less likely to randomly break
things. I don't actually understand where the difference of opinion
comes from here.
^ permalink raw reply
* Re: git-latexdiff: Git and Latexdiff working together
From: Jeff King @ 2012-02-14 21:16 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git
In-Reply-To: <vpq7gzph7mi.fsf@bauges.imag.fr>
On Tue, Feb 14, 2012 at 02:22:45PM +0100, Matthieu Moy wrote:
> I wrote a little shell-script that allows one to use latexdiff on files
> versionned by Git, with e.g.
>
> git latexdiff HEAD^ --main foo.tex --output foo.pdf
My latex usage is all from a past life, so I didn't even try out your
tool. But I did wonder what your rationale was in making a separate
command as opposed to providing a script that could be plugged in as an
external diff.
-Peff
^ permalink raw reply
* Re: [PATCH v3 0/2] test: tests for the "double > from mailmap" bug
From: Jonathan Nieder @ 2012-02-14 21:27 UTC (permalink / raw)
To: Jeff King; +Cc: Felipe Contreras, git
In-Reply-To: <20120214211402.GC23291@sigill.intra.peff.net>
Jeff King wrote:
> The short answer is that both patches look OK to me.
Thanks for looking them over and for a nice explanation of the process.
^ permalink raw reply
* Re: [PATCH 1/2] test: mailmap can change author name without changing email
From: Felipe Contreras @ 2012-02-14 21:35 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Jeff King
In-Reply-To: <20120214203519.GC13210@burratino>
On Tue, Feb 14, 2012 at 10:35 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> From: Felipe Contreras <felipe.contreras@gmail.com>
> test: mailmap can change author name without changing email
That doesn't say anything to me, which is weird if I supposedly wrote
this patch. What is the *purpose* of this?
At least 'add simple name translation test' is clear about the
purpose, albeit not clear enough as Junio pointed out.
> (2) 'email@example.com'
> becomes the canonical author email for commits with author name 'A U
> Thor'.
That's not true. I initially thought that was the case, and I think it
might be useful to have that, but it's not the case now, and your
patch doesn't test this.
> We already have tests for the effect (1) in the committer name, but
> not in the author name, so the tests do not cover the shortlog and
> blame codepaths as they should. Fix that.
In order to test that you would need additional changes, something
along the lines of:
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -157,8 +157,9 @@ A U Thor <author@example.com> (1):
CTO <cto@company.xx> (1):
seventh
-Committed <committer@example.com> (1):
+Committed <committer@example.com> (2):
eighth
+ nine
Other Author <other@author.xx> (2):
third
@@ -204,6 +205,11 @@ test_expect_success 'Shortlog output (complex mapping)' '
test_tick &&
git commit --author "C O Mitter <committer@example.com>" -m eighth &&
+ echo nine >>one &&
+ git add one &&
+ test_tick &&
+ git commit --author "Committed <bad@example.com>" -m nine &&
+
mkdir -p internal_mailmap &&
echo "Committed <committer@example.com>" > internal_mailmap/.mailmap &&
echo "<cto@company.xx>
<cto@coompany.xx>" >> internal_mailmap/.mailmap &&
@@ -220,6 +226,9 @@ test_expect_success 'Shortlog output (complex mapping)' '
# git log with --pretty format which uses the name and email mailmap
placemarkers
cat >expect <<\EOF
+Author Committed <committer@example.com> maps to Committed
<committer@example.com>
+Committer C O Mitter <committer@example.com> maps to Committed
<committer@example.com>
+
Author C O Mitter <committer@example.com> maps to Committed
<committer@example.com>
Committer C O Mitter <committer@example.com> maps to Committed
<committer@example.com>
@@ -260,6 +269,7 @@ OBJID (Santa Claus DATE 5) five
OBJID (Santa Claus DATE 6) six
OBJID (CTO DATE 7) seven
OBJID (Committed DATE 8) eight
+OBJID (Committed DATE 9) nine
EOF
test_expect_success 'Blame output (complex mapping)' '
git blame one >actual &&
But that of course fails.
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
I most definitely did not sign this off, and I didn't add any of these
lines, nor wrote anything about this commit message.
It might be possible to simplify my patch "t: mailmap: add simple name
translation test" using the already existing "Committed
<committer@example.com>" mapping, but that most likely is going to
remove only one line, and would make the code less clear about what
that translation is trying to test.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH 2/2] test: check that "git blame -e" uses mailmap correctly
From: Felipe Contreras @ 2012-02-14 21:41 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Jeff King
In-Reply-To: <20120214203603.GD13210@burratino>
On Tue, Feb 14, 2012 at 10:36 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> From: Felipe Contreras <felipe.contreras@gmail.com>
> test: check that "git blame -e" uses mailmap correctly
I wonder what extra information is in that text that is not in my
original "t: mailmap: add 'git blame -e' tests". I guess all tests
'check' something, and the purpose is to make sure things work
'correctly.
> Until f026358e ("mailmap: always return a plain mail address from
> map_user()", 2012-02-05), git blame -e would add a spurious '>' after
> the unchanged email address with brackets it passed to the mailmap
> machinery, resulting in lines with a doubled '>' like this:
>
> 620456e6 (<committer@example.com>> 2005-04-07 15:20:13 -0700 8) eight
>
> Add a test to make sure it doesn't happen again. This reuses the test
> data for the existing "shortlog -e" test so it
> also tests other kinds of mail mapping.
'Also' is a keyword that strongly denotes this patch is doing more
than one logical thing. My patch adds those checks *independently* of
the fix on f026358e, so it's truly logically independent.
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
I did not sign this.
--
Felipe Contreras
^ permalink raw reply
* Re: git-cherry filter
From: Jeff King @ 2012-02-14 21:44 UTC (permalink / raw)
To: Neal Kreitzinger; +Cc: git
In-Reply-To: <jhcnfc$aec$1@dough.gmane.org>
On Mon, Feb 13, 2012 at 10:20:59PM -0600, Neal Kreitzinger wrote:
> Is there a way to add a pre-git-patch-id filter to git-cherry?
Not directly, but see below.
> (a) perform "keyword contraction" to the patch before generating the
> git-patch-id.
>
> e.g. I want to run a git-cherry to see if two patches are identical other
> than keyword expansion values like $User: foo$ vs. $User: bar$. (I would
> have to tell git-cherry which keyword formats to "contract".)
Shouldn't these be contracted already in the canonical versions in the
repo?
> (b) ignore comments in the source code.
This one is tough, because you are talking about arbitrary
transformation of the diff.
> (c) exclude certain files from the diff (ie., binaries, comment files,
> etc.).
This one is perhaps easier, and git-cherry could learn to use the
regular pathspec-limiting when generating patch-ids.
In any case, git-cherry is basically just comparing the set of patch ids
in one set to the patch-ids in the other set. So you could do all of
the above transformations by hacking together something like:
merge_base=`git merge-base $upstream $head`
get_patch_ids() {
git rev-list $merge_base..$1 |
git diff-tree --stdin -p -- $path_limiters |
munge_your_diff_however_you_like |
git patch-id
}
get_patch_ids $head >head
get_patch_ids $upstream | cut -d' ' -f1 >upstream
perl -l <<-\EOF
open(my $head, '<', 'head');
open(my $upstream, '<', 'upstream');
my %upstream = map { $_ => 1 } <$upstream>;
while (<$head>) {
chomp;
my ($patch, $commit) = split;
print exists($upstream{$patch}) ? '+' : '-',
' ',
$commit;
}
EOF
-Peff
^ permalink raw reply
* Re: What's cooking in git.git (Feb 2012, #05; Mon, 13)
From: Jeff King @ 2012-02-14 21:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v4nuuea7r.fsf@alter.siamese.dyndns.org>
On Mon, Feb 13, 2012 at 12:42:48PM -0800, Junio C Hamano wrote:
> * jk/config-include (2012-02-06) 2 commits
> (merged to 'next' on 2012-02-13 at 307ddf6)
> + config: add include directive
> + docs: add a basic description of the config API
>
> An assignment to the include.path pseudo-variable causes the named file
> to be included in-place when Git looks up configuration variables.
Yikes. I was planning to re-roll this, but got sidetracked discussing
David's git-cola case. Besides a few minor tweaks in the documentation
patch, the actual include patch is buggy, and accidentally turns on
includes for "git config --list".
Do you want to revert and re-do to make master pretty, or should I just
build on top?
-Peff
^ permalink raw reply
* Re: diff --stat
From: Junio C Hamano @ 2012-02-14 21:49 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20120214202934.GA23291@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Feb 14, 2012 at 12:07:31PM -0800, Junio C Hamano wrote:
>
>> Of course, an easy way out without worrying about the correct math is to
>> scale the total and the smaller one and then declare that the scaled
>> larger one is the difference between the two. That way, both of these two
>> files have 109 in total so the length of the entire graph would be the
>> same ;-).
>
> It looks like we actually did that, pre-3ed74e6. I think it's a valid
> strategy. It is just pushing the error around,...
Yes, that is exactly why I suggested that approach. We have to deal with
rounding error somewhere no matter what we do, and the balance between
add/del is much less noticeable than the change with the same total not
lining up.
Anyway, here is an obvious patch to fix this. We did not have any test
that failed with this change, as all our test vectors fit comfortably on
the default 80-column output.
-- >8 --
Subject: diff --stat: resurrect "same for same" heuristic
When commit 3ed74e6 (diff --stat: ensure at least one '-' for deletions,
and one '+' for additions, 2006-09-28) improved the output for files with
tiny modifications, we accidentally broke rounding logic to ensure that
two equal sized changes are shown with the bars of the same length.
This updates the logic to compute the length of the graph bars, using the
same "non-zero changes is shown with at least one column" scaling logic,
but by scaling the sum of additions and deletions to come up with the
total length of the bar (this ensures that two equal sized changes result
in bars of the same length), and then scaling the smaller of the additions
or deletions. The other side is computed as the difference between the
two. This makes the apportioning between additions and deletions less
accurate due to rounding errors, but it is much less noticeable than two
files with the same amount of change showing bars of different length.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
diff.c | 27 +++++++++++++++++++++------
1 file changed, 21 insertions(+), 6 deletions(-)
diff --git a/diff.c b/diff.c
index 3550c18..76d4724 100644
--- a/diff.c
+++ b/diff.c
@@ -1273,13 +1273,17 @@ const char mime_boundary_leader[] = "------------";
static int scale_linear(int it, int width, int max_change)
{
+ if (!it)
+ return 0;
/*
- * make sure that at least one '-' is printed if there were deletions,
- * and likewise for '+'.
+ * make sure that at least one '-' or '+' is printed if
+ * there is any change to this path. The easiest way is to
+ * scale linearly as if all the quantities were one smaller
+ * than they actually are, and then add one to the result.
*/
if (max_change < 2)
- return it;
- return ((it - 1) * (width - 1) + max_change - 1) / (max_change - 1);
+ return 1;
+ return 1 + ((it - 1) * (width - 1) / (max_change - 1));
}
static void show_name(FILE *file,
@@ -1495,8 +1499,19 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
dels += del;
if (width <= max_change) {
- add = scale_linear(add, width, max_change);
- del = scale_linear(del, width, max_change);
+ int total = add + del;
+
+ total = scale_linear(add + del, width, max_change);
+ if (total < 2 && add && del)
+ /* width >= 2 due to the sanity check */
+ total = 2;
+ if (add < del) {
+ add = scale_linear(add, width, max_change);
+ del = total - add;
+ } else {
+ del = scale_linear(del, width, max_change);
+ add = total - del;
+ }
}
fprintf(options->file, "%s", line_prefix);
show_name(options->file, prefix, name, len);
^ permalink raw reply related
* Re: [PATCH 1/2] test: mailmap can change author name without changing email
From: Jonathan Nieder @ 2012-02-14 21:50 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Jeff King
In-Reply-To: <CAMP44s3di25SbMa1T1=0_s6f-rKnniwEcA+o5HWT7xedcghSeg@mail.gmail.com>
Felipe Contreras wrote:
> On Tue, Feb 14, 2012 at 10:35 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> (2) 'email@example.com'
>> becomes the canonical author email for commits with author name 'A U
>> Thor'.
>
> That's not true. I initially thought that was the case, and I think it
> might be useful to have that, but it's not the case now, and your
> patch doesn't test this.
Thanks for explaining. I had indeed confused myself into thinking 'A
U Thor <email@example.com>' would act like 'A U Thor
<email@example.com> <email@example.com>'.
I should have said:
-- 8< --
A mailmap entry in the format 'A U Thor <email@example.com>' means
that 'A U Thor' should be the canonical author name for commits
with author address 'email@example.com', and the email address
should be left alone.
We already have tests for this format regarding the committer name,
but not in the author name, so the tests do not cover the shortlog and
blame codepaths as they should. Fix that.
-- >8 --
[...]
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>
> I most definitely did not sign this off, and I didn't add any of these
> lines, nor wrote anything about this commit message.
That's why I described the changes I made, signed with my initials,
and put my own sign-off below yours. Did I screw up somewhere?
Note that I am making these changes because, at its heart, I think
your patch is good and useful. Otherwise I would have ignored it and
worked on something else. If you prefer that I don't make
improvements like this, please indicate why that's a good idea;
otherwise I will probably continue to do it when I see good patches,
despite all the signals you are giving that I have done something
awful by corrupting your perfect patch in this way.
Jonathan
^ permalink raw reply
* Re: [PATCH v3 0/2] test: tests for the "double > from mailmap" bug
From: Felipe Contreras @ 2012-02-14 21:52 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Nieder, git
In-Reply-To: <20120214211402.GC23291@sigill.intra.peff.net>
On Tue, Feb 14, 2012 at 11:14 PM, Jeff King <peff@peff.net> wrote:
> My general review process is (in this order):
>
> 1. Figure out why a change is needed. This should be explained in the
> commit message. And no, just adding tests is not assumed to be
> needed. Why did the old tests not cover this case?
As I already mentioned more than once; the first patch is not related
to any fix.
> The answer can
> be a simple "nobody bothered to write them", and that's OK.
That can be derived from the word "add". You can't add something that
is already there.
> But
> some description of the current state can help reviewers understand
> the rationale (e.g., "we tested with shortlog, but not mailmap, and
> certain code paths are only exposed through mailmap").
You are assuming too much. That's not the purpose, that's why I didn't menti
> 2. Figure out what the change should be doing.
t: mailmap: add 'git blame -e' tests
That's what the change should be doing; nothing more, nothing less.
I wonder why you have to assume the worst. When I see a commit message
like that, I assume that there were no previous tests for that (thus
the word 'add'), and that's all I need to know.
If you want to extrude meaning from where there isn't, well, go ahead,
but there's nothing else really.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: diff --stat
From: Jeff King @ 2012-02-14 21:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vty2t5bmw.fsf@alter.siamese.dyndns.org>
On Tue, Feb 14, 2012 at 01:49:11PM -0800, Junio C Hamano wrote:
> static int scale_linear(int it, int width, int max_change)
> {
> + if (!it)
> + return 0;
> /*
> - * make sure that at least one '-' is printed if there were deletions,
> - * and likewise for '+'.
> + * make sure that at least one '-' or '+' is printed if
> + * there is any change to this path. The easiest way is to
> + * scale linearly as if all the quantities were one smaller
> + * than they actually are, and then add one to the result.
> */
> if (max_change < 2)
> - return it;
> - return ((it - 1) * (width - 1) + max_change - 1) / (max_change - 1);
> + return 1;
> + return 1 + ((it - 1) * (width - 1) / (max_change - 1));
I'm not sure I understand why the "it - 1" and "max_change - 1" bits are
still there, or what they are doing in the first place. I.e., why is it
not simply "scale linearly to width-1, then add 1" as I posted earlier?
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] test: check that "git blame -e" uses mailmap correctly
From: Jonathan Nieder @ 2012-02-14 21:59 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Jeff King, Junio C Hamano
In-Reply-To: <CAMP44s2M8Ava6xyKX32h9+pbxG+zq1wH1RkdWwfNsZMzcVQEmw@mail.gmail.com>
Felipe Contreras wrote:
> On Tue, Feb 14, 2012 at 10:36 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>
> I did not sign this.
Do you mean you do not agree with the following?
Developer's Certificate of Origin 1.1
By making a contribution to this project, I certify that:
(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or
(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or
(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.
(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.
Anyway, Junio, please feel free to remove Felipe's sign-off from these
patches, or add an "Against-the-protest-of:" or whatever if that's
what he wants.
I've reached the point of diminishing returns. I'll be happy to help
in any way I can, given a polite explanation of how I can help.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox