* Re: What's cooking in git.git (Oct 2009, #01; Wed, 07)
From: Sverre Rabbelier @ 2009-10-08 17:58 UTC (permalink / raw)
To: Shawn O. Pearce, vcs-fast-import-devs; +Cc: Johannes Schindelin, git
In-Reply-To: <20091008173900.GI9261@spearce.org>
Heya,
[edited Shawn's message somewhat to be more relevant to vcs-fast-import-dev]
On Thu, Oct 8, 2009 at 19:39, Shawn O. Pearce <spearce@spearce.org> wrote:
> IIRC my problem with options was we weren't enforcing them, and yet
> they were necessary for a successful import, e.g. import-marks or
> export-marks. A minor error could cause a successful looking import
> that is wrong due to the marks being messed up, or not saved out.
>
> So I was leaning towards making these features, but then they
> aren't necessarily compatible with the other fast-import tools.
>
> I think we want to declare features for import-marks and export-marks:
>
> feature import-marks=in.marks
> feature export-marks=out.marks
>
> and define these as paths to local files which store a VCS specific
> formatted mapping of fast-import mark numbers to VCS labels.
>
>
> Other options that are clearly git should be declared as:
>
> option git max-pack-size=2048
>
> with the meaning of option being declared something like:
>
> If the parsing VCS name appears as the first argument, the parsing
> VCS must recognize and support the supplied option, and if not
> recognized or not supported must abort parsing altogether.
>
> If the parsing VCS name is not the first argument, it must entirely
> ignore the option command and not try to process its contents.
I think it makes to ignore options that are not for our vcs, as long
as options that change import behavior (such as marks, date-format)
are combined with, say, 'feature tool=git'. This way we can be sure
that when outputting out a vcs specific stream, it is only parsed by
that vcs.
Note: yes, I know that marks and date-format are features now, but
there's really no other suitable example that I could think of).
vcs fast import devs please ack this idea (and perhaps suggest
something other than "feature tool=git" if preferable) so that I can
reroll my gfi-options series :).
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2009, #01; Wed, 07)
From: Shawn O. Pearce @ 2009-10-08 17:39 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: Johannes Schindelin, git
In-Reply-To: <fabb9a1e0910072349q68d6756cgebb041a0bbe2ba65@mail.gmail.com>
Sverre Rabbelier <srabbelier@gmail.com> wrote:
> On Thu, Oct 8, 2009 at 08:49, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > Shawn, last time I heard of this issue, it was stuck in your review queue.
>
> Correct, am waiting for Shawn's decision on whether to drop options
> and replace them with additional features or not.
Uh. Wow, it has been a while.
IIRC my problem with options was we weren't enforcing them, and yet
they were necessary for a successful import, e.g. import-marks or
export-marks. A minor error could cause a successful looking import
that is wrong due to the marks being messed up, or not saved out.
So I was leaning towards making these features, but then they
aren't necessarily compatible with the other fast-import tools.
Which led me to a stalemate, and I forgot about the thread.
Dammit.
We should run this past the fast-import list but I think we want
to declare features for import-marks and export-marks:
feature import-marks=in.marks
feature export-marks=out.marks
and define these as paths to local files which store a VCS specific
formatted mapping of fast-import mark numbers to VCS labels.
Other options that are clearly git should be declared as:
option git max-pack-size=2048
with the meaning of option being declared something like:
If the parsing VCS name appears as the first argument, the parsing
VCS must recognize and support the supplied option, and if not
recognized or not supported must abort parsing altogether.
If the parsing VCS name is not the first argument, it must entirely
ignore the option command and not try to process its contents.
--
Shawn.
^ permalink raw reply
* Re: [PATCH JGit 5/5] added tests for the file based info cache update and made pass
From: Shawn O. Pearce @ 2009-10-08 17:12 UTC (permalink / raw)
To: mr.gaffo; +Cc: git, mike.gaffney
In-Reply-To: <1253062116-13830-6-git-send-email-mr.gaffo@gmail.com>
mr.gaffo@gmail.com wrote:
> From: mike.gaffney <mike.gaffney@asolutions.com>
> Subject: Re: [PATCH JGit 5/5] added tests for the file based info cache
> update and made pass
"and made pass" is the sneaky way of saying "and I actually
implemented what I should have implemented in the prior commit,
but didn't because ..." ?
> diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/CachedPacksInfoFileContentsGeneratorTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/CachedPacksInfoFileContentsGeneratorTest.java
> index bea0b70..10ce9e3 100644
> --- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/CachedPacksInfoFileContentsGeneratorTest.java
> +++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/CachedPacksInfoFileContentsGeneratorTest.java
> @@ -63,10 +63,10 @@ public void testGettingPacksContentsMultiplePacks() throws Exception {
> packs.add(new PackFile(TEST_IDX, TEST_PACK));
>
> StringBuilder expected = new StringBuilder();
> - expected.append("P ").append(TEST_PACK.getName()).append("\n");
> - expected.append("P ").append(TEST_PACK.getName()).append("\n");
> - expected.append("P ").append(TEST_PACK.getName()).append("\n");
> - expected.append("\n");
> + expected.append("P ").append(TEST_PACK.getName()).append('\n');
> + expected.append("P ").append(TEST_PACK.getName()).append('\n');
> + expected.append("P ").append(TEST_PACK.getName()).append('\n');
> + expected.append('\n');
This should be squashed to the patch that introduced the code,
not be twiddled in something that is completely unrelated to it.
> diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/InfoDirectoryDatabaseTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/InfoDirectoryDatabaseTest.java
> + public void testUpdateInfoCache() throws Exception {
> + Collection<Ref> refs = new ArrayList<Ref>();
> + refs.add(new Ref(Ref.Storage.LOOSE, "refs/heads/master", ObjectId.fromString("32aae7aef7a412d62192f710f2130302997ec883")));
> + refs.add(new Ref(Ref.Storage.LOOSE, "refs/heads/development", ObjectId.fromString("184063c9b594f8968d61a686b2f6052779551613")));
> +
> + File expectedFile = new File(testDir, "refs");
> + assertFalse(expectedFile.exists());
> +
> +
> + final StringWriter expectedString = new StringWriter();
> + new RefWriter(refs) {
> + @Override
> + protected void writeFile(String file, byte[] content) throws IOException {
> + expectedString.write(new String(content));
> + }
> + }.writeInfoRefs();
This feels a bit too much like testing the formatting code by
relying on the formatting code to produce the correct output.
Its a 2 line file with a very well known format that cannot change
without breaking every Git HTTP client out in the wild. We will
not break those clients anytime in the next few years. You have
the data hardcoded above *anyway*, hardcode the expected result
here to ensure we formatted it right.
Oh, and IIRC order doesn't matter in the file but I think almost
everyone assumes the order is as per git ls-remote, which matches the
order produced by RefComparator. Which means you want to assert that
development comes before master, and that tags come before heads.
Also, we need to assert that the peeled information for a tag appears
in the file. So you need a tag ref with a peeled ObjectId available.
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/InfoDirectoryDatabase.java b/org.spearce.jgit/src/org/spearce/jgit/lib/InfoDirectoryDatabase.java
> @@ -51,4 +54,16 @@ public void create() {
> info.mkdirs();
> }
>
> + @Override
> + public void updateInfoCache(Collection<Ref> refs) throws IOException {
> + new RefWriter(refs) {
> + @Override
> + protected void writeFile(String file, byte[] content) throws IOException {
> + FileOutputStream fos = new FileOutputStream(new File(info, "refs"));
> + fos.write(content);
> + fos.close();
I think you need to use a LockFile to avoid races between readers
and writers.
--
Shawn.
^ permalink raw reply
* [PATCH] Fix MSVC build on cygwin
From: Ramsay Jones @ 2009-10-08 15:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: mstormo, GIT Mailing-list
In the MSVC section of the Makefile, BASIC_CFLAGS is set to a
value which contains the string "-DWIN32-D_CONSOLE". This results
in a (single) malformed -Define being passed to the compiler.
At least on my cygwin installation, the msvc compiler seems to
ignore this parameter, without issuing an error or warning, and
results in the WIN32 and _CONSOLE macros being undefined. This
breaks the build.
In order to fix the build, we simply insert a space between the
two -Define parameters, "-DWIN32" and "-D_CONSOLE", as originally
intended.
Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
Hi *,
The original version of this patch used line-continuation to
wrap the over-long lines in the MSVC section. (the lines that
set up BASIC_CFLAGS, COMPAT_CFLAGS and BASIC_LDFLAGS.)
However, that somewhat obscured the important change in this
patch (and some people don't find line-continuation easier to
read anyway ;-).
ATB,
Ramsay Jones
Makefile | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/Makefile b/Makefile
index 12defd4..ae9bb03 100644
--- a/Makefile
+++ b/Makefile
@@ -914,7 +914,7 @@ ifdef MSVC
CC = compat/vcbuild/scripts/clink.pl
AR = compat/vcbuild/scripts/lib.pl
CFLAGS =
- BASIC_CFLAGS = -nologo -I. -I../zlib -Icompat/vcbuild -Icompat/vcbuild/include -DWIN32-D_CONSOLE -DHAVE_STRING_H -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE
+ BASIC_CFLAGS = -nologo -I. -I../zlib -Icompat/vcbuild -Icompat/vcbuild/include -DWIN32 -D_CONSOLE -DHAVE_STRING_H -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE
COMPAT_OBJS = compat/msvc.o compat/fnmatch/fnmatch.o compat/winansi.o
COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch -Icompat/regex -Icompat/fnmatch -DSTRIP_EXTENSION=\".exe\"
BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib
--
1.6.4
^ permalink raw reply related
* [PATCH] Fix the exit code of MSVC build scripts on cygwin
From: Ramsay Jones @ 2009-10-08 15:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: mstormo, GIT Mailing-list
During an MSVC build on cygwin, the make program did not notice
when the compiler or linker exited with an error. This was caused
by the scripts exiting with the value returned by system() directly.
On POSIX-like systems, such as cygwin, the return value of system()
has the exit code of the executed command encoded in the first byte
(ie the value is shifted up by 8 bits). This allows the bottom
7 bits to contain the signal number of a terminated process, while
the eighth bit indicates whether a core-dump was produced. (A value
of -1 indicates that the command failed to execute.)
The make program, however, expects the exit code to be encoded in the
bottom byte. Futhermore, it apparently masks off and ignores anything
in the upper bytes.
However, these scripts are (naturally) intended to be used on the
windows platform, where we can not assume POSIX-like semantics from
a perl implementation (eg ActiveState). So, in general, we can not
assume that shifting the return value right by eight will get us
the exit code.
In order to improve portability, we assume that a zero return from
system() indicates success, whereas anything else indicates failure.
Since we don't need to know the exact exit code from the compiler
or linker, we simply exit with 0 (success) or 1 (failure).
Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
Hi *,
I have tried to be conservative with this patch and, although I can
confirm that it works great on cygwin, it should probably be tested
(and Acked) by someone with an MSYS/Mingw installation.
(or whatever Marius has installed :)
ATB,
Ramsay Jones
compat/vcbuild/scripts/clink.pl | 4 +++-
compat/vcbuild/scripts/lib.pl | 4 +++-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/compat/vcbuild/scripts/clink.pl b/compat/vcbuild/scripts/clink.pl
index 0ffd59f..26aec61 100644
--- a/compat/vcbuild/scripts/clink.pl
+++ b/compat/vcbuild/scripts/clink.pl
@@ -45,4 +45,6 @@ if ($is_linking) {
push(@args, @cflags);
}
#printf("**** @args\n");
-exit system(@args);
+system(@args) == 0
+ or exit 1;
+exit 0;
diff --git a/compat/vcbuild/scripts/lib.pl b/compat/vcbuild/scripts/lib.pl
index 68f6644..c11016d 100644
--- a/compat/vcbuild/scripts/lib.pl
+++ b/compat/vcbuild/scripts/lib.pl
@@ -23,4 +23,6 @@ while (@ARGV) {
}
unshift(@args, "lib.exe");
# printf("**** @args\n");
-exit system(@args);
+system(@args) == 0
+ or exit 1;
+exit 0;
--
1.6.4
^ permalink raw reply related
* Re: [PATCH JGit 4/5] Adding in a InfoDatabase like ObjectDatabase and and implementation based upon a directory.
From: Shawn O. Pearce @ 2009-10-08 17:05 UTC (permalink / raw)
To: mr.gaffo; +Cc: git
In-Reply-To: <1253062116-13830-5-git-send-email-mr.gaffo@gmail.com>
mr.gaffo@gmail.com wrote:
> From: Mike Gaffney <mr.gaffo@gmail.com>
> Subject: Re: [PATCH JGit 4/5] Adding in a InfoDatabase like ObjectDatabase
> and and implementation based upon a directory.
Typo on "and and".
We should have a bit more justification for this change, the subject
sounds aggressive, but there's no rationle for 175 insertions.
You and I both can make a reaosnable guess about why, but not
everyone knows the code or what you are trying to accomplish.
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/InfoDatabase.java b/org.spearce.jgit/src/org/spearce/jgit/lib/InfoDatabase.java
> +public abstract class InfoDatabase {
> +
> + public void create() {
> + }
New public code should have Javadoc to document its purpose and
usage, especially for an abstract class that needs to be implemented.
But, that said, I think this direction is of dubious value. What we
really care about is having the contents of the current RefDatabase
(that is, packed-refs and files under refs/) written into info/refs.
There really isn't anything else of value under GIT_DIR/info, other
than GIT_DIR/info/exclude, but that is related to ignore processing
for a repository with a working directory and isn't something that
a bare repository on a server ever cares about.
IMHO, updating GIT_DIR/info/refs should be part of RefDatabase,
not some new InfoDirectoryDatabase class.
--
Shawn.
^ permalink raw reply
* [PATCH v2] git-send-email.perl: fold multiple entry "Cc:" and multiple single line "RCPT TO:"s
From: Joe Perches @ 2009-10-08 17:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jay Soffian
In-Reply-To: <7vy6nlhmw7.fsf@alter.siamese.dyndns.org>
Some MTAs reject Cc: lines longer than 78 chars.
Avoid this by using the same join as "To:" ",\n\t"
so each subsequent Cc entry is on a new line.
RCPT TO: should have a single entry per line.
see: http://www.ietf.org/rfc/rfc2821.txt
Signed-off-by: Joe Perches <joe@perches.com>
diff --git a/git-send-email.perl b/git-send-email.perl
index dd821f7..ce81425 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -835,7 +835,7 @@ sub send_message
$gitversion = Git::version();
}
- my $cc = join(", ", unique_email_list(@cc));
+ my $cc = join(",\n\t", unique_email_list(@cc));
my $ccline = "";
if ($cc ne '') {
$ccline = "\nCc: $cc";
@@ -976,7 +976,9 @@ X-Mailer: git-send-email $gitversion
if ($smtp_server !~ m#^/#) {
print "Server: $smtp_server\n";
print "MAIL FROM:<$raw_from>\n";
- print "RCPT TO:".join(',',(map { "<$_>" } @recipients))."\n";
+ foreach my $entry (@recipients) {
+ print "RCPT TO:<$entry>\n";
+ }
} else {
print "Sendmail: $smtp_server ".join(' ',@sendmail_parameters)."\n";
}
^ permalink raw reply related
* Re: [PATCH JGit 3/5] Implemented directory based info cache for objects/info/packs.
From: Shawn O. Pearce @ 2009-10-08 17:00 UTC (permalink / raw)
To: mr.gaffo; +Cc: git
In-Reply-To: <1253062116-13830-4-git-send-email-mr.gaffo@gmail.com>
mr.gaffo@gmail.com wrote:
> Add abstract method for updating the object db's info cache to directory.
>
> Implemented passthrough on Alternate for the update of infocache.
>
> Added utility that generates the contents of the objects/info/packs
> file as a string from a list of PackFiles.
>
> Added implementation from ObjectDirectory on down
> for creating the info cache.
>
> Added test for creating the info cache
Reading this message gave me the funny feeling that I'm going to
see a lot of unrelated code mashed into one patch. There doesn't
seem to be a general theme to the commit, at least as far as the
message is concerned.
[a bit later...] After reading the code itself, I agree with the
original guess on the commit message, there is too much happening
in this one patch that is unrelated, and there are several problems
lurking that are harder to spot because its a mash of changes.
Please try to break it down to more focused commits.
> diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/CachedPacksInfoFileContentsGeneratorTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/CachedPacksInfoFileContentsGeneratorTest.java
> new file mode 100644
> index 0000000..bea0b70
> --- /dev/null
> +++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/CachedPacksInfoFileContentsGeneratorTest.java
> @@ -0,0 +1,74 @@
FYI, our new package space is org.eclipse.jgit and any new changes
need to take that into account.
> + * - Neither the name of the Git Development Community nor the
Also FYI, since our move to the Eclipse Foundation the generic term
"Git Development Community" has been replaced in the license header
by "Eclipse Foundation, Inc.", otherwise the license header remains
as-is and copyright is still attributed to the contributor.
> +public class CachedPacksInfoFileContentsGeneratorTest extends TestCase {
...
> + public void testGettingPacksContentsMultiplePacks() throws Exception {
> + List<PackFile> packs = new ArrayList<PackFile>();
> + packs.add(new PackFile(TEST_IDX, TEST_PACK));
> + packs.add(new PackFile(TEST_IDX, TEST_PACK));
> + packs.add(new PackFile(TEST_IDX, TEST_PACK));
I think we should be testing multiple names here, to ensure the
generator didn't do something stupid like reuse the same array index
for pulling the name while looping for the size of the input list.
> 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
> @@ -54,9 +54,9 @@
>
> @Override
> protected void setUp() throws Exception {
> - testDir = new File(new File(System.getProperty("java.io.tmpdir")), UUID.randomUUID().toString());
> + testDir = JGitTestUtil.generateTempDirectoryFileObject();
Yea, I'm confused about this hunk. It isn't strictly necessary
for the new feature this commit is adding. Pull this into its own
commit, before the new feature, so you can take advantage of the
refactoring in your new tests, but you also don't muddle the new
feature addition with old code refactoring.
> diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/util/JGitTestUtil.java b/org.spearce.jgit.test/tst/org/spearce/jgit/util/JGitTestUtil.java
> @@ -74,7 +76,8 @@ public static File getTestResourceFile(final String fileName) {
> }
> }
>
> - public static void copyFile(final File fromFile, final File toFile) throws IOException {
> + public static void copyFile(final File fromFile, final File toFile)
> + throws IOException {
> InputStream in = new FileInputStream(fromFile);
> OutputStream out = new FileOutputStream(toFile);
Unnecessary reformatting hunk; we try to avoid these when possible.
> @@ -87,6 +90,21 @@ public static void copyFile(final File fromFile, final File toFile) throws IOExc
> out.close();
> }
>
> + public static String readFileAsString(final File file)
> + throws java.io.IOException {
This method already exists in some form in the RepositoryTest class.
Can we instead make a commit to refactor it out of there here?
> + StringBuilder fileData = new StringBuilder(1000);
> + BufferedReader reader = new BufferedReader(new FileReader(file));
We should be more specific about our encoding and not rely on the
platform default.
> + char[] buf = new char[1024];
> + int numRead = 0;
> + while ((numRead = reader.read(buf)) != -1) {
> + String readData = String.valueOf(buf, 0, numRead);
> + fileData.append(readData);
> + buf = new char[1024];
There is no need to reallocate the buffer, String.valueOf is required
to copy the array to ensure the returned String is immutable.
Worse, you don't need to convert the char array to string, there is
a method on StringBuilder to append a char[] taking char[],int,int.
> @@ -136,4 +154,8 @@ private static void reportDeleteFailure(final String name,
> +
> + public static File generateTempDirectoryFileObject() {
> + return new File(new File(System.getProperty("java.io.tmpdir")), UUID.randomUUID().toString());
> + }
Please generate temporary directories under the same area that
RepositoryTest produces them, which makes it easier to cleanup,
and ensures we aren't treading out of our build area.
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/AlternateRepositoryDatabase.java b/org.spearce.jgit/src/org/spearce/jgit/lib/AlternateRepositoryDatabase.java
> index 68ad488..70ce505 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/lib/AlternateRepositoryDatabase.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/AlternateRepositoryDatabase.java
> @@ -130,4 +130,9 @@ protected void closeAlternates(final ObjectDatabase[] alt) {
> public List<PackFile> listLocalPacks() {
> return odb.listLocalPacks();
> }
> +
> + @Override
> + public void updateInfoCache() throws IOException {
> + odb.updateInfoCache();
> + }
> }
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/CachedPacksInfoFileContentsGenerator.java b/org.spearce.jgit/src/org/spearce/jgit/lib/CachedPacksInfoFileContentsGenerator.java
> + * This file is used to generate the contents of the file system
> + * based pack file cache used by the dumb git-http client protocol.
> + * @author mike
We don't record @author annotations.
> +public class CachedPacksInfoFileContentsGenerator {
> +
> + private List<PackFile> packs;
> +
> + public CachedPacksInfoFileContentsGenerator(List<PackFile> packs) {
> + this.packs = packs;
> + }
> +
> + public String generateContents(){
Style nit: space between () and {.
I also wonder about the value of an instance for this class, if
all it does is produce one return value as a string, why not just
use a static method?
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/Constants.java b/org.spearce.jgit/src/org/spearce/jgit/lib/Constants.java
> index 9afea67..2d78dda 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/lib/Constants.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/Constants.java
> @@ -224,6 +224,9 @@
>
> /** Info refs folder */
> public static final String INFO_REFS = "info/refs";
> +
> + /** cached packs file */
> + public static final String CACHED_PACKS_FILE = "info/packs";
I think you should denote in the comment that this is relative to
the objects directory, and not the repository. The INFO_REFS file
right above you is relative to the repository and something looks
wrong seeing these together in context.
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDatabase.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDatabase.java
> index 722c802..5ded7bb 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDatabase.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDatabase.java
> @@ -75,6 +75,14 @@ protected ObjectDatabase() {
> public abstract List<PackFile> listLocalPacks();
>
> /**
> + * Creates the caches that are typically done by
> + * update-server-info, namely objects/info/packs and
> + * info/refs
Why is the object database updating info/refs? The info/refs file
has nothing to do with the raw object storage.
> + * @throws IOException
> + */
> + public abstract void updateInfoCache() throws IOException;
I'm not sure I'm happy with this being on ObjectDatabase but there
may not be any other choice. We'd like to eventually have other
types of ObjectDatabase that don't even store packs, in such a
database updating the info cache makes no sense. Asking them to
implement the operation is silly. But... we have no other way to
easily signal the database that it should do this update.
Hmmph. I wonder if it might be better to configure the
ObjectDirectory at creation time to automatically maintain
the info/packs file during openPack, and put this method on
ObjectDirectory for explicit invocation in case someone has made
edits outside of JGit and wants to force the cache to be current
again.
I'm not sold either way yet. I'm still open on the approach.
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java
> index cbe132d..f4251c1 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java
> @@ -514,4 +514,9 @@ boolean tryAgain(final long currLastModified) {
> tryAgain1();
> return new ArrayList<PackFile>(Arrays.asList(packList.get().packs));
> }
> +
> + @Override
> + public void updateInfoCache() throws IOException {
> + new UpdateDirectoryBasedPacksInfoCache(this.listLocalPacks(), new File(this.getDirectory(), Constants.CACHED_PACKS_FILE)).execute();
Style-nit: Line is far too long, we try to wrap at around 80
characters but sometimes allow a line to go longer if its only
longer by 1 or 2 characters and the wrapped result would be harder
to read than the unwrapped result. This is out at 140, too many
over the limit.
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/UpdateDirectoryBasedPacksInfoCache.java b/org.spearce.jgit/src/org/spearce/jgit/lib/UpdateDirectoryBasedPacksInfoCache.java
> +public class UpdateDirectoryBasedPacksInfoCache {
> + public void execute() throws IOException {
> + String packsContents = new CachedPacksInfoFileContentsGenerator(packsList).generateContents();
> + FileOutputStream fos = new FileOutputStream(infoPacksFile);
> + fos.write(packsContents.getBytes());
> + fos.close();
I'm not sure why this 4 line method requires its own top level
class and being able to create an instance. Code bloat?
You need to specify the character encoding here and not rely on
the platform default.
I'm not sure how C Git handles this update, but we probably should be
safer about it and use the LockFile class so the update is going to
be transactional. Here you are truncating a live file, which means
readers could see an empty content if they happen to request the file
from the web server at the wrong moment. LockFile does the writing
to a temporary file and then renames it over, which on a POSIX file
system means there is no way someone can see a partial update.
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/UpdateDirectoryInfoCache.java b/org.spearce.jgit/src/org/spearce/jgit/lib/UpdateDirectoryInfoCache.java
> +public class UpdateDirectoryInfoCache {
> + public void execute() throws IOException {
> + String packsContents = new CachedPacksInfoFileContentsGenerator(packsList).generateContents();
> + FileOutputStream fos = new FileOutputStream(infoPacksFile);
> + fos.write(packsContents.getBytes());
> + fos.close();
Uh, isn't this the same as UpdateDirectoryBasedPacksInfoCache?
Two top level classes for a 4 line method?
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/ReceivePack.java b/org.spearce.jgit/src/org/spearce/jgit/transport/ReceivePack.java
> + private void updateObjectInfoCache() {
> + try{
Style-nit: space after try
> + getRepository().getObjectDatabase().updateInfoCache();
> + }
> + catch (IOException e){
Style-nit: space between ) and {
> + sendMessage("error updating server info: " + e.getMessage());
--
Shawn.
^ permalink raw reply
* Re: [PATCH v2] perl/Makefile.PL: detect MakeMaker versions incompatible with DESTDIR
From: Brandon Casey @ 2009-10-08 16:58 UTC (permalink / raw)
To: Johannes Sixt; +Cc: gitster, git, c, Brandon Casey
In-Reply-To: <unyNhuV9VB06SYvOR8ONK47yVKPtJfgRVKs-sKMFc-8rKMQBz7DPnw@cipher.nrlssc.navy.mil>
Brandon Casey wrote:
> Johannes Sixt wrote:
>> Brandon Casey schrieb:
>>> diff --git a/perl/Makefile.PL b/perl/Makefile.PL
>>> index 320253e..0b9deca 100644
>>> --- a/perl/Makefile.PL
>>> +++ b/perl/Makefile.PL
>>> @@ -5,6 +5,14 @@ sub MY::postamble {
>>> instlibdir:
>>> @echo '$(INSTALLSITELIB)'
>>>
>>> +ifneq (,$(DESTDIR))
>>> +ifeq (0,$(shell expr '$(MM_VERSION)' '>' 6.10))
>> I don't think the test works as intended, because 6.2 *is* greater than
>> 6.10 (aka 6.1).
>
> Hmm... I think you're right.
I think we're safe. Looks like the MakeMaker folks have always used two
digits for the minor number. So version 6.2 was written like 6.02.
Here's their repo:
git://github.com/schwern/extutils-makemaker.git
git log -p -- lib/ExtUtils/MakeMaker.pm | grep -- '$VERSION = ' | less
I didn't search exhaustively, but I think all of 6.X has two digit minor
numbers, which means all versions should compare correctly since 5.X will
always compare less than 6.X and 7.X will be greater, etc.
-brandon
^ permalink raw reply
* Re: Git archive and trailing "/" in prefix
From: René Scharfe @ 2009-10-08 16:46 UTC (permalink / raw)
To: Sergio Callegari; +Cc: git, Junio C Hamano
In-Reply-To: <loom.20091008T172303-658@post.gmane.org>
Sergio Callegari schrieb:
> Hi!
>
> The git-archive man page indicates that if the --prefix option is passed to
> git-archive, it is compulsory to end the prefix with a "/"
>
> git archive [--format=<fmt>] [--list] [--prefix=<prefix>/] [<extra>] ...
>
> As a matter of fact, the archiver behaves quite strangely if that slash is
> missing. Files in the root of the working dir are added to the archive with
> their own name modified by the prefix and the same happens for working dir
> sub-directories. However, no file present in the sub-directories, nor
> sub-sub-directories are added.
The latter is a bug.
> I would like to know if there some reason why a trailing "/" is not added
> automatically to the prefix when it is missing and the prefix is not empty.
> Would that break anything?
The --prefix option is intended to add a string to the beginning (i.e. "to
prefix") of the name of the archive entries. I'm not sure if there's a use
case for anything else than adding a fake directory for all entries to live
in (thus requiring a trailing slash), but I also don't see why we should
disallow it.
The following patch fixes handling of prefixes without trailing slashes by
taking it out of the hands of get_pathspec() and read_tree_recursive() --
which can only handle prefixes that are path components -- and adding the
prefix later, in write_archive_entry().
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
archive.c | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/archive.c b/archive.c
index 73b8e8a..0cc79d2 100644
--- a/archive.c
+++ b/archive.c
@@ -115,6 +115,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
strbuf_reset(&path);
strbuf_grow(&path, PATH_MAX);
+ strbuf_add(&path, args->base, args->baselen);
strbuf_add(&path, base, baselen);
strbuf_addstr(&path, filename);
path_without_prefix = path.buf + args->baselen;
@@ -187,8 +188,8 @@ int write_archive_entries(struct archiver_args *args,
git_attr_set_direction(GIT_ATTR_INDEX, &the_index);
}
- err = read_tree_recursive(args->tree, args->base, args->baselen, 0,
- args->pathspec, write_archive_entry, &context);
+ err = read_tree_recursive(args->tree, "", 0, 0, args->pathspec,
+ write_archive_entry, &context);
if (err == READ_TREE_RECURSIVE)
err = 0;
return err;
@@ -211,7 +212,7 @@ static const struct archiver *lookup_archiver(const char *name)
static void parse_pathspec_arg(const char **pathspec,
struct archiver_args *ar_args)
{
- ar_args->pathspec = get_pathspec(ar_args->base, pathspec);
+ ar_args->pathspec = get_pathspec("", pathspec);
}
static void parse_treeish_arg(const char **argv,
^ permalink raw reply related
* Re: [JGIT] patch-id
From: Shawn O. Pearce @ 2009-10-08 16:28 UTC (permalink / raw)
To: Nasser Grainawi; +Cc: Robin Rosenberg, Git Mailing List
In-Reply-To: <4AC136CC.8040300@codeaurora.org>
Nasser Grainawi <nasser@codeaurora.org> wrote:
> I'm trying to add a public getPatchId method to the jgit Patch class [...]
>
> It seems Patch does some statistical number gathering, but at no point does
> it store a 'slimmed-down' version of a patch.
It parses the patch to create FileHeader objects, one for each
file mentioned in the script. Within each FileHeader there is a
HunkHeader object, one for each hunk present in the patch. Within
each HunkHeader there is an EditList composed of Edit instances;
each Edit instance denotes a contiguous line range within that hunk.
Edit instances come in one of 3 forms:
INSERT: a run of + lines with no - lines
DELETE: a run of - lines with no + lines
REPLACE: a mixture of - and + lines
and their type is actually determined by the line numbers attached
to them. A INSERT has the same starting and ending line number on
the A side, but on the B side the ending line number is at least
one higher than the starting number. DELETE is the reverse, and
REPLACE has both ending numbers higher than the starting number.
IIRC Edit uses 0 based offsets, so line 3 is actually position 2.
These HunkHeader and Edit instances are only available on a text
patch, binary patches use a different representation for the
binary delta. Combined diff patches (--cc format) also lack these
HunkHeader/Edit instances as we don't have a generic n-way patch
parser yet.
> I had the idea to just iterate
> over the FileHeader's and get the byte buffer of each, but I don't think
> those buffers have the parsed data.
The HunkHeader and Edit instances really don't have the actual
line data available to them, they only have the line numbers.
To generate a patch ID you'd need to get the line data too.
Worse, IIRC the patch ID generation in C git favors a 3 line context.
In theory you could modify FileHeader or HunkHeader to produce
a RawText that uses the underlying byte[] returned by getBuffer()
as the backing store, but create a specialized IntList which has the
actual file line numbers mapped to the positions in the patch script.
To do that you'd need to re-walk the patch, like the toEditList()
method in HunkHeader does.
Given that RawText you could feed it through something like
DiffFormatter to create a patch with 3 lines of context, and hash
the relevant bits.
But... that seems like a lot of work.
Also, there is a class in Gerrit Code Review called EditList (not
to be confused with JGit's EditList class!) that really should be
moved back over to JGit. It has some useful routines for walking
through a patch as a series of iterations.
> Short of that, suggestions for how to go about acquiring/storing a parsed
> representation of the data with maximal existing code re-use would be
> appreciated.
I'm coming up short on suggestions right now. I'm not seeing an
easy path to this without writing a bit of code. I think you really
just need to walk the patch... :-\
--
Shawn.
^ permalink raw reply
* Re: Git archive and trailing "/" in prefix
From: Junio C Hamano @ 2009-10-08 16:26 UTC (permalink / raw)
To: Sergio Callegari; +Cc: git
In-Reply-To: <loom.20091008T172303-658@post.gmane.org>
Sergio Callegari <sergio.callegari@gmail.com> writes:
> The git-archive man page indicates that if the --prefix option is passed to
> git-archive, it is compulsory to end the prefix with a "/"
No, it does not have to.
$ git archive --prefix=v1.6.0- v1.6.0 Makefile | tar xf -
$ make -f v1.6.0-Makefile
This is consistent with the way the same --prefix option can be used with
checkout-index. e.g. to swap Makefile in work tree and in the index:
$ edit Makefile
$ git checkout-index --prefix=old- Makefile
$ git update-index Makefile
$ mv old-Makefile Makefile
These may or may not be useful examples, but this feature has been with us
for a long time. I wouldn't be surprised if removing the ability to
archive or checkout with filename prefix (not leading directory path
prefix) causes grief to existing scripts of people.
^ permalink raw reply
* Re: [PATCH] git-send-email.perl: Fold long header lines to 78 chars
From: Junio C Hamano @ 2009-10-08 16:27 UTC (permalink / raw)
To: Joe Perches; +Cc: git, Jay Soffian
In-Reply-To: <1254979690.2056.103.camel@Joe-Laptop.home>
Joe Perches <joe@perches.com> writes:
>> I do not think this hunk is correct.
>> Shouldn't we be rather repeating "RCPT TO: " for each recipient, as
>> RFC2821 4.1.1.3 says (this is an issue with the original code)?
>
> Looks like you're right.
>
> Want a new patch or will you fix both issues?
>
> I suggest using the same join as "To:" for "Cc:" and
> multiple single line "RCPT TO:"s.
Sure. Please make it so.
^ permalink raw reply
* Git archive and trailing "/" in prefix
From: Sergio Callegari @ 2009-10-08 15:35 UTC (permalink / raw)
To: git
Hi!
The git-archive man page indicates that if the --prefix option is passed to
git-archive, it is compulsory to end the prefix with a "/"
git archive [--format=<fmt>] [--list] [--prefix=<prefix>/] [<extra>] ...
As a matter of fact, the archiver behaves quite strangely if that slash is
missing. Files in the root of the working dir are added to the archive with
their own name modified by the prefix and the same happens for working dir
sub-directories. However, no file present in the sub-directories, nor
sub-sub-directories are added.
I would like to know if there some reason why a trailing "/" is not added
automatically to the prefix when it is missing and the prefix is not empty.
Would that break anything?
Thanks!
Sergio
^ permalink raw reply
* Re: [PATCH] Speedup bash completion loading
From: Kirill Smelkov @ 2009-10-08 15:10 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Ted Pavlic, git
In-Reply-To: <20091008150206.GD9261@spearce.org>
On Thu, Oct 08, 2009 at 08:02:06AM -0700, Shawn O. Pearce wrote:
> Kirill Smelkov <kirr@mns.spb.ru> wrote:
> > diff --git a/contrib/completion/Makefile b/contrib/completion/Makefile
> > new file mode 100644
> > index 0000000..a0fbb66
> > --- /dev/null
> > +++ b/contrib/completion/Makefile
> > @@ -0,0 +1,11 @@
> > +all : git-completion.bash
> > +
> > +
> > +git-completion.bash: git-completion.bash.in git-completion.bash.generate
> > + # Generate completions for binaries we have just built
> > + PATH="$(shell pwd)/..:$$PATH" ./git-completion.bash.generate
>
> Is only one .. enough? Isn't that putting us into the contrib
> directory, and therefore not finding the 'git' we just compiled?
>
> I'm also concerned that git-completion.bash.generate requires
> bash to compile the completion for bash. IMHO, if we are building
> this code at compile time we shouldn't assume bash is available.
> What if this is a sandboxed build environment using another shell
> and /bin/bash isn't installed?
>
> I think the git-completion.bash.generate code needs to be a bit
> more sh agnostic than the completion routines themselves are.
>
> > +# pregenerated stuff (to save load time)
> > +__git_merge_strategylist=__GIT_MERGE_STRATEGYLIST
> > +__git_all_commandlist=__GIT_ALL_COMMANDLIST
> > +__git_porcelain_commandlist=__GIT_PORCELAIN_COMMANDLIST
>
> This also makes testing the completion a bit more difficult, now
> we have to build it before we can load it, making the testing cycle
> actually be:
>
> make && . git-completion.bash
>
> We probably should place a quick comment here to remind folks that
> they need to build the script in order to test it properly.
Agree with everything and thanks. Will reroll tommorow.
Кирилл
^ permalink raw reply
* Re: [PATCH] Speedup bash completion loading
From: Shawn O. Pearce @ 2009-10-08 15:02 UTC (permalink / raw)
To: Kirill Smelkov; +Cc: Ted Pavlic, git
In-Reply-To: <20091008132718.GA12161@tugrik.mns.mnsspb.ru>
Kirill Smelkov <kirr@mns.spb.ru> wrote:
> diff --git a/contrib/completion/Makefile b/contrib/completion/Makefile
> new file mode 100644
> index 0000000..a0fbb66
> --- /dev/null
> +++ b/contrib/completion/Makefile
> @@ -0,0 +1,11 @@
> +all : git-completion.bash
> +
> +
> +git-completion.bash: git-completion.bash.in git-completion.bash.generate
> + # Generate completions for binaries we have just built
> + PATH="$(shell pwd)/..:$$PATH" ./git-completion.bash.generate
Is only one .. enough? Isn't that putting us into the contrib
directory, and therefore not finding the 'git' we just compiled?
I'm also concerned that git-completion.bash.generate requires
bash to compile the completion for bash. IMHO, if we are building
this code at compile time we shouldn't assume bash is available.
What if this is a sandboxed build environment using another shell
and /bin/bash isn't installed?
I think the git-completion.bash.generate code needs to be a bit
more sh agnostic than the completion routines themselves are.
> +# pregenerated stuff (to save load time)
> +__git_merge_strategylist=__GIT_MERGE_STRATEGYLIST
> +__git_all_commandlist=__GIT_ALL_COMMANDLIST
> +__git_porcelain_commandlist=__GIT_PORCELAIN_COMMANDLIST
This also makes testing the completion a bit more difficult, now
we have to build it before we can load it, making the testing cycle
actually be:
make && . git-completion.bash
We probably should place a quick comment here to remind folks that
they need to build the script in order to test it properly.
--
Shawn.
^ permalink raw reply
* [PATCH] ls-files: make option parser keep argv[0]
From: Ben Walton @ 2009-10-08 14:20 UTC (permalink / raw)
To: gitster, git; +Cc: Ben Walton
The ls-files built-in was not asking the option parser to maintain
argv[0]. This led to the possibility of fprintf(stderr, "...", NULL).
On Solaris, this was causing a segfault. On glibc systems, printed
error messages didn't contain proper strings, but rather, "(null)":...
A trigger for this bug was: `git ls-files -i`
Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
---
builtin-ls-files.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin-ls-files.c b/builtin-ls-files.c
index f473220..9a3705a 100644
--- a/builtin-ls-files.c
+++ b/builtin-ls-files.c
@@ -482,7 +482,7 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
git_config(git_default_config, NULL);
argc = parse_options(argc, argv, prefix, builtin_ls_files_options,
- ls_files_usage, 0);
+ ls_files_usage, PARSE_OPT_KEEP_ARGV0);
if (show_tag || show_valid_bit) {
tag_cached = "H ";
tag_unmerged = "M ";
@@ -505,7 +505,7 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
if (require_work_tree && !is_inside_work_tree())
setup_work_tree();
- pathspec = get_pathspec(prefix, argv);
+ pathspec = get_pathspec(prefix, argv + 1);
/* be nice with submodule paths ending in a slash */
read_cache();
--
1.6.4.4
^ permalink raw reply related
* Re: [PATCH v2] perl/Makefile.PL: detect MakeMaker versions incompatible with DESTDIR
From: Brandon Casey @ 2009-10-08 13:34 UTC (permalink / raw)
To: Johannes Sixt; +Cc: gitster, git, c, Brandon Casey
In-Reply-To: <4ACDE76C.4040307@viscovery.net>
Johannes Sixt wrote:
> Brandon Casey schrieb:
>> From: Brandon Casey <drafnel@gmail.com>
>>
>> It appears that ExtUtils::MakeMaker versions older than 6.11 do not
>> implement the DESTDIR mechanism. So add a test to the generated perl.mak
>> to detect when DESTDIR is used along with a too old ExtUtils::MakeMaker and
>> abort with a message suggesting the use of NO_PERL_MAKEMAKER.
>>
>> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
>> ---
>>
>>
>> This just reverses the logic in the test on $(MM_VERSION) so that the test
>> will also fail if MM_VERSION is unset. Who knows if ancient versions set
>> it. Sorry for the quick v2.
>>
>> -brandon
>>
>>
>> perl/Makefile.PL | 8 ++++++++
>> 1 files changed, 8 insertions(+), 0 deletions(-)
>>
>> diff --git a/perl/Makefile.PL b/perl/Makefile.PL
>> index 320253e..0b9deca 100644
>> --- a/perl/Makefile.PL
>> +++ b/perl/Makefile.PL
>> @@ -5,6 +5,14 @@ sub MY::postamble {
>> instlibdir:
>> @echo '$(INSTALLSITELIB)'
>>
>> +ifneq (,$(DESTDIR))
>> +ifeq (0,$(shell expr '$(MM_VERSION)' '>' 6.10))
>
> I don't think the test works as intended, because 6.2 *is* greater than
> 6.10 (aka 6.1).
Hmm... I think you're right.
-brandon
^ permalink raw reply
* Re: [PATCH] Speedup bash completion loading
From: Kirill Smelkov @ 2009-10-08 13:27 UTC (permalink / raw)
To: Shawn O. Pearce, Ted Pavlic; +Cc: git
In-Reply-To: <20091005152504.GE9261@spearce.org>
On Mon, Oct 05, 2009 at 08:25:04AM -0700, Shawn O. Pearce wrote:
> Kirill Smelkov <kirr@mns.spb.ru> wrote:
> > I've tracked down that the most time is spent warming up merge_strategy,
> > all_command & porcelain_command caches.
>
> Nak.
>
> The problem is, during completion when we modify the value the
> change doesn't persist beyond the current completion invocation.
> Thus there is no value in the cache, so every completion attempt
> which needs the list has to rerun the command to compute it.
> > Yes, my mistake.
> >
> > To avoid spawning subshell on $(__git_porcelain_commands) I could come up
> > with "caching" at the upper level, as e.g.
> >
> > @@ -2107,7 +2111,10 @@ _git ()
> > --help
> > "
> > ;;
> > - *) __gitcomp "$(__git_porcelain_commands) $(__git_aliases)" ;;
> > + *) __gitcomp "
> > + ${__git_porcelain_commandlist:=$(__git_porcelain_commands)}
> > + $(__git_aliases)
> > + " ;;
> >
> >
> > but that's ugly and not maintainable since e.g. __git_all_commands() is used in
> > several places.
On Mon, Oct 05, 2009 at 03:18:48PM -0400, Ted Pavlic wrote:
> > The other possibility is to pregenerate all these lists at git build time, but
> > are we going to do it for things under contrib/ ?
>
> IIRC, that's what StGIT does with its completion script. As a
> consequence, StGIT completion is substantially faster than git
> completion.
Ok, it turned out that in bash, functions can't return values except
through their stdout, but then we are forever with spawning
subprocesses. So I've decided to go the pregenerate-it way.
[ While at it, I've also done chmod -x to git-completion.bash -- this
script is never executed, only sourced, so it does not need to be
executable. If needed I could split it in separate patch ]
---- 8< ----
>From 4178dcc2bd1005e26e069fe32a88b9a5b164ced6 Mon Sep 17 00:00:00 2001
From: Kirill Smelkov <kirr@mns.spb.ru>
Date: Mon, 5 Oct 2009 13:36:15 +0400
Subject: [PATCH v2] Speedup bash completion loading
On my slow laptop (P3 700MHz), system-wide bash completions take too
much time to load (> 1s), and significant fraction of this time is spent
loading git-completion.bash:
$ time bash -c '. git-completion.bash' # before this patch
real 0m0.317s
user 0m0.250s
sys 0m0.060s
I've tracked down that the most time is spent warming up merge_strategy,
all_command & porcelain_command caches.
Initially I thought that since git is not used in each and every
interactive xterm, it would be perfectly ok to load completion support
with cold caches, and then load needed thing lazily.
But for me this strategy turned out to be difficult to implement in
simple and maintainable way -- bash does not provide a way to return values
from inside functions, so one will have to use e.g.
${__git_all_commandlist:=$(__git_all_commands)}
everywhere in place where $(__git_all_commands) we used before, so as
also Ted Pavlic suggested let's pregenerate everything at build time so
that we have nothing to compute at runtime when git-completion.bash
script is loaded.
The result is that loading completion is significantly faster now:
$ time bash -c '. git-completion.bash' # after this patch
real 0m0.068s
user 0m0.060s
sys 0m0.010s
Cc: Ted Pavlic <ted@tedpavlic.com>
Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
contrib/completion/.gitignore | 1 +
contrib/completion/Makefile | 11 ++
contrib/completion/git-completion.bash.generate | 128 ++++++++++++++++
...{git-completion.bash => git-completion.bash.in} | 154 ++------------------
4 files changed, 155 insertions(+), 139 deletions(-)
create mode 100644 contrib/completion/.gitignore
create mode 100644 contrib/completion/Makefile
create mode 100755 contrib/completion/git-completion.bash.generate
rename contrib/completion/{git-completion.bash => git-completion.bash.in} (90%)
mode change 100755 => 100644
diff --git a/contrib/completion/.gitignore b/contrib/completion/.gitignore
new file mode 100644
index 0000000..578e6a8
--- /dev/null
+++ b/contrib/completion/.gitignore
@@ -0,0 +1 @@
+git-completion.bash
diff --git a/contrib/completion/Makefile b/contrib/completion/Makefile
new file mode 100644
index 0000000..a0fbb66
--- /dev/null
+++ b/contrib/completion/Makefile
@@ -0,0 +1,11 @@
+all : git-completion.bash
+
+
+git-completion.bash: git-completion.bash.in git-completion.bash.generate
+ # Generate completions for binaries we have just built
+ PATH="$(shell pwd)/..:$$PATH" ./git-completion.bash.generate
+
+
+clean:
+ rm -f git-completion.bash
+
diff --git a/contrib/completion/git-completion.bash.generate b/contrib/completion/git-completion.bash.generate
new file mode 100755
index 0000000..fa998dc
--- /dev/null
+++ b/contrib/completion/git-completion.bash.generate
@@ -0,0 +1,128 @@
+#!/bin/bash
+#
+# Generate bash completion for git.
+#
+# Precompute everything that can be known in advance at build time, so that
+# actual bash completion script is loaded faster.
+
+__git_merge_strategies ()
+{
+ git merge -s help 2>&1 |
+ sed -n -e '/[Aa]vailable strategies are: /,/^$/{
+ s/\.$//
+ s/.*://
+ s/^[ ]*//
+ s/[ ]*$//
+ p
+ }'
+}
+
+__git_all_commands ()
+{
+ local i IFS=" "$'\n'
+ for i in $(git help -a|egrep '^ ')
+ do
+ case $i in
+ *--*) : helper pattern;;
+ *) echo $i;;
+ esac
+ done
+}
+
+
+__git_porcelain_commands ()
+{
+ local i IFS=" "$'\n'
+ for i in "help" $(__git_all_commands)
+ do
+ case $i in
+ *--*) : helper pattern;;
+ applymbox) : ask gittus;;
+ applypatch) : ask gittus;;
+ archimport) : import;;
+ cat-file) : plumbing;;
+ check-attr) : plumbing;;
+ check-ref-format) : plumbing;;
+ checkout-index) : plumbing;;
+ commit-tree) : plumbing;;
+ count-objects) : infrequent;;
+ cvsexportcommit) : export;;
+ cvsimport) : import;;
+ cvsserver) : daemon;;
+ daemon) : daemon;;
+ diff-files) : plumbing;;
+ diff-index) : plumbing;;
+ diff-tree) : plumbing;;
+ fast-import) : import;;
+ fast-export) : export;;
+ fsck-objects) : plumbing;;
+ fetch-pack) : plumbing;;
+ fmt-merge-msg) : plumbing;;
+ for-each-ref) : plumbing;;
+ hash-object) : plumbing;;
+ http-*) : transport;;
+ index-pack) : plumbing;;
+ init-db) : deprecated;;
+ local-fetch) : plumbing;;
+ lost-found) : infrequent;;
+ ls-files) : plumbing;;
+ ls-remote) : plumbing;;
+ ls-tree) : plumbing;;
+ mailinfo) : plumbing;;
+ mailsplit) : plumbing;;
+ merge-*) : plumbing;;
+ mktree) : plumbing;;
+ mktag) : plumbing;;
+ pack-objects) : plumbing;;
+ pack-redundant) : plumbing;;
+ pack-refs) : plumbing;;
+ parse-remote) : plumbing;;
+ patch-id) : plumbing;;
+ peek-remote) : plumbing;;
+ prune) : plumbing;;
+ prune-packed) : plumbing;;
+ quiltimport) : import;;
+ read-tree) : plumbing;;
+ receive-pack) : plumbing;;
+ reflog) : plumbing;;
+ repo-config) : deprecated;;
+ rerere) : plumbing;;
+ rev-list) : plumbing;;
+ rev-parse) : plumbing;;
+ runstatus) : plumbing;;
+ sh-setup) : internal;;
+ shell) : daemon;;
+ show-ref) : plumbing;;
+ send-pack) : plumbing;;
+ show-index) : plumbing;;
+ ssh-*) : transport;;
+ stripspace) : plumbing;;
+ symbolic-ref) : plumbing;;
+ tar-tree) : deprecated;;
+ unpack-file) : plumbing;;
+ unpack-objects) : plumbing;;
+ update-index) : plumbing;;
+ update-ref) : plumbing;;
+ update-server-info) : daemon;;
+ upload-archive) : plumbing;;
+ upload-pack) : plumbing;;
+ write-tree) : plumbing;;
+ var) : infrequent;;
+ verify-pack) : infrequent;;
+ verify-tag) : plumbing;;
+ *) echo $i;;
+ esac
+ done
+}
+
+
+__git_merge_strategylist=$(__git_merge_strategies | tr '\n' ' ')
+__git_all_commandlist="$(__git_all_commands | tr '\n' ' ')"
+__git_porcelain_commandlist="$(__git_porcelain_commands | tr '\n' ' ')"
+
+
+sed -e "s/__GIT_MERGE_STRATEGYLIST/\"$__git_merge_strategylist\"/" \
+ -e "s/__GIT_ALL_COMMANDLIST/\"$__git_all_commandlist\"/" \
+ -e "s/__GIT_PORCELAIN_COMMANDLIST/\"$__git_porcelain_commandlist\"/" \
+ git-completion.bash.in > git-completion.bash
+
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash.in
old mode 100755
new mode 100644
similarity index 90%
rename from contrib/completion/git-completion.bash
rename to contrib/completion/git-completion.bash.in
index 88b1b3c..cf1b5fd
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash.in
@@ -21,13 +21,7 @@
# 2) Added the following line to your .bashrc:
# source ~/.git-completion.sh
#
-# 3) You may want to make sure the git executable is available
-# in your PATH before this script is sourced, as some caching
-# is performed while the script loads. If git isn't found
-# at source time then all lookups will be done on demand,
-# which may be slightly slower.
-#
-# 4) Consider changing your PS1 to also show the current branch:
+# 3) Consider changing your PS1 to also show the current branch:
# PS1='[\u@\h \W$(__git_ps1 " (%s)")]\$ '
#
# The argument to __git_ps1 will be displayed only if you
@@ -84,6 +78,14 @@ __gitdir ()
fi
}
+
+# pregenerated stuff (to save load time)
+__git_merge_strategylist=__GIT_MERGE_STRATEGYLIST
+__git_all_commandlist=__GIT_ALL_COMMANDLIST
+__git_porcelain_commandlist=__GIT_PORCELAIN_COMMANDLIST
+
+
+
# __git_ps1 accepts 0 or 1 arguments (i.e., format string)
# returns text to add to bash PS1 prompt (includes branch name)
__git_ps1 ()
@@ -324,23 +326,6 @@ __git_remotes ()
done
}
-__git_merge_strategies ()
-{
- if [ -n "${__git_merge_strategylist-}" ]; then
- echo "$__git_merge_strategylist"
- return
- fi
- git merge -s help 2>&1 |
- sed -n -e '/[Aa]vailable strategies are: /,/^$/{
- s/\.$//
- s/.*://
- s/^[ ]*//
- s/[ ]*$//
- p
- }'
-}
-__git_merge_strategylist=
-__git_merge_strategylist=$(__git_merge_strategies 2>/dev/null)
__git_complete_file ()
{
@@ -476,128 +461,19 @@ __git_complete_strategy ()
{
case "${COMP_WORDS[COMP_CWORD-1]}" in
-s|--strategy)
- __gitcomp "$(__git_merge_strategies)"
+ __gitcomp "$__git_merge_strategylist"
return 0
esac
local cur="${COMP_WORDS[COMP_CWORD]}"
case "$cur" in
--strategy=*)
- __gitcomp "$(__git_merge_strategies)" "" "${cur##--strategy=}"
+ __gitcomp "$__git_merge_strategylist" "" "${cur##--strategy=}"
return 0
;;
esac
return 1
}
-__git_all_commands ()
-{
- if [ -n "${__git_all_commandlist-}" ]; then
- echo "$__git_all_commandlist"
- return
- fi
- local i IFS=" "$'\n'
- for i in $(git help -a|egrep '^ ')
- do
- case $i in
- *--*) : helper pattern;;
- *) echo $i;;
- esac
- done
-}
-__git_all_commandlist=
-__git_all_commandlist="$(__git_all_commands 2>/dev/null)"
-
-__git_porcelain_commands ()
-{
- if [ -n "${__git_porcelain_commandlist-}" ]; then
- echo "$__git_porcelain_commandlist"
- return
- fi
- local i IFS=" "$'\n'
- for i in "help" $(__git_all_commands)
- do
- case $i in
- *--*) : helper pattern;;
- applymbox) : ask gittus;;
- applypatch) : ask gittus;;
- archimport) : import;;
- cat-file) : plumbing;;
- check-attr) : plumbing;;
- check-ref-format) : plumbing;;
- checkout-index) : plumbing;;
- commit-tree) : plumbing;;
- count-objects) : infrequent;;
- cvsexportcommit) : export;;
- cvsimport) : import;;
- cvsserver) : daemon;;
- daemon) : daemon;;
- diff-files) : plumbing;;
- diff-index) : plumbing;;
- diff-tree) : plumbing;;
- fast-import) : import;;
- fast-export) : export;;
- fsck-objects) : plumbing;;
- fetch-pack) : plumbing;;
- fmt-merge-msg) : plumbing;;
- for-each-ref) : plumbing;;
- hash-object) : plumbing;;
- http-*) : transport;;
- index-pack) : plumbing;;
- init-db) : deprecated;;
- local-fetch) : plumbing;;
- lost-found) : infrequent;;
- ls-files) : plumbing;;
- ls-remote) : plumbing;;
- ls-tree) : plumbing;;
- mailinfo) : plumbing;;
- mailsplit) : plumbing;;
- merge-*) : plumbing;;
- mktree) : plumbing;;
- mktag) : plumbing;;
- pack-objects) : plumbing;;
- pack-redundant) : plumbing;;
- pack-refs) : plumbing;;
- parse-remote) : plumbing;;
- patch-id) : plumbing;;
- peek-remote) : plumbing;;
- prune) : plumbing;;
- prune-packed) : plumbing;;
- quiltimport) : import;;
- read-tree) : plumbing;;
- receive-pack) : plumbing;;
- reflog) : plumbing;;
- repo-config) : deprecated;;
- rerere) : plumbing;;
- rev-list) : plumbing;;
- rev-parse) : plumbing;;
- runstatus) : plumbing;;
- sh-setup) : internal;;
- shell) : daemon;;
- show-ref) : plumbing;;
- send-pack) : plumbing;;
- show-index) : plumbing;;
- ssh-*) : transport;;
- stripspace) : plumbing;;
- symbolic-ref) : plumbing;;
- tar-tree) : deprecated;;
- unpack-file) : plumbing;;
- unpack-objects) : plumbing;;
- update-index) : plumbing;;
- update-ref) : plumbing;;
- update-server-info) : daemon;;
- upload-archive) : plumbing;;
- upload-pack) : plumbing;;
- write-tree) : plumbing;;
- var) : infrequent;;
- verify-pack) : infrequent;;
- verify-tag) : plumbing;;
- *) echo $i;;
- esac
- done
-}
-__git_porcelain_commandlist=
-__git_porcelain_commandlist="$(__git_porcelain_commands 2>/dev/null)"
-
__git_aliases ()
{
local i IFS=$'\n'
@@ -1077,7 +953,7 @@ _git_help ()
return
;;
esac
- __gitcomp "$(__git_all_commands)
+ __gitcomp "$__git_all_commandlist
attributes cli core-tutorial cvs-migration
diffcore gitk glossary hooks ignore modules
repository-layout tutorial tutorial-2
@@ -1423,7 +1299,7 @@ _git_config ()
return
;;
pull.twohead|pull.octopus)
- __gitcomp "$(__git_merge_strategies)"
+ __gitcomp "$__git_merge_strategylist"
return
;;
color.branch|color.diff|color.interactive|\
@@ -1524,7 +1400,7 @@ _git_config ()
pager.*)
local pfx="${cur%.*}."
cur="${cur#*.}"
- __gitcomp "$(__git_all_commands)" "$pfx" "$cur"
+ __gitcomp "$__git_all_commandlist" "$pfx" "$cur"
return
;;
remote.*.*)
@@ -2116,7 +1992,7 @@ _git ()
--help
"
;;
- *) __gitcomp "$(__git_porcelain_commands) $(__git_aliases)" ;;
+ *) __gitcomp "$__git_porcelain_commandlist $(__git_aliases)" ;;
esac
return
fi
--
1.6.5.rc2.18.g84f98.dirty
^ permalink raw reply related
* Re: [PATCH v2] perl/Makefile.PL: detect MakeMaker versions incompatible with DESTDIR
From: Johannes Sixt @ 2009-10-08 13:21 UTC (permalink / raw)
To: Brandon Casey; +Cc: gitster, git, c, Brandon Casey
In-Reply-To: <FE_WTi0YAHrCrSdGFemlb7ALatFkdSu5V7Yfb5CUgyoxfv3ZFXdFABKbT1boP7aeGWli-gJPcBA@cipher.nrlssc.navy.mil>
Brandon Casey schrieb:
> From: Brandon Casey <drafnel@gmail.com>
>
> It appears that ExtUtils::MakeMaker versions older than 6.11 do not
> implement the DESTDIR mechanism. So add a test to the generated perl.mak
> to detect when DESTDIR is used along with a too old ExtUtils::MakeMaker and
> abort with a message suggesting the use of NO_PERL_MAKEMAKER.
>
> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
> ---
>
>
> This just reverses the logic in the test on $(MM_VERSION) so that the test
> will also fail if MM_VERSION is unset. Who knows if ancient versions set
> it. Sorry for the quick v2.
>
> -brandon
>
>
> perl/Makefile.PL | 8 ++++++++
> 1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/perl/Makefile.PL b/perl/Makefile.PL
> index 320253e..0b9deca 100644
> --- a/perl/Makefile.PL
> +++ b/perl/Makefile.PL
> @@ -5,6 +5,14 @@ sub MY::postamble {
> instlibdir:
> @echo '$(INSTALLSITELIB)'
>
> +ifneq (,$(DESTDIR))
> +ifeq (0,$(shell expr '$(MM_VERSION)' '>' 6.10))
I don't think the test works as intended, because 6.2 *is* greater than
6.10 (aka 6.1).
(Found while staring at git diff v1.6.5-rc2..v1.6.5-rc3 in a spare minute.)
> +$(error ExtUtils::MakeMaker version "$(MM_VERSION)" is older than 6.11 and so \
> + is likely incompatible with the DESTDIR mechanism. Try setting \
> + NO_PERL_MAKEMAKER=1 instead)
> +endif
> +endif
> +
> MAKE_FRAG
> }
-- Hannes
^ permalink raw reply
* Re: git log -S not finding all commits?
From: Daniel @ 2009-10-08 12:49 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Andreas Ericsson, git
In-Reply-To: <vpq63aqxflu.fsf@bauges.imag.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
> > git log -p --format="%s\n%x00" | perl -0 -ne 'print if(/whatever-you-search/);'
>
> git log -p -z is better than my --format= indeed.
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/
>
Thanks, it's practically what I was looking for.
--
Daniel
^ permalink raw reply
* Re: git log -S not finding all commits?
From: Matthieu Moy @ 2009-10-08 11:57 UTC (permalink / raw)
To: Daniel; +Cc: Andreas Ericsson, git
In-Reply-To: <vpqbpkixgea.fsf@bauges.imag.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> git log -p --format="%s\n%x00" | perl -0 -ne 'print if(/whatever-you-search/);'
git log -p -z is better than my --format= indeed.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: git log -S not finding all commits?
From: Matthieu Moy @ 2009-10-08 11:40 UTC (permalink / raw)
To: Daniel; +Cc: Andreas Ericsson, git
In-Reply-To: <362436ca.6b5d0fc3.4acdc7e1.41b23@o2.pl>
Daniel <mjucde@o2.pl> writes:
> Andreas Ericsson <ae@op5.se> wrote:
>
>> Yes, it's the correct behaviour. -S finds only lines where what you search
>> for was added or deleted. It counts the number of occurrences of what you
>> specify in each resulting tree and only shows the commits where that number
>> changed. In your case, searching for "Free data " would have printed both
>> commits, since you first introduce that entire string and then remove it.
>
> Thanks. However, your suggestion doesn't work. It prints only commit 2. Maybe
> you meant:
>
> $ PAGER=cat git log --pickaxe-regex -S'Free data$' --oneline
>
> but that doesn't solve my problem. I want to find all commits which changed
> lines containing "Free data" (the example I posted is simplified).
>
> Seems I have to use "git log -p" and search its output using pager...
Search the ML's archives, this is a FAQ. People have proposed an
option to allow log -S to actually search the diff (much slower, but
sometimes what you really want), but AFAIK, no one wrote the code. But
I think someone posted a small perl script along the lines of
git log -p --format="%s\n%x00" | perl -0 -ne 'print if(/whatever-you-search/);'
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: git log -S not finding all commits?
From: Daniel @ 2009-10-08 11:07 UTC (permalink / raw)
To: Andreas Ericsson; +Cc: git
In-Reply-To: <4ACDACE6.9060509@op5.se>
Andreas Ericsson <ae@op5.se> wrote:
> Yes, it's the correct behaviour. -S finds only lines where what you search
> for was added or deleted. It counts the number of occurrences of what you
> specify in each resulting tree and only shows the commits where that number
> changed. In your case, searching for "Free data " would have printed both
> commits, since you first introduce that entire string and then remove it.
Thanks. However, your suggestion doesn't work. It prints only commit 2. Maybe
you meant:
$ PAGER=cat git log --pickaxe-regex -S'Free data$' --oneline
but that doesn't solve my problem. I want to find all commits which changed
lines containing "Free data" (the example I posted is simplified).
Seems I have to use "git log -p" and search its output using pager...
--
Daniel
^ permalink raw reply
* Re: git log -S not finding all commits?
From: Andreas Ericsson @ 2009-10-08 9:12 UTC (permalink / raw)
To: Daniel; +Cc: git
In-Reply-To: <7ae12651.522df17b.4acda0f5.21a31@o2.pl>
On 10/08/2009 10:21 AM, Daniel wrote:
> Hi,
>
> I did:
>
> $ git version
> git version 1.6.4.4
> $ mkdir a&& cd a&& git init
> $ echo "Free data"> a
> $ git add a
> $ git commit -m1
> $ echo "Free data allocated by other function"> a
> $ git commit -a -m2
> $ PAGER=cat git log -S'Free' --oneline
> 2f34241 1
>
> I would expect "git log" to show both 1 and 2 commit, but it prints only 1.
>
> Is it the correct behavior?
>
Yes, it's the correct behaviour. -S finds only lines where what you search
for was added or deleted. It counts the number of occurrences of what you
specify in each resulting tree and only shows the commits where that number
changed. In your case, searching for "Free data " would have printed both
commits, since you first introduce that entire string and then remove it.
--
Andreas Ericsson andreas.ericsson@op5.se
OP5 AB www.op5.se
Tel: +46 8-230225 Fax: +46 8-230231
Considering the successes of the wars on alcohol, poverty, drugs and
terror, I think we should give some serious thought to declaring war
on peace.
^ 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