Thanks. A new patch series will follow today (hopefully).
For brevity, I'm incorporating all your suggested changes unless noted
otherwise.
I think it's better. People shouldn't get an error message if they
write "use Git::Commit". Also we cannot split later if we don't split
now since people would write "use Git::Repo" and then access
Git::Commit; subsequently splitting the API would (might?) cause
breakage, I believe.
;-)
Unknown headers are now ignored.
Yeah, but this part is only a bunch of trivial accessor methods. If the
module grows and the documentation needs to be split, it can be done
later. No need to be purist here. ;-) Also ...
Several of the methods actually only exist in the Git::Object base
class. I still documented them in the Commit and Tag modules since
having to look up methods in base class documentation can be a tad
annoying, especially if the base class is never used by users of the API.
You understand correctly; I'd call it "lazy loading" though. I've added
that term for clarity.
I think the constructor shouldn't be internal (= underscore-prefixed,
you mean?), since the Commit/Tag APIs are usable on their own.
No, they don't. (Clarified that in the documentation.)
Array. I've replaced "list" with "array" in case it helps clarity.
If we add a Git::Tree API, the Git::Tree objects will stringify to their
SHA1s, so we shouldn't have compatibility issues. I've changed the
documentation of $commit->tree to this:
"Return an object that stringifies to the SHA1 of the tree that this
commit object refers to. (Currently this returns an actual string,
but don't rely on it.)"
Yes, but we don't actually care about those effective parents for the
purpose of the Git::Commit API. IOW, the effective parent should be
managed by the code that created a list of revisions, not by the
Git::Commit API.
Yup; that's why I wrote "{author,committer} *string*". ;)
Yes, just raw data. Decoding is too tricky (i.e. not guaranteed to
work) to just add a simple method to the API; IOW, it needs error
handling and perhaps fallback encodings.
Yes, but the API doesn't use any of those commands internally, if that's
what you're worried about.
No. (Otherwise I'd have written that ;-).)
Yes, I'd believe so. I basically wanted to make sure that those methods
always return a string; do you think that this is a bad idea?
$repo->cat_file (now renamed to get_object) actually doesn't fork but
uses a pipe (cat-file --batch); I don't think it should be a performance
issue.
Because we happen to get the raw text in a single string from the Repo
API. (It shouldn't be a performance/memory issue for Commit or Tag
objects at all. ;-))
Yes, changed, but ...
... let's not complicate the API unnecessarily. If a new header pops up
we can immediately add it to the Commit/Tag API.
I don't really care, and it's too much work to come up with a test case
for this. ;-) If the repository is borked to the point of invalid
header names, it's fine for Git::Commit to behave undefinedly.
Intuition. ;-) I think I'd read $repo->commit as "the commit of the
repository", akin to $commit->tree, which doesn't make sense here.
It could represent any object, though I don't see a need for Git::Blob
right now (though it's possible that it's needed later).
Why? I don't think it's necessary.
It signifies that this module won't run with Perl <5.6.2. I've had to
bump it to 5.008 (Perl 5.8); more about that in the message announcing
the next version of the patch series.
I wouldn't use discovery magic here, at least for now, since it's
non-trivial to get it right (and it interacts with possible future
extensions of the API, like Git::WC). Such a feature can be implemented
if/when it's needed.
I don't think something Git::Cmd is a good idea (as I pointed out in my
reply to Petr, <487BD0F3.2060508@gmail.com>), or at least it shouldn't
be implemented as part of this patch series. This method is really just
supposed to return an argument for exec*p, nothing more.
No. It's not helpful for error handling (which should happen in the
caller), and it's not helpful for bug detection (since it will die on
the first access to the repo anyway), but it causes performance penalty
that can be significant for programs like gitweb.
I find repo_dir somewhat clearer (and I don't like having more than one
name per method). We're not trying to mimic or wrap standard git
commands here, anyway.
Yup; it's been deleted anyway.
As I wrote in my reply to Petr, Git::Repo is not trying to be a wrapper
around git binaries, so this method really shouldn't be part of the
official API -- it's just auxiliary; I'll underscore-prefix it.
It's not part of the API anyway, so no need for complicated calling
conventions, IMO.
Apart from the fact that I don't do cargo-cult programming? ;-) Git.pm
forks, whereas Git::Repo uses open, '-|', so it's actually different
(and it's not possible to copy the code).
Why, what should it do? This just opens a pipe, nothing more. No need
for introducing complicated concepts.
You found it below. :-) (If you had snipped this, I wouldn't have spent
time finding and pasting an example. ;-))
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.
Yes to both questions. :-)
Yes, but only for generic command calls, and with a somewhat unpleasant
(cache-specific) interface. It'd need a bit of work for the API.
No. ;-) Doing fine without those.
This one has been removed as well since it would belong into Git::Tree.
Thanks. Since get_path isn't in the Git::Repo API anymore and gitweb's
get_path subroutine didn't handle quoted filenames even before my
patches, I'll only mark it as TODO for now. ;-)
This has been removed as well, since it's not used and the interface
would need work.
Feel free to add it. ;-) (It might take some work to come up with a
decent interface for that method.)
No. If it's needed it could reasonably be extracted from gitweb though
(I think).
Yeah. At some point. ;-)
Sure, for the message.
No, since 'validate' sounds like it would have to do error handling.
If you mean that this should check if the object exists (and has the
advertised type), the user of the API should test for "defined
$tag->repo->get_sha1($tag->object)" or somesuch and do error handling
themselves.
-- Lea
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
