patchperl/Git.pm: add rev_parse method

The rev_parse method translates a revision name to a SHA1 hash, like the git-rev-parse command.

Signed-off-by: Lea Wiemann <LeWiemann@gmail.com>
---

This is part of my work to extend Git.pm to create a usable repository access abstraction layer for gitweb.

I've tested this method by calling it with a few parameters, but there's no test suite yet. I'll probably send a message to the mailing list about testing Git.pm soon.

This is my first patch to Git.pm, so please let me know if there is anything wrong with it!

 perl/Git.pm |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

I wrote:

Oh, here's one problem: I'll probably do a lot of changes to Git.pm, and it might be handy for me to be able to change my own methods later. I definitely wouldn't like to see Git.pm end up in some release while I'm in the middle of a major refactoring.

Should I perhaps stay on my branch with these changes, and then merge when it has stabilized (in 1-3 months)?

One thing I'd be concerned about is that I might introduce fundamental issues in my API, since I'm neither a Git nor a Perl expert (yet ^^). What's the best way to avoid discovering such issues only at the Big Merge? Is there anyone who'd be willing to monitor my commits and give me feedback on a semi-continuous basis?

(I'm working on this more-or-less fulltime as part of a Google Summer of Code project.)

Best,

     Lea

On Fri, May 30, 2008 at 09:03:15AM +0200, Lea Wiemann wrote:

I have two proposals:

(i) Tell Junio you would like the changes to stay in pu or next for now. But he will probably do that by default anyway. :-) Thus, you do not need to worry about them getting into a release any soon, but you will still see some real-life testing (in theory).

(ii) When introducing new interface, introduce a user to it right away. So, if I were you, my roadmap would be something like:

	(a) Make Git.pm use gitweb's config parser
	(b) Add 'use Git' to gitweb and convert all Git calls possible
	(c) For the rest, introduce the necessary methods to Git.pm,
	    one patch per method (I would even bundle the Git.pm
	    addition with the gitweb changes)

I'm not saying you _have_ to do it this way, but I believe it's one of the smoothest paths. Me and I think many others of the Git project are huge believers in "small steps" instead of "grand designs"; you will be getting immediate feedback about your changes as you go and your work will be useful at any point of time.

(I would love to provide feedback and review your patches, but please note that at the same time I cannot promise I will be able to do it all the time; I have failed these hopes in the past already, and again likely will in the future. But hopefully there's plenty of other Perl hackers on the list.)

Petr Baudis wrote:

Junio, any comments on this? Doing doing small iterations is great IMO, and I'll be doing it in any case. However, right now I see three possible problems with merging to pu continuously (rather than keeping it on my branch and merging in large chunks):

1. I'm working full-time on this, so I might produce patches that 
loosely depend on one another at a peak rate of 2-3 per day.  (I can do 
other project-related stuff while my patches are waiting for review, but 
only so much of course.)  Do you have any experience with working with 
full-time developers on Git?  Do you see problems with my potentially 
high patch frequency?

2. I'll be changing my own API.  In other words, the API is really 
unstable while I work on this (with the only user of the API being 
Gitweb, which I'll update as I go).  Is that OK for the pu branch?

3. I try to be careful with my commits, but it might still cause more 
work for whoever reviews my patches, compared to reviewing larger 
chunks.  (That's because some of the stuff I write might end up being 
deleted or rewritten later.)

Apart from that, I think merging (and getting feedback) continuously is great.

So, comments would be much appreciated!

-- Lea

Lea Wiemann <lewiemann@gmail.com> writes:

As long as experienced people on the list review patches, I do not see a problem.

    There seems to be a misconception (not you particularly but some parts
    of the list audience in general) that a review cycle of a patch sent
    to the list is a dialog between the submitter and me.  It is _NOT_.
    If it has rooms for improvements, other people would help you polish
    it, which happens often.  However, if it is very well done, I'd also
    like other people to say so after reviewing more often.

It does not _have_ to even land on 'pu'. Sending the patches to the list, asking interesting parties to look at them, _and_ having people actually review them is more valuable part.

If your series will become big, I do not mind (re)merging it from time to time in 'pu' to give it a wider exposure. If you want to go this route, you would have a publicly fetchable repository of your own to house your changes (repo.or.cz?).

I do not even mind merging the branch to 'next' if the series is 'next' worthy, but that places heavier responsibility on your part to keep the history of that branch clean.

One thing you could do, when sending out [PATCH v$n] for the value of $n greater than 1, is to mention what the improvements are since the previous round in the message (typically after the three-dash separator). This helps reviewers who have already seen your previous iteration. The full rationale of the change needs to be kept in the proposed commit log message.

For example, your first patch may look like this:

	Subject: [PATCH] Git.pm: Add rev_parse() sub

	This adds a rev_parse() sub to return the 40-byte object name
        from given "extended SHA-1" expression.

	Signed-off-by: A U Thor <au.thor@example.com>
	---
         <<diffstat and patch>>

Then after Pasky and others suggest improvements, your second message will appear on the list:

	Subject: [PATCH v2] Git.pm: Add parse_rev()

	This adds 'parse_rev()' sub to return the 40-byte object name from
        given "extended SHA-1" expression.  It returns undef if the given
        string is malformed.

	Signed-off-by: A U Thor <au.thor@example.com>
	---

         Changes relative to v1 are:
         
         * Fixed indentation;
         * Use -q to squelch non-fatal errors so that they do not leak
           to the STDERR;
         * Improved in-code documentation.

	 <<diffstat and patch>>

Some people also sends an interdiff between v$n-1 and v$n as a separate message, and it helps reviewing when the change is big.

Hi,

On Fri, 30 May 2008, Lea Wiemann wrote:

I think it would be best for you to have that in a personal fork of git on repo.or.cz. It is really easy to set up, and working with you will be easier, since one can always "git fetch" your newest stuff.

Then, I would recommend making many many small patches. You can always reorder/squash them with "git rebase -i" later, and you will probably need to rebase them to newer upstream revisions anyway.

Ciao, Dscho

Johannes Schindelin wrote:

Thanks Junio and Johannes for you comments! My repository is here: http://repo.or.cz/w/git/gitweb-caching.git

-- Lea

Hi,

On Sat, 31 May 2008, Lea Wiemann wrote:

Nice!

Ciao, Dscho

Lea Wiemann <lewiemann@gmail.com> writes:

First of all, we do not do "Big Merge". We merge small and we merge often. Nobody has perfect foresight, so you shouldn't be too afraid of contaminating the public history with experiments that did not pan out well.

Isn't it what your GSoC mentor is for ;-)?

You can seek wider exposure in various different ways:

 * Send [RFC] patches to the list; that's how this community is supposed
   to work, although I do not see as much reviews as I would personally
   want to see from other people these days for some reason [*1*].

   I may pick up "next" worthy ones to "next", and perhaps other ones to
   "pu" as time permits.

 * Have your repository on repo.or.cz (I thought GSoC student project for
   git were supposed to be hosted there?)  People interested in Perl
   interface in general and Gitweb in particular can try your progress
   out.

[Footnote]

*1* I suspect that maybe there is a misconception that patch submission and review on the list is a dialogue between the submitter and the maintainer. It is _NOT_ the case. I'd rather stay back, sipping my Caipirinha, listening to _other_ people argue and improve the submitted patches, while occasionally giving some guidance to the course of the discussion. And when the polished result emerges finally, apply it to my tree, taking all the credit. _That_ is how the community is supposed to work, isn't it? ;-)

Hi,

On Fri, May 30, 2008 at 06:43:05AM +0200, Lea Wiemann wrote:

I believe it would be more consistent to call it parse_rev() and in fact also less confusing since this is not full-fledged rev-parse frontend (and probably should not be; rev-parse is terribly overloaded with much more stuff).

I think it is better style to use the regular pattern of checking $E->value() and either return undef right away or pass the exception (e.g. in case rev-parse cannot be executed for some reason).

When can this trigger?

The parse_rev method takes a revision name and returns a SHA1 hash, like the git-rev-parse command.

Signed-off-by: Lea Wiemann <LeWiemann@gmail.com>
---
Hi Petr,

This patch incorporates all your suggestions. Thanks for your help on IRC!

 perl/Git.pm |   38 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 38 insertions(+), 0 deletions(-)

On Fri, May 30, 2008 at 10:27:50PM +0200, Lea Wiemann wrote:

Nice, this looks quite sane!

Acked-by: Petr Baudis <pasky@suse.cz>

				Petr "Pasky" Baudis

Petr Baudis <pasky@suse.cz> writes:

Perhaps, but except for the use of nonstandard try...catch. I have been wondering if we can move away from it, with the goal of eventually getting rid of the construct altogether.

Didn't we hear from Randal that the construct is known to be leaky?

Junio> Perhaps, but except for the use of nonstandard try...catch. I have been Junio> wondering if we can move away from it, with the goal of eventually getting Junio> rid of the construct altogether.

Junio> Didn't we hear from Randal that the construct is known to be leaky?

It'd be trivial to avoid this try/catch.

eval {
  ... 
};
if ($@) {
  if (UNIVERSAL::isa($@, "That::Class::Which::Should::Be::Ignored")) {
    # ignore it
  } else {
    die $@; # re-throw it
  }
}

No leak there.

Randal L. Schwartz wrote:

I'm not an experienced Perl hacker, but I intuitively liked the throw/catch method better than the eval/die method.

If I read http://www.nntp.perl.org/group/perl.perl5.porters/2006/03/msg110331.html correctly, this has been fixed in recent 5.8 versions, so it would really only affect 5.6. But perhaps we don't care about 5.6. ;-) People who run into serious memory issues with 5.6 will have to upgrade, and the existing functionality that programs like git-send-email or git-svn rely on seems to work fine with the memory leaks, so using throw/catch further probably won't cause any regressions for them.

I'm honestly not too keen on sacrificing time (or code prettiness) on 5.6 compatibility, so if there are no reasons besides the memory leak to move away from throw/catch, perhaps we can just keep using it?

-- Lea

Lea> Randal L. Schwartz wrote:

Lea> I'm not an experienced Perl hacker, but I intuitively liked the throw/catch Lea> method better than the eval/die method.

I *am* an experienced hacker, and every Perl hacker worth their salt understands that "eval/die" *means* try/catch from other languages.

The extra layer of syntactic sugar is *not* worth it.  It merely obfuscates.

Randal L. Schwartz wrote:

Cool, thanks for clarifying this. As Petr suggested, this should probably be fixed in a separate patch. (Perhaps after we have a test suite, to avoid regressions.)

-- Lea

Lea Wiemann <lewiemann@gmail.com> writes:

If we aim for code prettyness, I would say try/catch should definitely go. They are spelled as "eval" in properly written Perl, and not doing so makes your program look ugly.

Hi,

On Fri, 30 May 2008, Lea Wiemann wrote:

I think your opinion would change dramatically if you were stuck on a platform with Perl 5.6. In general, I deem it not nice to sacrifice backwards compatibility just because _you_ do not need it.

I mean, by that argument we could scrap the whole Git UI and rewrite it anew: a lot of compatibility warts would Just Go Away.

Ciao, Dscho

  Hi,

On Sat, May 31, 2008 at 12:50:14PM +0100, Johannes Schindelin wrote:

  let's get some perspective here: 5.6.1 was released on 2001-Apr-08.
5.8.0 followed on 2002-Jul-18.  Is there anyone on the list who _is_
stuck on a platform with Perl 5.6 _and_ uses Git on it? Heck, we
are even approaching GNU Interactive Tools 4.3.20 release here,
walking that much back.

  Of course, there's no sense in arbitrarily requiring newer Perl
versions until we know we are not compatible anymore, but frankly,
I don't think wasting time on being compatible with 5.6 is worth it
either unless its actual users speak up.

Hi,

On Sat, 31 May 2008, Petr Baudis wrote:

I think this is not an interesting question. Those stuck with Perl 5.6 are most likely not those who lurk on this list.

Sure, we could just require users to upgrade to Linux, newest glibc and everything and be done. We could also require our users to stick their fingers where the sun don't shine.

The really interesting question is: is the time of a single developer (who gets all the upsides of requiring a certain setup) worth the hassle and pain of possibly more than one person getting all the _downsides_?

In the case that started this thread, I would not hesitate a single microsecond to answer "No, hell no".

Hth, Dscho

On Fri, May 30, 2008 at 02:25:58PM -0700, Junio C Hamano wrote:

It's consistent with the rest of the code; we may want to remove it, but we should either do it everywhere or not at all and I think it's an issue independent on this patch.

I agree that in hindsight, using this construct was probably not the best idea, and I'm in principle not against removing it as long as there is no functionality lost - the ability to let the program just fail if I don't care in my code, or catch specifically for nonzero exit codes and let the program fail in other cases, is very useful and I do use it in some of my code; I would hate to lose this flexibility.

If there is an alternative (perhaps even slightly more clumsy) way to solve this, I have no problem with switching over; however, I don't have much time or interest to research this myself. If Lea wants to figure something out, she (*) is welcome as far as I'm concerned.

(*) Lea, I hope I figured this out right - sorry if not. :-)

The parse_rev method takes a revision name and returns a SHA1 hash, like the git-rev-parse command.

Signed-off-by: Lea Wiemann <LeWiemann@gmail.com>
---
Use tabs instead of blanks to match the rest of the file.  No changes
apart from that.

 perl/Git.pm |   38 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 38 insertions(+), 0 deletions(-)

The parse_rev method takes a revision name and returns a SHA1 hash, like the git-rev-parse command.

Signed-off-by: Lea Wiemann <LeWiemann@gmail.com>
---
Changes since PATCH v2: Updated the documentation (found this while
testing).  Here's the diff excerpt from v2 to v3:

 =item parse_rev ( REVISION_NAME )

 Look up the specified revision name and return the SHA1 hash, or
-return undef if the lookup failed.  See git rev-parse --help, section
-"Specifying Revisions".
+return undef if the lookup failed.  When passed a SHA1 hash, always
+return it even if it doesn't exist in the repository.
+
+See git rev-parse --help, section "Specifying Revisions", for valid
+revision name formats.

 perl/Git.pm |   41 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 41 insertions(+), 0 deletions(-)

The parse_rev method takes a revision name and returns a SHA1 hash, like the git-rev-parse command.

Signed-off-by: Lea Wiemann <LeWiemann@gmail.com>
---
Changes since PATCH v2: Updated the documentation (found this while
testing).  Here's the diff excerpt from v2 to v3:

 =item parse_rev ( REVISION_NAME )

 Look up the specified revision name and return the SHA1 hash, or
-return undef if the lookup failed.  See git rev-parse --help, section
-"Specifying Revisions".
+return undef if the lookup failed.  When passed a SHA1 hash, always
+return it even if it doesn't exist in the repository.
+
+See git rev-parse --help, section "Specifying Revisions", for valid
+revision name formats.

 perl/Git.pm |   41 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 41 insertions(+), 0 deletions(-)

Lea Wiemann wrote:

I just discovered that you can also pass tree and blob identifiers to git-rev-parse, like <commit>:<path>.

Hence, perhaps this method would be more appropriately named get_hash (or get_sha1), given that you can pass in things other than revisions. I'll probably post new patch versions soon.

Terminology question: Is there *any* kind of agreed-on name for the identifiers you pass into git-rev-parse (like HEAD^2 or master:test/foo.txt)? I called it "revision name" before, but that's wrong for the "...:<path>" syntaxes, and "object identifier" is reserved for hashes only (according to the glossary). If there no better suggestions, I'll probably go for "extended identifiers", since rev-parse --help calls this the "extended SHA1 syntax", and it also seems to be an unused term.

-- Lea

On Sun, Jun 01, 2008 at 07:38:00PM +0200, Lea Wiemann <lewiemann@gmail.com> wrote:

`man git-rev-parse` calls them "revisions". Yes, even the commit:path ones. So I would use "revision", or if you introduce a new term, I would strongly suggest updating not just the glossary but git-rev-parse.txt as well.

Thanks.

Miklos Vajna wrote:

True -- it's quite cringeworthy indeed. ;) As long as it only affects the documentation for that particular method, I'll go with "revision".

-- Lea

Lea Wiemann <lewiemann@gmail.com> writes:

I think that is sensible, and the method can stay parse_rev, not get_hash, don't you think?

Junio C Hamano wrote:

I'm not sure -- on the hand hand, parse_rev resembles "rev-parse" and might be more intuitive for that reason.

On the other hand though, the mis-named "revision" arguments get passed into many methods (right now get_hash, get_type, and cat_blob, but many more to come). Calling them "revision" everywhere is bound to cause major confusion, so I'll probably have to rename the arguments anyway. But then, get_hash will be much more intuitive to understand than parse_rev since the API won't talk about "revisions" anywhere.

Also, get_hash indicates that it returns a hash and implies that it takes an object name, but parse_rev indicates neither of those. Since Git.pm doesn't mimic Git's command-line API anyway, I'd rather have clear method names.

-- Lea

The get_hash method takes a "revision" (as described in git-rev-parse --help) and returns the SHA1 hash of the commit, tree, file, or tag object it refers to.

Signed-off-by: Lea Wiemann <LeWiemann@gmail.com>
---
Changes since v3: Renamed parse_rev to get_hash, improved
documentation to point out that the parameter can refer to file, tree
and tag object as well, and added example in "Synopsis" section.

 perl/Git.pm |   45 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 45 insertions(+), 0 deletions(-)

The get_hash method takes a "revision" (as described in git-rev-parse --help) and returns the SHA1 hash of the commit, tree, file, or tag object it refers to.

Signed-off-by: Lea Wiemann <LeWiemann@gmail.com>
---
Changes since v3: Renamed parse_rev to get_hash, improved
documentation to point out that the parameter can refer to file, tree
and tag object as well, and added example in "Synopsis" section.

 perl/Git.pm |   45 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 45 insertions(+), 0 deletions(-)