Re: [PATCH/RFC 1/4] contrib: add git-contacts helper

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 2, 2013, 18:32 UTC
Message-ID: <7vk3l895in.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CAPig+cRbBGqrXj-Anib1ESdBBbdUGM-9um4XoPcwG2QxJBubuA@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 10 quoted lines

>> The author name and email can be grabbed from the "blame" output
>> without doing this (and the result may be more robust), but you
>> would need to read from the log message anyway, so I think this is
>> OK.
>>
>> Note that the names and emails in blame output are sanitized via the
>> mailmap mechanism, but "cat-file commit" will certainly not be.
>
> Thanks for pointing this out. Grabbing the author name and email from
> git-blame output does seem like a better approach.

Well, that was not what I was suggesting. Reading from blame output will map only the author/committer names/addresses, and you have two choices:

 (1) if you read author/committer from blame output and other names
     from the commit object without applying mailmap, the same
     person can appear under different names, from his authorship
     (mapped name) and from his name on footers (unmapped), which
     would be inconsistent.  By reading from "author" and
     "committer" header lines in the commit object, you will be at
     least consistently using unmapped names.

 (2) if you want to apply mailmap to names you collect by reading
     the footer, you will write the mapping logic yourself anyway,
     and at that point, passing the names you collect by reading the
     "author" and "committer" header lines in the commit object to
     the same logic will give you mapped names. At that point, you
     do not gain much by reading names from the blame output.

So in either case, you would be better off not reading the author/committer from blame output, as long as you need to pick up other names from the commit object payload.

Show 18 quoted lines

>>> +sub import_commits {
>>> +     my ($commits) = @_;
>>> +     return unless %$commits;
>>> +     my $pid = open2 my $reader, my $writer, qw(git cat-file --batch);
>>
>> Hmph.
>>
>> I vaguely recall that people wanted not to use open2/IPC::Open2 in
>> other parts of the system.
>>
>> I think "cat-file --batch" is designed to behave on a regular bidi
>> pipe, as long as the caller strictly does a ping-pong of issuing one
>> request, flush (with an empty line) and always read the response, so
>> if open2 becomes problematic, we could switch to regular pipes.
>
> Checking the log, I see several cases where deprecated Python popen2
> was removed but find nothing mentioning Perl. Git.pm,
> git-archimport.perl and git-cvsimport.perl appear to use open2.

OK, then I was misremembering.  Thanks for sanity checking.

> ... Extending git-blame to recognize
> multiple -L sounds even better, though definitely outside the scope of
> this series.

Yes. I might be able to find some time to do that myself later this week, but no promises ;-).