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

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

From
Petr Baudis <pasky@suse.cz>
Date
Jul 18, 2008, 16:48 UTC
Message-ID
<20080718164828.GT10151@machine.or.cz>
In-Reply-To
<200807150141.39186.jnareb@gmail.com>

In order to keep this mail within sensible size, I have trimmed some of the bits - basically, the default reply is usually ranging from "Fair enough" to "Great!" :-)

On Tue, Jul 15, 2008 at 12:19:31AM +0200, Lea Wiemann wrote:
Show 11 quoted lines
> > First, I don't think it's good idea at all to put the pipe-related stuff
> > to Git::Repo - this is botched up API just like the current one.
> 
> Well, they're more like helper methods.  Since they don't fit into the
> design goals of the Git::Repo API at all, I'd suggest we just
> underscore-prefix them and take them out of the man page.  (The only
> reason why I hadn't done this is that gitweb uses $repo->cmd_output
> extensively, so it'd end up with a lot of underscore calls.  But I
> suppose we can either alias _cmd_output to cmd_output in gitweb's
> CachedRepo subclass, or live with $repo->_cmd_output calls.)  Does
> underscore-prefixing sound good to you?

I think this is one problematic point we keep hitting - my opinion is that we _need_ such a wrapper _publically_, because it tends to be actually the main use-case of Git.pm, and that this wrapper should be:

	(i) both available standalone for commands like ls-remote
	(ii) and available as part of Git::Repo instance, to have the
	right arguments passed to Git automagically

So you define cmd_output() and seem to argue that this command should not be called directly and is not interesting for the outside. But the experience shows that the pipe interface is actually the _most_ used part of the Git Perl API, and in fact you mentioned that as part of your gitweb migration to Git::Repo, you will temporarily introduce calls to _cmd_output(), the "internal" API. :-) Sure, it's only temporary, but many won't have the luxury to adjust the Git::Repo API to provide all the operations they need, and ultimately they will need to defer to the pipe interface.

Show 6 quoted lines
> > It should be actually very easy to start with moving all the pipe
> > functionality to Git::Command.
> 
> Creating a new (Git::Command) API is very much non-trivial, apart from
> the fact that I'm not convinced that we need Git::Command, and that a
> clean command interface neither falls out of Git.pm nor Git::Repo.

As I said, majority of Git API usage is actually the pipe API. So we should figure out how to provide it. I agree that it's not immediately within your scope, but you are introducing new Perl API and this just needs to be embedded somewhere there consistently.

Show 16 quoted lines
> >> [Git::Commit->new, Git::Tag->new:]
> >> +Calls to this method are free, since it does not check whether $sha1
> >> +exists and has the right type.  However, accessing any of the commit
> >> +object's properties will fail if $sha1 is not a valid commit object.
> > 
> > This is nice idea, but I'd also provide a well-defined way for the user
> > to verify the object's validity at a good moment; basically, make load()
> > a public method. The user can deal with errors then and rely on
> > error-free behavior later.
> 
> No, you should never pass in an invalid SHA1 in the first place.  The
> above piece of documentation is just a warning that bugs will show up
> delayed.  IOW, this is not the right place to have your error handling.
> 
> If you're getting a SHA1 through the user-interface, check its existence
> with get_sha1 before passing it to the constructor.

But that's an expensive operation, you need extra Git exec for this, while all the Git commands can do the checks for you, if you give them the chance.

I was doing pretty much this thing in Cogito (initially out of necessity) and it made it ungodly slow for any kind of batch operations.

Show 13 quoted lines
> >> +Note that $sha1 must be the SHA1 of a commit object; tag objects are
> >> +not dereferenced.
> > 
> > Why not?
> 
> Because the SHA1 might resolve to an object of the wrong type, which
> means you have to do error handling in Git::Object objects; that's the
> wrong place.
> 
> If tag-resolving is really needed, we can add an optional $type
> parameter to get_sha1, which will cause get_sha1 to resolve the object
> until a $type object is found, or return undef if the object is or
> resolves to an object of the wrong type.

See above why I think you should reconsider requiring the explicit "resolving" step.

Show 5 quoted lines
> I have resolving code in gitweb's git_get_sha1_or_die (which I didn't
> implement in Git::Repo since it uses some customized error reporting).
> The resolving code could conceivably be extracted and moved to get_sha1.
>  I think there are a few things to ponder and maybe discuss, so I'd do
> that in a separate patch (if I get around it before the end of the project).

The thing that concerns me about this is that this might show that your approach to error handling is not flexible enough for some real-world usage and this might be a design mistake - is that not so? I didn't look at the code.

Show 13 quoted lines
> >> [Snipped a lot of quoting --LW]
> >> +=item $repo->repo_dir
> >> +=item $repo->git_binary
> >> +=item $repo->version
> >> +sub _get_git_cmd {
> > 
> > This definitely does not belong to a Git::Repo object.
> 
> Which of those methods are you referring to?  I think $repo->version
> might reasonably be removed (and the code re-added to gitweb); I'll do
> so unless you object.  _get_git_cmd is already underscored, and repo_dir
> and git_binary only access attributes passed in through the constructor,
> so I think those three should stay.

Sorry, you're right about repo_dir and possibly git_binary. My main concern was about the command pipe handling itself, but I elaborated on that above already.

Show 14 quoted lines
> >> +=item $repo->get_refs
> >> +=item $repo->get_refs($pattern)
> > 
> > Again, the refs should be properly integrated into the object structure.
> 
> Really?  I think it's generally fine for get_refs to exist and to live
> in Git::Repo.
> 
> Its return value (currently an an arrayref of [$sha1, $object_type,
> $ref_name] arrayrefs) might need improvement though, and I find the
> $pattern parameter pretty suspect (in that it smells like a for-each-ref
> wrapper).  Since get_refs is unused at the moment (gitweb ended up
> needing the slightly different show-ref), I'll remove it for now.  (Same
> thing about me not being a fan of premature API design applies.)

Just a note, the thing is that you might want to add some methods for inspecting and mutating the refs, and at the same time this is not a repository-specific concept, but you can get the same structure from git ls-remote call. That's why I think it would make sense to make a separate object out of it. But that's moot point now that the API won't be there yet.

-- 
				Petr "Pasky" Baudis
As in certain cults it is possible to kill a process if you know
its true name.  -- Ken Thompson and Dennis M. Ritchie
Previous: Lea WiemannNext: Jakub Narebski
Message 12 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.