[PATCH] perl/Git.pm: add rev_parse method

[PATCH] perl/Git.pm: add rev_parse method

[Lea Wiemann]

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(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index d05b633..9ef8cb0 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -716,6 +716,28 @@ sub ident_person {
 	return "$ident[0] <$ident[1]>";
 }
 
+=item rev_parse ( REVISION_NAME )
+
+Look up the specified revision name and return the SHA1 hash, or
+return undef if the lookup failed.  See the git-rev-parse command.
+
+=cut
+
+sub rev_parse {
+    # We could allow for a list of revisions here.
+    my ($self, $rev_name) = @_;
+
+    my $hash;
+    try {
+        # The --default option works around rev-parse's lack of
+        # support for getopt style "--" separators (it would fail for
+        # tags named "--foo" without it).
+        $hash = $self->command_oneline("rev-parse", "--verify", "--default",
+                                       $rev_name);
+    } catch Git::Error::Command with { };
+    return undef unless defined $hash and $hash =~ /^([0-9a-fA-F]{40})$/;
+    $hash;
+}
 
 =item hash_object ( TYPE, FILENAME )
 
-- 
1.5.5.GIT

Re: [PATCH] perl/Git.pm: add rev_parse method

[Lea Wiemann]

I wrote:
> [Commit:] The rev_parse method translates a revision name to a SHA1 hash, like
> the git-rev-parse command.

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

Re: [PATCH] perl/Git.pm: add rev_parse method

[Petr Baudis]

On Fri, May 30, 2008 at 09:03:15AM +0200, Lea Wiemann wrote:
> I wrote:
>> [Commit:] The rev_parse method translates a revision name to a SHA1 hash, 
>> like
>> the git-rev-parse command.
>
> 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)?

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.

> 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 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 "Pasky" Baudis
Whatever you can do, or dream you can, begin it.
Boldness has genius, power, and magic in it.	-- J. W. von Goethe

Merging strategy for extending Git.pm (was: [PATCH] perl/Git.pm: add rev_parse method)

[Lea Wiemann]

Petr Baudis wrote:
> (i) Tell Junio you would like the changes to stay in pu or next for now.
> [...]
> you will be getting immediate feedback about your changes as you go and 
> your work will be useful at any point of time.

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

Re: Merging strategy for extending Git.pm

[Junio C Hamano]

Lea Wiemann <lewiemann@gmail.com> writes:

> 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?

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.

> 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?

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.

> 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.)

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.

Re: Merging strategy for extending Git.pm (was: [PATCH] perl/Git.pm: add rev_parse method)

[Johannes Schindelin]

Hi,

On Fri, 30 May 2008, Lea Wiemann wrote:

> 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).

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

Re: Merging strategy for extending Git.pm

[Lea Wiemann]

Johannes Schindelin wrote:
> I think it would be best for you to have that in a personal fork of git on 
> repo.or.cz.

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

-- Lea

Re: Merging strategy for extending Git.pm

[Johannes Schindelin]

Hi,

On Sat, 31 May 2008, Lea Wiemann wrote:

> Johannes Schindelin wrote:
> > I think it would be best for you to have that in a personal fork of 
> > git on repo.or.cz.
> 
> Thanks Junio and Johannes for you comments!  My repository is here: 
> http://repo.or.cz/w/git/gitweb-caching.git

Nice!

Ciao,
Dscho

Re: [PATCH] perl/Git.pm: add rev_parse method

[Junio C Hamano]

Lea Wiemann <lewiemann@gmail.com> writes:

> 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?

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.

> Is there anyone who'd be willing to monitor my commits and
> give me feedback on a semi-continuous basis?

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? ;-)

Re: [PATCH] perl/Git.pm: add rev_parse method

[Petr Baudis]

Hi,

On Fri, May 30, 2008 at 06:43:05AM +0200, Lea Wiemann wrote:
> diff --git a/perl/Git.pm b/perl/Git.pm
> index d05b633..9ef8cb0 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -716,6 +716,28 @@ sub ident_person {
>  	return "$ident[0] <$ident[1]>";
>  }
>  
> +=item rev_parse ( REVISION_NAME )

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).

> +
> +Look up the specified revision name and return the SHA1 hash, or
> +return undef if the lookup failed.  See the git-rev-parse command.
> +
> +=cut
> +
> +sub rev_parse {
> +    # We could allow for a list of revisions here.
> +    my ($self, $rev_name) = @_;
> +
> +    my $hash;
> +    try {
> +        # The --default option works around rev-parse's lack of
> +        # support for getopt style "--" separators (it would fail for
> +        # tags named "--foo" without it).
> +        $hash = $self->command_oneline("rev-parse", "--verify", "--default",
> +                                       $rev_name);
> +    } catch Git::Error::Command with { };

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).

> +    return undef unless defined $hash and $hash =~ /^([0-9a-fA-F]{40})$/;

When can this trigger?

> +    $hash;
> +}
>  
>  =item hash_object ( TYPE, FILENAME )
>  

-- 
				Petr "Pasky" Baudis
Whatever you can do, or dream you can, begin it.
Boldness has genius, power, and magic in it.	-- J. W. von Goethe

[PATCH] perl/Git.pm: add parse_rev method

[Lea Wiemann]

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(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index d05b633..4bc3604 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -716,6 +716,44 @@ sub ident_person {
 	return "$ident[0] <$ident[1]>";
 }
 
+=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".
+
+=cut
+
+sub parse_rev {
+    # We could allow for a list of revisions here.
+    my ($self, $rev_name) = @_;
+
+    my $hash;
+    try {
+        # The --quiet --verify options cause git-rev-parse to fail
+        # with exit status 1 (instead of 128) if the given revision
+        # name is not found, which enables us to distinguish not-found
+        # from serious errors.  The --default option works around
+        # git-rev-parse's lack of support for getopt style "--"
+        # separators (it would fail for tags named "--foo" without
+        # it).
+        $hash = $self->command_oneline("rev-parse", "--verify", "--quiet",
+                                       "--default", $rev_name);
+    } catch Git::Error::Command with {
+        my $E = shift;
+        if ($E->value() == 1) {
+            # Revision name not found.
+            $hash = undef;
+        } else {
+            throw $E;
+        }
+    };
+    # Guard against unexpected output.
+    throw Error::Simple(
+        "parse_rev: unexpected output for \"$rev_name\": $hash")
+        if defined $hash and $hash !~ /^([0-9a-fA-F]{40})$/;
+    return $hash;
+}
 
 =item hash_object ( TYPE, FILENAME )
 
-- 
1.5.5.GIT

Re: [PATCH] perl/Git.pm: add parse_rev method

[Petr Baudis]

On Fri, May 30, 2008 at 10:27:50PM +0200, Lea Wiemann wrote:
> 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>

Nice, this looks quite sane!

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

				Petr "Pasky" Baudis

Re: [PATCH] perl/Git.pm: add parse_rev method

[Junio C Hamano]

Petr Baudis <pasky@suse.cz> writes:

> On Fri, May 30, 2008 at 10:27:50PM +0200, Lea Wiemann wrote:
>> 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>
>
> Nice, this looks quite sane!

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?

Re: [PATCH] perl/Git.pm: add parse_rev method

[Randal L. Schwartz]

>>>>> "Junio" == Junio C Hamano <gitster@pobox.com> writes:

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 - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Smalltalk/Perl/Unix consulting, Technical writing, Comedy, etc. etc.
See http://methodsandmessages.vox.com/ for Smalltalk and Seaside discussion

Re: [PATCH] perl/Git.pm: add parse_rev method

[Lea Wiemann]

Randal L. Schwartz wrote:
> [Move to eval/die.]  No leak there.

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

Re: [PATCH] perl/Git.pm: add parse_rev method

[Randal L. Schwartz]

>>>>> "Lea" == Lea Wiemann <lewiemann@gmail.com> writes:

Lea> Randal L. Schwartz wrote:
>> [Move to eval/die.]  No leak there.

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 - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Smalltalk/Perl/Unix consulting, Technical writing, Comedy, etc. etc.
See http://methodsandmessages.vox.com/ for Smalltalk and Seaside discussion

Re: [PATCH] perl/Git.pm: add parse_rev method

[Lea Wiemann]

Randal L. Schwartz wrote:
> "eval/die" *means* try/catch from other languages.
> 
> The extra layer of syntactic sugar is *not* worth it.  It merely obfuscates.

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

Re: [PATCH] perl/Git.pm: add parse_rev method

[Junio C Hamano]

Lea Wiemann <lewiemann@gmail.com> writes:

> 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?

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.

Re: [PATCH] perl/Git.pm: add parse_rev method

[Johannes Schindelin]

Hi,

On Fri, 30 May 2008, Lea Wiemann wrote:

> 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?

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

Support for old Perl versions

[Petr Baudis]

  Hi,

On Sat, May 31, 2008 at 12:50:14PM +0100, Johannes Schindelin wrote:
> On Fri, 30 May 2008, Lea Wiemann wrote:
> 
> > 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?
> 
> 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.

  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.

-- 
				Petr "Pasky" Baudis
Whatever you can do, or dream you can, begin it.
Boldness has genius, power, and magic in it.	-- J. W. von Goethe

Re: Support for old Perl versions

[Johannes Schindelin]

Hi,

On Sat, 31 May 2008, Petr Baudis wrote:

> On Sat, May 31, 2008 at 12:50:14PM +0100, Johannes Schindelin wrote:
> > On Fri, 30 May 2008, Lea Wiemann wrote:
> > 
> > > 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?
> > 
> > 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.
> 
>   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.

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

Re: [PATCH] perl/Git.pm: add parse_rev method

[Petr Baudis]

On Fri, May 30, 2008 at 02:25:58PM -0700, Junio C Hamano wrote:
> 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.

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.

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

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. :-)

-- 
				Petr "Pasky" Baudis
Whatever you can do, or dream you can, begin it.
Boldness has genius, power, and magic in it.	-- J. W. von Goethe

[PATCH v2] perl/Git.pm: add parse_rev method

[Lea Wiemann]

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(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index d05b633..249daad 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -716,6 +716,44 @@ sub ident_person {
 	return "$ident[0] <$ident[1]>";
 }
 
+=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".
+
+=cut
+
+sub parse_rev {
+	# We could allow for a list of revisions here.
+	my ($self, $rev_name) = @_;
+
+	my $hash;
+	try {
+		# The --quiet --verify options cause git-rev-parse to fail
+		# with exit status 1 (instead of 128) if the given revision
+		# name is not found, which enables us to distinguish not-found
+		# from serious errors.  The --default option works around
+		# git-rev-parse's lack of support for getopt style "--"
+		# separators (it would fail for tags named "--foo" without
+		# it).
+		$hash = $self->command_oneline("rev-parse", "--verify", "--quiet",
+					       "--default", $rev_name);
+	} catch Git::Error::Command with {
+		my $E = shift;
+		if ($E->value() == 1) {
+			# Revision name not found.
+			$hash = undef;
+		} else {
+			throw $E;
+		}
+	};
+	# Guard against unexpected output.
+	throw Error::Simple(
+		"parse_rev: unexpected output for \"$rev_name\": $hash")
+	    if defined $hash and $hash !~ /^([0-9a-fA-F]{40})$/;
+	return $hash;
+}
 
 =item hash_object ( TYPE, FILENAME )
 
-- 
1.5.5.GIT

[PATCH v3] perl/Git.pm: add parse_rev method

[Lea Wiemann]

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(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index d05b633..80f7669 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -716,6 +716,47 @@ sub ident_person {
 	return "$ident[0] <$ident[1]>";
 }
 
+=item parse_rev ( REVISION_NAME )
+
+Look up the specified revision name and return the SHA1 hash, or
+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.
+
+=cut
+
+sub parse_rev {
+	# We could allow for a list of revisions here.
+	my ($self, $rev_name) = @_;
+
+	my $hash;
+	try {
+		# The --quiet --verify options cause git-rev-parse to fail
+		# with exit status 1 (instead of 128) if the given revision
+		# name is not found, which enables us to distinguish not-found
+		# from serious errors.  The --default option works around
+		# git-rev-parse's lack of support for getopt style "--"
+		# separators (it would fail for tags named "--foo" without
+		# it).
+		$hash = $self->command_oneline("rev-parse", "--verify", "--quiet",
+					       "--default", $rev_name);
+	} catch Git::Error::Command with {
+		my $E = shift;
+		if ($E->value() == 1) {
+			# Revision name not found.
+			$hash = undef;
+		} else {
+			throw $E;
+		}
+	};
+	# Guard against unexpected output.
+	throw Error::Simple(
+		"parse_rev: unexpected output for \"$rev_name\": $hash")
+	    if defined $hash and $hash !~ /^([0-9a-fA-F]{40})$/;
+	return $hash;
+}
 
 =item hash_object ( TYPE, FILENAME )
 
-- 
1.5.5.GIT

[PATCH v3] perl/Git.pm: add parse_rev method

[Lea Wiemann]

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(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index d05b633..80f7669 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -716,6 +716,47 @@ sub ident_person {
 	return "$ident[0] <$ident[1]>";
 }
 
+=item parse_rev ( REVISION_NAME )
+
+Look up the specified revision name and return the SHA1 hash, or
+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.
+
+=cut
+
+sub parse_rev {
+	# We could allow for a list of revisions here.
+	my ($self, $rev_name) = @_;
+
+	my $hash;
+	try {
+		# The --quiet --verify options cause git-rev-parse to fail
+		# with exit status 1 (instead of 128) if the given revision
+		# name is not found, which enables us to distinguish not-found
+		# from serious errors.  The --default option works around
+		# git-rev-parse's lack of support for getopt style "--"
+		# separators (it would fail for tags named "--foo" without
+		# it).
+		$hash = $self->command_oneline("rev-parse", "--verify", "--quiet",
+					       "--default", $rev_name);
+	} catch Git::Error::Command with {
+		my $E = shift;
+		if ($E->value() == 1) {
+			# Revision name not found.
+			$hash = undef;
+		} else {
+			throw $E;
+		}
+	};
+	# Guard against unexpected output.
+	throw Error::Simple(
+		"parse_rev: unexpected output for \"$rev_name\": $hash")
+	    if defined $hash and $hash !~ /^([0-9a-fA-F]{40})$/;
+	return $hash;
+}
 
 =item hash_object ( TYPE, FILENAME )
 
-- 
1.5.5.GIT

Re: [PATCH v3] perl/Git.pm: add parse_rev method

[Lea Wiemann]

Lea Wiemann wrote:
> Re: [PATCH v3] perl/Git.pm: add parse_rev method
>
> The parse_rev method takes a revision name and returns a SHA1 hash,
> like the git-rev-parse command.

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

Re: [PATCH v3] perl/Git.pm: add parse_rev method

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 842 bytes --]

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

`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.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH v3] perl/Git.pm: add parse_rev method

[Lea Wiemann]

Miklos Vajna wrote:
> Lea Wiemann <lewiemann@gmail.com> wrote:
>> Is there *any* name for the identifiers you pass into git-rev-parse
>> (like HEAD^2 or master:test/foo.txt)?
> 
> `man git-rev-parse` calls them "revisions". Yes, even the commit:path
> ones.

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

-- Lea

Re: [PATCH v3] perl/Git.pm: add parse_rev method

[Junio C Hamano]

Lea Wiemann <lewiemann@gmail.com> writes:

> Miklos Vajna wrote:
>> Lea Wiemann <lewiemann@gmail.com> wrote:
>>> Is there *any* name for the identifiers you pass into git-rev-parse
>>> (like HEAD^2 or master:test/foo.txt)?
>>
>> `man git-rev-parse` calls them "revisions". Yes, even the commit:path
>> ones.
>
> True -- it's quite cringeworthy indeed. ;)  As long as it only affects
> the documentation for that particular method, I'll go with "revision".

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

Re: [PATCH v3] perl/Git.pm: add parse_rev method

[Lea Wiemann]

Junio C Hamano wrote:
> I think that is sensible, and the method can stay parse_rev, not get_hash,
> don't you think?

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

[PATCH v4] perl/Git.pm: add get_hash method

[Lea Wiemann]

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(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 97e61ef..f2e9e29 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -43,6 +43,9 @@ $VERSION = '0.01';
   my $tempfile = tempfile();
   my $size = $repo->cat_blob($sha1, $tempfile);
 
+  my $head_commit_sha1 = $repo->get_hash('HEAD');  # see git-rev-parse --help
+  my $file_blob_sha1   = $repo->get_hash('HEAD:path/file.txt');
+
 =cut
 
 
@@ -716,6 +719,48 @@ sub ident_person {
 	return "$ident[0] <$ident[1]>";
 }
 
+=item get_hash ( REVISION )
+
+Look up the object referred to by C<REVISION> and return its SHA1
+hash, or return undef if the lookup failed.  When passed a SHA1 hash,
+always return it even if it doesn't exist in the repository.
+
+Note that C<REVISION> can refer to a commit, file, tree, or tag
+object!  See git rev-parse --help, section "Specifying Revisions", for
+valid formats of the C<REVISION> parameter.
+
+=cut
+
+sub get_hash {
+	# We could allow for a list of revisions here.
+	my ($self, $rev_name) = @_;
+
+	my $hash;
+	try {
+		# The --quiet --verify options cause git-rev-parse to fail
+		# with exit status 1 (instead of 128) if the given revision
+		# name is not found, which enables us to distinguish not-found
+		# from serious errors.  The --default option works around
+		# git-rev-parse's lack of support for getopt style "--"
+		# separators (it would fail for tags named "--foo" without
+		# it).
+		$hash = $self->command_oneline("rev-parse", "--verify", "--quiet",
+					       "--default", $rev_name);
+	} catch Git::Error::Command with {
+		my $E = shift;
+		if ($E->value() == 1) {
+			# Revision name not found.
+			$hash = undef;
+		} else {
+			throw $E;
+		}
+	};
+	# Guard against unexpected output.
+	throw Error::Simple(
+		"get_hash: unexpected output for \"$rev_name\": $hash")
+	    if defined $hash and $hash !~ /^([0-9a-fA-F]{40})$/;
+	return $hash;
+}
 
 =item hash_object ( TYPE, FILENAME )
 
-- 
1.5.5.GIT

[PATCH v4] perl/Git.pm: add get_hash method

[Lea Wiemann]

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(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 97e61ef..f2e9e29 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -43,6 +43,9 @@ $VERSION = '0.01';
   my $tempfile = tempfile();
   my $size = $repo->cat_blob($sha1, $tempfile);
 
+  my $head_commit_sha1 = $repo->get_hash('HEAD');  # see git-rev-parse --help
+  my $file_blob_sha1   = $repo->get_hash('HEAD:path/file.txt');
+
 =cut
 
 
@@ -716,6 +719,48 @@ sub ident_person {
 	return "$ident[0] <$ident[1]>";
 }
 
+=item get_hash ( REVISION )
+
+Look up the object referred to by C<REVISION> and return its SHA1
+hash, or return undef if the lookup failed.  When passed a SHA1 hash,
+always return it even if it doesn't exist in the repository.
+
+Note that C<REVISION> can refer to a commit, file, tree, or tag
+object!  See git rev-parse --help, section "Specifying Revisions", for
+valid formats of the C<REVISION> parameter.
+
+=cut
+
+sub get_hash {
+	# We could allow for a list of revisions here.
+	my ($self, $rev_name) = @_;
+
+	my $hash;
+	try {
+		# The --quiet --verify options cause git-rev-parse to fail
+		# with exit status 1 (instead of 128) if the given revision
+		# name is not found, which enables us to distinguish not-found
+		# from serious errors.  The --default option works around
+		# git-rev-parse's lack of support for getopt style "--"
+		# separators (it would fail for tags named "--foo" without
+		# it).
+		$hash = $self->command_oneline("rev-parse", "--verify", "--quiet",
+					       "--default", $rev_name);
+	} catch Git::Error::Command with {
+		my $E = shift;
+		if ($E->value() == 1) {
+			# Revision name not found.
+			$hash = undef;
+		} else {
+			throw $E;
+		}
+	};
+	# Guard against unexpected output.
+	throw Error::Simple(
+		"get_hash: unexpected output for \"$rev_name\": $hash")
+	    if defined $hash and $hash !~ /^([0-9a-fA-F]{40})$/;
+	return $hash;
+}
 
 =item hash_object ( TYPE, FILENAME )
 
-- 
1.5.5.GIT