Lea Wiemann wrote: > Jakub Narebski wrote: >>>>> +=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. >> 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. >> 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? >>> I wouldn't -- see my blurb about error handling at the top of my reply >>> to Petr (<487BD0F3.2060508@xxxxxxxxx>). 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. ;-) [...] >> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
