* RE: [PATCH] Re: Gitk --all error when there are more than 797 refs in a repository
From: Murphy, John @ 2009-09-21 14:02 UTC (permalink / raw)
To: Paul Mackerras, Pat Thoyts, Johannes Sixt; +Cc: git
In-Reply-To: <19124.8378.975976.347711@cargo.ozlabs.ibm.com>
Johannes Sixt writes:
> I cannot reproduce the error. I have a repository with 100 commits in
a
> linear history and 5000 refs (50 refs per commit). They are named
> refs/heads/branch-XXXX. I don't see any problems with 'gitk --all'.
That still leave you with only 100 unique refs.
You need over 797 unique refs.
> The error must be in some other git invocation.
I put many debug pop-ups around the code and I believe that this the
call that is dying on.
This is the only part of the code that has the error text "fatal:
ambiguous argument".
Paul Mackerras writes:
> I knew about the 32k command-line limit under windows, but I don't see
> how that applies in this case unless it is $revs that is too long (and
> if that is the case then I don't see how your patch helps). Is there
> also a 32k limit on the size of data returned by a command executed
> with [exec]?
In this case $revs is "--all"
I believe what I am experiencing is a 32K limit with [exec]
Additional info:
My repo has: 17,737 commits
^ permalink raw reply
* Re: [PATCH] Re: Gitk --all error when there are more than 797 refs in a repository
From: Johannes Sixt @ 2009-09-21 14:09 UTC (permalink / raw)
To: Murphy, John; +Cc: Paul Mackerras, Pat Thoyts, git
In-Reply-To: <6F87406399731F489FBACE5C5FFA0458518DE8@ex2k.bankofamerica.com>
Murphy, John schrieb:
> Paul Mackerras writes:
>
>> I knew about the 32k command-line limit under windows, but I don't see
>> how that applies in this case unless it is $revs that is too long (and
>> if that is the case then I don't see how your patch helps). Is there
>> also a 32k limit on the size of data returned by a command executed
>> with [exec]?
>
> In this case $revs is "--all"
>
> I believe what I am experiencing is a 32K limit with [exec]
But in order to have a $revs that exceeds 32K, you would already have to
invoke gitk with a huge command line that exceeds the limit (but this is
not possible), no?
How do you run gitk?
-- Hannes
^ permalink raw reply
* Re: [PATCH] pre-commit.sample: don't use [...] around a tr range
From: Jim Meyering @ 2009-09-21 14:10 UTC (permalink / raw)
To: Jeff King; +Cc: Alex Riesen, git list
In-Reply-To: <20090921134427.GA20567@sigio.peff.net>
Jeff King wrote:
> On Mon, Sep 21, 2009 at 01:00:34PM +0200, Jim Meyering wrote:
>
>> > We have (had?) people trying to support Git on HP-UX and SunOS.
>> > Do these count?
>>
>> I had my doubts, but have just confirmed that Solaris 10's
>> /usr/bin/tr is still doing it the SYSV way:
>>
>> $ echo foo | LC_ALL=C /usr/bin/tr a-z A-Z
>> foo
>>
>> There, you have to use /usr/xpg4/bin/tr to get the expected behavior:
>>
>> $ echo foo | LC_ALL=C /usr/xpg4/bin/tr a-z A-Z
>> FOO
>>
>> So you're right. Thanks!
>
> See:
>
> http://article.gmane.org/gmane.comp.version-control.git/76991
>
>> + # Note that the use of brackets around a tr range is ok here, (it's
>> + # even required, for portability to Solaris 10's /usr/bin/tr), since
>> + # the square bracket bytes happen to fall in the designated range.
>> test "$(git diff --cached --name-only --diff-filter=A -z |
>> LC_ALL=C tr -d '[ -~]\0')"
>
> Does this work on non-bracket systems?
Yes, since [] happen to fall in the range.
> I would think that enumerating
> the sequence would be the most portable thing.
Enumerating is more portable, at the expense of
readability and maintainability.
In case you want to go that route, here's one more:
(note that this, like the original range, treats TAB as nonportable)
>From 40a368a7bcf0ac6524bbe36ba3bfdaa0711897b8 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@redhat.com>
Date: Mon, 21 Sep 2009 10:58:02 +0200
Subject: [PATCH] pre-commit.sample: use tr more portability; fix grammar
Avoid tr's M-N range notation altogether.
Instead, enumerate ascii bytes 32..126.
Correct spelling and grammar.
Signed-off-by: Jim Meyering <meyering@redhat.com>
---
templates/hooks--pre-commit.sample | 19 +++++++++++++------
1 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/templates/hooks--pre-commit.sample b/templates/hooks--pre-commit.sample
index b11ad6a..896eb9e 100755
--- a/templates/hooks--pre-commit.sample
+++ b/templates/hooks--pre-commit.sample
@@ -10,19 +10,26 @@
# If you want to allow non-ascii filenames set this variable to true.
allownonascii=$(git config hooks.allownonascii)
+printables=' !"#$%&'\''()*+,-./0123456789:;<=>?@'
+printables="$printables"'ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`'
+printables="$printables"'abcdefghijklmnopqrstuvwxyz{|}~'
+
# Cross platform projects tend to avoid non-ascii filenames; prevent
# them from being added to the repository. We exploit the fact that the
# printable range starts at the space character and ends with tilde.
if [ "$allownonascii" != "true" ] &&
- test "$(git diff --cached --name-only --diff-filter=A -z |
- LC_ALL=C tr -d '[ -~]\0')"
+ # Filter out printable ascii bytes, and NUL.
+ # If anything remains, you lose.
+ rem=$(git diff --cached --name-only --diff-filter=A -z |
+ LC_ALL=C tr -d "$printables"'\0')
+ test -n "$rem"
then
- echo "Error: Attempt to add a non-ascii filename."
+ echo "Error: Attempt to add a non-ascii file name."
echo
- echo "This can cause problems if you want to work together"
- echo "with people on other platforms than you."
+ echo "This can cause problems if you want to work"
+ echo "with people on other platforms."
echo
- echo "To be portable it is adviseable to rename the file ..."
+ echo "To be portable it is advisable to rename the file ..."
echo
echo "If you know what you are doing you can disable this"
echo "check using:"
--
1.6.5.rc1.214.g13c5a
^ permalink raw reply related
* RE: [PATCH] Re: Gitk --all error when there are more than 797 refs in a repository
From: Murphy, John @ 2009-09-21 14:11 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Paul Mackerras, Pat Thoyts, git
In-Reply-To: <4AB78910.7010402@viscovery.net>
Johannes Sixt writes:
> But in order to have a $revs that exceeds 32K, you would already have
to
> invoke gitk with a huge command line that exceeds the limit (but this
is
> not possible), no?
>How do you run gitk?
gitk --all
^ permalink raw reply
* Re: [PATCH] pre-commit.sample: don't use [...] around a tr range
From: Jeff King @ 2009-09-21 14:21 UTC (permalink / raw)
To: Jim Meyering; +Cc: Alex Riesen, git list
In-Reply-To: <87tyywgzhu.fsf@meyering.net>
On Mon, Sep 21, 2009 at 04:10:21PM +0200, Jim Meyering wrote:
> >> test "$(git diff --cached --name-only --diff-filter=A -z |
> >> LC_ALL=C tr -d '[ -~]\0')"
> >
> > Does this work on non-bracket systems?
>
> Yes, since [] happen to fall in the range.
>
> > I would think that enumerating
> > the sequence would be the most portable thing.
>
> Enumerating is more portable, at the expense of
> readability and maintainability.
> In case you want to go that route, here's one more:
Agreed. If we can avoid enumeration, we should. If your original is
portable, then I think it is preferable. Thanks for looking into it.
-Peff
^ permalink raw reply
* Re: [PATCH] Trivial fix: Make all the builtin usage strings to use the same pattern.
From: Brandon Casey @ 2009-09-21 15:28 UTC (permalink / raw)
To: Thiago Farina; +Cc: git
In-Reply-To: <1253452645-3220-1-git-send-email-tfransosi@gmail.com>
Thiago Farina wrote:
> They follow the pattern of file name:
> File name pattern: builtin-command-name.c
> Usage string pattern: builtin_command_name_usage
>
> Signed-off-by: Thiago Farina <tfransosi@gmail.com>
> ---
> diff --git a/builtin-blame.c b/builtin-blame.c
> index 7512773..4adae84 100644
> --- a/builtin-blame.c
> +++ b/builtin-blame.c
> @@ -21,10 +21,11 @@
> #include "parse-options.h"
> #include "utf8.h"
>
> -static char blame_usage[] = "git blame [options] [rev-opts] [rev] [--] file";
> +static char builtin_blame_usage[] =
Should this one also use 'const'?
> +"git blame [options] [rev-opts] [rev] [--] file";
>
> static const char *blame_opt_usage[] = {
> - blame_usage,
> + builtin_blame_usage,
> "",
> "[rev-opts] are documented in git-rev-list(1)",
> NULL
> diff --git a/builtin-count-objects.c b/builtin-count-objects.c
> index 1b0b6c8..fbe0972 100644
> --- a/builtin-count-objects.c
> +++ b/builtin-count-objects.c
> @@ -65,7 +65,7 @@ static void count_objects(DIR *d, char *path, int len, int verbose,
> }
> }
>
> -static char const * const count_objects_usage[] = {
> +static char const * const builtin_count_objects_usage[] = {
This one is different from the others. I assume 'char const *' is
the same as 'const char *', though I'm used to seeing the latter
form. If we're going for consistency, maybe this should be changed
too. Ultra minor nit-pick. I noticed it, so I mentioned it, but
its your choice.
> "git count-objects [-v]",
> NULL
> };
> diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
> index a5a83f1..715d378 100644
> --- a/builtin-for-each-ref.c
> +++ b/builtin-for-each-ref.c
> @@ -881,7 +881,7 @@ static int opt_parse_sort(const struct option *opt, const char *arg, int unset)
> return 0;
> }
>
> -static char const * const for_each_ref_usage[] = {
> +static char const * const builtin_for_each_ref_usage[] = {
ditto
> "git for-each-ref [options] [<pattern>]",
> NULL
> };
> diff --git a/builtin-fsck.c b/builtin-fsck.c
> index c58b0e3..0857357 100644
> --- a/builtin-fsck.c
> +++ b/builtin-fsck.c
> @@ -564,7 +564,7 @@ static int fsck_cache_tree(struct cache_tree *it)
> return err;
> }
>
> -static char const * const fsck_usage[] = {
> +static char const * const builtin_fsck_usage[] = {
ditto
> "git fsck [options] [<object>...]",
> NULL
> };
^ permalink raw reply
* Re: [PATCH] pre-commit.sample: don't use [...] around a tr range
From: Brandon Casey @ 2009-09-21 15:58 UTC (permalink / raw)
To: Jeff King; +Cc: Jim Meyering, Alex Riesen, git list
In-Reply-To: <20090921134427.GA20567@sigio.peff.net>
Jeff King wrote:
> On Mon, Sep 21, 2009 at 01:00:34PM +0200, Jim Meyering wrote:
>
>>> We have (had?) people trying to support Git on HP-UX and SunOS.
>>> Do these count?
>> I had my doubts, but have just confirmed that Solaris 10's
>> /usr/bin/tr is still doing it the SYSV way:
>>
>> $ echo foo | LC_ALL=C /usr/bin/tr a-z A-Z
>> foo
>>
>> There, you have to use /usr/xpg4/bin/tr to get the expected behavior:
>>
>> $ echo foo | LC_ALL=C /usr/xpg4/bin/tr a-z A-Z
>> FOO
>>
>> So you're right. Thanks!
By the way, modern git inserts /usr/xpg4/bin into PATH before /usr/bin on
Solaris. So /usr/xpg4/bin/tr should always be used on that platform.
-brandon
^ permalink raw reply
* Re: [PATCH] Re: Gitk --all error when there are more than 797 refs in a repository
From: Johannes Sixt @ 2009-09-21 15:59 UTC (permalink / raw)
To: Murphy, John; +Cc: Paul Mackerras, Pat Thoyts, git
In-Reply-To: <6F87406399731F489FBACE5C5FFA0458518E11@ex2k.bankofamerica.com>
Murphy, John schrieb:
> Johannes Sixt writes:
>
>> But in order to have a $revs that exceeds 32K, you would already have
> to
>> invoke gitk with a huge command line that exceeds the limit (but this
> is
>> not possible), no?
>
>> How do you run gitk?
>
> gitk --all
I see it. Here is a bash script that creates a repository that reproduces
the error. It is important that refs which sort alphabetically earlier
also point to earlier commits.
-- snip --
#!/bin/bash
git init
echo initial > file && git add file && git commit -m initial
for ((i = 0; i < 1000; i++))
do
echo $i > file &&
git commit -m $i file > /dev/null &&
printf -v l "branch-%04d" $i &&
git update-ref refs/heads/$l HEAD
done
git gc
-- snip --
On Windows, 'gitk --all' starts with branch-0797, on Linux it starts with
branch-0999 aka master.
I'm just throwing this out to interested parties; I'll not look into it at
this time.
Thanks,
-- Hannes
^ permalink raw reply
* Re: [PATCH] Trivial fix: Make all the builtin usage strings to use the same pattern.
From: Johannes Sixt @ 2009-09-21 16:09 UTC (permalink / raw)
To: Thiago Farina; +Cc: git
In-Reply-To: <1253452645-3220-1-git-send-email-tfransosi@gmail.com>
Thiago Farina schrieb:
> They follow the pattern of file name:
> File name pattern: builtin-command-name.c
> Usage string pattern: builtin_command_name_usage
Well, file names are not so constant: every now and then an external
command is turned into a builtin (and the filename is changed), and we
have even seen that a builtin was turned into an external command.
Moreover, some commands are implemented in the same file, for example 'git
log' and 'git show'. How about naming the variable after the command name?
E.g.:
command: git cat-file
variable: git_cat_file_usage
-- Hannes
^ permalink raw reply
* install does not obey DESTDIR or --prefix for perl modules
From: Craig Taylor @ 2009-09-21 16:05 UTC (permalink / raw)
To: git
Hi all
I'm compiling/installing git in a Solaris environment without root.
Using 'make DESTDIR=<some path> install' to stage an install to an
alternate location.
The perl module component of 'make DESTDIR=<some path> install' installs
into the system perl lib path without prepending the forced install
destination or my '--prefix'.
This seems counter intuitive and I would consider a bug.
Regards
CraigT
--
c^ [c%5e]
The bamboo-shadows move over the stone steps
as if to sweep them, but no dust is stirred;
The moon is reflected deep in the pool, but the
water shows no trace of its penetration.
^ permalink raw reply
* [PATCH] git-submodule should obey --quiet for subcommands
From: Jonathan del Strother @ 2009-09-21 16:46 UTC (permalink / raw)
To: Git Mailing List
Make sure that --quiet is passed through to git-clone & git-fetch.
Signed-off-by: Jonathan del Strother <jon.delStrother@bestbefore.tv>
---
clone & fetch are extremely noisy - perhaps something like the following?
git-submodule.sh | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index bfbd36b..1e1066e 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -106,9 +106,9 @@ module_clone()
if test -n "$reference"
then
- git-clone "$reference" -n "$url" "$path"
+ git-clone ${GIT_QUIET:+-q} "$reference" -n "$url" "$path"
else
- git-clone -n "$url" "$path"
+ git-clone ${GIT_QUIET:+-q} -n "$url" "$path"
fi ||
die "Clone of '$url' into submodule path '$path' failed"
}
@@ -450,7 +450,7 @@ cmd_update()
if test -z "$nofetch"
then
(unset GIT_DIR; cd "$path" &&
- git-fetch) ||
+ git-fetch ${GIT_QUIET:+-q}) ||
die "Unable to fetch in submodule path '$path'"
fi
--
1.6.5.rc1.44.ga1675.dirty
^ permalink raw reply related
* Re: install does not obey DESTDIR or --prefix for perl modules
From: Brandon Casey @ 2009-09-21 16:58 UTC (permalink / raw)
To: c; +Cc: git
In-Reply-To: <20090921160551.GD8173@gryning.com>
Craig Taylor wrote:
> Hi all
>
> I'm compiling/installing git in a Solaris environment without root.
> Using 'make DESTDIR=<some path> install' to stage an install to an
> alternate location.
> The perl module component of 'make DESTDIR=<some path> install' installs
> into the system perl lib path without prepending the forced install
> destination or my '--prefix'.
> This seems counter intuitive and I would consider a bug.
Try 'make prefix=<some path>'.
btw, this is in the first paragraph of the INSTALL document.
-brandon
^ permalink raw reply
* Re: install does not obey DESTDIR or --prefix for perl modules
From: Junio C Hamano @ 2009-09-21 17:58 UTC (permalink / raw)
To: Brandon Casey; +Cc: c, git
In-Reply-To: <Ow6bpZou9Vi0tKlyAN-qfjlAAtXvMqpXEAiG54zZ3C8fLI_6_Bt3oA@cipher.nrlssc.navy.mil>
Brandon Casey <brandon.casey.ctr@nrlssc.navy.mil> writes:
> Craig Taylor wrote:
>> Hi all
>>
>> I'm compiling/installing git in a Solaris environment without root.
>> Using 'make DESTDIR=<some path> install' to stage an install to an
>> alternate location.
>> The perl module component of 'make DESTDIR=<some path> install' installs
>> into the system perl lib path without prepending the forced install
>> destination or my '--prefix'.
>> This seems counter intuitive and I would consider a bug.
>
> Try 'make prefix=<some path>'.
>
> btw, this is in the first paragraph of the INSTALL document.
But is that what Craig is trying to do?
I think he wants to build git to be installed in /usr/bin/git or whatever,
and he would say "prefix=/usr". He however wants "make install" to write
into /var/tmp/g/usr/bin/git, not /usr/bin/git, so that he can for example
make a tarball with "cd /var/tmp/g && tar cf ../git.tar .", and extract it
as root under the real '/'. "make DESTDIR=/var/tmp/g" is exactly for
that, and if it is not working I would say it is a bug.
^ permalink raw reply
* [PATCH v2] Remove '< >' from [<options>] since it's not necessary.
From: Thiago Farina @ 2009-09-21 18:12 UTC (permalink / raw)
To: git; +Cc: Thiago Farina
Signed-off-by: Thiago Farina <tfransosi@gmail.com>
---
Documentation/git-log.txt | 2 +-
Documentation/git-stash.txt | 4 ++--
builtin-log.c | 2 +-
git-stash.sh | 2 +-
4 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 3d79de1..985ffab 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -8,7 +8,7 @@ git-log - Show commit logs
SYNOPSIS
--------
-'git log' [<options>] [<since>..<until>] [[\--] <path>...]
+'git log' [options] [<since>..<until>] [[\--] <path>...]
DESCRIPTION
-----------
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 3f14b72..8a6d85c 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -8,7 +8,7 @@ git-stash - Stash the changes in a dirty working directory away
SYNOPSIS
--------
[verse]
-'git stash' list [<options>]
+'git stash' list [options]
'git stash' show [<stash>]
'git stash' drop [-q|--quiet] [<stash>]
'git stash' ( pop | apply ) [--index] [-q|--quiet] [<stash>]
@@ -64,7 +64,7 @@ from your worktree.
The `--patch` option implies `--keep-index`. You can use
`--no-keep-index` to override this.
-list [<options>]::
+list [options]::
List the stashes that you currently have. Each 'stash' is listed
with its name (e.g. `stash@\{0}` is the latest stash, `stash@\{1}` is
diff --git a/builtin-log.c b/builtin-log.c
index 25e21ed..1685546 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -28,7 +28,7 @@ static const char *fmt_patch_subject_prefix = "PATCH";
static const char *fmt_pretty;
static const char * const builtin_log_usage =
- "git log [<options>] [<since>..<until>] [[--] <path>...]\n"
+ "git log [options] [<since>..<until>] [[--] <path>...]\n"
" or: git show [options] <object>...";
static void cmd_log_init(int argc, const char **argv, const char *prefix,
diff --git a/git-stash.sh b/git-stash.sh
index 4febbbf..e5ab34a 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -2,7 +2,7 @@
# Copyright (c) 2007, Nanako Shiraishi
dashless=$(basename "$0" | sed -e 's/-/ /')
-USAGE="list [<options>]
+USAGE="list [options]
or: $dashless show [<stash>]
or: $dashless drop [-q|--quiet] [<stash>]
or: $dashless ( pop | apply ) [--index] [-q|--quiet] [<stash>]
--
1.6.5.rc1.37.gf5c31.dirty
^ permalink raw reply related
* Re: install does not obey DESTDIR or --prefix for perl modules
From: Brandon Casey @ 2009-09-21 18:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: c, git
In-Reply-To: <7vskeguqmb.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Brandon Casey <brandon.casey.ctr@nrlssc.navy.mil> writes:
>
>> Craig Taylor wrote:
>>> Hi all
>>>
>>> I'm compiling/installing git in a Solaris environment without root.
>>> Using 'make DESTDIR=<some path> install' to stage an install to an
>>> alternate location.
>>> The perl module component of 'make DESTDIR=<some path> install' installs
>>> into the system perl lib path without prepending the forced install
>>> destination or my '--prefix'.
>>> This seems counter intuitive and I would consider a bug.
>> Try 'make prefix=<some path>'.
>>
>> btw, this is in the first paragraph of the INSTALL document.
>
> But is that what Craig is trying to do?
>
> I think he wants to build git to be installed in /usr/bin/git or whatever,
> and he would say "prefix=/usr". He however wants "make install" to write
> into /var/tmp/g/usr/bin/git, not /usr/bin/git, so that he can for example
> make a tarball with "cd /var/tmp/g && tar cf ../git.tar .", and extract it
> as root under the real '/'. "make DESTDIR=/var/tmp/g" is exactly for
> that, and if it is not working I would say it is a bug.
Ah. I did not realize that that is what DESTDIR is for.
Thanks for the correction.
-brandon
^ permalink raw reply
* Re: install does not obey DESTDIR or --prefix for perl modules
From: Craig Taylor @ 2009-09-21 19:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Casey, git
In-Reply-To: <7vskeguqmb.fsf@alter.siamese.dyndns.org>
On Mon, Sep 21, 2009 at 10:58:20AM -0700, Junio C Hamano wrote:
> Brandon Casey <brandon.casey.ctr@nrlssc.navy.mil> writes:
>
> > Craig Taylor wrote:
> >> Hi all
> >>
> >> I'm compiling/installing git in a Solaris environment without root.
> >> Using 'make DESTDIR=<some path> install' to stage an install to an
> >> alternate location.
> >> The perl module component of 'make DESTDIR=<some path> install' installs
> >> into the system perl lib path without prepending the forced install
> >> destination or my '--prefix'.
> >> This seems counter intuitive and I would consider a bug.
> >
> > Try 'make prefix=<some path>'.
> >
> > btw, this is in the first paragraph of the INSTALL document.
>
> But is that what Craig is trying to do?
>
> I think he wants to build git to be installed in /usr/bin/git or whatever,
> and he would say "prefix=/usr". He however wants "make install" to write
> into /var/tmp/g/usr/bin/git, not /usr/bin/git, so that he can for example
> make a tarball with "cd /var/tmp/g && tar cf ../git.tar .", and extract it
> as root under the real '/'. "make DESTDIR=/var/tmp/g" is exactly for
> that, and if it is not working I would say it is a bug.
Exactly as you describe here, different paths but same goal.
All binaries follow the DESTDIR path except the perl modules.
To register this as a bug do I need to do more than send this email?
Thanks
CraigT
--
c^ [c%5e]
No think. No Talk. Train.
^ permalink raw reply
* Re: [PATCH JGit 1/5] adding tests for ObjectDirectory
From: Shawn O. Pearce @ 2009-09-21 19:30 UTC (permalink / raw)
To: mr.gaffo; +Cc: git, mike.gaffney
In-Reply-To: <1253062116-13830-2-git-send-email-mr.gaffo@gmail.com>
mr.gaffo@gmail.com wrote:
> diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/ObjectDirectoryTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/ObjectDirectoryTest.java
> + private File testDir;
> +
> + @Override
> + protected void setUp() throws Exception {
> + testDir = new File(new File(System.getProperty("java.io.tmpdir")), UUID.randomUUID().toString());
> + }
Can't we use the same logic we use in RepositoryTestCase to create
the temporary directory for this test? I would rather keep the
temporary space under target/ when testing under Maven, as it
makes it far easier to clean up the directory. Plus we know we
have sufficient write space there.
> + @Override
> + protected void tearDown() throws Exception {
> + if (testDir.exists()){
Style nit: Space between ) and {
> + public void testExistsWithNonExistantDirectory() throws Exception {
> + assertFalse(new ObjectDirectory(new File("/some/nonexistant/file")).exists());
Please create a path name below your testDir which you know won't
exist. I don't want this test to rely upon the fact that some
absolute path doesn't exist that is outside of our namespace control.
> + private void createTestDir(){
You use this method once, inline it inside
testExistsWithExistingDirectory().
Otherwise, the test case is OK, but is still quite sparse with
regards to functionality of the class being tested. Was it your
intention to only cover the most basic parts at this time? Its more
coverage than we have now, so I'm happy, but just wanted to point
out it certainly isn't complete (e.g. no pack support).
--
Shawn.
^ permalink raw reply
* Re: [PATCH JGit 2/5] Create abstract method on ObjectDatabase for accessing the list of local pack files.
From: Shawn O. Pearce @ 2009-09-21 19:40 UTC (permalink / raw)
To: mr.gaffo; +Cc: git, mike.gaffney
In-Reply-To: <1253062116-13830-3-git-send-email-mr.gaffo@gmail.com>
mr.gaffo@gmail.com wrote:
> diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/ObjectDirectoryTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/ObjectDirectoryTest.java
> + public void testListLocalPacksWhenThereIsAPack() throws Exception {
> + createTestDir();
> + File packsDir = new File(testDir, "pack");
> + packsDir.mkdirs();
Why not allow the ObjectDirectory code to create the directory
before copying the pack into it?
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDatabase.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDatabase.java
> + /**
> + * The list of Packs THIS repo contains
Don't you mean the list of packs this object database contains?
An object database may not be a git repository. Though yes, the
common case is that it is a repository.
> + * @return List<PackFile> of package names contained in this repo.
> + * Should be an empty list if there are none.
> + */
> + public abstract List<PackFile> listLocalPacks();
I think you should define this to be an unmodifiable list, not just
any list. Its sad that the Java type system didn't support this
idea back when they added the new collections APIs.
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java
> + @Override
> + public List<PackFile> listLocalPacks() {
> + tryAgain1();
> + return new ArrayList<PackFile>(Arrays.asList(packList.get().packs));
Instead of copying, why not return an unmodifiableList wrapped
around the array? PackList will never modify its internal array.
--
Shawn.
^ permalink raw reply
* Re: install does not obey DESTDIR or --prefix for perl modules
From: Junio C Hamano @ 2009-09-21 19:44 UTC (permalink / raw)
To: c; +Cc: Brandon Casey, git
In-Reply-To: <20090921191943.GE8173@gryning.com>
Craig Taylor <c@gryning.com> writes:
> Exactly as you describe here, different paths but same goal.
> All binaries follow the DESTDIR path except the perl modules.
>
> To register this as a bug do I need to do more than send this email?
Do you use NO_PERL_MAKEMAKER in your build?
If not, we need to summon an expert on ExtUtils::MakeMaker to look into
this issue, but if you do, perhaps you can try this patch and report how
well it works for you.
---
perl/Makefile | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/perl/Makefile b/perl/Makefile
index e3dd1a5..4ab21d6 100644
--- a/perl/Makefile
+++ b/perl/Makefile
@@ -29,11 +29,11 @@ $(makfile): ../GIT-CFLAGS Makefile
'$(PERL_PATH_SQ)' -MError -e 'exit($$Error::VERSION < 0.15009)' || \
echo ' cp private-Error.pm blib/lib/Error.pm' >> $@
echo install: >> $@
- echo ' mkdir -p "$(instdir_SQ)"' >> $@
- echo ' $(RM) "$(instdir_SQ)/Git.pm"; cp Git.pm "$(instdir_SQ)"' >> $@
- echo ' $(RM) "$(instdir_SQ)/Error.pm"' >> $@
+ echo ' mkdir -p "$$(DESTDIR)$(instdir_SQ)"' >> $@
+ echo ' $(RM) "$$(DESTDIR)$(instdir_SQ)/Git.pm"; cp Git.pm "$$(DESTDIR)$(instdir_SQ)"' >> $@
+ echo ' $(RM) "$$(DESTDIR)$(instdir_SQ)/Error.pm"' >> $@
'$(PERL_PATH_SQ)' -MError -e 'exit($$Error::VERSION < 0.15009)' || \
- echo ' cp private-Error.pm "$(instdir_SQ)/Error.pm"' >> $@
+ echo ' cp private-Error.pm "$$(DESTDIR)$(instdir_SQ)/Error.pm"' >> $@
echo instlibdir: >> $@
echo ' echo $(instdir_SQ)' >> $@
else
^ permalink raw reply related
* Re: install does not obey DESTDIR or --prefix for perl modules
From: Craig Taylor @ 2009-09-21 19:46 UTC (permalink / raw)
To: Ben Walton; +Cc: Junio C Hamano, Brandon Casey, git
In-Reply-To: <1253561929-sup-8931@ntdws12.chass.utoronto.ca>
On Mon, Sep 21, 2009 at 03:40:04PM -0400, Ben Walton wrote:
> Excerpts from Craig Taylor's message of Mon Sep 21 15:19:43 -0400 2009:
>
> > Exactly as you describe here, different paths but same goal.
> > All binaries follow the DESTDIR path except the perl modules.
>
> I haven't had this problem and I've been packaging git for solaris (8
> or newer) since early 2009. If you're interested in my build notes,
> feel free to ping me.
>
> Thanks
> -Ben
Will take you up on this. I'll re-raise this if they don't help.
Thanks
CraigT
--
c^ [c%5e]
Even when you are standing in front of defeat, do it smiling.
^ permalink raw reply
* Re: [PATCH JGit 2/5] Create abstract method on ObjectDatabase for accessing the list of local pack files.
From: Michael Gaffney @ 2009-09-21 19:51 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: git
In-Reply-To: <20090921194035.GM14660@spearce.org>
Shawn O. Pearce wrote:
> Why not allow the ObjectDirectory code to create the directory
> before copying the pack into it?
Good point, this was one of the first tests I did before I got to that
part of ObjectDirectory. Will fix.
> Don't you mean the list of packs this object database contains?
> An object database may not be a git repository. Though yes, the
> common case is that it is a repository.
What's the difference in terminology? Not aruging, just wanting to know
what we're calling a repo and what we're not so that I use it correctly.
Will fix.
>> + public abstract List<PackFile> listLocalPacks();
>
> I think you should define this to be an unmodifiable list, not just
> any list. Its sad that the Java type system didn't support this
> idea back when they added the new collections APIs.
Should it be a collection as well instead of a list; what would you
specifically suggest?
> Instead of copying, why not return an unmodifiableList wrapped
> around the array? PackList will never modify its internal array.
Same as above
-Mike
^ permalink raw reply
* Re: install does not obey DESTDIR or --prefix for perl modules
From: Craig Taylor @ 2009-09-21 19:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Casey, git
In-Reply-To: <7vocp4ulq2.fsf@alter.siamese.dyndns.org>
On Mon, Sep 21, 2009 at 12:44:05PM -0700, Junio C Hamano wrote:
> Craig Taylor <c@gryning.com> writes:
>
> > Exactly as you describe here, different paths but same goal.
> > All binaries follow the DESTDIR path except the perl modules.
> >
> > To register this as a bug do I need to do more than send this email?
>
> Do you use NO_PERL_MAKEMAKER in your build?
>
> If not, we need to summon an expert on ExtUtils::MakeMaker to look into
> this issue, but if you do, perhaps you can try this patch and report how
> well it works for you.
>
> ---
> perl/Makefile | 8 ++++----
> 1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/perl/Makefile b/perl/Makefile
> index e3dd1a5..4ab21d6 100644
> --- a/perl/Makefile
> +++ b/perl/Makefile
> @@ -29,11 +29,11 @@ $(makfile): ../GIT-CFLAGS Makefile
> '$(PERL_PATH_SQ)' -MError -e 'exit($$Error::VERSION < 0.15009)' || \
> echo ' cp private-Error.pm blib/lib/Error.pm' >> $@
> echo install: >> $@
> - echo ' mkdir -p "$(instdir_SQ)"' >> $@
> - echo ' $(RM) "$(instdir_SQ)/Git.pm"; cp Git.pm "$(instdir_SQ)"' >> $@
> - echo ' $(RM) "$(instdir_SQ)/Error.pm"' >> $@
> + echo ' mkdir -p "$$(DESTDIR)$(instdir_SQ)"' >> $@
> + echo ' $(RM) "$$(DESTDIR)$(instdir_SQ)/Git.pm"; cp Git.pm "$$(DESTDIR)$(instdir_SQ)"' >> $@
> + echo ' $(RM) "$$(DESTDIR)$(instdir_SQ)/Error.pm"' >> $@
> '$(PERL_PATH_SQ)' -MError -e 'exit($$Error::VERSION < 0.15009)' || \
> - echo ' cp private-Error.pm "$(instdir_SQ)/Error.pm"' >> $@
> + echo ' cp private-Error.pm "$$(DESTDIR)$(instdir_SQ)/Error.pm"' >> $@
> echo instlibdir: >> $@
> echo ' echo $(instdir_SQ)' >> $@
> else
I have 2 perl environments available to me on the system (at home now so
can't dig further).
Both can pretty stripped which can make thing "challenging" it's
possible ExtUtils::MakeMaker doesn't even exist.
Please everyone who is looking at this thread let it sleep for 24h and I
shall test further/revive it tomorrow.
Many thanks
CraigT
--
c^ [c%5e]
Learn first whatever hardship you may have to endure is but temporary
^ permalink raw reply
* Re: [PATCH] Trivial fix: Make all the builtin usage strings to use the same pattern.
From: Thiago Farina @ 2009-09-21 20:05 UTC (permalink / raw)
To: Brandon Casey; +Cc: git
In-Reply-To: <pN7ragma8Ra75P8lN7iHuXE6FyUXHXFlnHUVFZqJV4WM7w5mCkVHcQ@cipher.nrlssc.navy.mil>
On Mon, Sep 21, 2009 at 12:28 PM, Brandon Casey
<brandon.casey.ctr@nrlssc.navy.mil> wrote:
> Thiago Farina wrote:
>> They follow the pattern of file name:
>> File name pattern: builtin-command-name.c
>> Usage string pattern: builtin_command_name_usage
>>
>> Signed-off-by: Thiago Farina <tfransosi@gmail.com>
>> ---
>
>> diff --git a/builtin-blame.c b/builtin-blame.c
>> index 7512773..4adae84 100644
>> --- a/builtin-blame.c
>> +++ b/builtin-blame.c
>> @@ -21,10 +21,11 @@
>> #include "parse-options.h"
>> #include "utf8.h"
>>
>> -static char blame_usage[] = "git blame [options] [rev-opts] [rev] [--] file";
>> +static char builtin_blame_usage[] =
>
> Should this one also use 'const'?
It can use const here. I will do this change in the next patch.
>
>> +"git blame [options] [rev-opts] [rev] [--] file";
>>
>> static const char *blame_opt_usage[] = {
>> - blame_usage,
>> + builtin_blame_usage,
>> "",
>> "[rev-opts] are documented in git-rev-list(1)",
>> NULL
>
>> diff --git a/builtin-count-objects.c b/builtin-count-objects.c
>> index 1b0b6c8..fbe0972 100644
>> --- a/builtin-count-objects.c
>> +++ b/builtin-count-objects.c
>> @@ -65,7 +65,7 @@ static void count_objects(DIR *d, char *path, int len, int verbose,
>> }
>> }
>>
>> -static char const * const count_objects_usage[] = {
>> +static char const * const builtin_count_objects_usage[] = {
>
> This one is different from the others. I assume 'char const *' is
> the same as 'const char *', though I'm used to seeing the latter
> form. If we're going for consistency, maybe this should be changed
> too. Ultra minor nit-pick. I noticed it, so I mentioned it, but
> its your choice.
To maintain the consistency I will follow the latter form, since all
others are using "const char *".
Thanks for catching the ones that I missed. I will update them in the
next patch.
^ permalink raw reply
* Re: install does not obey DESTDIR or --prefix for perl modules
From: Ben Walton @ 2009-09-21 19:40 UTC (permalink / raw)
To: c; +Cc: Junio C Hamano, Brandon Casey, git
In-Reply-To: <20090921191943.GE8173@gryning.com>
[-- Attachment #1: Type: text/plain, Size: 572 bytes --]
Excerpts from Craig Taylor's message of Mon Sep 21 15:19:43 -0400 2009:
> Exactly as you describe here, different paths but same goal.
> All binaries follow the DESTDIR path except the perl modules.
I haven't had this problem and I've been packaging git for solaris (8
or newer) since early 2009. If you're interested in my build notes,
feel free to ping me.
Thanks
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302
GPG Key Id: 8E89F6D2; Key Server: pgp.mit.edu
Contact me to arrange for a CAcert assurance meeting.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: [PATCH] Trivial fix: Display a more friendly message with git-shell.
From: Thiago Farina @ 2009-09-21 20:45 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Johannes Sixt, git
In-Reply-To: <vpqeiq0ssn7.fsf@bauges.imag.fr>
Hi Matthieu
On Mon, Sep 21, 2009 at 3:45 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Perhaps one more line saying what 'man git-shell' says in addition
> would help:
>
> $ git-shell
> git-shell: Restricted login shell for GIT-only SSH access
> Usage: ...
I added this line to the output usage, but would be good to show the
commands that git-shell accept too?
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox