Re: [PATCH] git-p4: Corrected typo.

From: Vitor Antunes <vitor.hda@gmail.com>
Date: Jan 29, 2011, 02:41 UTC
Message-ID: <AANLkTimLQxiB=dnd6=iB5uEpzOSfqks=FDJ=xORehuw8@mail.gmail.com>
In-Reply-To: <AANLkTikeB724f_vE6qvu1h1o5JG150mcmaHVBjLkOEWP@mail.gmail.com>

Hi Thomas,

First of all I'd like to thank you on your feedback. It's my first try on creating submitting a patch, so having someone's guidance helps a lot :)

I'll rebase my patches against the head of the tree and squash the fix to avoid multiple commits. While I do that I'll also review my commit message and sign-off the patches according to what you said. Hopefully I will be able to do this during this weekend.

From git-diff-tree man page:

"""
-M[<n>]
    Detect renames. If n is specified, it is a is a threshold on the
similarity index (i.e. amount of addition/deletions compared to the
file’s
    size). For example, -M90% means git should consider a delete/add
pair to be a rename if more than 90% of the file hasn’t changed.
"""

But from my latest tests I think that this option is ignored in diff-tree (I think it's only used in git log). With this in mind I'll need to add some code to implement the check of the score value of diff-tree output string. Again from its man page:

""" Status letters C and R are always followed by a score (denoting the percentage of similarity between the source and target of the move or copy), and are the only ones to be so. """

Thanks, Vitor

On Fri, Jan 28, 2011 at 3:19 PM, Thomas Berg <merlin66b@gmail.com> wrote:

Show 51 quoted lines

> Hi,
>
> On Fri, Jan 28, 2011 at 12:35 AM, Vitor Antunes <vitor.hda@gmail.com> wrote:
>> Hi everyone,
>>
>> Could anyone comment the 3 patches I sent (being this the last one)?
>>
> [...]
>> On Thu, Nov 25, 2010 at 1:26 AM, Vitor Antunes <vitor.hda@gmail.com> wrote:
>>> ---
>>>  contrib/fast-import/git-p4 |    2 +-
>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
>>> index 0ea3a44..a466847 100755
>>> --- a/contrib/fast-import/git-p4
>>> +++ b/contrib/fast-import/git-p4
>>> @@ -618,7 +618,7 @@ class P4Submit(Command):
>>>         if len(detectRenames) > 0:
>>>             diffOpts = "-M%s" % detectRenames
>>>         else:
>>> -            diffOpts = ("", "-M")[self.detectRenames]
>>> +            diffOpts = ("", "-M")[self.detectRename]
>>>
>
> This appears to me to be a bugfix for one of the other patches you
> sent, is that right?
>
> If so, maybe you could squash it with the previous patch and re-send
> it all to the list?
>
> My other comments for now are:
> - you have forgotten to sign off on the patches
> - commit messages are normally in imperative rather than past tense
> (see Documentation/SubmittingPatches in git)
>
> - In your first patch you wrote:
>> The detectRenames option should be set to the desired threshold value.
> I'm not sure what threshold value you refer to here, and what values
> you can set it to. Am I missing something?
> (I'm not very familiar with git rename detection options)
>
> I'm a git-p4 user, so I can test your changes and look a bit more at
> your code. Someone verifying it could help getting the patches
> applied.
>
> Thanks for improving git-p4!
>
> Cheers,
> Thomas Berg
>

-- 
Vitor Antunes