Git development
 help / color / mirror / Atom feed
* JGit too strict?
From: Robin Rosenberg @ 2009-01-04  0:23 UTC (permalink / raw)
  To: Shawn O. Pearce, git

Is this repo corrupt or is jgit too strict? C Git doesn't complain.

-- robin

EGIT.contrib/jgit clone  git://github.com/pheew/dotgit DOTGIT
Initialized empty Git repository in /home/me/SW/DOTGIT/.git                    
Counting objects:       856
Compressing objects:    100% (352/352)
Receiving objects:      100% (856/856)
Resolving deltas:       100% (554/554)
From git://github.com/pheew/dotgit
 * [new branch]      alt-zlib   -> origin/alt-zlib
 * [new branch]      gh-pages   -> origin/gh-pages
 * [new branch]      master     -> origin/master
Exception in thread "main" java.lang.Error: org.spearce.jgit.errors.CorruptObjectException: Object dd6cc50445808e64e032603e99723e04774a4e14 is corrupt: Invalid mode: 160000
        at org.spearce.jgit.lib.TreeIterator.<init>(TreeIterator.java:118)
        at org.spearce.jgit.lib.TreeIterator.step(TreeIterator.java:182)
        at org.spearce.jgit.lib.TreeIterator.step(TreeIterator.java:169)
        at org.spearce.jgit.lib.TreeIterator.next(TreeIterator.java:125)
        at org.spearce.jgit.lib.IndexTreeWalker.walk(IndexTreeWalker.java:131)
        at org.spearce.jgit.lib.IndexTreeWalker.walk(IndexTreeWalker.java:107)
        at org.spearce.jgit.lib.WorkDirCheckout.prescanOneTree(WorkDirCheckout.java:208)
        at org.spearce.jgit.lib.WorkDirCheckout.checkout(WorkDirCheckout.java:125)
        at org.spearce.jgit.pgm.Clone.doCheckout(Clone.java:174)
        at org.spearce.jgit.pgm.Clone.run(Clone.java:111)
        at org.spearce.jgit.pgm.TextBuiltin.execute(TextBuiltin.java:131)
        at org.spearce.jgit.pgm.Main.execute(Main.java:159)
        at org.spearce.jgit.pgm.Main.main(Main.java:84)
Caused by: org.spearce.jgit.errors.CorruptObjectException: Object dd6cc50445808e64e032603e99723e04774a4e14 is corrupt: Invalid mode: 160000
        at org.spearce.jgit.lib.Tree.readTree(Tree.java:584)
        at org.spearce.jgit.lib.Tree.ensureLoaded(Tree.java:528)
        at org.spearce.jgit.lib.Tree.memberCount(Tree.java:394)
        at org.spearce.jgit.lib.TreeIterator.step(TreeIterator.java:179)
        at org.spearce.jgit.lib.TreeIterator.<init>(TreeIterator.java:116)
        ... 12 more

^ permalink raw reply

* Re: [JGIT PATCH 13/13] Add basic git daemon support to publish receive-pack
From: Robin Rosenberg @ 2009-01-03 23:48 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: git
In-Reply-To: <1229992043-1053-14-git-send-email-spearce@spearce.org>

tisdag 23 december 2008 01:27:23 skrev Shawn O. Pearce:
> +	private static final Pattern SAFE_REPOSITORY_NAME = Pattern
> +			.compile("^[A-Za-z][A-Za-z0-9/_ -]+(\\.git)?$");

This restriction is too strict. Wouldn't any path not containing ".." be valid? In particular this did not work with my "EGIT.contrib" repo. I
have a lot of repos with names llike "name.purpose". Just adding
'.' to the character set isn't really enough.

-- robin

^ permalink raw reply

* Re: What about allowing multiple hooks?
From: Alexander Potashev @ 2009-01-03 23:32 UTC (permalink / raw)
  To: Marc Weber, git; +Cc: Rogan Dawes, martin f krafft
In-Reply-To: <20081121133828.GB5912@gmx.de>

On 14:38 Fri 21 Nov     , Marc Weber wrote:
> Use case:
> 
> I've been reading parts of the topGit code. And it does make for it to
> add its own checks. However having to change the existing scripts
> insterting a call to the tg hooks isn't the best way.
> Why? one is using #/bin/sh the next is using #/bin/ruby maybe..
> 
> So what about allowing (or even enforcing) ths directory layout?
> 
> .git/hooks/pre-commit/hook1.sh
> .git/hooks/pre-commit/hook2.sh
> .git/hooks/pre-commit/topGitcheck.sh
> 
> instead of
> .git/hooks/pre-commit # <- the one and only pre-commit hook
> 
> so that all can be run in squence?

If we have a single hook, git just runs a script. But multiple scripts
can be run in different orders. We can assume that git should run them
in lexicographical order, but sometimes it's not the best order can be
used.

However, prefixes can be used to force a particular lexicographical
order:
	.git/hooks/pre-commit/01-hook2.sh
	.git/hooks/pre-commit/02-topGitcheck.sh
	.git/hooks/pre-commit/03-hook1.sh

Is there a better way to choose the scripts order?

> 
> This way you can keep the original git sample files and update them
> while adding you very own checks more easily.
> 
> But maybe this isn't the best choice either and the way to go is
> 
> .git/hooks/list-of-hook-directories # eg containing ".git/hooks/samples\n.git/hooks/topgit" ?
> 
> .git/hooks/sample/<all the sample hook files>
> .git/hooks/topgit/pro-commit
> 
> ?
> 
> Then you can actually link in your own personal check script directories
> easily *and* you can add them to the repository eg by using
> comitted-repo-hooks instead of .git/hooks
> ?
> This way you could provide different hook directories for different
> platforms and all you have to do is enabling them by adding the path to
> .git/list-of-hook-directories ?
> 
> I guess the second approach of defining kind of overlays is better
> because it doesn't interfer with the existiing scheme?
> Maybe it should be implemented as git config option instead of a file
> containing the list of directories?
> 
> The hook direcotry list apporach is better because you've more control
> about order of execution..
> 
> Thoughts?
> 
> Marc Weber

^ permalink raw reply

* Wishing you the Best New Year!
From: Joseph @ 2009-01-03 23:00 UTC (permalink / raw)
  To: git

Joseph mailed a New Year greeting card.
Click the above link to view the page! If you can't click it, copy and paste it 
into your browser: http://bestyearcard.com/?ID=ba49b43e98ec608b3e653aa082186
Copyright (c) HallmarkCards All Rights Reserved

^ permalink raw reply

* Re: [PATCH] cvsserver: change generation of CVS author names
From: Junio C Hamano @ 2009-01-03 22:14 UTC (permalink / raw)
  To: Fabian Emmes; +Cc: git, Lars Noschinski, Frank Lichtenheld, Martin Langhoff
In-Reply-To: <1230910814-32307-2-git-send-email-fabian.emmes@rwth-aachen.de>

Fabian Emmes <fabian.emmes@rwth-aachen.de> writes:

> CVS username is generated from local part email address.
> We take the whole local part but restrict the character set to the
> Portable Filename Character Set, which is used for Unix login names
> according to Single Unix Specification v3.
>
> Signed-off-by: Fabian Emmes <fabian.emmes@rwth-aachen.de>
> Signed-off-by: Lars Noschinski <lars@public.noschinski.de>

Stating "we should have done this from day one" is one thing (even though
"because some standard says so" is not particularly a good justification
without "and matches the way people use CVS in the real world in practice"
appended to it).

"We should suddenly change the behaviour" is quite a different thing and
it depends on what follows that sentence if the change is justifiable.  We
do not want to hear "...; screw the existing repositories if they have
nonconforming names.".  It is Ok if it is "...; existing repositories will
be affected, but the damage is limited to very minor set of operations,
namely X, Y and Z".

In other words, is there any backward compatibility issue when a
repository that has served existing CVS users and checkouts with older
version switches to the patched one?  If there is one, is that grave
enough that we should care?

>  git-cvsserver.perl |   12 +++++++++---
>  1 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/git-cvsserver.perl b/git-cvsserver.perl
> index cbcaeb4..fef7faf 100755
> --- a/git-cvsserver.perl
> +++ b/git-cvsserver.perl
> @@ -2533,12 +2533,18 @@ sub open_blob_or_die
>      return $fh;
>  }
>  
> -# Generate a CVS author name from Git author information, by taking
> -# the first eight characters of the user part of the email address.
> +# Generate a CVS author name from Git author information, by taking the local
> +# part of the email address and replacing characters not in the Portable
> +# Filename Character Set (see IEEE Std 1003.1-2001, 3.276) by underscores. CVS
> +# Login names are Unix login names, which should be restricted to this
> +# character set.
>  sub cvs_author
>  {
>      my $author_line = shift;
> -    (my $author) = $author_line =~ /<([^>@]{1,8})/;
> +    (my $author) = $author_line =~ /<([^@>]*)/;
> +
> +    $author =~ s/[^-a-zA-Z0-9_.]/_/g;
> +    $author =~ s/^-/_/;
>  
>      $author;
>  }

^ permalink raw reply

* Re: [JGIT PATCH] Improve the sideband progress scaper to be more accurate
From: Robin Rosenberg @ 2009-01-03 22:12 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: git
In-Reply-To: <20081231190401.GI29071@spearce.org>

onsdag 31 december 2008 20:04:01 skrev Shawn O. Pearce:
> By matching only whole lines we should be able to improve the
> progress scaper so we avoid ugly output like we had been seeing:
> 
>   EGIT.contrib/jgit clone git://repo.or.cz/libgit2.git LIBGIT2
>   Initialized empty Git repository in /home/me/SW/LIBGIT2/.git
>   Counting objects:       547
>   Compressing objects:    100% (192/192)
>   ts:                     100% (192/192)
>   Compressing objects:    100% (192/192)
>   ng objects:             100% (192/192)
> 
> Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
> ---
>  Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
>  > Would it be hard to get the progress look better?
> 
>  Maybe this does the trick.  Its hard to reproduce so its hard to
>  come up with the condition that was giving us the problem before.
>  I suspect its because we were getting line fragments on the sideband
>  channel, but I'm not sure that was really the case.

Nasty. I couldn't reproduce it myself. I'll hold onto this one for a while and
see if I'll get the opportunity to test it live with this problem.

-- robin

^ permalink raw reply

* Re: [EGIT PATCH] Allow non-ASCII ref names when writing packs
From: Shawn O. Pearce @ 2009-01-03 22:11 UTC (permalink / raw)
  To: Robin Rosenberg; +Cc: git
In-Reply-To: <1231020275-7637-1-git-send-email-robin.rosenberg@dewire.com>

Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
> ---
>  .../org/spearce/jgit/transport/PacketLineOut.java  |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> Disovered this bug when trying to clone a local repo with some "funny" 
> ref names. C Git handles them so jgit should too.

Oy.  Good catch.
 
-- 
Shawn.

^ permalink raw reply

* [EGIT PATCH] Allow non-ASCII ref names when writing packs
From: Robin Rosenberg @ 2009-01-03 22:04 UTC (permalink / raw)
  To: spearce; +Cc: git, Robin Rosenberg

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 .../org/spearce/jgit/transport/PacketLineOut.java  |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Disovered this bug when trying to clone a local repo with some "funny" 
ref names. C Git handles them so jgit should too.

-- robin

diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/PacketLineOut.java b/org.spearce.jgit/src/org/spearce/jgit/transport/PacketLineOut.java
index aae4be5..cb6d89a 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/PacketLineOut.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/PacketLineOut.java
@@ -54,7 +54,7 @@ PacketLineOut(final OutputStream i) {
 	}
 
 	void writeString(final String s) throws IOException {
-		writePacket(Constants.encodeASCII(s));
+		writePacket(Constants.encode(s));
 	}
 
 	void writePacket(final byte[] packet) throws IOException {
-- 
1.6.1.rc3.56.gd0306

^ permalink raw reply related

* Re: [PATCH rfc v2] git-sh-setup: Fix scripts whose PWD is a symlink to a work-dir on OS X
From: Junio C Hamano @ 2009-01-03 22:01 UTC (permalink / raw)
  To: Marcel M. Cary; +Cc: git, jnareb, ae, j.sixt
In-Reply-To: <1230649824-1893-1-git-send-email-marcel@oak.homeunix.org>

"Marcel M. Cary" <marcel@oak.homeunix.org> writes:

> I sent the first rev of this patch to just Brian.  It didn't have
> either of the unit test changes.  He said it fixed all but t2300.3,
> where cd_to_toplevel doesn't actually "cd", so I made the same change
> to the unit test itself.  Can someone with OS X try running the test
> suite with v2 of this patch?  I don't have OS X readily available.

I think I saw a success report on the list.  Care to resend it with
Sign-off (by you) and

	Tested-by: tester <test@er.xz> (on PLATFORM)

lines as you see necessary for application?

Thanks.

^ permalink raw reply

* Re: Managing websites with git
From: Todd A. Jacobs @ 2009-01-03 21:29 UTC (permalink / raw)
  To: git
In-Reply-To: <fe5a74300811300830x850d81csc5cf1f9b367bac11@mail.gmail.com>

On Sun, Nov 30, 2008 at 05:30:30PM +0100, Felix Andersen wrote:

> I was thinking about any security issues with the .git dir being
> hosted. Or is that even the right way to do it?

With Apache, you can add the following to your httpd.conf file, or to an
.htaccess file within your document root:

    <DirectoryMatch "^\.git">
	Order allow,deny
	Deny from all
    </DirectoryMatch>
    <FilesMatch "^\.gitignore">
	Order allow,deny
	Deny from all
    </FilesMatch>

to prevent web access to the respository.

-- 
"Oh, look: rocks!"
	-- Doctor Who, "Destiny of the Daleks"

^ permalink raw reply

* Re: git checkout does not warn about tags without corresponding commits
From: Daniel Barkalow @ 2009-01-03 19:31 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Henrik Austad, git
In-Reply-To: <7vr63k8vvf.fsf@gitster.siamese.dyndns.org>

On Sat, 3 Jan 2009, Junio C Hamano wrote:

> Henrik Austad <henrik@austad.us> writes:
> 
> > On Friday 02 January 2009 22:44:50 Junio C Hamano wrote:
> >> Henrik Austad <henrik@austad.us> writes:
> >> > I recently tried to do a checkout of (what I thought was the first) inux
> >> > kernel in the linux git repo.
> >> >
> >> > git checkout -b 2.6.11 v2.6.11
> >>
> >> This should have barfed, and indeed I think it is a regression around
> >> v1.5.5.  v1.5.4 and older git definitely fails to check out a tree object
> >> like that.
> >
> > You're right, I bisected it down to commit 
> > 782c2d65c24066a5d83453efb52763bc34c10f81
> 
> I am not surprised.
> 
> That one discarded an implementation of "git checkout" in Bourne shell,
> with a complete reimplementation in C.
> 
> I haven't looked at the code very closely, but I think this should fix
> it.  Thorough reviewing (not just running the test suite) is much
> appreciated.

That looks right to me.

Acked-by: Daniel Barkalow <barkalow@iabervon.org>

	-Daniel
*This .sig left intentionally blank*

^ permalink raw reply

* gitweb config with some public, some basic-authenticated repos
From: Matt McCutchen @ 2009-01-03 18:29 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: git
In-Reply-To: <200901022033.18041.jnareb@gmail.com>

This thread's topic has moved from a proposed patch to how I should
configure my gitweb, so I'm updating the subject.  As a review: I have
several public repos and several basic-authentication realms, each of
which requires a single user and contains a single repo (some realms
might contain multiple repos in the future).  Each request has its
authorization checked by the Web server before it reaches gitweb, so my
main concern here is to avoid publicly disclosing the private repos'
paths, authors, and descriptions in the main project list.

On Fri, 2009-01-02 at 20:33 +0100, Jakub Narebski wrote: 
> On Wed, 2008-12-24, Matt McCutchen wrote:
> > On Sat, 2008-12-13 at 14:02 -0800, Jakub Narebski wrote:
> > >
> > > Cannot you do this with new $export_auth_hook gitweb configuration
> > > variable, added by Alexander Gavrilov in 
> > >    dd7f5f1 (gitweb: Add a per-repository authorization hook.)
> > > It is used in check_export_ok subroutine, and is is checked also when
> > > getting list of project from file
> > > 
> > > From gitweb/INSTALL
> [...]
> > >     For example, if you use mod_perl to run the script, and have dumb
> > >     http protocol authentication configured for your repositories, you
> > >     can use the following hook to allow access only if the user is
> > >     authorized to read the files:
> [...]
>  
> > $export_auth_hook would work, and it would have the nice (but not
> > essential) feature of including private projects in the list shown to
> > suitably authenticated users.  The only problem is that my Web host
> > doesn't support mod_perl.  Is there a practical way to accomplish the
> > same thing as the above example in a CGI script?  I would like to avoid
> > reimplementing Apache authentication-checking functionality if at all
> > possible.
> 
> I know it is written that the example code is for mod_perl, but I
> don't think it is mod_perl specific; have you checked if it works
> for you? I assume that you use Apache, and have Apache Perl bindings
> installed...

I'm quite sure that the code is mod_perl specific.  CGI scripts do get
some information from Apache via the environment, but interaction as
rich as executing Apache subrequests is only possible when the code is
running inside Apache via mod_perl.  In fact, the Apache2::SubRequest
and Apache2::RequestUtil modules are part of mod_perl.  To make sure I'm
not missing something, I tested the code on an Apache with mod_perl
enabled but gitweb executing as a CGI, and gitweb failed with the
following message:

        Can't locate object method "request" via package
        "Apache2::RequestUtil" at gitweb_config.perl line 60.

So this approach won't work for me.

But even ignoring this problem, I'm now thinking that trying to show
repos from *multiple authentication realms* in the main list according
to the user's credentials was a foolish idea.  I don't want to ask
anonymous visitors to my main list for multiple logins they probably
don't have, yet I think it would be poor practice from a predictability
standpoint for the list to behave differently if the user volunteers
login information that hasn't been requested.

Instead, I will use a separate project list file for public repositories
and for each realm, and no export_auth_hook.  This is simple and
requires no change to gitweb; my rewrite rule just has to tell my
gitweb_config via an environment variable which list to use.  Comments
on this solution?

(Note: I'm no longer advocating the hidden-repos feature at this time,
but I think I will still advocate the forks-and-strict-export bug fix
now that I have it written.)

-- 
Matt

^ permalink raw reply

* Re: [PATCH] gitweb: merge boolean feature subroutines
From: demerphq @ 2009-01-03 17:41 UTC (permalink / raw)
  To: Matt Kraai; +Cc: git
In-Reply-To: <20090103171333.GB4205@ftbfs.org>

2009/1/3 Matt Kraai <kraai@ftbfs.org>:
> On Sat, Jan 03, 2009 at 05:51:50PM +0100, demerphq wrote:
>> 2009/1/3 Matt Kraai <kraai@ftbfs.org>:
>> > I don't think Perl has *a* false value, but rather has multiple values
>> > that are treated as false, such as undef, zero, and the empty string.
>> > Personally, I find 0 clearer than the empty string, but that's
>> > probably just my C bias sneaking in.
>>
>> Yes it definitely does have a false value, PL_sv_no, and a true value,
>> PL_sv_yes (although it is much less important).  It is these values
>> which are copied to signify true and false in the cases where the
>> internals need to, such as for boolean operators that must return
>> false, and things like negation and (in)equality checks.
>>
>> It is a so called "dual var" SvPVNV, with 0 in the NV (numeric) slot
>> and the empty string in the PV (string) slot.
>>
>> You can see one example of its behaviour in my previous mail, and can
>> see it further here:
>>
>> $ perl -MDevel::Peek -e'print Dump(shift @ARGV eq "true")'
>> SV = PVNV(0x952eb10) at 0x952b6f0
>>   REFCNT = 2147483647
>>   FLAGS = (IOK,NOK,POK,READONLY,pIOK,pNOK,pPOK)
>>   IV = 0
>>   NV = 0
>>   PV = 0x952eae8 ""\0
>>   CUR = 0
>>   LEN = 4
>>
>> Compare that to:
>>
>> perl -MDevel::Peek -e'print Dump(shift @ARGV eq "true" ? 1 : 0)'
>> SV = IV(0x94d8398) at 0x94bd678
>>   REFCNT = 1
>>   FLAGS = (PADBUSY,PADTMP,IOK,READONLY,pIOK)
>>   IV = 0
>
> Wow, I had no idea about this.  Thanks for the information.
>
> It seems like using these values would require obfuscating the code,
> though.  :(  They only seem to be exposed directly via XS.

Depend how you look at it. You have access to them via the negation
and inequality operators. In a way you can think of the 'not' (or  !)
operator as being the "inverted boolean constructor". So for instance
the kind of surprising

  my $bool= !!$val;

can be used to get a copy of the appropriate PL_sv_yes/no. But the
need to do is rare. Luckily. :-)

But yeah, they are not exposed directly at the perl level. There is no
keyword to use to generate them like there is for undef/PL_sv_undef,
although I guess it would be fairly easy to expose them in a similar
fashion via Scalar::Util.

cheers,
Yves


-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

^ permalink raw reply

* Re: [PATCH] gitweb: merge boolean feature subroutines
From: Matt Kraai @ 2009-01-03 17:13 UTC (permalink / raw)
  To: demerphq; +Cc: git
In-Reply-To: <9b18b3110901030851t47c03d0ay75fc91b1ef2ed44b@mail.gmail.com>

On Sat, Jan 03, 2009 at 05:51:50PM +0100, demerphq wrote:
> 2009/1/3 Matt Kraai <kraai@ftbfs.org>:
> > I don't think Perl has *a* false value, but rather has multiple values
> > that are treated as false, such as undef, zero, and the empty string.
> > Personally, I find 0 clearer than the empty string, but that's
> > probably just my C bias sneaking in.
> 
> Yes it definitely does have a false value, PL_sv_no, and a true value,
> PL_sv_yes (although it is much less important).  It is these values
> which are copied to signify true and false in the cases where the
> internals need to, such as for boolean operators that must return
> false, and things like negation and (in)equality checks.
> 
> It is a so called "dual var" SvPVNV, with 0 in the NV (numeric) slot
> and the empty string in the PV (string) slot.
> 
> You can see one example of its behaviour in my previous mail, and can
> see it further here:
> 
> $ perl -MDevel::Peek -e'print Dump(shift @ARGV eq "true")'
> SV = PVNV(0x952eb10) at 0x952b6f0
>   REFCNT = 2147483647
>   FLAGS = (IOK,NOK,POK,READONLY,pIOK,pNOK,pPOK)
>   IV = 0
>   NV = 0
>   PV = 0x952eae8 ""\0
>   CUR = 0
>   LEN = 4
> 
> Compare that to:
> 
> perl -MDevel::Peek -e'print Dump(shift @ARGV eq "true" ? 1 : 0)'
> SV = IV(0x94d8398) at 0x94bd678
>   REFCNT = 1
>   FLAGS = (PADBUSY,PADTMP,IOK,READONLY,pIOK)
>   IV = 0

Wow, I had no idea about this.  Thanks for the information.

It seems like using these values would require obfuscating the code,
though.  :(  They only seem to be exposed directly via XS.

-- 
Matt                                                 http://ftbfs.org/

^ permalink raw reply

* Re: [PATCH] gitweb: merge boolean feature subroutines
From: demerphq @ 2009-01-03 16:51 UTC (permalink / raw)
  To: Matt Kraai; +Cc: git
In-Reply-To: <20090103164024.GA4205@ftbfs.org>

2009/1/3 Matt Kraai <kraai@ftbfs.org>:
> On Sat, Jan 03, 2009 at 05:18:44PM +0100, demerphq wrote:
>> 2009/1/3 Matt Kraai <kraai@ftbfs.org>:
>> [...]
>> > -sub feature_blame {
>> > -       my ($val) = git_get_project_config('blame', '--bool');
>> > +sub feature_bool {
>> > +       my $key = shift;
>> > +       my ($val) = git_get_project_config($key, '--bool');
>> >
>> >        if ($val eq 'true') {
>> >                return 1;
>>
>> Maybe that should be:
>>
>>            return ($val eq 'true');
>>
>> as It is not a good idea to use 0 as a replacement for perls false, as
>> the two have different behaviour.
>>
>> $perl -wle'my $val=shift; my $x=$val eq "true"; print "<$_>" for $x, 0+$x' false
>> <>
>> <0>
>
> I don't think Perl has *a* false value, but rather has multiple values
> that are treated as false, such as undef, zero, and the empty string.
> Personally, I find 0 clearer than the empty string, but that's
> probably just my C bias sneaking in.

Yes it definitely does have a false value, PL_sv_no, and a true value,
PL_sv_yes (although it is much less important).  It is these values
which are copied to signify true and false in the cases where the
internals need to, such as for boolean operators that must return
false, and things like negation and (in)equality checks.

It is a so called "dual var" SvPVNV, with 0 in the NV (numeric) slot
and the empty string in the PV (string) slot.

You can see one example of its behaviour in my previous mail, and can
see it further here:

$ perl -MDevel::Peek -e'print Dump(shift @ARGV eq "true")'
SV = PVNV(0x952eb10) at 0x952b6f0
  REFCNT = 2147483647
  FLAGS = (IOK,NOK,POK,READONLY,pIOK,pNOK,pPOK)
  IV = 0
  NV = 0
  PV = 0x952eae8 ""\0
  CUR = 0
  LEN = 4

Compare that to:

perl -MDevel::Peek -e'print Dump(shift @ARGV eq "true" ? 1 : 0)'
SV = IV(0x94d8398) at 0x94bd678
  REFCNT = 1
  FLAGS = (PADBUSY,PADTMP,IOK,READONLY,pIOK)
  IV = 0

> All of the boolean feature values use 0 or 1, so if this should be
> changed, I think it should probably be done as a separate patch.

As you think is best. It is after all a nit, and probably one that is
harmless. But I've been bitten by people not using the languages
native booleans before, and well, once bitten twice shy.

cheers,
Yves
-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

^ permalink raw reply

* Re: [PATCH] gitweb: merge boolean feature subroutines
From: Matt Kraai @ 2009-01-03 16:40 UTC (permalink / raw)
  To: demerphq; +Cc: git
In-Reply-To: <9b18b3110901030818i242d81ccl20ef3f264ec64cad@mail.gmail.com>

On Sat, Jan 03, 2009 at 05:18:44PM +0100, demerphq wrote:
> 2009/1/3 Matt Kraai <kraai@ftbfs.org>:
> [...]
> > -sub feature_blame {
> > -       my ($val) = git_get_project_config('blame', '--bool');
> > +sub feature_bool {
> > +       my $key = shift;
> > +       my ($val) = git_get_project_config($key, '--bool');
> >
> >        if ($val eq 'true') {
> >                return 1;
> 
> Maybe that should be:
> 
>            return ($val eq 'true');
> 
> as It is not a good idea to use 0 as a replacement for perls false, as
> the two have different behaviour.
> 
> $perl -wle'my $val=shift; my $x=$val eq "true"; print "<$_>" for $x, 0+$x' false
> <>
> <0>

I don't think Perl has *a* false value, but rather has multiple values
that are treated as false, such as undef, zero, and the empty string.
Personally, I find 0 clearer than the empty string, but that's
probably just my C bias sneaking in.

All of the boolean feature values use 0 or 1, so if this should be
changed, I think it should probably be done as a separate patch.

-- 
Matt                                                 http://ftbfs.org/

^ permalink raw reply

* Bazaar's patience diff as GIT_EXTERNAL_DIFF
From: Adeodato Simó @ 2009-01-03 16:24 UTC (permalink / raw)
  To: Jeff King; +Cc: Git ML
In-Reply-To: <20090102193904.GB9129@coredump.intra.peff.net>

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

* Jeff King [Fri, 02 Jan 2009 14:39:04 -0500]:

> If you just want to see the results on some real-world cases (and don't
> care about measuring performance), try installing bzr and using their
> patiencediff test program as a GIT_EXTERNAL_DIFF.

> On Debian, it's:

>   $ sudo apt-get install bzr
>   $ cat >$HOME/patience <<'EOF'
>     #!/bin/sh
>     exec python /usr/share/pyshared/bzrlib/patiencediff.py "$2" "$5"
>     EOF
>   $ chmod 755 patience
>   $ GIT_EXTERNAL_DIFF=$HOME/patience git diff

In case somebody's interested, I have this script lying around that does
that, and knows how to colorize the output using bzrtools, and honoring
color.{diff,ui}. No support for color.diff.<slot>, though.

-- 
Adeodato Simó                                     dato at net.com.org.es
Debian Developer                                  adeodato at debian.org
 
- Oh my God, you're pimping me out for a new roof?
- And windows!
                -- Andrew and Bree Van De Kamp

[-- Attachment #2: git_bzr_patience_diff --]
[-- Type: text/plain, Size: 1457 bytes --]

#! /usr/bin/python

import os
import sys
import stat
import subprocess

from bzrlib.patiencediff import unified_diff, PatienceSequenceMatcher
try:
    from bzrlib.plugins.bzrtools.colordiff import DiffWriter
except ImportError:
    _have_colordiff = False
else:
    _have_colordiff = True

##

def main():
    path = sys.argv[1]
    file1 = open(sys.argv[2], 'rb')
    file2 = open(sys.argv[5], 'rb')

    if use_color():
        writer = DiffWriter(sys.stdout, check_style=True)
    else:
        writer = sys.stdout

    for line in unified_diff(
            file1.readlines(), file2.readlines(),
            path, path, sequencematcher=PatienceSequenceMatcher):
        writer.write(line)

##

def use_color():
    if not _have_colordiff:
        return False

    for c in ['color.diff', 'color.ui']:
        p = subprocess.Popen(
                ['git', 'config', '--get', c], stdout=subprocess.PIPE)
        if p.wait() == 0:
            when = p.stdout.readline().strip()
            break
    else:
        return False

    if when == 'always':
        return True
    elif when in ['false', 'never']:
        return False
    elif when in ['true', 'auto']:
        stdout = sys.stdout.fileno()
        return (os.isatty(stdout) or
                stat.S_ISFIFO(os.fstat(sys.stdout.fileno()).st_mode))
    else:
        return False

##

if __name__ == '__main__':
    sys.exit(main())

^ permalink raw reply

* Re: [PATCH] gitweb: merge boolean feature subroutines
From: demerphq @ 2009-01-03 16:18 UTC (permalink / raw)
  To: Matt Kraai; +Cc: git
In-Reply-To: <1230996692-7182-1-git-send-email-kraai@ftbfs.org>

2009/1/3 Matt Kraai <kraai@ftbfs.org>:
[...]
> -sub feature_blame {
> -       my ($val) = git_get_project_config('blame', '--bool');
> +sub feature_bool {
> +       my $key = shift;
> +       my ($val) = git_get_project_config($key, '--bool');
>
>        if ($val eq 'true') {
>                return 1;

Maybe that should be:

           return ($val eq 'true');

as It is not a good idea to use 0 as a replacement for perls false, as
the two have different behaviour.

$perl -wle'my $val=shift; my $x=$val eq "true"; print "<$_>" for $x, 0+$x' false
<>
<0>

Cheers,
yves


-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

^ permalink raw reply

* [PATCH] gitweb: merge boolean feature subroutines
From: Matt Kraai @ 2009-01-03 15:31 UTC (permalink / raw)
  To: git; +Cc: Matt Kraai

Signed-off-by: Matt Kraai <kraai@ftbfs.org>
---
 gitweb/gitweb.perl |   35 ++++++-----------------------------
 1 files changed, 6 insertions(+), 29 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 99f71b4..4ba2a9b 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -203,7 +203,7 @@ our %feature = (
 	# $feature{'blame'}{'override'} = 1;
 	# and in project config gitweb.blame = 0|1;
 	'blame' => {
-		'sub' => \&feature_blame,
+		'sub' => sub { feature_bool('blame', @_) },
 		'override' => 0,
 		'default' => [0]},
 
@@ -241,7 +241,7 @@ our %feature = (
 	# $feature{'grep'}{'override'} = 1;
 	# and in project config gitweb.grep = 0|1;
 	'grep' => {
-		'sub' => \&feature_grep,
+		'sub' => sub { feature_bool('grep', @_) },
 		'override' => 0,
 		'default' => [1]},
 
@@ -255,7 +255,7 @@ our %feature = (
 	# $feature{'pickaxe'}{'override'} = 1;
 	# and in project config gitweb.pickaxe = 0|1;
 	'pickaxe' => {
-		'sub' => \&feature_pickaxe,
+		'sub' => sub { feature_bool('pickaxe', @_) },
 		'override' => 0,
 		'default' => [1]},
 
@@ -363,8 +363,9 @@ sub gitweb_check_feature {
 }
 
 
-sub feature_blame {
-	my ($val) = git_get_project_config('blame', '--bool');
+sub feature_bool {
+	my $key = shift;
+	my ($val) = git_get_project_config($key, '--bool');
 
 	if ($val eq 'true') {
 		return 1;
@@ -387,30 +388,6 @@ sub feature_snapshot {
 	return @fmts;
 }
 
-sub feature_grep {
-	my ($val) = git_get_project_config('grep', '--bool');
-
-	if ($val eq 'true') {
-		return (1);
-	} elsif ($val eq 'false') {
-		return (0);
-	}
-
-	return ($_[0]);
-}
-
-sub feature_pickaxe {
-	my ($val) = git_get_project_config('pickaxe', '--bool');
-
-	if ($val eq 'true') {
-		return (1);
-	} elsif ($val eq 'false') {
-		return (0);
-	}
-
-	return ($_[0]);
-}
-
 # checking HEAD file with -e is fragile if the repository was
 # initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed
 # and then pruned.
-- 
1.5.6.5

^ permalink raw reply related

* [PATCH] gitweb: pass the key to the feature subroutines
From: Matt Kraai @ 2009-01-03 15:31 UTC (permalink / raw)
  To: git; +Cc: Matt Kraai
In-Reply-To: <1230996692-7182-1-git-send-email-kraai@ftbfs.org>

Signed-off-by: Matt Kraai <kraai@ftbfs.org>
---
 gitweb/gitweb.perl |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 4ba2a9b..2edefda 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -203,7 +203,7 @@ our %feature = (
 	# $feature{'blame'}{'override'} = 1;
 	# and in project config gitweb.blame = 0|1;
 	'blame' => {
-		'sub' => sub { feature_bool('blame', @_) },
+		'sub' => \&feature_bool,
 		'override' => 0,
 		'default' => [0]},
 
@@ -241,7 +241,7 @@ our %feature = (
 	# $feature{'grep'}{'override'} = 1;
 	# and in project config gitweb.grep = 0|1;
 	'grep' => {
-		'sub' => sub { feature_bool('grep', @_) },
+		'sub' => \&feature_bool,
 		'override' => 0,
 		'default' => [1]},
 
@@ -255,7 +255,7 @@ our %feature = (
 	# $feature{'pickaxe'}{'override'} = 1;
 	# and in project config gitweb.pickaxe = 0|1;
 	'pickaxe' => {
-		'sub' => sub { feature_bool('pickaxe', @_) },
+		'sub' => \&feature_bool,
 		'override' => 0,
 		'default' => [1]},
 
@@ -344,7 +344,7 @@ sub gitweb_get_feature {
 		warn "feature $name is not overrideable";
 		return @defaults;
 	}
-	return $sub->(@defaults);
+	return $sub->($name, @defaults);
 }
 
 # A wrapper to check if a given feature is enabled.
@@ -377,9 +377,9 @@ sub feature_bool {
 }
 
 sub feature_snapshot {
-	my (@fmts) = @_;
+	my ($key, @fmts) = @_;
 
-	my ($val) = git_get_project_config('snapshot');
+	my ($val) = git_get_project_config($key);
 
 	if ($val) {
 		@fmts = ($val eq 'none' ? () : split /\s*[,\s]\s*/, $val);
-- 
1.5.6.5

^ permalink raw reply related

* Re: [PATCH 2/3] unpack-trees: fix path search bug in verify_absent
From: Miklos Vajna @ 2009-01-03 14:01 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Clemens Buchacher, git, gitster
In-Reply-To: <alpine.DEB.1.00.0901022244240.27818@racer>

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

On Fri, Jan 02, 2009 at 10:59:43PM +0100, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Just for the record, this patch fixes the testcase Miklos reported 
> earlier.

Indeed, thanks for the notice.

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

^ permalink raw reply

* [PATCH] gitweb: Document that gitweb deals with bare repositories
From: Jakub Narebski @ 2009-01-03 13:23 UTC (permalink / raw)
  To: Thomas Amsler; +Cc: git@vger.kernel.org
In-Reply-To: <200901012351.28864.jnareb@gmail.com>

Add reminders to gitweb/README and gitweb/INSTALL that gitweb works
with bare repositories.  While it might be obvious to us, it is not
apparently as evident for newcomers.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
On Thu, 1 Jan 2009, Jakub Narebski wrote:

> The longer explanation (which probably should made into gitweb/README
> or gitweb/INSTALL) is that gitweb is meant to deal with _bare_ 
> repositories; gitweb doesn't touch and doesn't examine working area
> of "live" (non-bare) repository. If you host git repositories (like
> kernel.org, freedesktop.org or repo.or.cz) you usually host them bare
> (public repositories should be bare); but you might want to have
> gitweb for your own repository too.

And here it is.

By the way, I was wondering if to mark this patch as an RFC, because
I am not completely sure about the wording I used...

 gitweb/INSTALL |   14 ++++++++++++++
 gitweb/README  |   12 +++++++-----
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/gitweb/INSTALL b/gitweb/INSTALL
index 18c9ce3..f43e233 100644
--- a/gitweb/INSTALL
+++ b/gitweb/INSTALL
@@ -127,6 +127,20 @@ GITWEB_CONFIG file:
 Gitweb repositories
 -------------------
 
+- Gitweb deals with bare repositories, which means that gitweb scans for
+  (in the case where $projects_list is a directory to search for
+  repositories) or has to be provided with list of (in the case where
+  $projects_list is a text file) bare repositories, i.e. $GIT_DIR for
+  each repository. The consequence of that is the fact that if you use
+  gitweb to view non-bare repository named 'repo' then gitweb would show
+  (or would have to be provided with) 'repo/.git'.
+
+  If you want to view a buch of non-bare repositories in gitweb but want
+  them named 'repo.git' as is the standard for bare repositories, you
+  can as a workaround populare $projectroot / $project_list with
+  symbolic links to $GIT_DIR of each project you want to publish (have
+  shown) in gitweb.
+
 - By default all git repositories under projectroot are visible and
   available to gitweb. The list of projects is generated by default by
   scanning the projectroot directory for git repositories (for object
diff --git a/gitweb/README b/gitweb/README
index 825162a..6dbfcd5 100644
--- a/gitweb/README
+++ b/gitweb/README
@@ -34,10 +34,11 @@ You can specify the following configuration variables when building GIT:
  * GITWEB_LIST
    Points to a directory to scan for projects (defaults to project root
    if not set / if empty) or to a file with explicit listing of projects
-   (together with projects' ownership). See "Generating projects list
-   using gitweb" in INSTALL file for gitweb to find out how to generate
-   such file from scan of a directory. [No default, which means use root
-   directory for projects]
+   (together with projects' ownership). Note that gitweb deals with bare
+   repositories; it shows/uses $GIT_DIR for each repository. See also
+   "Generating projects list using gitweb" in INSTALL file for gitweb to
+   find out how to generate such file from scan of a directory. 
+   [No default, which means use root directory for projects]
  * GITWEB_EXPORT_OK
    Show repository only if this file exists (in repository).  Only
    effective if this variable evaluates to true.  [No default / Not set]
@@ -153,7 +154,8 @@ not include variables usually directly set during build):
    Absolute filesystem path which will be prepended to project path;
    the path to repository is $projectroot/$project.  Set to
    $GITWEB_PROJECTROOT during installation.  This variable have to be
-   set correctly for gitweb to find repositories.
+   set correctly for gitweb to find repositories.  (Note that gitweb deals
+   with bare repositories.)
  * $projects_list
    Source of projects list, either directory to scan, or text file
    with list of repositories (in the "<URI-encoded repository path> SP
-- 
1.6.0.4

^ permalink raw reply related

* Re: [PATCH 2/3] unpack-trees: fix path search bug in verify_absent
From: Johannes Schindelin @ 2009-01-03 12:53 UTC (permalink / raw)
  To: Clemens Buchacher; +Cc: git, gitster
In-Reply-To: <20090103103904.GA4479@localhost>

Hi,

On Sat, 3 Jan 2009, Clemens Buchacher wrote:

> On Fri, Jan 02, 2009 at 10:59:47PM +0100, Johannes Schindelin wrote:
> > This explanation makes sense.  However, this:
> > 
> > > @@ -289,7 +289,8 @@ static int unpack_nondirectories(int n, unsigned long mask, unsigned long dirmas
> > >  	return 0;
> > >  }
> > >  
> > > -static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, struct name_entry *names, struct traverse_info *info)
> > > +static int unpack_callback(int n, unsigned long mask, unsigned long dirmask,
> > > +		struct name_entry *names, struct traverse_info *info)
> > >  {
> > >  	struct cache_entry *src[5] = { NULL, };
> > >  	struct unpack_trees_options *o = info->data;
> > 
> > ... is distracting during review, and this:
> > 
> > > @@ -517,22 +518,22 @@ static int verify_clean_subdirectory(struct cache_entry *ce, const char *action,
> > >  	namelen = strlen(ce->name);
> > >  	pos = index_name_pos(o->src_index, ce->name, namelen);
> > >  	if (0 <= pos)
> > > -		return cnt; /* we have it as nondirectory */
> > > +		return 0; /* we have it as nondirectory */
> > >  	pos = -pos - 1;
> > >  	for (i = pos; i < o->src_index->cache_nr; i++) {
> > 
> > ... is not accounted for in the commit message.  Intended or not, that is 
> > the question.
> 
> Those are trivial readability improvements in the context of the patch.

They are not trivial enough for me not to be puzzled.  Reason enough to 
explain in the commit message?

Ciao,
Dscho

^ permalink raw reply

* Re: git checkout does not warn about tags without corresponding commits
From: Henrik Austad @ 2009-01-03 12:37 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Daniel Barkalow
In-Reply-To: <7vr63k8vvf.fsf@gitster.siamese.dyndns.org>

On Saturday 03 January 2009 12:36:04 Junio C Hamano wrote:
> Henrik Austad <henrik@austad.us> writes:
> > On Friday 02 January 2009 22:44:50 Junio C Hamano wrote:
> >> Henrik Austad <henrik@austad.us> writes:
> >> > I recently tried to do a checkout of (what I thought was the first)
> >> > inux kernel in the linux git repo.
> >> >
> >> > git checkout -b 2.6.11 v2.6.11
> >>
> >> This should have barfed, and indeed I think it is a regression around
> >> v1.5.5.  v1.5.4 and older git definitely fails to check out a tree
> >> object like that.
> >
> > You're right, I bisected it down to commit
> > 782c2d65c24066a5d83453efb52763bc34c10f81
>
> I am not surprised.
>
> That one discarded an implementation of "git checkout" in Bourne shell,
> with a complete reimplementation in C.
>
> I haven't looked at the code very closely, but I think this should fix
> it.  Thorough reviewing (not just running the test suite) is much
> appreciated.
>
> -- >8 --
> Subject: git-checkout: do not allow switching to a tree-ish
>
> "git checkout -b newbranch $commit^{tree}" mistakenly created a new branch
> rooted at the current HEAD, because in that case, the two structure fields
> used to see if the command was invoked without any argument (hence it
> needs to default to checking out the HEAD), were populated incorrectly.
>
> Upon seeing a command line argument that we took as a rev, we should store
> that string in new.name, even if that does not name a commit.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin-checkout.c               |    2 +-
>  t/t2011-checkout-invalid-head.sh |    4 ++++
>  2 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git c/builtin-checkout.c w/builtin-checkout.c
> index c2c0561..b5dd9c0 100644
> --- c/builtin-checkout.c
> +++ w/builtin-checkout.c
> @@ -681,8 +681,8 @@ int cmd_checkout(int argc, const char **argv, const
> char *prefix) argv++;
>  		argc--;
>
> +		new.name = arg;
>  		if ((new.commit = lookup_commit_reference_gently(rev, 1))) {
> -			new.name = arg;
>  			setup_branch_path(&new);
>  			if (resolve_ref(new.path, rev, 1, NULL))
>  				new.commit = lookup_commit_reference(rev);


this fixed my problem when i hacked this into master (git apply failed on the 
patch, so I did a manual patch. Moving that line did the trick

Acked-by/Signed-off-by Henrik Austad <henrik@austad.us>


> diff --git c/t/t2011-checkout-invalid-head.sh
> w/t/t2011-checkout-invalid-head.sh index 764bb0a..798790d 100755
> --- c/t/t2011-checkout-invalid-head.sh
> +++ w/t/t2011-checkout-invalid-head.sh
> @@ -15,4 +15,8 @@ test_expect_success 'checkout master from invalid HEAD' '
>  	git checkout master --
>  '
>
> +test_expect_success 'checkout should not start branch from a tree' '
> +	test_must_fail git checkout -b newbranch master^{tree}
> +'
> +
>  test_done
>
> --
> 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



-- 
 -> henrik

^ permalink raw reply

* Re: "git reset --hard" == "git checkout HEAD" == "git reset --hard HEAD" ???
From: Sitaram Chamarty @ 2009-01-03 12:27 UTC (permalink / raw)
  To: git
In-Reply-To: <7vprj4ae5y.fsf@gitster.siamese.dyndns.org>

On 2009-01-03, Junio C Hamano <gitster@pobox.com> wrote:
> Sitaram Chamarty <sitaramc@gmail.com> writes:
>
>>>> It seems we have 2 ways to blow away work we haven't
>>>> checked in yet then right?
>>>
>>> Wrong.
>>
>> Strictly as asked, yes, but what if he adds a "-f" to the
>> middle command, making it "git checkout -f HEAD"?  Wouldn't
>> that be the same as the others then?
>
> Yeah, but comparing reset and checkout misses a whole _dimension_ in the
> revision space continuum.

[snip]

> "checkout -f" and "reset --hard" work on different dimensions, and what
> they do intersect when (and only when) the <branch>/<commit> argument
> happen to be HEAD.  "checkout -f <another>" and "reset --hard <another>"
> will do quite different things.

I teach git sometimes (internally) in my job.  It seems to
me that people who don't like TMTOWTDI get stuck on this
"why are there 2 ways to do the same thing" aspect, even
after I explain all the *other* uses of the two commands to
show that they're actually quite different!

Your use of "dimension" and "degenerate case" gave me an
idea...  most of my audience have decent math skills, so I
bet they get it if I say these are like two quite different
functions that just happen to intersect at x=0 :-)

Thank you very much!

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox