* Re: [PATCH v3 0/2] test: tests for the "double > from mailmap" bug
From: Felipe Contreras @ 2012-02-14 22:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Jonathan Nieder, git
In-Reply-To: <7v8vk55a99.fsf@alter.siamese.dyndns.org>
On Wed, Feb 15, 2012 at 12:18 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Tue, Feb 14, 2012 at 11:14 PM, Jeff King <peff@peff.net> wrote:
>> ...
>>> 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.
>
> You are correct to say that you cannot add something that is already
> there, but that does not mean you explained why that new thing is a good
> thing to add. In other words, you can add a new thing that we did not
> have, but it would not result in a good addition if that new thing is
> irrelevant. Relevance needs to be explained.
>
> I do think in this particular case, the new check *is* relevant, because
>
> Although we did have "blame" test to see how the name part is shown,
> we had no "blame -e" test to see how the email part is shown.
>
> I do not understand why you are resisting to explain how your addition
> adds value to the system with such a simple two-liner, and instead are
> endlessly arguing. Is it to make sure you are the one to utter the last
> word in the thread?
And I don't understand why people want the obvious to be explained.
For each one of the questions I've heard, the answer can be derived
from the summary *directly*.
Your new point is "you can add a new thing that we did not have, but
it would not result in a good addition if that new thing is
irrelevant", but you already know what is the new thing from the
summary "'git blame -e' tests".
Everybody seems to assume that a simple commit message = bad. I disagree.
> As I am sort of getting tired of seeing you making things more difficult
> for yourself,
The same argument can be applied both ways; you are the one that is
making things more difficult. And I already pointed out the double
standards.
> I'll refrain from commenting on this topic at least for a
> few days to wait until things cool down.
Does this mean that disagreement is discouraged? From the discussion
it seems we are in clear disagreement, but from the rhetoric it seems
to me that you all assume you are right by default, and that simple
commit messages are by bad definition.
If that's the case I am disappointed, but I would rather just drop the
discussion rather than defend my position.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* [PATCHv3] git-p4: add initial support for RCS keywords
From: Luke Diamand @ 2012-02-14 22:33 UTC (permalink / raw)
To: git; +Cc: Pete Wyckoff, Eric Scouten, Luke Diamand
In-Reply-To: <1329258835-17223-1-git-send-email-luke@diamand.org>
RCS keywords cause problems for git-p4 as perforce always
expands them (if +k is set) and so when applying the patch,
git reports that the files have been modified by both sides,
when in fact they haven't.
This change means that when git-p4 detects a problem applying
a patch, it will check to see if keyword expansion could be
the culprit. If it is, it strips the keywords in the p4
repository so that they match what git is expecting. It then
has another go at applying the patch.
This behaviour is enabled with a new git-p4 configuration
option and is off by default.
Improved-by: Pete Wyckoff <pw@padd.com>
Signed-off-by: Luke Diamand <luke@diamand.org>
---
Documentation/git-p4.txt | 5 +
contrib/fast-import/git-p4 | 116 +++++++++++++++--
t/t9810-git-p4-rcs.sh | 304 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 415 insertions(+), 10 deletions(-)
create mode 100755 t/t9810-git-p4-rcs.sh
diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 8b92cc0..7fa47c8 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -483,6 +483,11 @@ git-p4.skipUserNameCheck::
user map, 'git p4' exits. This option can be used to force
submission regardless.
+git-p4.attemptRCSCleanup:
+ If enabled, 'git p4 submit' will attempt to sort cleanup RCS keywords
+ ($Header$, etc). These would otherwise cause merge conflicts and prevent
+ the submit going ahead. This option should be considered experimental at
+ present.
IMPLEMENTATION DETAILS
----------------------
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index a78d9c5..8130447 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -10,7 +10,7 @@
import optparse, sys, os, marshal, subprocess, shelve
import tempfile, getopt, os.path, time, platform
-import re
+import re, shutil
verbose = False
@@ -186,6 +186,44 @@ def split_p4_type(p4type):
mods = s[1]
return (base, mods)
+#
+# return the raw p4 type of a file (text, text+ko, etc)
+#
+def p4_type(file):
+ files = p4_read_pipe_lines(["files", file])
+ info = files[0]
+ m = re.search(r'\(([a-z0-9A-Z+]+)\)\s*$', info)
+ if m:
+ ret = m.group(1)
+ if verbose:
+ print "%s => %s" % (file, ret)
+ return ret
+ else:
+ die("Could not extract file type from '%s'" % info)
+
+#
+# Given a type base and modifier, return a regexp matching
+# the keywords that can be expanded in the file
+#
+def p4_keywords_regexp_for_type(base, type_mods):
+ if base in ("text", "unicode", "binary"):
+ if "ko" in type_mods:
+ return r'\$(Id|Header)[^$]*\$'
+ elif "k" in type_mods:
+ return r'\$(Id|Header|Author|Date|DateTime|Change|File|Revision)[^$]*\$'
+ else:
+ return None
+ else:
+ return None
+
+#
+# Given a file, return a regexp matching the possible
+# RCS keywords that will be expanded, or None for files
+# with kw expansion turned off.
+#
+def p4_keywords_regexp_for_file(file):
+ (type_base, type_mods) = split_p4_type(p4_type(file))
+ return p4_keywords_regexp_for_type(type_base, type_mods)
def setP4ExecBit(file, mode):
# Reopens an already open file and changes the execute bit to match
@@ -753,6 +791,28 @@ class P4Submit(Command, P4UserMap):
return result
+ def patchRCSKeywords(self, file, pattern):
+ # Attempt to zap the RCS keywords in a p4 controlled file matching the given pattern
+ (handle, outFileName) = tempfile.mkstemp(dir='.')
+ try:
+ outFile = os.fdopen(handle, "w+")
+ inFile = open(file, "r")
+ for line in inFile.readlines():
+ line = re.sub(pattern, r'$\1$', line)
+ outFile.write(line)
+ inFile.close()
+ outFile.close()
+ # Forcibly overwrite the original file
+ os.unlink(file)
+ shutil.move(outFileName, file)
+ except:
+ # cleanup our temporary file
+ os.unlink(outFileName)
+ print "Failed to strip RCS keywords in %s" % file
+ raise
+
+ print "Patched up RCS keywords in %s" % file
+
def p4UserForCommit(self,id):
# Return the tuple (perforce user,git email) for a given git commit id
self.getUserMapFromPerforceServer()
@@ -918,6 +978,7 @@ class P4Submit(Command, P4UserMap):
filesToDelete = set()
editedFiles = set()
filesToChangeExecBit = {}
+
for line in diff:
diff = parseDiffTreeEntry(line)
modifier = diff['status']
@@ -964,9 +1025,48 @@ class P4Submit(Command, P4UserMap):
patchcmd = diffcmd + " | git apply "
tryPatchCmd = patchcmd + "--check -"
applyPatchCmd = patchcmd + "--check --apply -"
+ patch_succeeded = True
if os.system(tryPatchCmd) != 0:
+ fixed_rcs_keywords = False
+ patch_succeeded = False
print "Unfortunately applying the change failed!"
+
+ # Patch failed, maybe it's just RCS keyword woes. Look through
+ # the patch to see if that's possible.
+ if gitConfig("git-p4.attemptRCSCleanup","--bool") == "true":
+ file = None
+ pattern = None
+ kwfiles = {}
+ for line in read_pipe_lines(diffcmd):
+ # read diff lines: for each file reported, if it can have
+ # keywords expanded, and the diff contains keywords, then
+ # try zapping the p4 file.
+ m = re.match(r'^diff --git a/(.*)\s+b/(.*)', line)
+ if m:
+ file = m.group(1)
+ pattern = p4_keywords_regexp_for_file(file)
+ next
+
+ if pattern:
+ print line
+
+ if pattern and re.search(pattern, line):
+ print "got match on %s in %s in %s" % (pattern, line, file)
+ kwfiles[file] = pattern
+
+ for file in kwfiles:
+ if verbose:
+ print "zapping %s with %s" % (line,pattern)
+ self.patchRCSKeywords(file, kwfiles[file])
+ fixed_rcs_keywords = True
+
+ if fixed_rcs_keywords:
+ print "Retrying the patch with RCS keywords cleaned up"
+ if os.system(tryPatchCmd) == 0:
+ patch_succeeded = True
+
+ if not patch_succeeded:
print "What do you want to do?"
response = "x"
while response != "s" and response != "a" and response != "w":
@@ -1585,15 +1685,11 @@ class P4Sync(Command, P4UserMap):
# Note that we do not try to de-mangle keywords on utf16 files,
# even though in theory somebody may want that.
- if type_base in ("text", "unicode", "binary"):
- if "ko" in type_mods:
- text = ''.join(contents)
- text = re.sub(r'\$(Id|Header):[^$]*\$', r'$\1$', text)
- contents = [ text ]
- elif "k" in type_mods:
- text = ''.join(contents)
- text = re.sub(r'\$(Id|Header|Author|Date|DateTime|Change|File|Revision):[^$]*\$', r'$\1$', text)
- contents = [ text ]
+ pattern = p4_keywords_regexp_for_type(type_base, type_mods)
+ if pattern:
+ text = ''.join(contents)
+ text = re.sub(pattern, r'$\1$', text)
+ contents = [ text ]
self.gitStream.write("M %s inline %s\n" % (git_mode, relPath))
diff --git a/t/t9810-git-p4-rcs.sh b/t/t9810-git-p4-rcs.sh
new file mode 100755
index 0000000..77eebd2
--- /dev/null
+++ b/t/t9810-git-p4-rcs.sh
@@ -0,0 +1,304 @@
+#!/bin/sh
+
+test_description='git-p4 rcs keywords'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+ start_p4d
+'
+
+#
+# Make one file with keyword lines at the top, and
+# enough plain text to be able to test modifications
+# far away from the keywords.
+#
+test_expect_success 'init depot' '
+ (
+ cd "$cli" &&
+ cat <<-\EOF >filek &&
+ $Id$
+ /* $Revision$ */
+ # $Change$
+ line4
+ line5
+ line6
+ line7
+ line8
+ EOF
+ cp filek fileko &&
+ sed -i "s/Revision/Revision: do not scrub me/" fileko
+ cp fileko file_text &&
+ sed -i "s/Id/Id: do not scrub me/" file_text
+ p4 add -t text+k filek &&
+ p4 submit -d "filek" &&
+ p4 add -t text+ko fileko &&
+ p4 submit -d "fileko" &&
+ p4 add -t text file_text &&
+ p4 submit -d "file_text"
+ )
+'
+
+#
+# Generate these in a function to make it easy to use single quote marks.
+#
+write_scrub_scripts() {
+ cat >"$TRASH_DIRECTORY/scrub_k.py" <<-\EOF &&
+ import re, sys
+ sys.stdout.write(re.sub(r'(?i)\$(Id|Header|Author|Date|DateTime|Change|File|Revision):[^$]*\$', r'$\1$', sys.stdin.read()))
+ EOF
+ cat >"$TRASH_DIRECTORY/scrub_ko.py" <<-\EOF
+ import re, sys
+ sys.stdout.write(re.sub(r'(?i)\$(Id|Header):[^$]*\$', r'$\1$', sys.stdin.read()))
+ EOF
+}
+
+test_expect_success 'scrub scripts' '
+ write_scrub_scripts
+'
+
+#
+# Compare $cli/file to its scrubbed version, should be different.
+# Compare scrubbed $cli/file to $git/file, should be same.
+#
+scrub_k_check() {
+ file=$1 &&
+ scrub="$TRASH_DIRECTORY/$file" &&
+ "$PYTHON_PATH" "$TRASH_DIRECTORY/scrub_k.py" <"$git/$file" >"$scrub" &&
+ ! test_cmp "$cli/$file" "$scrub" &&
+ test_cmp "$git/$file" "$scrub" &&
+ rm "$scrub"
+}
+scrub_ko_check() {
+ file=$1 &&
+ scrub="$TRASH_DIRECTORY/$file" &&
+ "$PYTHON_PATH" "$TRASH_DIRECTORY/scrub_ko.py" <"$git/$file" >"$scrub" &&
+ ! test_cmp "$cli/$file" "$scrub" &&
+ test_cmp "$git/$file" "$scrub" &&
+ rm "$scrub"
+}
+
+#
+# Modify far away from keywords. If no RCS lines show up
+# in the diff, there is no conflict.
+#
+test_expect_success 'edit far away from RCS lines' '
+ test_when_finished cleanup_git &&
+ "$GITP4" clone --dest="$git" //depot &&
+ (
+ cd "$git" &&
+ git config git-p4.skipSubmitEdit true &&
+ sed -i "s/^line7/line7 edit/" filek &&
+ git commit -m "filek line7 edit" filek &&
+ "$GITP4" submit &&
+ scrub_k_check filek
+ )
+'
+
+#
+# Modify near the keywords. This will require RCS scrubbing.
+#
+test_expect_success 'edit near RCS lines' '
+ test_when_finished cleanup_git &&
+ "$GITP4" clone --dest="$git" //depot &&
+ (
+ cd "$git" &&
+ git config git-p4.skipSubmitEdit true &&
+ git config git-p4.attemptRCSCleanup true &&
+ sed -i "s/^line4/line4 edit/" filek &&
+ git commit -m "filek line4 edit" filek &&
+ "$GITP4" submit &&
+ scrub_k_check filek
+ )
+'
+
+#
+# Modify the keywords themselves. This also will require RCS scrubbing.
+#
+test_expect_success 'edit keyword lines' '
+ test_when_finished cleanup_git &&
+ "$GITP4" clone --dest="$git" //depot &&
+ (
+ cd "$git" &&
+ git config git-p4.skipSubmitEdit true &&
+ git config git-p4.attemptRCSCleanup true &&
+ sed -i "/Revision/d" filek &&
+ git commit -m "filek remove Revision line" filek &&
+ "$GITP4" submit &&
+ scrub_k_check filek
+ )
+'
+
+#
+# Scrubbing text+ko files should not alter all keywords, just Id, Header.
+#
+test_expect_success 'scrub ko files differently' '
+ test_when_finished cleanup_git &&
+ "$GITP4" clone --dest="$git" //depot &&
+ (
+ cd "$git" &&
+ git config git-p4.skipSubmitEdit true &&
+ git config git-p4.attemptRCSCleanup true &&
+ sed -i "s/^line4/line4 edit/" fileko &&
+ git commit -m "fileko line4 edit" fileko &&
+ "$GITP4" submit &&
+ scrub_ko_check fileko &&
+ ! scrub_k_check fileko
+ )
+'
+
+# hack; git-p4 submit should do it on its own
+test_expect_success 'cleanup after failure' '
+ (
+ cd "$cli" &&
+ p4 revert ...
+ )
+'
+
+#
+# Do not scrub anything but +k or +ko files. Sneak a change into
+# the cli file so that submit will get a conflict. Make sure that
+# scrubbing doesn't make a mess of things.
+#
+# Assumes that git-p4 exits leaving the p4 file open, with the
+# conflict-generating patch unapplied.
+#
+# This might happen only if the git repo is behind the p4 repo at
+# submit time, and there is a conflict.
+#
+test_expect_success 'do not scrub plain text' '
+ test_when_finished cleanup_git &&
+ "$GITP4" clone --dest="$git" //depot &&
+ (
+ cd "$git" &&
+ git config git-p4.skipSubmitEdit true &&
+ git config git-p4.attemptRCSCleanup true &&
+ sed -i "s/^line4/line4 edit/" file_text &&
+ git commit -m "file_text line4 edit" file_text &&
+ (
+ cd "$cli" &&
+ p4 open file_text &&
+ sed -i "s/^line5/line5 p4 edit/" file_text &&
+ p4 submit -d "file5 p4 edit"
+ ) &&
+ ! "$GITP4" submit &&
+ (
+ # exepct something like:
+ # file_text - file(s) not opened on this client
+ # but not copious diff output
+ cd "$cli" &&
+ p4 diff file_text >wc &&
+ test_line_count = 1 wc
+ )
+ )
+'
+
+# hack; git-p4 submit should do it on its own
+test_expect_success 'cleanup after failure 2' '
+ (
+ cd "$cli" &&
+ p4 revert ...
+ )
+'
+
+create_kw_file() {
+ cat <<\EOF >"$1"
+/* A file
+ Id: $Id$
+ Revision: $Revision$
+ File: $File$
+ */
+int main(int argc, const char **argv) {
+ return 0;
+}
+EOF
+}
+
+test_expect_success 'add kwfile' '
+ (
+ cd "$cli" &&
+ echo file1 >file1 &&
+ p4 add file1 &&
+ p4 submit -d "file 1" &&
+ create_kw_file kwfile1.c &&
+ p4 add kwfile1.c &&
+ p4 submit -d "Add rcw kw file" kwfile1.c
+ )
+'
+
+p4_append_to_file() {
+ f=$1 &&
+ p4 edit -t ktext "$f" &&
+ echo "/* $(date) */" >>"$f" &&
+ p4 submit -d "appending a line in p4"
+}
+
+# Create some files with RCS keywords. If they get modified
+# elsewhere then the version number gets bumped which then
+# results in a merge conflict if we touch the RCS kw lines,
+# even though the change itself would otherwise apply cleanly.
+test_expect_success 'cope with rcs keyword expansion damage' '
+ test_when_finished cleanup_git &&
+ "$GITP4" clone --dest="$git" //depot &&
+ (
+ cd "$git" &&
+ git config git-p4.skipSubmitEdit true &&
+ git config git-p4.attemptRCSCleanup true &&
+ (cd ../cli && p4_append_to_file kwfile1.c) &&
+ old_lines=$(wc -l <kwfile1.c) &&
+ perl -n -i -e "print unless m/Revision:/" kwfile1.c &&
+ new_lines=$(wc -l <kwfile1.c) &&
+ expr $new_lines == $old_lines - 1 &&
+
+ git add kwfile1.c &&
+ git commit -m "Zap an RCS kw line" &&
+ "$GITP4" submit &&
+ "$GITP4" rebase &&
+ git diff p4/master &&
+ "$GITP4" commit &&
+ echo "try modifying in both" &&
+ cd "$cli" &&
+ p4 edit kwfile1.c &&
+ echo "line from p4" >>kwfile1.c &&
+ p4 submit -d "add a line in p4" kwfile1.c &&
+ cd "$git" &&
+ echo "line from git at the top" | cat - kwfile1.c >kwfile1.c.new &&
+ mv kwfile1.c.new kwfile1.c &&
+ git commit -m "Add line in git at the top" kwfile1.c &&
+ "$GITP4" rebase &&
+ "$GITP4" submit
+ )
+'
+
+test_expect_success 'cope with rcs keyword file deletion' '
+ test_when_finished cleanup_git &&
+ (
+ cd "$cli" &&
+ echo "\$Revision\$" >kwdelfile.c &&
+ p4 add -t ktext kwdelfile.c &&
+ p4 submit -d "Add file to be deleted" &&
+ cat kwdelfile.c &&
+ grep -q 1 kwdelfile.c
+ ) &&
+ "$GITP4" clone --dest="$git" //depot &&
+ (
+ cd "$git" &&
+ grep -q Revision kwdelfile.c &&
+ git rm -f kwdelfile.c &&
+ git commit -m "Delete a file containing RCS keywords" &&
+ git config git-p4.skipSubmitEdit true &&
+ git config git-p4.attemptRCSCleanup true &&
+ "$GITP4" submit
+ ) &&
+ (
+ cd "$cli" &&
+ p4 sync &&
+ [ ! -f kwdelfile.c ]
+ )
+'
+
+test_expect_success 'kill p4d' '
+ kill_p4d
+'
+
+test_done
--
1.7.9.259.ga92e
^ permalink raw reply related
* [PATCHv3] git-p4: RCS keyword handling
From: Luke Diamand @ 2012-02-14 22:33 UTC (permalink / raw)
To: git; +Cc: Pete Wyckoff, Eric Scouten, Luke Diamand
Updated patch to handle RCS keyword expansion performed by perforce
in git-p4. This incorporates Pete Wyckoff's suggestions and test
cases, and also adds a test for handling deletion of a file containing
RCS keywords.
Eric - could I ask you to take a look at this and see if it
solves your particular problem.
Thanks.
Luke Diamand (1):
git-p4: add initial support for RCS keywords
Documentation/git-p4.txt | 5 +
contrib/fast-import/git-p4 | 116 +++++++++++++++--
t/t9810-git-p4-rcs.sh | 304 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 415 insertions(+), 10 deletions(-)
create mode 100755 t/t9810-git-p4-rcs.sh
--
1.7.9.259.ga92e
^ permalink raw reply
* Re: [PATCH v3 0/2] test: tests for the "double > from mailmap" bug
From: Felipe Contreras @ 2012-02-14 22:22 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Nieder, git
In-Reply-To: <20120214220732.GB24802@sigill.intra.peff.net>
On Wed, Feb 15, 2012 at 12:07 AM, Jeff King <peff@peff.net> wrote:
> On Tue, Feb 14, 2012 at 11:52:11PM +0200, Felipe Contreras wrote:
>
>> 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.
>
> Really? I didn't see it mentioned in your commit message, which
> consisted entirely of:
>
> t: mailmap: add 'git blame -e' tests
>
> Yes, I know you mentioned it elsewhere in the thread. But review should
> happen on what is actually in the posted patch. That is what reviewers
> are guaranteed to have read, and it is what people reading "git log" in
> a year will see. If there was other discussion or context that led to
> the patch, it's helpful in both cases to summarize it.
Seriously? You want to add a note saying "These tests don't tackle any
known issues". Well, if it did, logically, it would have been
mentioned.
>> > 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.
>
> Are there already git-blame tests, but not "blame -e" tests? If there
> are already "blame" tests, why do we additionally need "blame -e" tests?
You are right, it's impossible to decipher that the output of 'blame
-e' is different than 'blame'. Surely, somebody must have made a
mistake when the patch was applied.
Or we can apply common sense.
> I can guess, or I can do my own digging in the history to find out, but
> that makes review a lot more painful. You already did the digging and
> came to understand the problem when you made your patch. Why not just
> share it?
Because there's no need.
Why would anybody add tests that are already there? Why do you have to
assume the worst?
>> > 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
>
> Sorry, that should have been s/mailmap/blame/ above. But if I am making
> wrong assumptions about the rationale, then isn't that a sign that the
> commit message is insufficient?
If you mean the second patch, this was already pointed out by Junio,
and I already said I would clarify that these are meant to target 'git
blame' output.
>> > 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.
>
> Yes, I think you did describe the "what", which in this case is very
> simple. But as I mentioned before, it is not just knowing the "what" but
> evaluating that the "what" meets the "why" from step 1.
The why can be derived. Unless you seriously think Junio would allow
patches that say they "add" tests for something (which imply there
isn't tests for that), where they don't.
>> 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.
>
> I don't necessarily assume the worst. If I were the maintainer, I might
> even have taken your patch as-is, as it's pretty simple. But I found a
> description like the one Jonathan included to be _much_ easier to
> review. Whether yours was above a minimum threshold or not, I think it's
> worth striving to be better.
Better != more words. English, like code, is good as simple as
possible (but not simpler).
If you start from the premise that the patch is good; it has been
s-o-b by Junio, and it has been applied, and released, what more do
you need from the summary "t: mailmap: add 'git blame -e' tests". Can
you derive the rest?
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH v3 0/2] test: tests for the "double > from mailmap" bug
From: Jonathan Nieder @ 2012-02-14 22:21 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Jeff King, Junio C Hamano
In-Reply-To: <CAMP44s1mV2cE=R49qYSLd8eZPhCpRx0hRnnG_-K3iBxp_YQEpQ@mail.gmail.com>
Felipe Contreras wrote:
> The difference of opinion is that I consider the patch already good
> enough (adding the comments from Junio).
Ok, I can understand where you're coming from there.
Now I have offered some suggestions for improving some of your
patches. Applying these suggestions would be effortless, since I sent
them in patch form. What is your response? You send a point-by-point
rebuttal explaining how each detail of my suggestions is bad, bad,
bad, and then resend the original with comparatively small changes.
Can you see how this might indicate a stronger difference of opinion
than you have mentioned? And how a reviewer might make the mistake of
thinking his comments were unwelcome after such an experience?
^ permalink raw reply
* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users
From: Jeff King @ 2012-02-14 22:20 UTC (permalink / raw)
To: Sven Strickroth; +Cc: Jakub Narebski, Junio C Hamano, git
In-Reply-To: <4F37E843.6070107@tu-clausthal.de>
On Sun, Feb 12, 2012 at 05:26:43PM +0100, Sven Strickroth wrote:
> Am 12.02.2012 17:11 schrieb Jakub Narebski:
> > Doesn't Subversion use SSH directly? If it is so, the question is
> > about how SSH itself supports SSH_ASKPASS.
>
> Oh sorry, I mixed up SSH and SVN_ASKPASS. :( Of couse SSH_ASKPASS is
> provided by the ssh-client itself.
That raises an interesting point for git (I don't remember seeing this
in the previous discussion, so apologies if I'm repeating). We sometimes
use SSH_ASKPASS for internal prompting, and sometimes via calling out to
ssh. So forgetting about git being consistent with the rest of the
world for a moment, I think we are inconsistent with ourselves. E.g.:
export SSH_ASKPASS=whatever
# this will try the terminal first, then SSH_ASKPASS, because it is
# ssh doing the asking
git push ssh://example.com/repo.git
# this will try SSH_ASKPASS first, then the terminal, because git is
# doing the asking
git push https://example.com/repo.git
So now I'm more convinced than ever that the order should be
GIT_ASKPASS, terminal, SSH_ASKPASS.
-Peff
^ permalink raw reply
* Re: [PATCH v3 0/2] test: tests for the "double > from mailmap" bug
From: Junio C Hamano @ 2012-02-14 22:18 UTC (permalink / raw)
To: Felipe Contreras; +Cc: Jeff King, Jonathan Nieder, git
In-Reply-To: <CAMP44s0Dp9Av+ikFHa=QcqKFA5XL9ESBrzWLY0jkSCdH-NxhMw@mail.gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
> On Tue, Feb 14, 2012 at 11:14 PM, Jeff King <peff@peff.net> wrote:
> ...
>> 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.
You are correct to say that you cannot add something that is already
there, but that does not mean you explained why that new thing is a good
thing to add. In other words, you can add a new thing that we did not
have, but it would not result in a good addition if that new thing is
irrelevant. Relevance needs to be explained.
I do think in this particular case, the new check *is* relevant, because
Although we did have "blame" test to see how the name part is shown,
we had no "blame -e" test to see how the email part is shown.
I do not understand why you are resisting to explain how your addition
adds value to the system with such a simple two-liner, and instead are
endlessly arguing. Is it to make sure you are the one to utter the last
word in the thread?
As I am sort of getting tired of seeing you making things more difficult
for yourself, I'll refrain from commenting on this topic at least for a
few days to wait until things cool down.
^ permalink raw reply
* Re: Git documentation at kernel.org
From: Jeff King @ 2012-02-14 22:15 UTC (permalink / raw)
To: Junio C Hamano
Cc: Scott Chacon, Matthieu Moy, Clemens Buchacher, ftpadmin,
Petr Onderka, git
In-Reply-To: <7vmx8nju1k.fsf@alter.siamese.dyndns.org>
On Sun, Feb 12, 2012 at 07:23:19PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > That sounds great to me. I'd like to be link-compatible with the old
> > kernel.org docs section (even if through redirects) so that old links
> > work (assuming kernel.org gives us a wholesale redirect). Which means
> > importing all of the docs for released versions. I don't know if the old
> > kernel.org doc tree was saved anywhere, but if I understand correctly,
> > they are identical to what's in the "git-htmldocs" repository (which I
> > _thought_ Junio wasn't going to keep updating, but it seems pretty up to
> > date).
> [...]
> But the contents of htmldocs does _not_ match what used to be at k.org in
> that its git.html does not have links to documentation pages for older
> releases, iow, formatted without "stalenotes" defined.
Ah, that makes sense. We might have to just rebuild the old versions
with stalenotes, then (our doc toolchain is so finicky that I worry
about minor incompatibilities in building old versions with a newer
toolchain, but it is probably good enough).
-Peff
^ permalink raw reply
* Re: What's cooking in git.git (Feb 2012, #05; Mon, 13)
From: Jeff King @ 2012-02-14 22:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vmx8l5aw3.fsf@alter.siamese.dyndns.org>
On Tue, Feb 14, 2012 at 02:05:16PM -0800, Junio C Hamano wrote:
> > 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".
>
> Hmm, I thought t1305 covered "config --list", and ... oops, it makes sure
> the output contains the inclusion.
Yes. It should include it (and does correctly) when not using any
per-file options, but does not correctly turn it off for the per-file
case (because we bail to regular git_config instead of custom lookup
code).
> > Do you want to revert and re-do to make master pretty, or should I just
> > build on top?
>
> Do you mean 'next'?
I meant "revert from next and re-reroll, so that when the re-roll gets
merged to master, the result there will look pretty".
-Peff
^ permalink raw reply
* Re: [PATCH v3 0/2] test: tests for the "double > from mailmap" bug
From: Felipe Contreras @ 2012-02-14 22:09 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Jeff King, Junio C Hamano
In-Reply-To: <20120214211552.GA9651@burratino>
On Tue, Feb 14, 2012 at 11:15 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> 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?
You also said I didn't find value in any of the comments which is
being passively aggressive. Comments always have value, that doesn't
meant they must all be turned into actionables.
> [...]
>>> 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?
I didn't say that. I say the *purpose* of the patch is different.
> 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.
The difference of opinion is that I consider the patch already good
enough (adding the comments from Junio). Yeah, now that f026358e is
committed might be worth mentioning, and what's the relationship, but
that's an *extra*. Even if f026358e wasn't there, and nobody knew what
was the issue, and possible fixes, the patch would be good by itself.
Again, some tests > no tests, and don't let the perfect be the enemy
of the good. It even looks like you prefer no changes at all (status
quo) to my proposed changes as you have never said these are "good",
but always paint them as "wrong" or "broken" in fundamental ways, as
if they are not worthy of being applied.
In any case, I will address the comments from Junio, and perhaps add a
note regarding f026358e.
I also can't help but feel you are applying double standards, as this
is the original patch that seems to have broken things:
ommit d20d654fe8923a502527547b17fe284d15d6aec9
Author: Marius Storm-Olsen <marius@trolltech.com>
Date: Sun Feb 8 15:34:30 2009 +0100
Change current mailmap usage to do matching on both name and email
of author/committer.
Signed-off-by: Marius Storm-Olsen <marius@trolltech.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Documentation/pretty-formats.txt | 2 +
builtin-blame.c | 50 +++++++++++-------
builtin-shortlog.c | 22 ++++++--
pretty.c | 57 +++++++++++----------
t/t4203-mailmap.sh | 106 ++++++++++++++++++++++++++++++++++++++
5 files changed, 186 insertions(+), 51 deletions(-)
Notice the short commit message. It would have been _nice_ to have a
bigger commit message, but it's better to have this commit than
nothing at all.
And see the relevant commit f026358:
f026358 mailmap: always return a plain mail address from map_user()
mailmap.c | 18 ++++++++++--------
1 files changed, 10 insertions(+), 8 deletions(-)
Notice how there's no tests, which would have reason enough to reject
the patches from many contributors.
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 22:07 UTC (permalink / raw)
To: Felipe Contreras; +Cc: Jonathan Nieder, git
In-Reply-To: <CAMP44s0Dp9Av+ikFHa=QcqKFA5XL9ESBrzWLY0jkSCdH-NxhMw@mail.gmail.com>
On Tue, Feb 14, 2012 at 11:52:11PM +0200, Felipe Contreras wrote:
> 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.
Really? I didn't see it mentioned in your commit message, which
consisted entirely of:
t: mailmap: add 'git blame -e' tests
Yes, I know you mentioned it elsewhere in the thread. But review should
happen on what is actually in the posted patch. That is what reviewers
are guaranteed to have read, and it is what people reading "git log" in
a year will see. If there was other discussion or context that led to
the patch, it's helpful in both cases to summarize it.
> > 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.
Are there already git-blame tests, but not "blame -e" tests? If there
are already "blame" tests, why do we additionally need "blame -e" tests?
I can guess, or I can do my own digging in the history to find out, but
that makes review a lot more painful. You already did the digging and
came to understand the problem when you made your patch. Why not just
share it?
> > 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
Sorry, that should have been s/mailmap/blame/ above. But if I am making
wrong assumptions about the rationale, then isn't that a sign that the
commit message is insufficient?
> > 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.
Yes, I think you did describe the "what", which in this case is very
simple. But as I mentioned before, it is not just knowing the "what" but
evaluating that the "what" meets the "why" from step 1.
> 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.
I don't necessarily assume the worst. If I were the maintainer, I might
even have taken your patch as-is, as it's pretty simple. But I found a
description like the one Jonathan included to be _much_ easier to
review. Whether yours was above a minimum threshold or not, I think it's
worth striving to be better.
-Peff
^ permalink raw reply
* Re: diff --stat
From: Junio C Hamano @ 2012-02-14 22:06 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20120214215318.GA24802@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I'm not sure I understand why the "it - 1" and "max_change - 1" bits are
> still there,...
Simple; I wasn't thinking straight.
^ permalink raw reply
* Re: What's cooking in git.git (Feb 2012, #05; Mon, 13)
From: Junio C Hamano @ 2012-02-14 22:05 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20120214214729.GA24711@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> 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".
Hmm, I thought t1305 covered "config --list", and ... oops, it makes sure
the output contains the inclusion.
> Do you want to revert and re-do to make master pretty, or should I just
> build on top?
Do you mean 'next'?
^ 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
* 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 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: [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: 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: 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: 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: [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: [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 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: 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: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
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