git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 2/3] add new Git::Repo API

From
Jakub Narebski <jnareb@gmail.com>
Date
Jul 18, 2008, 15:35 UTC
Message-ID
<200807181735.15278.jnareb@gmail.com>
In-Reply-To
<48809D31.5030008@gmail.com>
Lea Wiemann wrote:
> Jakub Narebski wrote:
Show 8 quoted lines
>>>>> +=item $commit->message
>> 
>> I'd rather then have _git_ convert it to UTF=8 for us (using 
>> --encoding=<encoding> option to git-log/git-rev-list)
> 
> Yeah, I guess the API should actually decode it.  You wouldn't want to
> have the message in UTF-8 but in Unicode (I suggest you read man
> perlunitut if you haven't done so).
You mean perluniintro(1) here, isn't it?

Besides if decoding is done in Perl API, we can convert it simply to Perl internal form (which, IIUC, in modern Perl is UTF-8 and marked as such).

> We cannot have git do the decoding, 
> since (apart from the fact that it doesn't smell right) it isn't
> guaranteed to emit valid UTF-8 [...]

Well, if that is the case then Perl API has to do conversion, that is the only sensible way.

Show 14 quoted lines
>> It is (much) better than forking git-cat-file for each commit shown
>> on the list; nevertheless I think that it would be better to use git-log
>> to generate list (or Git::Revlist) of Git::Commit objects.  It is one
>> fork less, but what more important you don't have to access repository
>> twice for the very same objects.
> 
> You're confused; it's not one fork less, it's a write to a pipe less.
> (Pleeeease look at the code before you write something.  It's there, in
> this very thread.)  And I don't believe the "access the repository
> twice" thing is anywhere near an actual issue.  To summarize, you're
> asking me to (a) write code and (presumably) (b) add something to the
> interface of a public API, based on some (most probably faulty)
> assumptions about performance?  You should really read
> <http://c2.com/cgi/wiki?PrematureOptimization>.

Code is there, in gitweb, in parse_commits subroutine, or rather in parse_commit_text subroutine.

[cut]
But I can agree that possible (and possibly minuscule) performance
improvement is not worth introducing new API and complicating (I think)
gitweb code.
Show 9 quoted lines
>> I think that _not using_ Git::Cmd (or somesuch) API results in botched,
>> horrible API
>>   our $git_version = $repo_root->repo(directory => 'dummy')->version;
>> (Unless it is not needed any longer, or not used any longer; if it is
>> so, then perhaps implementing Git::Cmd as generic wrapper around git
>> commands, hiding for example ActivePerl hack, could be left for later).
> 
> It isn't used any longer -- I really suggest you read the whole thread
> before replying. ;-)

O.K. Still I think that putting cmd_output and other in Git::Repo is not a good API. I'd rather route calling git commands via Git or Git::Cmd object (but Git::Repo would have Git/Git::Cmd object which automatically adds '--git-dir=<path>', and possibly also '--work-dir=<path>').

By the way, would you prefer if I commented on 3/3 patch as it is now,
taking into account what I remember from discussion on this and 2/3
patch (latter only as relevant), or would you rather I wait for next
round (next version) of patches?
Show 12 quoted lines
>>> I wouldn't -- see my blurb about error handling at the top of my reply
>>> to Petr (<487BD0F3.2060508@gmail.com>).  You're not supposed to pass
>>> anything that you didn't get from get_sha1 into Git::Commit or
>>> Git::Tag constructors, or your error handling is invariably broken.
>> 
>> I can understand this simpler, although less than optimal, and geared
>> mainly towards gitweb needs.
> 
> FTR, yes it is simpler, but no, it is not really geared toward gitweb
> needs, and it's definitely not "less than optimal" in the sense of being
> worse than the exception-based error handling Git.pm does.  Trust on me
> on this one. ;-)
[...]
Show 7 quoted lines
>> Why do you _need_ name_rev, if you are not to include git-describe
>> equivalent?
> 
> I needed it for gitweb.  As I said, I'm not trying to create a complete
> API.  A describe_rev (or so) method can be added later, if and when it's
> needed.  (As I said, I don't think writing APIs without at least one use
> case is a good idea anyway.)
Errr... I guess I misspoke. I should not say 'geared toward gitweb
needs', as perhaps it is 'created according [at least somewhat] to
what gitweb would need'.
 
-- 
Jakub Narebski
Poland
Previous: Lea WiemannNext: Lea Wiemann
Message 36 of 55 in “Git::Repo API and gitweb caching”
  1. 0/3 Git::Repo API and gitweb cachingLea Wiemann, Jul 11, 2008
  2. 1/3 gitweb: add test suite with Test::WWW::Mechanize::CGILea Wiemann, Jul 11, 2008
  3. 2/3 add new Git::Repo APILea Wiemann, Jul 11, 2008
  4. Junio C HamanoJul 13, 2008
  5. Lea WiemannJul 14, 2008
  6. Jakub NarebskiJul 13, 2008
  7. Lea WiemannJul 14, 2008
  8. Petr BaudisJul 14, 2008
  9. Lea WiemannJul 14, 2008
  10. Jakub NarebskiJul 14, 2008
  11. Lea WiemannJul 15, 2008
  12. Petr BaudisJul 18, 2008
  13. Jakub NarebskiJul 18, 2008
  14. Petr BaudisJul 18, 2008
  15. Lea WiemannJul 18, 2008
  16. Petr BaudisJul 18, 2008
  17. Johannes SchindelinJul 18, 2008
  18. Statictics on Git.pm usage in git commands (was: [PATCH 2/3] add new Git::Repo API)Jakub Narebski, Jul 19, 2008
  19. Petr BaudisJul 19, 2008
  20. Jakub NarebskiJul 20, 2008
  21. Petr BaudisJul 20, 2008
  22. Johannes SchindelinJul 20, 2008
  23. Petr BaudisJul 20, 2008
  24. Johannes SchindelinJul 20, 2008
  25. Petr BaudisJul 20, 2008
  26. Johannes SchindelinJul 20, 2008
  27. Petr BaudisJul 18, 2008
  28. Jakub NarebskiJul 19, 2008
  29. Jakub NarebskiJul 19, 2008
  30. Petr BaudisJul 20, 2008
  31. Jakub NarebskiJul 20, 2008
  32. Jakub NarebskiJul 16, 2008
  33. Lea WiemannJul 16, 2008
  34. Jakub NarebskiJul 17, 2008
  35. Lea WiemannJul 18, 2008
  36. Jakub NarebskiJul 18, 2008
  37. Lea WiemannJul 18, 2008
  38. 3/3 gitweb: use new Git::Repo API, and add optional cachingLea Wiemann, Jul 11, 2008
  39. Jakub NarebskiJul 14, 2008
  40. Lea WiemannJul 14, 2008
  41. Jakub NarebskiJul 14, 2008
  42. Lea WiemannJul 14, 2008
  43. Jakub NarebskiJul 15, 2008
  44. Lea WiemannJul 15, 2008
  45. Johannes SchindelinJul 15, 2008
  46. J.H.Jul 15, 2008
  47. Lea WiemannJul 15, 2008
  48. J.H.Jul 15, 2008
  49. Johannes SchindelinJul 11, 2008
  50. Jakub NarebskiJul 11, 2008
  51. Lea WiemannJul 11, 2008
  52. Abhijit Menon-SenJul 11, 2008
  53. Jakub NarebskiJul 12, 2008
  54. Lea WiemannJul 19, 2008
  55. Lea WiemannAug 18, 2008

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.