* Re: Re* ext3: fix ext3_dx_readdir hash collision handling - Regression
From: Junio C Hamano @ 2008-10-24 22:35 UTC (permalink / raw)
To: Linus Torvalds; +Cc: git
In-Reply-To: <alpine.LFD.2.00.0810241524350.3287@nehalem.linux-foundation.org>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Fri, 24 Oct 2008, Junio C Hamano wrote:
>>
>> Subject: allow readdir(3) to return the same entry twice
>
> The thing is, this really is a kernel bug. We have even bisected it (and
> it hasn't hit any released kernel). The original reporter showed it with a
> simple "rm -r".
>
> So it really isn't a git bug, even though I initially thought it might be,
> before I looked closer.
>
> That said, the git patch may be worth it just because two *concurrent*
> invocations of "git clean" could then cause one (or both) to fail this
> way.
Yeah, or on perhaps a buggy implementation of readdir(3) on somebody
else's system. In either case, I just thought it might be a low impact
belt-and-suspenders fix that is worth considering.
^ permalink raw reply
* Re: [PATCH 3/5] refactor userdiff textconv code
From: Jeff King @ 2008-10-24 22:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Johannes Sixt, Matthieu Moy
In-Reply-To: <7v3ail3dfy.fsf@gitster.siamese.dyndns.org>
On Fri, Oct 24, 2008 at 02:12:17PM -0700, Junio C Hamano wrote:
> Either that or drop data_is_textconv and have two sets of <ptr,size> pair
> in filespec, one for real data and another purely for diff text
> generation.
I thought about that. My reasoning against it was two-fold:
1. We don't want to keep two copies in memory unnecessarily. Of
course, we could easily free the original, but just store the
information in a different pointer to make sure they never get
confused. So that is a non-issue.
2. I had some vague notion that it is more convenient in the long run
to do this to the filespec, since we can then transparently pass the
munged filespec around and pretend like the converted text was the
original content.
However, I'm not sure exactly _where_ we would want to do this.
The obvious places are for patch, for diffstat, or for whitespace
checking. But all of those places use mmfile, so we can munge them
in the same way. I haven't looked at using this with blame, but I do
think "git blame --textconv foo.jpg" would be useful.
(Actually not true. I did just look for 30 seconds at using this
with blame, but blame doesn't seem to to use builtin_diff at all).
> That is, you let fill_mmfile() to fill the real data in mf1 and mf2 as
> before, ask one/two if they have textconv, and if so, convert mf1 and mf2
> in place, and let xdl work on it, like...
I think your example would work fine, if there is no other advantage to
having a transparently-munged diff_filespec (as above).
-Peff
^ permalink raw reply
* Re: [PATCH 2/3] Add -n/--no-prompt option to mergetool
From: Charles Bailey @ 2008-10-24 22:55 UTC (permalink / raw)
To: William Pursell
Cc: Junio C Hamano, git, Jeff King, Andreas Ericsson,
Theodore Ts'o
In-Reply-To: <49024CF1.5040406@gmail.com>
On Fri, Oct 24, 2008 at 11:32:17PM +0100, William Pursell wrote:
>
> If the short option is dropped, the config option should
> probably associated with mergetool.<tool>.interactive rather
> than mergetool.interactive. (s/interactive/whatever)
I'm not sure I understand your reasoning. The no-prompt/interactive
option affects the behaviour of the mergetool script independent of
which particular merge tool is being used. Why should the presence or
absence of a short option affect whether the config option is global
or per tool?
--
Charles Bailey
http://ccgi.hashpling.plus.com/blog/
^ permalink raw reply
* Re: [PATCH 3/5] refactor userdiff textconv code
From: Jeff King @ 2008-10-24 22:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Johannes Sixt, Matthieu Moy
In-Reply-To: <20081024225001.GA13978@coredump.intra.peff.net>
On Fri, Oct 24, 2008 at 06:50:01PM -0400, Jeff King wrote:
> However, I'm not sure exactly _where_ we would want to do this.
> The obvious places are for patch, for diffstat, or for whitespace
> checking. But all of those places use mmfile, so we can munge them
> in the same way. I haven't looked at using this with blame, but I do
> think "git blame --textconv foo.jpg" would be useful.
>
> (Actually not true. I did just look for 30 seconds at using this
> with blame, but blame doesn't seem to to use builtin_diff at all).
Ah, I see. It looks like we would have to munge fill_blob_sha1 in
builtin-blame.c. In which case totally splitting this out from
diff_filespec is even better, since we don't have one there.
So let me try to re-roll my series based on your suggestion, and then
I'll see if I can add "blame --textconv" on top.
-Peff
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2008, #05; Wed, 22)
From: Jakub Narebski @ 2008-10-24 23:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vprlp1w1u.fsf@gitster.siamese.dyndns.org>
Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>> Junio C Hamano wrote:
>>
>>> * gb/gitweb-pathinfo (Tue Oct 21 21:34:54 2008 +0200) 5 commits
>>> - gitweb: generate parent..current URLs
>>> - gitweb: parse parent..current syntax from PATH_INFO
>>> - gitweb: use_pathinfo filenames start with /
>>> - gitweb: generate project/action/hash URLs
>>> - gitweb: parse project/action/hash_base:filename PATH_INFO
>>>
>>> Seventh iteration; hopefully the usual gitweb gangs will give quick
>>> comments and ack to push this out to 'next' soon.
>>
>> If I remember correctly v7 is mainly cosmetic changes, and I have
>> acked all or almost all of the equivalent patches from v6.
All of v6 patches were acked.
> I take it to mean you reviewed v7 and we have your Ack on them;
> thanks.
Yes.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: git performance
From: Pete Harlan @ 2008-10-24 23:10 UTC (permalink / raw)
To: Edward Ned Harvey; +Cc: git
In-Reply-To: <49017F8F.3000908@pcharlan.com>
Pete Harlan wrote:
> Edward Ned Harvey wrote:
>>> Yes, it does stat all the files. How many files are you talking about,
>>> and what platform? From a warm cache on Linux, the 23,000 files kernel
>>> repo takes about a tenth of a second to stat all files for me (and this
>> I'm talking about 40-50,000 files, on multi-user production linux,
>> which means the cache is never warm, except when I'm benchmarking.
>> Specifically RHEL 4 with the files on NFS mount. Cold cache "svn
>> st" takes ~10 mins. Warm cache 20-30 sec. Surprisingly to me,
>
> I did some tests with a repo with ~32k files, and git was slightly
> slower than svn with a cold cache (10.2s vs 8.4s), and around twice as
> fast with a warm cache (.5s vs 1s).
>
> Git 1.6.0.2, svn 1.4.6. Cache made cold with
> "echo 1 >/proc/sys/vm/drop_caches". Timings best of 5 runs.
After redoing this test with "echo 3 >/proc/sys/vm/drop_caches" (which
also discards metadata, as pointed out by Linus), the cold-cache
timings are:
svn 12.65 seconds
git 10.3 seconds
So no Earth-shattering difference, but now git is somewhat quicker
than Subversion at everything I tested.
--Pete
^ permalink raw reply
* Re: [PATCH] Implement git remote rename
From: Junio C Hamano @ 2008-10-24 23:33 UTC (permalink / raw)
To: Miklos Vajna; +Cc: Jeff King, Brandon Casey, git
In-Reply-To: <1224766597-16034-1-git-send-email-vmiklos@frugalware.org>
Miklos Vajna <vmiklos@frugalware.org> writes:
> +static int mv(int argc, const char **argv)
> +{
> + struct option options[] = {
> + OPT_END()
> + };
> + struct remote *oldremote, *newremote;
> + struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT;
> + struct string_list remote_branches = { NULL, 0, 0, 0 };
> + struct rename_info rename;
> + int i;
> +
> + if (argc != 3)
> + usage_with_options(builtin_remote_usage, options);
> +
> + rename.old = argv[1];
> + rename.new = argv[2];
> + rename.remote_branches = &remote_branches;
> +
> + oldremote = remote_get(rename.old);
> + if (!oldremote)
> + die("No such remote: %s", rename.old);
> +
> + newremote = remote_get(rename.new);
> + if (newremote && (newremote->url_nr > 1 || newremote->fetch_refspec_nr))
> + die("remote %s already exists.", rename.new);
> +
> + strbuf_addf(&buf, "refs/heads/test:refs/remotes/%s/test", rename.new);
> + if (!valid_fetch_refspec(buf.buf))
> + die("'%s' is not a valid remote name", rename.new);
> +
> + strbuf_reset(&buf);
> + strbuf_addf(&buf, "remote.%s", rename.old);
> + strbuf_addf(&buf2, "remote.%s", rename.new);
> + if (git_config_rename_section(buf.buf, buf2.buf) < 1)
> + return error("Could not rename config section '%s' to '%s'",
> + buf.buf, buf2.buf);
Hmm, remote_get() can read from all three supported places that you can
define remotes. Could you explain what happens if the old remote is read
from say $GIT_DIR/remotes/origin and you are renaming it to "upstream"
with "git remote rename origin upstream"?
I suspect that if you record where you read the configuration from in
"struct remote" and add necessary code to remove the original when
rename.old is *not* coming from in-config definition, you would make it
possible for repositories initialized with older git that has either
$GIT_DIR/branches/origin or $GIT_DIR/remotes/origin to be migrated to the
in-config format using "git remote rename origin origin".
^ permalink raw reply
* Re: [PATCH 3/5] refactor userdiff textconv code
From: Jeff King @ 2008-10-25 0:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Johannes Sixt, Matthieu Moy
In-Reply-To: <20081024225605.GA16774@coredump.intra.peff.net>
On Fri, Oct 24, 2008 at 06:56:05PM -0400, Jeff King wrote:
> Ah, I see. It looks like we would have to munge fill_blob_sha1 in
> builtin-blame.c. In which case totally splitting this out from
> diff_filespec is even better, since we don't have one there.
>
> So let me try to re-roll my series based on your suggestion, and then
> I'll see if I can add "blame --textconv" on top.
Actually, the blame thing is a little trickier than that. The textconv
stuff is still tied to a diff_filespec because it uses the diff
prepare_temp_file interface. So I will have to work around that if I
want to add blame support.
However, I re-did the existing patches according to your suggestions,
and I think the result is pretty reasonable. Six patches to follow.
-Peff
^ permalink raw reply
* [PATCH 1/7] diff: add missing static declaration
From: Jeff King @ 2008-10-25 0:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Matthieu Moy, git
In-Reply-To: <20081025004815.GA23851@coredump.intra.peff.net>
This function isn't used outside of diff.c; the 'static' was
simply overlooked in the original writing.
Signed-off-by: Jeff King <peff@peff.net>
---
Oops, it is actually 7 patches. This one is the same as before.
diff.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/diff.c b/diff.c
index e368fef..d1fd594 100644
--- a/diff.c
+++ b/diff.c
@@ -1282,7 +1282,7 @@ static void emit_binary_diff(FILE *file, mmfile_t *one, mmfile_t *two)
emit_binary_diff_body(file, two, one);
}
-void diff_filespec_load_driver(struct diff_filespec *one)
+static void diff_filespec_load_driver(struct diff_filespec *one)
{
if (!one->driver)
one->driver = userdiff_find_by_path(one->path);
--
1.6.0.3.523.g38597.dirty
^ permalink raw reply related
* [PATCH 2/7] add userdiff textconv tests
From: Jeff King @ 2008-10-25 0:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Matthieu Moy, git
In-Reply-To: <20081025004815.GA23851@coredump.intra.peff.net>
These tests provide a basic sanity check that textconv'd
files work. The tests try to describe how this configuration
_should_ work; thus some of the tests are marked to expect
failure.
In particular, we fail to actually textconv anything because
the 'diff.foo.binary' config option is not set, which will
be fixed in the next patch.
This also means that some "expect_failure" tests actually
seem to be fixed; in reality, this is just because textconv
is broken and its failure mode happens to make these tests
work.
Signed-off-by: Jeff King <peff@peff.net>
---
This brings in Johannes' typechange test, and adds a diffstat test
which we will fix along the way.
t/t4030-diff-textconv.sh | 118 ++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 118 insertions(+), 0 deletions(-)
create mode 100755 t/t4030-diff-textconv.sh
diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh
new file mode 100755
index 0000000..1b09648
--- /dev/null
+++ b/t/t4030-diff-textconv.sh
@@ -0,0 +1,118 @@
+#!/bin/sh
+
+test_description='diff.*.textconv tests'
+. ./test-lib.sh
+
+find_diff() {
+ sed '1,/^index /d' | sed '/^-- $/,$d'
+}
+
+cat >expect.binary <<'EOF'
+Binary files a/file and b/file differ
+EOF
+
+cat >expect.text <<'EOF'
+--- a/file
++++ b/file
+@@ -1 +1,2 @@
+ 0
++1
+EOF
+
+cat >hexdump <<'EOF'
+#!/bin/sh
+perl -e '$/ = undef; $_ = <>; s/./ord($&)/ge; print $_' "$1"
+EOF
+chmod +x hexdump
+
+test_expect_success 'setup binary file with history' '
+ printf "\\0\\n" >file &&
+ git add file &&
+ git commit -m one &&
+ printf "\\1\\n" >>file &&
+ git add file &&
+ git commit -m two
+'
+
+test_expect_success 'file is considered binary by porcelain' '
+ git diff HEAD^ HEAD >diff &&
+ find_diff <diff >actual &&
+ test_cmp expect.binary actual
+'
+
+test_expect_success 'file is considered binary by plumbing' '
+ git diff-tree -p HEAD^ HEAD >diff &&
+ find_diff <diff >actual &&
+ test_cmp expect.binary actual
+'
+
+test_expect_success 'setup textconv filters' '
+ echo file diff=foo >.gitattributes &&
+ git config diff.foo.textconv "$PWD"/hexdump &&
+ git config diff.fail.textconv false
+'
+
+test_expect_failure 'diff produces text' '
+ git diff HEAD^ HEAD >diff &&
+ find_diff <diff >actual &&
+ test_cmp expect.text actual
+'
+
+test_expect_success 'diff-tree produces binary' '
+ git diff-tree -p HEAD^ HEAD >diff &&
+ find_diff <diff >actual &&
+ test_cmp expect.binary actual
+'
+
+test_expect_failure 'log produces text' '
+ git log -1 -p >log &&
+ find_diff <log >actual &&
+ test_cmp expect.text actual
+'
+
+test_expect_failure 'format-patch produces binary' '
+ git format-patch --no-binary --stdout HEAD^ >patch &&
+ find_diff <patch >actual &&
+ test_cmp expect.binary actual
+'
+
+cat >expect.stat <<'EOF'
+ file | Bin 2 -> 4 bytes
+ 1 files changed, 0 insertions(+), 0 deletions(-)
+EOF
+test_expect_failure 'diffstat does not run textconv' '
+ echo file diff=fail >.gitattributes &&
+ git diff --stat HEAD^ HEAD >actual &&
+ test_cmp expect.stat actual
+'
+# restore working setup
+echo file diff=foo >.gitattributes
+
+cat >expect.typechange <<'EOF'
+--- a/file
++++ /dev/null
+@@ -1,2 +0,0 @@
+-0
+-1
+diff --git a/file b/file
+new file mode 120000
+index ad8b3d2..67be421
+--- /dev/null
++++ b/file
+@@ -0,0 +1 @@
++frotz
+\ No newline at end of file
+EOF
+# make a symlink the hard way that works on symlink-challenged file systems
+test_expect_failure 'textconv does not act on symlinks' '
+ echo -n frotz > file &&
+ git add file &&
+ git ls-files -s | sed -e s/100644/120000/ |
+ git update-index --index-info &&
+ git commit -m typechange &&
+ git show >diff &&
+ find_diff <diff >actual &&
+ test_cmp expect.typechange actual
+'
+
+test_done
--
1.6.0.3.523.g38597.dirty
^ permalink raw reply related
* [PATCH 3/7] textconv: assume text-converted contents are not binary
From: Jeff King @ 2008-10-25 0:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Matthieu Moy, git
In-Reply-To: <20081025004815.GA23851@coredump.intra.peff.net>
Previously, we would use a 'textconv' filter when filling
the mmfile_t, but then still check the diff_filespec struct,
which was not modified at all by the text conversion,
whether it was binary. And in general, it was, since the
point of text conversion is to munge binary contents. This
meant that the user had to manually set "diff.foo.binary =
false" to convince the text conversion to actually happen.
This patch pulls the logic for "can we textconv this
diff_filespec?" into a separate function. We can then check
this to see if a filespec which is binary would have been
converted.
Signed-off-by: Jeff King <peff@peff.net>
---
This used to be part 1 of the big refactoring patch.
diff.c | 18 ++++++++++++++----
t/t4030-diff-textconv.sh | 4 ++--
2 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/diff.c b/diff.c
index d1fd594..8fad215 100644
--- a/diff.c
+++ b/diff.c
@@ -284,8 +284,18 @@ static void emit_rewrite_diff(const char *name_a,
copy_file_with_prefix(o->file, '+', two->data, two->size, new, reset);
}
+static const char *get_textconv(struct diff_filespec *one)
+{
+ if (!DIFF_FILE_VALID(one))
+ return NULL;
+ diff_filespec_load_driver(one);
+ return one->driver->textconv;
+}
+
static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)
{
+ const char *textconv;
+
if (!DIFF_FILE_VALID(one)) {
mf->ptr = (char *)""; /* does not matter */
mf->size = 0;
@@ -294,10 +304,9 @@ static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)
else if (diff_populate_filespec(one, 0))
return -1;
- diff_filespec_load_driver(one);
- if (one->driver->textconv) {
+ if ((textconv = get_textconv(one))) {
size_t size;
- mf->ptr = run_textconv(one->driver->textconv, one, &size);
+ mf->ptr = run_textconv(textconv, one, &size);
if (!mf->ptr)
return -1;
mf->size = size;
@@ -1391,7 +1400,8 @@ static void builtin_diff(const char *name_a,
die("unable to read files to diff");
if (!DIFF_OPT_TST(o, TEXT) &&
- (diff_filespec_is_binary(one) || diff_filespec_is_binary(two))) {
+ ( (diff_filespec_is_binary(one) && !get_textconv(one)) ||
+ (diff_filespec_is_binary(two) && !get_textconv(two)) )) {
/* Quite common confusing case */
if (mf1.size == mf2.size &&
!memcmp(mf1.ptr, mf2.ptr, mf1.size))
diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh
index 1b09648..af94e1a 100755
--- a/t/t4030-diff-textconv.sh
+++ b/t/t4030-diff-textconv.sh
@@ -52,7 +52,7 @@ test_expect_success 'setup textconv filters' '
git config diff.fail.textconv false
'
-test_expect_failure 'diff produces text' '
+test_expect_success 'diff produces text' '
git diff HEAD^ HEAD >diff &&
find_diff <diff >actual &&
test_cmp expect.text actual
@@ -64,7 +64,7 @@ test_expect_success 'diff-tree produces binary' '
test_cmp expect.binary actual
'
-test_expect_failure 'log produces text' '
+test_expect_success 'log produces text' '
git log -1 -p >log &&
find_diff <log >actual &&
test_cmp expect.text actual
--
1.6.0.3.523.g38597.dirty
^ permalink raw reply related
* [PATCH 4/7] textconv: don't convert for every operation
From: Jeff King @ 2008-10-25 0:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Matthieu Moy, git
In-Reply-To: <20081025004815.GA23851@coredump.intra.peff.net>
Since we do the text conversion in fill_mmfile, the
conversion was happening any time we fed data to xdiff,
including diffstat, whitespace checking, etc.
This patch makes it optional for each caller, and
conservatively chooses to turn it on only for actual patch
generation.
Signed-off-by: Jeff King <peff@peff.net>
---
And this was part 2 of the big refactoring patch.
diff.c | 19 ++++++++++---------
t/t4030-diff-textconv.sh | 2 +-
2 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/diff.c b/diff.c
index 8fad215..fcdfd7b 100644
--- a/diff.c
+++ b/diff.c
@@ -292,7 +292,8 @@ static const char *get_textconv(struct diff_filespec *one)
return one->driver->textconv;
}
-static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)
+static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one,
+ int try_textconv)
{
const char *textconv;
@@ -304,7 +305,7 @@ static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)
else if (diff_populate_filespec(one, 0))
return -1;
- if ((textconv = get_textconv(one))) {
+ if (try_textconv && (textconv = get_textconv(one))) {
size_t size;
mf->ptr = run_textconv(textconv, one, &size);
if (!mf->ptr)
@@ -1396,7 +1397,7 @@ static void builtin_diff(const char *name_a,
}
}
- if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)
+ if (fill_mmfile(&mf1, one, 1) < 0 || fill_mmfile(&mf2, two, 1) < 0)
die("unable to read files to diff");
if (!DIFF_OPT_TST(o, TEXT) &&
@@ -1486,7 +1487,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
data->added = count_lines(two->data, two->size);
goto free_and_return;
}
- if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)
+ if (fill_mmfile(&mf1, one, 0) < 0 || fill_mmfile(&mf2, two, 0) < 0)
die("unable to read files to diff");
if (diff_filespec_is_binary(one) || diff_filespec_is_binary(two)) {
@@ -1528,7 +1529,7 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,
data.o = o;
data.ws_rule = whitespace_rule(attr_path);
- if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)
+ if (fill_mmfile(&mf1, one, 0) < 0 || fill_mmfile(&mf2, two, 0) < 0)
die("unable to read files to diff");
/*
@@ -2085,8 +2086,8 @@ static void run_diff(struct diff_filepair *p, struct diff_options *o)
if (DIFF_OPT_TST(o, BINARY)) {
mmfile_t mf;
- if ((!fill_mmfile(&mf, one) && diff_filespec_is_binary(one)) ||
- (!fill_mmfile(&mf, two) && diff_filespec_is_binary(two)))
+ if ((!fill_mmfile(&mf, one, 0) && diff_filespec_is_binary(one)) ||
+ (!fill_mmfile(&mf, two, 0) && diff_filespec_is_binary(two)))
abbrev = 40;
}
strbuf_addf(&msg, "index %.*s..%.*s",
@@ -2983,8 +2984,8 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)
diff_fill_sha1_info(p->one);
diff_fill_sha1_info(p->two);
- if (fill_mmfile(&mf1, p->one) < 0 ||
- fill_mmfile(&mf2, p->two) < 0)
+ if (fill_mmfile(&mf1, p->one, 0) < 0 ||
+ fill_mmfile(&mf2, p->two, 0) < 0)
return error("unable to read files to diff");
len1 = remove_space(p->one->path, strlen(p->one->path));
diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh
index af94e1a..090a21d 100755
--- a/t/t4030-diff-textconv.sh
+++ b/t/t4030-diff-textconv.sh
@@ -80,7 +80,7 @@ cat >expect.stat <<'EOF'
file | Bin 2 -> 4 bytes
1 files changed, 0 insertions(+), 0 deletions(-)
EOF
-test_expect_failure 'diffstat does not run textconv' '
+test_expect_success 'diffstat does not run textconv' '
echo file diff=fail >.gitattributes &&
git diff --stat HEAD^ HEAD >actual &&
test_cmp expect.stat actual
--
1.6.0.3.523.g38597.dirty
^ permalink raw reply related
* [PATCH 5/7] userdiff: require explicitly allowing textconv
From: Jeff King @ 2008-10-25 0:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Matthieu Moy, git
In-Reply-To: <20081025004815.GA23851@coredump.intra.peff.net>
Diffs that have been produced with textconv almost certainly
cannot be applied, so we want to be careful not to generate
them in things like format-patch.
This introduces a new diff options, ALLOW_TEXTCONV, which
controls this behavior. It is off by default, but is
explicitly turned on for the "log" family of commands, as
well as the "diff" porcelain.
Because both text conversion and external diffing are
controlled by these diff options, we can get rid of the
"plumbing versus porcelain" distinction when reading the
config. This was an attempt to control the same thing, but
suffered from being too coarse-grained.
Signed-off-by: Jeff King <peff@peff.net>
---
Same idea as before, but adapted to the new calling convention for
fill_mmfile. It makes the "did we textconv this?" test a little bit
more hairy. We could perhaps just set a "we did a textconv" flag on the
mmfile, but I am also fine with it as-is (especially since there is only
one call-site).
builtin-diff.c | 1 +
builtin-log.c | 1 +
diff.c | 27 ++++++++++++---------------
diff.h | 1 +
t/t4030-diff-textconv.sh | 2 +-
userdiff.c | 10 +---------
userdiff.h | 3 +--
7 files changed, 18 insertions(+), 27 deletions(-)
diff --git a/builtin-diff.c b/builtin-diff.c
index 9c8c295..2de5834 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -300,6 +300,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
}
DIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL);
DIFF_OPT_SET(&rev.diffopt, RECURSIVE);
+ DIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);
/*
* If the user asked for our exit code then don't start a
diff --git a/builtin-log.c b/builtin-log.c
index a0944f7..75d698f 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -59,6 +59,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
} else
die("unrecognized argument: %s", arg);
}
+ DIFF_OPT_SET(&rev->diffopt, ALLOW_TEXTCONV);
}
/*
diff --git a/diff.c b/diff.c
index fcdfd7b..e3bb1ed 100644
--- a/diff.c
+++ b/diff.c
@@ -93,12 +93,6 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
if (!strcmp(var, "diff.external"))
return git_config_string(&external_diff_cmd_cfg, var, value);
- switch (userdiff_config_porcelain(var, value)) {
- case 0: break;
- case -1: return -1;
- default: return 0;
- }
-
return git_diff_basic_config(var, value, cb);
}
@@ -109,6 +103,12 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
return 0;
}
+ switch (userdiff_config(var, value)) {
+ case 0: break;
+ case -1: return -1;
+ default: return 0;
+ }
+
if (!prefixcmp(var, "diff.color.") || !prefixcmp(var, "color.diff.")) {
int slot = parse_diff_color_slot(var, 11);
if (!value)
@@ -123,12 +123,6 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
return 0;
}
- switch (userdiff_config_basic(var, value)) {
- case 0: break;
- case -1: return -1;
- default: return 0;
- }
-
return git_color_default_config(var, value, cb);
}
@@ -1397,12 +1391,15 @@ static void builtin_diff(const char *name_a,
}
}
- if (fill_mmfile(&mf1, one, 1) < 0 || fill_mmfile(&mf2, two, 1) < 0)
+ if (fill_mmfile(&mf1, one, DIFF_OPT_TST(o, ALLOW_TEXTCONV)) < 0 ||
+ fill_mmfile(&mf2, two, DIFF_OPT_TST(o, ALLOW_TEXTCONV)) < 0)
die("unable to read files to diff");
if (!DIFF_OPT_TST(o, TEXT) &&
- ( (diff_filespec_is_binary(one) && !get_textconv(one)) ||
- (diff_filespec_is_binary(two) && !get_textconv(two)) )) {
+ ( (diff_filespec_is_binary(one) &&
+ !(DIFF_OPT_TST(o, ALLOW_TEXTCONV) && get_textconv(one))) ||
+ (diff_filespec_is_binary(two) &&
+ !(DIFF_OPT_TST(o, ALLOW_TEXTCONV) && get_textconv(two))) )) {
/* Quite common confusing case */
if (mf1.size == mf2.size &&
!memcmp(mf1.ptr, mf2.ptr, mf1.size))
diff --git a/diff.h b/diff.h
index a49d865..42582ed 100644
--- a/diff.h
+++ b/diff.h
@@ -65,6 +65,7 @@ typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,
#define DIFF_OPT_IGNORE_SUBMODULES (1 << 18)
#define DIFF_OPT_DIRSTAT_CUMULATIVE (1 << 19)
#define DIFF_OPT_DIRSTAT_BY_FILE (1 << 20)
+#define DIFF_OPT_ALLOW_TEXTCONV (1 << 21)
#define DIFF_OPT_TST(opts, flag) ((opts)->flags & DIFF_OPT_##flag)
#define DIFF_OPT_SET(opts, flag) ((opts)->flags |= DIFF_OPT_##flag)
#define DIFF_OPT_CLR(opts, flag) ((opts)->flags &= ~DIFF_OPT_##flag)
diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh
index 090a21d..1df48ae 100755
--- a/t/t4030-diff-textconv.sh
+++ b/t/t4030-diff-textconv.sh
@@ -70,7 +70,7 @@ test_expect_success 'log produces text' '
test_cmp expect.text actual
'
-test_expect_failure 'format-patch produces binary' '
+test_expect_success 'format-patch produces binary' '
git format-patch --no-binary --stdout HEAD^ >patch &&
find_diff <patch >actual &&
test_cmp expect.binary actual
diff --git a/userdiff.c b/userdiff.c
index d95257a..3681062 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -120,7 +120,7 @@ static int parse_tristate(int *b, const char *k, const char *v)
return 1;
}
-int userdiff_config_basic(const char *k, const char *v)
+int userdiff_config(const char *k, const char *v)
{
struct userdiff_driver *drv;
@@ -130,14 +130,6 @@ int userdiff_config_basic(const char *k, const char *v)
return parse_funcname(&drv->funcname, k, v, REG_EXTENDED);
if ((drv = parse_driver(k, v, "binary")))
return parse_tristate(&drv->binary, k, v);
-
- return 0;
-}
-
-int userdiff_config_porcelain(const char *k, const char *v)
-{
- struct userdiff_driver *drv;
-
if ((drv = parse_driver(k, v, "command")))
return parse_string(&drv->external, k, v);
if ((drv = parse_driver(k, v, "textconv")))
diff --git a/userdiff.h b/userdiff.h
index f29c18f..ba29457 100644
--- a/userdiff.h
+++ b/userdiff.h
@@ -14,8 +14,7 @@ struct userdiff_driver {
const char *textconv;
};
-int userdiff_config_basic(const char *k, const char *v);
-int userdiff_config_porcelain(const char *k, const char *v);
+int userdiff_config(const char *k, const char *v);
struct userdiff_driver *userdiff_find_by_name(const char *name);
struct userdiff_driver *userdiff_find_by_path(const char *path);
--
1.6.0.3.523.g38597.dirty
^ permalink raw reply related
* [PATCH 6/7] document the diff driver textconv feature
From: Jeff King @ 2008-10-25 0:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Matthieu Moy, git
In-Reply-To: <20081025004815.GA23851@coredump.intra.peff.net>
This patch also changes the term "custom diff driver" to
"external diff driver"; now that there are more facets of a
"custom driver" than just external diffing, it makes sense
to refer to the configuration of "diff.foo.*" as the "foo
diff driver", with "diff.foo.command" as the "external
driver for foo".
Signed-off-by: Jeff King <peff@peff.net>
---
Same as before.
Documentation/gitattributes.txt | 66 +++++++++++++++++++++++++++++++--------
1 files changed, 53 insertions(+), 13 deletions(-)
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 2694559..314e2d3 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -213,10 +213,12 @@ with `crlf`, and then `ident` and fed to `filter`.
Generating diff text
~~~~~~~~~~~~~~~~~~~~
-The attribute `diff` affects if 'git-diff' generates textual
-patch for the path or just says `Binary files differ`. It also
-can affect what line is shown on the hunk header `@@ -k,l +n,m @@`
-line.
+The attribute `diff` affects how 'git' generates diffs for particular
+files. It can tell git whether to generate a textual patch for the path
+or to treat the path as a binary file. It can also affect what line is
+shown on the hunk header `@@ -k,l +n,m @@` line, tell git to use an
+external command to generate the diff, or ask git to convert binary
+files to a text format before generating the diff.
Set::
@@ -227,7 +229,8 @@ Set::
Unset::
A path to which the `diff` attribute is unset will
- generate `Binary files differ`.
+ generate `Binary files differ` (or a binary patch, if
+ binary patches are enabled).
Unspecified::
@@ -238,21 +241,21 @@ Unspecified::
String::
- Diff is shown using the specified custom diff driver.
- The driver program is given its input using the same
- calling convention as used for GIT_EXTERNAL_DIFF
- program. This name is also used for custom hunk header
- selection.
+ Diff is shown using the specified diff driver. Each driver may
+ specify one or more options, as described in the following
+ section. The options for the diff driver "foo" are defined
+ by the configuration variables in the "diff.foo" section of the
+ git config file.
-Defining a custom diff driver
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+Defining an external diff driver
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
The definition of a diff driver is done in `gitconfig`, not
`gitattributes` file, so strictly speaking this manual page is a
wrong place to talk about it. However...
-To define a custom diff driver `jcdiff`, add a section to your
+To define an external diff driver `jcdiff`, add a section to your
`$GIT_DIR/config` file (or `$HOME/.gitconfig` file) like this:
----------------------------------------------------------------
@@ -328,6 +331,43 @@ patterns are available:
- `tex` suitable for source code for LaTeX documents.
+Performing text diffs of binary files
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Sometimes it is desirable to see the diff of a text-converted
+version of some binary files. For example, a word processor
+document can be converted to an ASCII text representation, and
+the diff of the text shown. Even though this conversion loses
+some information, the resulting diff is useful for human
+viewing (but cannot be applied directly).
+
+The `textconv` config option is used to define a program for
+performing such a conversion. The program should take a single
+argument, the name of a file to convert, and produce the
+resulting text on stdout.
+
+For example, to show the diff of the exif information of a
+file instead of the binary information (assuming you have the
+exif tool installed):
+
+------------------------
+[diff "jpg"]
+ textconv = exif
+------------------------
+
+NOTE: The text conversion is generally a one-way conversion;
+in this example, we lose the actual image contents and focus
+just on the text data. This means that diffs generated by
+textconv are _not_ suitable for applying. For this reason,
+only `git diff` and the `git log` family of commands (i.e.,
+log, whatchanged, show) will perform text conversion. `git
+format-patch` will never generate this output. If you want to
+send somebody a text-converted diff of a binary file (e.g.,
+because it quickly conveys the changes you have made), you
+should generate it separately and send it as a comment _in
+addition to_ the usual binary diff that you might send.
+
+
Performing a three-way merge
~~~~~~~~~~~~~~~~~~~~~~~~~~~~
--
1.6.0.3.523.g38597.dirty
^ permalink raw reply related
* [PATCH 7/7] only textconv regular files
From: Jeff King @ 2008-10-25 0:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Matthieu Moy, git
In-Reply-To: <20081025004815.GA23851@coredump.intra.peff.net>
We treat symlinks as text containing the results of the
symlink, so it doesn't make much sense to text-convert them.
Similarly gitlink components just end up as the text
"Subproject commit $sha1", which we should leave intact.
Note that a typechange may be broken into two parts: the
removal of the old part and the addition of the new. In that
case, we _do_ show the textconv for any part which is the
addition or removal of a file we would ordinarily textconv,
since it is purely acting on the file contents.
Signed-off-by: Jeff King <peff@peff.net>
---
Similar to before, except we already have the expect_failure test now.
diff.c | 2 ++
t/t4030-diff-textconv.sh | 2 +-
2 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/diff.c b/diff.c
index e3bb1ed..a87ce97 100644
--- a/diff.c
+++ b/diff.c
@@ -282,6 +282,8 @@ static const char *get_textconv(struct diff_filespec *one)
{
if (!DIFF_FILE_VALID(one))
return NULL;
+ if (!S_ISREG(one->mode))
+ return NULL;
diff_filespec_load_driver(one);
return one->driver->textconv;
}
diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh
index 1df48ae..3945731 100755
--- a/t/t4030-diff-textconv.sh
+++ b/t/t4030-diff-textconv.sh
@@ -104,7 +104,7 @@ index ad8b3d2..67be421
\ No newline at end of file
EOF
# make a symlink the hard way that works on symlink-challenged file systems
-test_expect_failure 'textconv does not act on symlinks' '
+test_expect_success 'textconv does not act on symlinks' '
echo -n frotz > file &&
git add file &&
git ls-files -s | sed -e s/100644/120000/ |
--
1.6.0.3.523.g38597.dirty
^ permalink raw reply related
* Re: Performance impact of a large number of commits
From: Samuel Abels @ 2008-10-25 5:18 UTC (permalink / raw)
To: david; +Cc: git
In-Reply-To: <alpine.DEB.1.10.0810241301430.27333@asgard.lang.hm>
On Fri, 2008-10-24 at 13:11 -0700, david@lang.hm wrote:
> > git commit explicitly (i.e., walking the tree to stat files for finding
> > changes is not necessary).
>
> I suspect that your limits would be filesystem/OS limits more than git
> limits
>
> at 5-10 files/commit you are going to be creating .5-1m files/day, even
> spread across 256 directories this is going to be a _lot_ of files.
The files are organized in a way that places no more than ~1.000 files
into each directory. Will Git create a directory containing a larger
number of object files? I can see that this would be a problem in our
use case.
> packing this may help (depending on how much the files change), but with
> this many files the work of doing the packing would be expensive.
We can probably do that even if it takes several hours.
-Samuel
^ permalink raw reply
* Re: Performance impact of a large number of commits
From: david @ 2008-10-25 5:29 UTC (permalink / raw)
To: Samuel Abels; +Cc: git
In-Reply-To: <1224911915.7566.35.camel@localhost>
On Sat, 25 Oct 2008, Samuel Abels wrote:
> On Fri, 2008-10-24 at 13:11 -0700, david@lang.hm wrote:
>>> git commit explicitly (i.e., walking the tree to stat files for finding
>>> changes is not necessary).
>>
>> I suspect that your limits would be filesystem/OS limits more than git
>> limits
>>
>> at 5-10 files/commit you are going to be creating .5-1m files/day, even
>> spread across 256 directories this is going to be a _lot_ of files.
>
> The files are organized in a way that places no more than ~1.000 files
> into each directory. Will Git create a directory containing a larger
> number of object files? I can see that this would be a problem in our
> use case.
when git stores the copies of the files it does a sha1 hash of the file
contents and then stores the file in the directory
.git/objects/<first two digits of the hash>/<hash>
this means that if you have files that have the same content they all fold
togeather, but with lots of files changing rapidly the result is a lot of
files in these object directories.
it would be a pretty minor change to git to have it use more directories
(in fact, there's another thread going on today where people are looking
at making this configurable, in that case to reduce the number of
directories)
the other storage format that git has is the pack file. it takes a bunch
of the objects, does some comparisons between them (to find duplicate bits
of files), and then stores the result (base files plus deltas to re-create
other files). the resulting compression is _extremely_ efficiant, and it
collapses many file objects into one pack file (addressing the issues of
many files in one directory)
>> packing this may help (depending on how much the files change), but with
>> this many files the work of doing the packing would be expensive.
>
> We can probably do that even if it takes several hours.
my concern is that spending time creating the pack files will mean that
you don't have time to insert the new files.
that being said, there may be other ways of dealing with this data rather
than putting it into files and then adding it to the git repository.
Git has a fast-import streaming format that is designed for programs to
use that are converting repositories from other SCM systems. if you can
tell more about what you are doing (how the data is being gathered, are
the files re-created for each commit, or are they being modified? if they
are being modified is it appending data, changing some data, or randomly
writing throughout the file? etc) there may be some other options
available.
at this point I don't know if git can work for you or not, but I'm pretty
sure nothing else will have a chance with your size.
David Lang
^ permalink raw reply
* Re: [PATCH 4/7] textconv: don't convert for every operation
From: Junio C Hamano @ 2008-10-25 5:41 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Sixt, Matthieu Moy, git
In-Reply-To: <20081025005256.GD23903@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> -static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)
> +static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one,
> + int try_textconv)
> {
> const char *textconv;
>
> @@ -304,7 +305,7 @@ static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)
> else if (diff_populate_filespec(one, 0))
> return -1;
>
> - if ((textconv = get_textconv(one))) {
> + if (try_textconv && (textconv = get_textconv(one))) {
> size_t size;
> mf->ptr = run_textconv(textconv, one, &size);
> if (!mf->ptr)
> @@ -1396,7 +1397,7 @@ static void builtin_diff(const char *name_a,
> }
> }
Isn't this function "fill_mmfile()" and its callers leaky as a sieve?
The original fill_mmfile() was to only borrow one->data and users of
mmfile_t never had to worry about freeing what's in mf->ptr (and freeing
them would have been actively wrong).
I am also somewhat worried about the performance impact of running
get_textconv() to the same filespec many times when no textconv is
defined, which is the normal case we should optimize for. It appears that
diff_filespec_load_driver() is optimized for a wrong case (i.e. "we
already know this needs a custom driver and we know which one").
I am inclined to suggest reverting the whole series (including the ones
that are already in 'master') and start over from scratch, limiting the
run_textconv() to only inside diff.c::builtin_diff() (in the else {} block
where "Crazy xcl interfaces.." comment appears).
^ permalink raw reply
* Re* [PATCH/RFC] Introduce a built-in attribute "text"
From: Junio C Hamano @ 2008-10-25 6:51 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git
In-Reply-To: <1224856547-30533-1-git-send-email-git@drmicha.warpmail.net>
Michael J Gruber <git@drmicha.warpmail.net> writes:
> "text is the opposite of "binary": It sets the attributes "crlf" and
> "diff". It is needed because attribute macros can't be negated,
If the reason for this patch is really (and only) because macros cannot be
negated, perhaps we should be allowing to negate them? That is, given:
[attr] binary = -diff -crlf
*.bar !binary
you would set diff and crlf to TRUE for b.bar, because the normal action
is to set them to FALSE.
If the original attribute macro expands to anything but setting other
attributes to TRUE or FALSE (i.e. to a string value, or reseting to
UNSPECIFIED), or if a related attribute is not the exact opposite, you
cannot simply negate, but need to introduce a new attribute like your
patch does. For example, it is conceivable you might instead want to
define:
[attr] text = diff
i.e. without "crlf" bit.
Since what you want to do here is merely to negate what is already there,
it might make more sense to simply bite the bullet and give the attribute
mechanism the ability to negate an attribute macro.
This is of course untested (you can tell that by noticing that it does not
come with patch to the test scripts) nor undocumented, but it looks
obvious enough ;-). Originally we silently ignored attempts to negate or
to reset an attribute macro. We now allow you to negate an attribute
macro, and the definition of "negating" is:
- negation of setting to TRUE is setting to FALSE.
- negation of setting to FALSE is setting to TRUE.
- negation of anything else is itself.
The third one is arbitrary and people may want to come up with a different
behaviour (with solid argument explaining why that behaviour is more
sensible).
attr.c | 34 +++++++++++++++++++++++++++++-----
1 files changed, 29 insertions(+), 5 deletions(-)
diff --git c/attr.c w/attr.c
index 17f6a4d..0021cec 100644
--- c/attr.c
+++ w/attr.c
@@ -558,7 +558,24 @@ static int path_matches(const char *pathname, int pathlen,
return fnmatch(pattern, pathname + baselen, FNM_PATHNAME) == 0;
}
-static int fill_one(const char *what, struct match_attr *a, int rem)
+static const char *attr_negate(const char *value)
+{
+ if (value == ATTR__TRUE)
+ return ATTR__FALSE;
+ else if (value == ATTR__FALSE)
+ return ATTR__TRUE;
+ else
+ /*
+ * NEEDSWORK: here we define negation of setting
+ * to anything but TRUE and FALSE is itself. We
+ * may want to instead say it is not touching the
+ * attribute, in which case you could simply return
+ * NULL from here.
+ */
+ return value;
+}
+
+static int fill_one(const char *what, struct match_attr *a, int rem, int negated)
{
struct git_attr_check *check = check_all_attr;
int i;
@@ -569,6 +586,11 @@ static int fill_one(const char *what, struct match_attr *a, int rem)
const char *v = a->state[i].setto;
if (*n == ATTR__UNKNOWN) {
+ if (negated) {
+ v = attr_negate(v);
+ if (!v)
+ continue;
+ }
debug_set(what, a->u.pattern, attr, v);
*n = v;
rem--;
@@ -588,7 +610,7 @@ static int fill(const char *path, int pathlen, struct attr_stack *stk, int rem)
continue;
if (path_matches(path, pathlen,
a->u.pattern, base, strlen(base)))
- rem = fill_one("fill", a, rem);
+ rem = fill_one("fill", a, rem, 0);
}
return rem;
}
@@ -602,9 +624,11 @@ static int macroexpand(struct attr_stack *stk, int rem)
struct match_attr *a = stk->attrs[i];
if (!a->is_macro)
continue;
- if (check[a->u.attr->attr_nr].value != ATTR__TRUE)
- continue;
- rem = fill_one("expand", a, rem);
+ if (check[a->u.attr->attr_nr].value == ATTR__TRUE)
+ rem = fill_one("expand", a, rem, 0);
+ else if (check[a->u.attr->attr_nr].value == ATTR__FALSE)
+ rem = fill_one("expand", a, rem, 1);
+
}
return rem;
}
^ permalink raw reply related
* Re: [PATCH 4/7] textconv: don't convert for every operation
From: Jeff King @ 2008-10-25 7:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Matthieu Moy, git
In-Reply-To: <7vhc71b5ai.fsf@gitster.siamese.dyndns.org>
On Fri, Oct 24, 2008 at 10:41:09PM -0700, Junio C Hamano wrote:
> Isn't this function "fill_mmfile()" and its callers leaky as a sieve?
Yes, it is (for the textconv case; the other is unaffected). That was
another reason I had attached the data to the diff_filespec, but
obviously I forgot about that when re-rolling the patch.
> I am also somewhat worried about the performance impact of running
> get_textconv() to the same filespec many times when no textconv is
> defined, which is the normal case we should optimize for. It appears that
> diff_filespec_load_driver() is optimized for a wrong case (i.e. "we
> already know this needs a custom driver and we know which one").
No, it is the same as before. We always end up with a driver at the end
of the function, so further calls will be no-ops. So we do exactly one
attribute lookup per filespec, caching the result.
> I am inclined to suggest reverting the whole series (including the ones
> that are already in 'master') and start over from scratch, limiting the
> run_textconv() to only inside diff.c::builtin_diff() (in the else {} block
> where "Crazy xcl interfaces.." comment appears).
I am not terribly opposed to that (I was quite surprised to see the
original make it to 'next', let alone 'master'). OTOH, the real reason
to do so would be to keep a clean history, and it is already too late
for that.
I think it makes sense to figure out _what_ needs fixed first, because
it might be somewhat minor. So far I see:
- leak from fill_mmfile; this definitely needs fixed. The quick fix is
minor (free if we did a textconv). A more involved fix is to pull it
out of fill_mmfile entirely and put the code directly into
builtin_diff, which would be part of a re-roll of this latest
series. But see below.
- Keep fill_mmfile allocation semantics clear. I was trying to keep
it simple for other fill_mmfile callers to opt-in to textconv, even
if they chose to do it by some user-controlled mechanism instead of
by default (e.g., diff.TextconvDiffstat or something). But maybe
that is not of value to us. Again, that is a re-roll of this series.
- performance considerations with driver loading. I believe this is a
non-issue. So either you are reading the code wrong, or I am not
understanding your concern correctly (or _I_ am reading the code
wrong, of course).
Others?
-Peff
^ permalink raw reply
* Re: [RFC] Zit: the git-based single file content tracker
From: Giuseppe Bilotta @ 2008-10-25 7:48 UTC (permalink / raw)
To: Jakub Narebski; +Cc: David Lang, Junio C Hamano, Jean-Luc Herren, git
In-Reply-To: <200810242230.49238.jnareb@gmail.com>
On Fri, Oct 24, 2008 at 10:30 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> Well, with all zit repositories in '.zit/' directory (similar to RCS/)
> you could have point core.excludesfile to _common_ '.zit/excludes';
> the pattern doesn't change from zit repository to zit repository?
>
> You could even use per-user ~/.zitignore (I'm not sure if git expands
> '~' in paths; there was some patch for it, but was it accepted?) or
> system-wide /usr/lib/zitignore or /usr/libexec/zitignore file.
System-wide means maximum space save, but it require system
administration to install Zit, and considering that one of the things
I love of Zit now is its being self contained, I would rather not
depend on anything system-wide anyway.
The user .zitignore file is probably the best approach: we can create
it ourselves (usually), and even if Git doesn't expand the pathname
itself, we can just use an absolute path. I'll go that way.
--
Giuseppe "Oblomov" Bilotta
^ permalink raw reply
* Re: [RFC] Zit: the git-based single file content tracker
From: Jakub Narebski @ 2008-10-25 9:10 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: David Lang, Junio C Hamano, Jean-Luc Herren, git
In-Reply-To: <cb7bb73a0810250048q7ad8595bt565de05ec2ec37cb@mail.gmail.com>
On Sat, 25 Oct 2008, Giuseppe Bilotta wrote:
> On Fri, Oct 24, 2008 at 10:30 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> > Well, with all zit repositories in '.zit/' directory (similar to RCS/)
> > you could have point core.excludesfile to _common_ '.zit/excludes';
> > the pattern doesn't change from zit repository to zit repository?
> >
> > You could even use per-user ~/.zitignore (I'm not sure if git expands
> > '~' in paths; there was some patch for it, but was it accepted?) [...]
[...]
> The user .zitignore file is probably the best approach: we can create
> it ourselves (usually), and even if Git doesn't expand the pathname
> itself, we can just use an absolute path. I'll go that way.
First, absolute path to ~/.zitignore is a bit fragile: what if layout
of home directories for users change, for example because of increasing
number of users some fan-out is required (/home/nick -> /home/2/nick)?
Second, ~/.zitignore looks like something that user can change; if
you install zit, it can install libexec/zitignore somewhere... or just
use ./zit/excludes (with 'do not edit' comment perhaps...).
--
Jakub Narebski
Poland
^ permalink raw reply
* Problem with git filter-branch
From: Pascal Obry @ 2008-10-25 9:33 UTC (permalink / raw)
To: git list
I'm using Git svn but I do not think this matters.
I'm used to add manually some branches from the Subversion repository. I
usually fetch the branch without following the parent then connect the
branch to the proper branch point (I really need to do this as the
Subversion repository structure is not following the standard layout).
Anyway, I used to run the following command:
$ git filter-branch --tag-name-filter cat --parent-filter "sed -e
's/^$/-p c96d4da294667de1800687d25340551683153002/'" svn-release_2_6
without problem, I now get this:
Namespace refs/original/ not empty
rm: cannot remove directory
`/home/obry/dev/repositories/git/proj/.git-rewrite': Directory not empty
The .git-rewrite is empty and when the command return I can do:
$ rmdir .git-rewrite
without problem.
Is this is known issue? Any idea?
Thanks,
Pascal.
--
--|------------------------------------------------------
--| Pascal Obry Team-Ada Member
--| 45, rue Gabriel Peri - 78114 Magny Les Hameaux FRANCE
--|------------------------------------------------------
--| http://www.obry.net
--| "The best way to travel is by means of imagination"
--|
--| gpg --keyserver wwwkeys.pgp.net --recv-key C1082595
^ permalink raw reply
* Re: [PATCH 2/3] Add -n/--no-prompt option to mergetool
From: William Pursell @ 2008-10-25 10:11 UTC (permalink / raw)
To: Charles Bailey
Cc: Junio C Hamano, git, Jeff King, Andreas Ericsson,
Theodore Ts'o
In-Reply-To: <20081024225539.GA6119@hashpling.org>
Charles Bailey wrote:
> On Fri, Oct 24, 2008 at 11:32:17PM +0100, William Pursell wrote:
>> If the short option is dropped, the config option should
>> probably associated with mergetool.<tool>.interactive rather
>> than mergetool.interactive. (s/interactive/whatever)
>
> I'm not sure I understand your reasoning. The no-prompt/interactive
> option affects the behaviour of the mergetool script independent of
> which particular merge tool is being used. Why should the presence or
> absence of a short option affect whether the config option is global
> or per tool?
>
My thinking is that when using an interactive tool
like vimdiff, the user is probably not going to
care as much about being prompted, or may prefer
to have the prompt in that situation. However,
if they've written a script to do the merge
non-interactively, then the prompt is undesirable.
So a person might want to be prompted with
git mergetool -t vimdiff, and prefer no prompt
with git mergetool -t my-script. Being able
to configure the behavior on a per-tool
basis would allow that.
--
William Pursell
^ permalink raw reply
* Re: [RFC] Zit: the git-based single file content tracker
From: Giuseppe Bilotta @ 2008-10-25 10:30 UTC (permalink / raw)
To: Jakub Narebski; +Cc: David Lang, Junio C Hamano, Jean-Luc Herren, git
In-Reply-To: <200810251110.25704.jnareb@gmail.com>
On Sat, Oct 25, 2008 at 11:10 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Sat, 25 Oct 2008, Giuseppe Bilotta wrote:
>
>> The user .zitignore file is probably the best approach: we can create
>> it ourselves (usually), and even if Git doesn't expand the pathname
>> itself, we can just use an absolute path. I'll go that way.
>
> First, absolute path to ~/.zitignore is a bit fragile: what if layout
> of home directories for users change, for example because of increasing
> number of users some fan-out is required (/home/nick -> /home/2/nick)?
> Second, ~/.zitignore looks like something that user can change; if
> you install zit, it can install libexec/zitignore somewhere... or just
> use ./zit/excludes (with 'do not edit' comment perhaps...).
(Actually, I just found another interesting thing about the config, in
that it stores the path to the work tree. This is not a problem,
though, because zit_setup() sets GIT_WORK_TREE.)
As I said, I don't like depending on stuff that needs to be installed.
For example, what about user (non-system) installs? the libexec (or
whatever) solution would have the same problem as the ~/.zitignore
solution, with the moving $HOME.
I guess this leaves the .zit/ solution as the most robust one,
although it's not the most space-effective, especially if you have
many directories, each with a single tracked file. On the plus side,
going for the .zit/ solution and dropping support for .somefile.git/
means some significant code semplification.
--
Giuseppe "Oblomov" Bilotta
^ 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