* Re: [PATCH 2/2] Don't use excessive non-standard border width
From: Aneesh Kumar K.V @ 2006-02-28 5:16 UTC (permalink / raw)
To: Pavel Roskin; +Cc: git
In-Reply-To: <20060228045631.21880.27670.stgit@dv.roinet.com>
On Mon, Feb 27, 2006 at 11:56:31PM -0500, Pavel Roskin wrote:
> From: Pavel Roskin <proski@gnu.org>
>
>
> ---
>
> contrib/gitview/gitview | 2 --
> 1 files changed, 0 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/gitview/gitview b/contrib/gitview/gitview
> index aded7ed..00cb8ee 100755
> --- a/contrib/gitview/gitview
> +++ b/contrib/gitview/gitview
> @@ -492,7 +492,6 @@ class GitView:
> def construct_top(self):
> """Construct the top-half of the window."""
> vbox = gtk.VBox(spacing=6)
> - vbox.set_border_width(12)
> vbox.show()
>
> menu_bar = gtk.MenuBar()
> @@ -574,7 +573,6 @@ class GitView:
> def construct_bottom(self):
> """Construct the bottom half of the window."""
> vbox = gtk.VBox(False, spacing=6)
> - vbox.set_border_width(12)
> (width, height) = self.window.get_size()
> vbox.set_size_request(width, int(height / 2.5))
> vbox.show()
I am not sure about this. I guess it looks much better with a border.
Not applied.
-aneesh
^ permalink raw reply
* Re: [PATCH 1/2] Use solid text for refs in gitview
From: Aneesh Kumar K.V @ 2006-02-28 5:15 UTC (permalink / raw)
To: Pavel Roskin; +Cc: git
In-Reply-To: <20060228045629.21880.19007.stgit@dv.roinet.com>
On Mon, Feb 27, 2006 at 11:56:29PM -0500, Pavel Roskin wrote:
> From: Pavel Roskin <proski@gnu.org>
>
> Select the text color based on whether the entry in highlighted. Use
> standard font.
> ---
Applied
-aneesh
^ permalink raw reply
* Re: [PATCH 3/3] git-apply --whitespace=nowarn
From: Junio C Hamano @ 2006-02-28 5:08 UTC (permalink / raw)
To: A Large Angry SCM; +Cc: git, Andrew Morton
In-Reply-To: <4403C303.70602@gmail.com>
A Large Angry SCM <gitzilla@gmail.com> writes:
> Junio C Hamano wrote:
>> Andrew insists --whitespace=warn should be the default, and I
>> tend to agree. This introduces --whitespace=warn, so if your
>> project policy is more lenient, you can squelch them by having
>> apply.whitespace=nowarn in your configuration file.
>> Signed-off-by: Junio C Hamano <junkio@cox.net>
>
> I think this is wrong. The default policy of, non-security, tools
> SHOULD be the least restrictive and most flexible.
The reason is that --whitespace=warn does not refuse to apply
but spits out some warning messages. Admittedly, going from
zero, any amount of new warning message makes it infinitely more
chatty, but then people can squelch it by having the
configuration option "apply.whitespace = nowarn".
^ permalink raw reply
* [PATCH] diff-delta: allow reusing of the reference buffer index
From: Nicolas Pitre @ 2006-02-28 4:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
When a reference buffer is used multiple times then its index can be
computed only once and reused multiple times. This patch adds an extra
pointer to a pointer argument (from_index) to diff_delta() for this.
If from_index is NULL then everything is like before.
If from_index is non NULL and *from_index is NULL then the index is
created and its location stored to *from_index. In this case the caller
has the responsibility to free the memory pointed to by *from_index.
If from_index and *from_index are non NULL then the index is reused as
is.
This currently saves about 10% of CPU time to repack the git archive.
Signed-off-by: Nicolas Pitre <nico@cam.org>
---
This is partly what Linus suggested recently. However I only made the
minimum changes to pack-objects.c to try it out and demonstrate how it
works. I however don't feel confortable enough with the changes
required to implement the full solution which is to turn the window
processing around i.e. keep the reference buffer stable while different
targets are tested against this reference. This is something worth
doing since that would reduce memory usage significantly. But Like I
said, pack-objects as grown more complex with Junio latest additions
so...
delta.h | 3 ++-
diff-delta.c | 41 +++++++++++++++++++++++++++--------------
diffcore-break.c | 2 +-
diffcore-rename.c | 2 +-
pack-objects.c | 11 ++++++++---
test-delta.c | 2 +-
6 files changed, 40 insertions(+), 21 deletions(-)
f660c44250f5c02a7e773ed991316f7f16950f3e
diff --git a/delta.h b/delta.h
index a15350d..00fef0b 100644
--- a/delta.h
+++ b/delta.h
@@ -4,7 +4,8 @@
/* handling of delta buffers */
extern void *diff_delta(void *from_buf, unsigned long from_size,
void *to_buf, unsigned long to_size,
- unsigned long *delta_size, unsigned long max_size);
+ unsigned long *delta_size, unsigned long max_size,
+ void **from_index);
extern void *patch_delta(void *src_buf, unsigned long src_size,
void *delta_buf, unsigned long delta_size,
unsigned long *dst_size);
diff --git a/diff-delta.c b/diff-delta.c
index bb07926..67561cd 100644
--- a/diff-delta.c
+++ b/diff-delta.c
@@ -30,8 +30,7 @@ struct index {
static struct index ** delta_index(const unsigned char *buf,
unsigned long bufsize,
- unsigned long trg_bufsize,
- unsigned int *hash_shift)
+ unsigned long trg_bufsize)
{
unsigned long hsize;
unsigned int i, hshift, hlimit, *hash_count;
@@ -44,14 +43,17 @@ static struct index ** delta_index(const
for (i = 8; (1 << i) < hsize && i < 24; i += 2);
hsize = 1 << i;
hshift = (i - 8) / 2;
- *hash_shift = hshift;
- /* allocate lookup index */
- mem = malloc(hsize * sizeof(*hash) + bufsize * sizeof(*entry));
+ /*
+ * Allocate lookup index. Note the first hash pointer
+ * is used to store the hash shift value.
+ */
+ mem = malloc((1 + hsize) * sizeof(*hash) + bufsize * sizeof(*entry));
if (!mem)
return NULL;
hash = mem;
- entry = mem + hsize * sizeof(*hash);
+ *hash++ = (void *)hshift;
+ entry = mem + (1 + hsize) * sizeof(*hash);
memset(hash, 0, hsize * sizeof(*hash));
/* allocate an array to count hash entries */
@@ -107,7 +109,7 @@ static struct index ** delta_index(const
}
free(hash_count);
- return hash;
+ return hash-1;
}
/* provide the size of the copy opcode given the block offset and size */
@@ -121,7 +123,8 @@ static struct index ** delta_index(const
void *diff_delta(void *from_buf, unsigned long from_size,
void *to_buf, unsigned long to_size,
unsigned long *delta_size,
- unsigned long max_size)
+ unsigned long max_size,
+ void **from_index)
{
unsigned int i, outpos, outsize, inscnt, hash_shift;
const unsigned char *ref_data, *ref_top, *data, *top;
@@ -130,9 +133,16 @@ void *diff_delta(void *from_buf, unsigne
if (!from_size || !to_size)
return NULL;
- hash = delta_index(from_buf, from_size, to_size, &hash_shift);
- if (!hash)
- return NULL;
+ if (from_index && *from_index) {
+ hash = *from_index;
+ } else {
+ hash = delta_index(from_buf, from_size, to_size);
+ if (!hash)
+ return NULL;
+ if (from_index)
+ *from_index = hash;
+ }
+ hash_shift = (unsigned int)(*hash++);
outpos = 0;
outsize = 8192;
@@ -140,7 +150,8 @@ void *diff_delta(void *from_buf, unsigne
outsize = max_size + MAX_OP_SIZE + 1;
out = malloc(outsize);
if (!out) {
- free(hash);
+ if (!from_index)
+ free(hash-1);
return NULL;
}
@@ -241,7 +252,8 @@ void *diff_delta(void *from_buf, unsigne
out = realloc(out, outsize);
if (!out) {
free(tmp);
- free(hash);
+ if (!from_index)
+ free(hash-1);
return NULL;
}
}
@@ -250,7 +262,8 @@ void *diff_delta(void *from_buf, unsigne
if (inscnt)
out[outpos - inscnt - 1] = inscnt;
- free(hash);
+ if (!from_index)
+ free(hash-1);
*delta_size = outpos;
return out;
}
diff --git a/diffcore-break.c b/diffcore-break.c
index c57513a..6dc22d5 100644
--- a/diffcore-break.c
+++ b/diffcore-break.c
@@ -67,7 +67,7 @@ static int should_break(struct diff_file
delta = diff_delta(src->data, src->size,
dst->data, dst->size,
- &delta_size, 0);
+ &delta_size, 0, NULL);
if (!delta)
return 0; /* error but caught downstream */
diff --git a/diffcore-rename.c b/diffcore-rename.c
index 39d9126..5b4cde4 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -168,7 +168,7 @@ static int estimate_similarity(struct di
delta_limit = base_size * (MAX_SCORE-minimum_score) / MAX_SCORE;
delta = diff_delta(src->data, src->size,
dst->data, dst->size,
- &delta_size, delta_limit);
+ &delta_size, delta_limit, NULL);
if (!delta)
/* If delta_limit is exceeded, we have too much differences */
return 0;
diff --git a/pack-objects.c b/pack-objects.c
index ceb107f..2b2d9a9 100644
--- a/pack-objects.c
+++ b/pack-objects.c
@@ -202,7 +202,7 @@ static void *delta_against(void *buf, un
if (!otherbuf)
die("unable to read %s", sha1_to_hex(entry->delta->sha1));
delta_buf = diff_delta(otherbuf, othersize,
- buf, size, &delta_size, 0);
+ buf, size, &delta_size, 0, NULL);
if (!delta_buf || delta_size != entry->delta_size)
die("delta size changed");
free(buf);
@@ -707,6 +707,7 @@ static int type_size_sort(const struct o
struct unpacked {
struct object_entry *entry;
void *data;
+ void **delta_index;
};
/*
@@ -789,7 +790,8 @@ static int try_delta(struct unpacked *cu
if (sizediff >= max_size)
return -1;
delta_buf = diff_delta(old->data, oldsize,
- cur->data, size, &delta_size, max_size);
+ cur->data, size, &delta_size,
+ max_size, old->delta_index);
if (!delta_buf)
return 0;
cur_entry->delta = old_entry;
@@ -830,6 +832,7 @@ static void find_deltas(struct object_en
*/
continue;
+ free(n->delta_index);
free(n->data);
n->entry = entry;
n->data = read_sha1_file(entry->sha1, type, &size);
@@ -853,8 +856,10 @@ static void find_deltas(struct object_en
idx = 0;
}
- for (i = 0; i < window; ++i)
+ for (i = 0; i < window; ++i) {
+ free(array[i].delta_index);
free(array[i].data);
+ }
free(array);
}
diff --git a/test-delta.c b/test-delta.c
index 1be8ee0..89eb68e 100644
--- a/test-delta.c
+++ b/test-delta.c
@@ -63,7 +63,7 @@ int main(int argc, char *argv[])
if (argv[1][1] == 'd')
out_buf = diff_delta(from_buf, from_size,
data_buf, data_size,
- &out_size, 0);
+ &out_size, 0, NULL);
else
out_buf = patch_delta(from_buf, from_size,
data_buf, data_size,
--
1.2.3.g8fcf1-dirty
^ permalink raw reply related
* [PATCH] diff-delta: bound hash list length to avoid O(m*n) behavior
From: Nicolas Pitre @ 2006-02-28 4:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
The diff-delta code can exhibit O(m*n) behavior with some patological
data set where most hash entries end up in the same hash bucket.
The latest code rework reduced the block size making it particularly
vulnerable to this issue, but the issue was always there and can be
triggered regardless of the block size.
This patch does two things:
1) the hashing has been reworked to offer a better distribution to
atenuate the problem a bit, and
2) a limit is imposed to the number of entries that can exist in the
same hash bucket.
Because of the above the code is a bit more expensive on average, but
the problematic samples used to diagnoze the issue are now orders of
magnitude less expensive to process with only a slight loss in
compression.
Signed-off-by: Nicolas Pitre <nico@cam.org>
---
For example, Carl Baldwin provided me with a couple 20MB files, and
deltifying one against another one with test-delta takes around
SEVENTEEN MINUTES for only one delta on my P4 @ 3GHz when using the
original adler32 version with 16 byte blocks. With the latest delta
code in the pu branch it simply takes forever (I interrupted it after 20
minutes of processing). now imagine using git-repack -a with a window
of 10. With this patch on top of the small block patch (still in the pu
branch) this time dropped to only 10 seconds. And the resulting delta
is still smaller than the 16 byte adler32 based version:
blob 02220dae8cd219916ce52a12cda67322659e0e57 ->
blob af8f99dd11ca288f4e4a08f2626af2de8b3ecfd2
size (21187748 -> 19424744)
delta size (16 byte blocks): 5238542 in 16m55
delta size (3 byte blocks): [interrupted after 20 minutes]
delta size (3 byte blocks + this patch): 4910988 in 9.69 secs
Other data points from the Linux kernel repository:
blob 9af06ba723df75fed49f7ccae5b6c9c34bc5115f ->
blob dfc9cd58dc065d17030d875d3fea6e7862ede143
size (491102 -> 496045)
delta size (16 byte blocks): 248899 in less than 0.1 sec
delta size (3 byte blocks): 136000 in 11.8 secs
delta size (3 byte blocks + this patch): 171688 in 0.79 sec
blob 4917ec509720a42846d513addc11cbd25e0e3c4f ->
blob dfc9cd58dc065d17030d875d3fea6e7862ede143
size (495831 -> 496045)
delta size (16 byte blocks): 226218 in less than 0.1 sec
delta size (3 byte blocks): 120948 in 11.7 secs
delta size (3 byte blocks + this patch): 157135 in 0.77 sec
diff-delta.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++-----------
1 files changed, 56 insertions(+), 13 deletions(-)
1682f4b1b2899288d7761844a4cfd02772c464d1
diff --git a/diff-delta.c b/diff-delta.c
index 27f83a0..bb07926 100644
--- a/diff-delta.c
+++ b/diff-delta.c
@@ -30,19 +30,20 @@ struct index {
static struct index ** delta_index(const unsigned char *buf,
unsigned long bufsize,
+ unsigned long trg_bufsize,
unsigned int *hash_shift)
{
unsigned long hsize;
- unsigned int hshift, i;
+ unsigned int i, hshift, hlimit, *hash_count;
const unsigned char *data;
struct index *entry, **hash;
void *mem;
/* determine index hash size */
hsize = bufsize / 4;
- for (i = 8; (1 << i) < hsize && i < 16; i++);
+ for (i = 8; (1 << i) < hsize && i < 24; i += 2);
hsize = 1 << i;
- hshift = i - 8;
+ hshift = (i - 8) / 2;
*hash_shift = hshift;
/* allocate lookup index */
@@ -53,15 +54,59 @@ static struct index ** delta_index(const
entry = mem + hsize * sizeof(*hash);
memset(hash, 0, hsize * sizeof(*hash));
- /* then populate it */
+ /* allocate an array to count hash entries */
+ hash_count = calloc(hsize, sizeof(*hash_count));
+ if (!hash_count) {
+ free(hash);
+ return NULL;
+ }
+
+ /* then populate the index */
data = buf + bufsize - 2;
while (data > buf) {
entry->ptr = --data;
- i = data[0] ^ data[1] ^ (data[2] << hshift);
+ i = data[0] ^ ((data[1] ^ (data[2] << hshift)) << hshift);
entry->next = hash[i];
hash[i] = entry++;
+ hash_count[i]++;
}
+ /*
+ * Determine a limit on the number of entries in the same hash
+ * bucket. This guard us against patological data sets causing
+ * really bad hash distribution with most entries in the same hash
+ * bucket that would bring us to O(m*n) computing costs (m and n
+ * corresponding to reference and target buffer sizes).
+ *
+ * The more the target buffer is large, the more it is important to
+ * have small entry lists for each hash buckets. With such a limit
+ * the cost is bounded to something more like O(m+n).
+ */
+ hlimit = (1 << 26) / trg_bufsize;
+ if (hlimit < 16)
+ hlimit = 16;
+
+ /*
+ * Now make sure none of the hash buckets has more entries than
+ * we're willing to test. Otherwise we short-circuit the entry
+ * list uniformly to still preserve a good repartition across
+ * the reference buffer.
+ */
+ for (i = 0; i < hsize; i++) {
+ if (hash_count[i] < hlimit)
+ continue;
+ entry = hash[i];
+ do {
+ struct index *keep = entry;
+ int skip = hash_count[i] / hlimit / 2;
+ do {
+ entry = entry->next;
+ } while(--skip && entry);
+ keep->next = entry;
+ } while(entry);
+ }
+ free(hash_count);
+
return hash;
}
@@ -85,7 +130,7 @@ void *diff_delta(void *from_buf, unsigne
if (!from_size || !to_size)
return NULL;
- hash = delta_index(from_buf, from_size, &hash_shift);
+ hash = delta_index(from_buf, from_size, to_size, &hash_shift);
if (!hash)
return NULL;
@@ -126,8 +171,8 @@ void *diff_delta(void *from_buf, unsigne
while (data < top) {
unsigned int moff = 0, msize = 0;
- if (data + 2 < top) {
- i = data[0] ^ data[1] ^ (data[2] << hash_shift);
+ if (data + 3 <= top) {
+ i = data[0] ^ ((data[1] ^ (data[2] << hash_shift)) << hash_shift);
for (entry = hash[i]; entry; entry = entry->next) {
const unsigned char *ref = entry->ptr;
const unsigned char *src = data;
@@ -138,11 +183,9 @@ void *diff_delta(void *from_buf, unsigne
ref_size = 0x10000;
if (ref_size <= msize)
break;
- while (ref_size && *src++ == *ref) {
- ref++;
- ref_size--;
- }
- ref_size = ref - entry->ptr;
+ if (*ref != *src)
+ continue;
+ while (ref_size-- && *++src == *++ref);
if (msize < ref - entry->ptr) {
/* this is our best match so far */
msize = ref - entry->ptr;
--
1.2.3.g8fcf1-dirty
^ permalink raw reply related
* Re: [PATCH 3/3] git-apply --whitespace=nowarn
From: A Large Angry SCM @ 2006-02-28 3:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Andrew Morton
In-Reply-To: <7v4q2kuvxk.fsf@assigned-by-dhcp.cox.net>
Junio C Hamano wrote:
> Andrew insists --whitespace=warn should be the default, and I
> tend to agree. This introduces --whitespace=warn, so if your
> project policy is more lenient, you can squelch them by having
> apply.whitespace=nowarn in your configuration file.
>
> Signed-off-by: Junio C Hamano <junkio@cox.net>
I think this is wrong. The default policy of, non-security, tools SHOULD
be the least restrictive and most flexible.
^ permalink raw reply
* someone changed the contents of my HEAD.
From: Dave Jones @ 2006-02-28 3:04 UTC (permalink / raw)
To: git
I just tried to check in some changes to some trees
on master.kernel.org, and found after the first checkin
that git claimed..
fatal: Not a git repository
A lot of head-scratching later, I think I've figured out
what's happened. It seems there was a recent upgrade
to the version of git on m.k.o, which is incompatible
with the helper scripts I used before.
When checking in changes previously, I used this..
#!/bin/sh
export GIT_AUTHOR_NAME="$1"
export GIT_AUTHOR_EMAIL="$2"
tree=$(git-write-tree) || exit 1
commit=$(git-commit-tree $tree -p HEAD) || exit 1
echo $commit > .git/HEAD
and called it thus..
commit-as "Dave Jones" "<davej@redhat.com>"
Previously, this updated .git/HEAD to a ptr to the latest committed change.
All was well, as I only ever have one HEAD in my trees.
With the new .git however, when I clone a new tree, .git/HEAD
contains ref: refs/heads/master, so my script destroys the git metadata.
For my newly created repos, this isn't a problem, as I can fudge my
commit-as script to write to .git/refs/heads/master instead, but
my concern now is the unpulled changes in the existing repos
I have on master. Will Linus be able to pull those into his tree
with git 1.2.3, or will I have to recreate those repos with the
new-style .git/HEAD ?
Dave
^ permalink raw reply
* Re: Quick question: how to generate a patch?
From: Aubrey @ 2006-02-28 2:48 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Andreas Ericsson, git
In-Reply-To: <Pine.LNX.4.64.0602271802110.22647@g5.osdl.org>
> If you can re-create this at will, it would be interesting to see _what_
> makes git think you've modified a file. A small mod to "read-cache.c" like
> the appended patch should give you (very very verbose) output that could
> help us figure this out.
Oops, It's my fault. My mistake is that copy the clone git directory
to another directory for backup. And work on another directory. Sorry
for my incaution.
-Aubrey
^ permalink raw reply
* Re: Quick question: how to generate a patch?
From: Linus Torvalds @ 2006-02-28 2:09 UTC (permalink / raw)
To: Aubrey; +Cc: Andreas Ericsson, git
In-Reply-To: <6d6a94c50602271755v41e0d31ch25892192547003a9@mail.gmail.com>
On Tue, 28 Feb 2006, Aubrey wrote:
>
> I'm using suse9.3. The filesystem is EXT3.
Ok, something else is going on. There's no way ext3 can get confused about
times that I can see.
> I think I forgot one thing last night. When I changed the file, I
> compiled the package to verify my modification. It should be the
> reason. But should it really affect the result of "git diff"?
Nope. Something else is happening.
If you can re-create this at will, it would be interesting to see _what_
makes git think you've modified a file. A small mod to "read-cache.c" like
the appended patch should give you (very very verbose) output that could
help us figure this out.
Linus
----
diff --git a/read-cache.c b/read-cache.c
index f97f92d..4946163 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -183,6 +183,8 @@ int ce_match_stat(struct cache_entry *ce
index_file_timestamp <= ntohl(ce->ce_mtime.sec))
changed |= ce_modified_check_fs(ce, st);
+if (changed) error("changed: 0x%x %s", changed, ce->name);
+
return changed;
}
^ permalink raw reply related
* Re: Quick question: how to generate a patch?
From: Aubrey @ 2006-02-28 1:55 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Andreas Ericsson, git
In-Reply-To: <Pine.LNX.4.64.0602270923120.22647@g5.osdl.org>
On 2/28/06, Linus Torvalds <torvalds@osdl.org> wrote:
> I could well imagine that we still have some bug like that (ctime in
> particular is much less used than mtime, and thus more likely to have not
> been noticed). And it could be much worse on some less-commonly-used and
> less-unixy networked filesystem like smb, which is why I was wondering
> what OS version and filesystem Aubrey might be using.
I'm using suse9.3. The filesystem is EXT3.
I think I forgot one thing last night. When I changed the file, I
compiled the package to verify my modification. It should be the
reason. But should it really affect the result of "git diff"?
Thanks,
-Aubrey
^ permalink raw reply
* [PATCH 3/3] git-apply --whitespace=nowarn
From: Junio C Hamano @ 2006-02-28 1:13 UTC (permalink / raw)
To: git; +Cc: Andrew Morton
In-Reply-To: <7vhd6kxuea.fsf@assigned-by-dhcp.cox.net>
Andrew insists --whitespace=warn should be the default, and I
tend to agree. This introduces --whitespace=warn, so if your
project policy is more lenient, you can squelch them by having
apply.whitespace=nowarn in your configuration file.
Signed-off-by: Junio C Hamano <junkio@cox.net>
---
* Not in "next" but will be shortly.
apply.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
114b085dd7b82c3ca74760c896e86c425127cf76
diff --git a/apply.c b/apply.c
index a5cdd8e..d5cb5b1 100644
--- a/apply.c
+++ b/apply.c
@@ -39,7 +39,7 @@ static enum whitespace_eol {
warn_on_whitespace,
error_on_whitespace,
strip_whitespace,
-} new_whitespace = nowarn_whitespace;
+} new_whitespace = warn_on_whitespace;
static int whitespace_error = 0;
static int squelch_whitespace_errors = 5;
static int applied_after_stripping = 0;
@@ -55,6 +55,10 @@ static void parse_whitespace_option(cons
new_whitespace = warn_on_whitespace;
return;
}
+ if (!strcmp(option, "nowarn")) {
+ new_whitespace = nowarn_whitespace;
+ return;
+ }
if (!strcmp(option, "error")) {
new_whitespace = error_on_whitespace;
return;
--
1.2.3.gbfea
^ permalink raw reply related
* [PATCH 2/3] apply --whitespace: configuration option.
From: Junio C Hamano @ 2006-02-28 1:13 UTC (permalink / raw)
To: git; +Cc: Andrew Morton
In-Reply-To: <7vhd6kxuea.fsf@assigned-by-dhcp.cox.net>
The new configuration option apply.whitespace can take one of
"warn", "error", "error-all", or "strip". When git-apply is run
to apply the patch to the index, they are used as the default
value if there is no command line --whitespace option.
Andrew can now tell people who feed him git trees to update to
this version and say:
git repo-config apply.whitespace error
Signed-off-by: Junio C Hamano <junkio@cox.net>
---
* Already in "next".
apply.c | 72 ++++++++++++++++++++++++++++++++++++++-------------------
cache.h | 2 ++
environment.c | 1 +
3 files changed, 51 insertions(+), 24 deletions(-)
2ae1c53b51ff78b13cc8abf8e9798a12140b7638
diff --git a/apply.c b/apply.c
index 8139d83..a5cdd8e 100644
--- a/apply.c
+++ b/apply.c
@@ -35,16 +35,42 @@ static const char apply_usage[] =
"git-apply [--stat] [--numstat] [--summary] [--check] [--index] [--apply] [--no-add] [--index-info] [--allow-binary-replacement] [-z] [-pNUM] <patch>...";
static enum whitespace_eol {
- nowarn,
+ nowarn_whitespace,
warn_on_whitespace,
error_on_whitespace,
- strip_and_apply,
-} new_whitespace = nowarn;
+ strip_whitespace,
+} new_whitespace = nowarn_whitespace;
static int whitespace_error = 0;
static int squelch_whitespace_errors = 5;
static int applied_after_stripping = 0;
static const char *patch_input_file = NULL;
+static void parse_whitespace_option(const char *option)
+{
+ if (!option) {
+ new_whitespace = nowarn_whitespace;
+ return;
+ }
+ if (!strcmp(option, "warn")) {
+ new_whitespace = warn_on_whitespace;
+ return;
+ }
+ if (!strcmp(option, "error")) {
+ new_whitespace = error_on_whitespace;
+ return;
+ }
+ if (!strcmp(option, "error-all")) {
+ new_whitespace = error_on_whitespace;
+ squelch_whitespace_errors = 0;
+ return;
+ }
+ if (!strcmp(option, "strip")) {
+ new_whitespace = strip_whitespace;
+ return;
+ }
+ die("unrecognized whitespace option '%s'", option);
+}
+
/*
* For "diff-stat" like behaviour, we keep track of the biggest change
* we've seen, and the longest filename. That allows us to do simple
@@ -832,7 +858,7 @@ static int parse_fragment(char *line, un
* That is, an addition of an empty line would check
* the '+' here. Sneaky...
*/
- if ((new_whitespace != nowarn) &&
+ if ((new_whitespace != nowarn_whitespace) &&
isspace(line[len-2])) {
whitespace_error++;
if (squelch_whitespace_errors &&
@@ -1129,7 +1155,7 @@ static int apply_line(char *output, cons
* patch[plen] is '\n'.
*/
int add_nl_to_tail = 0;
- if ((new_whitespace == strip_and_apply) &&
+ if ((new_whitespace == strip_whitespace) &&
1 < plen && isspace(patch[plen-1])) {
if (patch[plen] == '\n')
add_nl_to_tail = 1;
@@ -1824,10 +1850,21 @@ static int apply_patch(int fd, const cha
return 0;
}
+static int git_apply_config(const char *var, const char *value)
+{
+ if (!strcmp(var, "apply.whitespace")) {
+ apply_default_whitespace = strdup(value);
+ return 0;
+ }
+ return git_default_config(var, value);
+}
+
+
int main(int argc, char **argv)
{
int i;
int read_stdin = 1;
+ const char *whitespace_option = NULL;
for (i = 1; i < argc; i++) {
const char *arg = argv[i];
@@ -1895,30 +1932,17 @@ int main(int argc, char **argv)
continue;
}
if (!strncmp(arg, "--whitespace=", 13)) {
- if (!strcmp(arg+13, "warn")) {
- new_whitespace = warn_on_whitespace;
- continue;
- }
- if (!strcmp(arg+13, "error")) {
- new_whitespace = error_on_whitespace;
- continue;
- }
- if (!strcmp(arg+13, "error-all")) {
- new_whitespace = error_on_whitespace;
- squelch_whitespace_errors = 0;
- continue;
- }
- if (!strcmp(arg+13, "strip")) {
- new_whitespace = strip_and_apply;
- continue;
- }
- die("unrecognized whitespace option '%s'", arg+13);
+ whitespace_option = arg + 13;
+ parse_whitespace_option(arg + 13);
+ continue;
}
if (check_index && prefix_length < 0) {
prefix = setup_git_directory();
prefix_length = prefix ? strlen(prefix) : 0;
- git_config(git_default_config);
+ git_config(git_apply_config);
+ if (!whitespace_option && apply_default_whitespace)
+ parse_whitespace_option(apply_default_whitespace);
}
if (0 < prefix_length)
arg = prefix_filename(prefix, prefix_length, arg);
diff --git a/cache.h b/cache.h
index 58eec00..0d3b244 100644
--- a/cache.h
+++ b/cache.h
@@ -161,11 +161,13 @@ extern int hold_index_file_for_update(st
extern int commit_index_file(struct cache_file *);
extern void rollback_index_file(struct cache_file *);
+/* Environment bits from configuration mechanism */
extern int trust_executable_bit;
extern int assume_unchanged;
extern int only_use_symrefs;
extern int diff_rename_limit_default;
extern int shared_repository;
+extern const char *apply_default_whitespace;
#define GIT_REPO_VERSION 0
extern int repository_format_version;
diff --git a/environment.c b/environment.c
index 251e53c..16c08f0 100644
--- a/environment.c
+++ b/environment.c
@@ -17,6 +17,7 @@ int only_use_symrefs = 0;
int repository_format_version = 0;
char git_commit_encoding[MAX_ENCODING_LENGTH] = "utf-8";
int shared_repository = 0;
+const char *apply_default_whitespace = NULL;
static char *git_dir, *git_object_dir, *git_index_file, *git_refs_dir,
*git_graft_file;
--
1.2.3.gbfea
^ permalink raw reply related
* Re: [PATCH] git pull cannot find remote refs.
From: Junio C Hamano @ 2006-02-28 1:13 UTC (permalink / raw)
To: Stefan-W. Hahn; +Cc: git
In-Reply-To: <20060227214936.GA7205@scotty.home>
"Stefan-W. Hahn" <stefan.hahn@s-hahn.de> writes:
> Tracking it down, I found a gap between how git-ls-remote prints out the tags
> and git-fetch scans them with sed. Looking at the code of git-ls-remote the
> there is an tab character between the sha1 and the refname, while there is a
> space and a tab character in the regular expression for th sed command.
>
> As a result the while where all is piped in cannot read the two values.
Sorry.
I do not understand the above comment, nor the following code.
> git-ls-remote $upload_pack --tags "$remote" |
> - sed -ne 's|^\([0-9a-f]*\)[ ]\(refs/tags/.*\)^{}$|\1 \2|p' |
> + sed -ne 's|^\([0-9a-f]*\)[\t]\(refs/tags/.*\)^{}$|\1 \2|p' |
ls-remote shows "SHA1\tPATH". The original says "hexadecimal
followed by [either a single space or a single tab] followed by
a refpath", while yours says "hexadecimal followed by a single tab
followed by a refpath". I do not see how that would make any
difference. Puzzled...
I've seen two servers DNS round-robin and one of them fail to
respond. The first "fetch" goes to the good one and the second
ls-remote goes to the bad one, then you would see "Oops, we
cannot peek tags". But this patch does not have anything to do
with that problem..
^ permalink raw reply
* [PATCH 1/3] apply: squelch excessive errors and --whitespace=error-all
From: Junio C Hamano @ 2006-02-28 1:13 UTC (permalink / raw)
To: git; +Cc: Andrew Morton
In-Reply-To: <7vhd6kxuea.fsf@assigned-by-dhcp.cox.net>
This by default makes --whitespace=warn, error, and strip to
warn only the first 5 additions of trailing whitespaces. A new
option --whitespace=error-all can be used to view all of them
before applying.
Signed-off-by: Junio C Hamano <junkio@cox.net>
---
* This is already in "next".
apply.c | 53 +++++++++++++++++++++++++++++++++++++++++++++--------
1 files changed, 45 insertions(+), 8 deletions(-)
fc96b7c9ba5034a408d508c663a96a15b8f8729c
diff --git a/apply.c b/apply.c
index 7dbbeb4..8139d83 100644
--- a/apply.c
+++ b/apply.c
@@ -41,6 +41,8 @@ static enum whitespace_eol {
strip_and_apply,
} new_whitespace = nowarn;
static int whitespace_error = 0;
+static int squelch_whitespace_errors = 5;
+static int applied_after_stripping = 0;
static const char *patch_input_file = NULL;
/*
@@ -832,11 +834,16 @@ static int parse_fragment(char *line, un
*/
if ((new_whitespace != nowarn) &&
isspace(line[len-2])) {
- fprintf(stderr, "Added whitespace\n");
- fprintf(stderr, "%s:%d:%.*s\n",
- patch_input_file,
- linenr, len-2, line+1);
- whitespace_error = 1;
+ whitespace_error++;
+ if (squelch_whitespace_errors &&
+ squelch_whitespace_errors <
+ whitespace_error)
+ ;
+ else {
+ fprintf(stderr, "Adds trailing whitespace.\n%s:%d:%.*s\n",
+ patch_input_file,
+ linenr, len-2, line+1);
+ }
}
added++;
newlines--;
@@ -1129,6 +1136,7 @@ static int apply_line(char *output, cons
plen--;
while (0 < plen && isspace(patch[plen]))
plen--;
+ applied_after_stripping++;
}
memcpy(output, patch + 1, plen);
if (add_nl_to_tail)
@@ -1895,11 +1903,16 @@ int main(int argc, char **argv)
new_whitespace = error_on_whitespace;
continue;
}
+ if (!strcmp(arg+13, "error-all")) {
+ new_whitespace = error_on_whitespace;
+ squelch_whitespace_errors = 0;
+ continue;
+ }
if (!strcmp(arg+13, "strip")) {
new_whitespace = strip_and_apply;
continue;
}
- die("unrecognixed whitespace option '%s'", arg+13);
+ die("unrecognized whitespace option '%s'", arg+13);
}
if (check_index && prefix_length < 0) {
@@ -1919,7 +1932,31 @@ int main(int argc, char **argv)
}
if (read_stdin)
apply_patch(0, "<stdin>");
- if (whitespace_error && new_whitespace == error_on_whitespace)
- return 1;
+ if (whitespace_error) {
+ if (squelch_whitespace_errors &&
+ squelch_whitespace_errors < whitespace_error) {
+ int squelched =
+ whitespace_error - squelch_whitespace_errors;
+ fprintf(stderr, "warning: squelched %d whitespace error%s\n",
+ squelched,
+ squelched == 1 ? "" : "s");
+ }
+ if (new_whitespace == error_on_whitespace)
+ die("%d line%s add%s trailing whitespaces.",
+ whitespace_error,
+ whitespace_error == 1 ? "" : "s",
+ whitespace_error == 1 ? "s" : "");
+ if (applied_after_stripping)
+ fprintf(stderr, "warning: %d line%s applied after"
+ " stripping trailing whitespaces.\n",
+ applied_after_stripping,
+ applied_after_stripping == 1 ? "" : "s");
+ else if (whitespace_error)
+ fprintf(stderr, "warning: %d line%s add%s trailing"
+ " whitespaces.\n",
+ whitespace_error,
+ whitespace_error == 1 ? "" : "s",
+ whitespace_error == 1 ? "s" : "");
+ }
return 0;
}
--
1.2.3.gbfea
^ permalink raw reply related
* Re: [PATCH] First cut at libifying revlist generation
From: Junio C Hamano @ 2006-02-28 1:13 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Git Mailing List
In-Reply-To: <Pine.LNX.4.63.0602270947380.5937@wbgn013.biozentrum.uni-wuerzburg.de>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> http://article.gmane.org/gmane.comp.version-control.git/10718, which
> helped me tremendously in identyfing the bug.
I'm willing to accept a response to "Yup. That sounds sensible."
^ permalink raw reply
* Re: the war on trailing whitespace
From: linux @ 2006-02-28 1:07 UTC (permalink / raw)
To: git
The only language I know of where the presence of whitespace on blank
lines matters is make(1). Even there, it's subtle. It's okay to have
(using "cat -e" syntax)
foo : bar$
command$
$
command$
But there's a difference between
foo : bar$
$
which specifies an empty command and will therefore not use a default rule, and
foo : bar$
$
which does not specify any command and so will use a default rule if one exists.
(Of course, you can also get [ \t]\n inside an arbitrary binary file, too.)
^ permalink raw reply
* Re: git-svn and huge data and modifying the git-svn-HEAD branch directly
From: Martin Langhoff @ 2006-02-28 0:58 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Eric Wong, git
In-Reply-To: <Pine.LNX.4.64.0602271634410.22647@g5.osdl.org>
On 2/28/06, Linus Torvalds <torvalds@osdl.org> wrote:
> On Tue, 28 Feb 2006, Martin Langhoff wrote:
> > git-svn-HEAD "moves" so it's really a bad idea to have it as a tag.
> > Nothing within core git prevents it from moving, but I think that
> > porcelains will start breaking. Tags and heads are the same thing,
> > except that heads are expected to change (specifically, to move
> > forward), and tags are expected to stand still.
>
> Well, I wouldn't say that tags are expected to stand still. Some kinds of
> tags are expected to move: a "this is the last tested version" tag would
> be expected to move with testing.
Alrighty... in my git projects where things like these matter, my
"latest tested" and "current in production" refs are actually in
refs/heads.
> That said, the movement is _different_ from a branch. A branch is expected
> to move _with_ development, while a tag is expected to either stay the
> same, or move _after_ development.
Grumble. I'd say a head is expected to reliably move _forward_...
"with" development, yes, but definitely forward. In my book a tag
wouldn't move, but if I take your word for it, then a tag can perhaps
change arbitrarily?
I'm not sure how much support we have in porcelains for "tracking" a
tag if it starts changing. Right now I think we'd find all sorts of
problems, we'd need to think carefully what moving tags means for
porcelains.
> Or something even more specific, like "refs/svn-tracking/". Git
> shouldn't care - all the tools _should_ work fine with any subdirectory
> structure.
I think the moving-forward (therefore is trackable) vs stays reliably
in place distinction *is* useful. "Moves randomly" may also be useful,
but it should get a different treatment, because it's not "trackable".
Not that git and porcelains can't deal with all this stuff. But if
there is a clear convention then porcelains can be smart and refuse to
commit to the wrong place... it'd be a bit of a UI enhancement
perhaps?
martin
^ permalink raw reply
* Re: git-svn and huge data and modifying the git-svn-HEAD branch directly
From: Linus Torvalds @ 2006-02-28 0:41 UTC (permalink / raw)
To: Martin Langhoff; +Cc: Eric Wong, git
In-Reply-To: <46a038f90602271625y6c7e9072u372b8dd3662e272c@mail.gmail.com>
On Tue, 28 Feb 2006, Martin Langhoff wrote:
>
> git-svn-HEAD "moves" so it's really a bad idea to have it as a tag.
> Nothing within core git prevents it from moving, but I think that
> porcelains will start breaking. Tags and heads are the same thing,
> except that heads are expected to change (specifically, to move
> forward), and tags are expected to stand still.
Well, I wouldn't say that tags are expected to stand still. Some kinds of
tags are expected to move: a "this is the last tested version" tag would
be expected to move with testing.
That said, the movement is _different_ from a branch. A branch is expected
to move _with_ development, while a tag is expected to either stay the
same, or move _after_ development.
However, in many ways git really doesn't care much. The "refs/heads"
directory is the only one that is really special, in that "git checkout"
refuses to check out a moving branch in anything but that subdirectory.
The "tags" subdirectory is slightly special to some helpers (like "git
pull"), which have flags to pull everythying in that subdirectory.
But other than those two pretty trivial issues, any ref under "refs/"
should work perfectly fine. I would argue that a specialized tracking tool
might well be better off without using either "refs/heads" _or_
"refs/tags", since those have accepted meaning outside of tracking.
Using a "refs/remotes" subdirectory makes tons of sense for something like
this. Or something even more specific, like "refs/svn-tracking/". Git
shouldn't care - all the tools _should_ work fine with any subdirectory
structure.
Linus
^ permalink raw reply
* Re: git-svn and huge data and modifying the git-svn-HEAD branch directly
From: Martin Langhoff @ 2006-02-28 0:25 UTC (permalink / raw)
To: Eric Wong; +Cc: git
In-Reply-To: <20060227192422.GB9518@hand.yhbt.net>
On 2/28/06, Eric Wong <normalperson@yhbt.net> wrote:
> > If it is not supposed to be changed by the user, maybe it could be
> > stored as a tag.
> >
> > Or maybe another type of reference can be introduced. refs/remote/, for
> > branches we are tracking, but which should not be modified locally.
>
> Either of those could work for me. Changing git-svn-HEAD to become a
> tag would probably be easier (not having to update other tools, such as
> git-fetch), but refs/remote may make more sense.
git-svn-HEAD "moves" so it's really a bad idea to have it as a tag.
Nothing within core git prevents it from moving, but I think that
porcelains will start breaking. Tags and heads are the same thing,
except that heads are expected to change (specifically, to move
forward), and tags are expected to stand still.
Something else is needed -- a convention to mark a head as 'readonly'
so that git-commit/cg-commit refuse to commit to it. cg-commit already
does that for any head matching the name of a branch.
cheers,
martin
^ permalink raw reply
* Re: the war on trailing whitespace
From: Junio C Hamano @ 2006-02-28 0:10 UTC (permalink / raw)
To: Peter Williams; +Cc: git
In-Reply-To: <44038B73.4030004@bigpond.net.au>
Peter Williams <pwil3058@bigpond.net.au> writes:
>> This whitespace policy should be at least per-project (people
>> working on both kernel and other things may have legitimate
>> reason to want trailing whitespace in the other project),
>
> I'd be interested to hear these reasons. My experience is that people
> don't put trailing white space in deliberately (or even tolerate it if
> they notice it) and it's usually there as a side effect of the way
> their text editor works (and that's also the reason that they don't
> usually notice it). But if my experience is misleading me and there
> are valid reasons for having trailing white space I'm genuinely
> interested in knowing what they are.
For example:
http://compsoc.dur.ac.uk/whitespace/
Jokes aside, I can imagine people want to keep format=flowed
text messages (i.e. not programming language source code) under
git control. Maybe pulling and pushing would be the default
mode of operation for those people, so what git-apply does would
not be in the picture for those people, but who knows.
One way to find it out is to push out a strict one and see who
screams ;-), but the point is I am reluctant to make a stricter
policy the default, thinking, but not knowing as a fact, that it
is good enough for everybody.
^ permalink raw reply
* Re: [PATCH] First cut at libifying revlist generation
From: Junio C Hamano @ 2006-02-27 23:55 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Git Mailing List
In-Reply-To: <Pine.LNX.4.64.0602270851560.22647@g5.osdl.org>
Linus Torvalds <torvalds@osdl.org> writes:
> Here's a cleanup patch that does that. It also moves "unpacked" (and the
> flag parsing) into rev_info, since that actually does affect the revision
> walker too, and thus logically belongs there.
Makes sense. Thanks.
^ permalink raw reply
* Re: the war on trailing whitespace
From: Andrew Morton @ 2006-02-27 23:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vhd6kxuea.fsf@assigned-by-dhcp.cox.net>
Junio C Hamano <junkio@cox.net> wrote:
>
> tell the higher echelon folks
> to do:
>
> $ git repo-config apply.whitespace error
>
> in their repositories
That might be reasonable, some might object to tiny amounts of extra work..
In my setup, trailing whitespace purely causes warnings. But with a
quilt-style thing, it's trivial to unapply the patch, strip it, reapply.
git is different - I'd imagine that if warnings were generated while an
mbox was being applied, it's too much hassle to go and unapply the mbox,
fix the diffs up and then apply the mbox again.
I'd suggest that git have options to a) generate trailing-whitespace
warnings, b) generate trailing-whitespace errors and c) strip trailing
whitespace while applying. And that the as-shipped default be a).
^ permalink raw reply
* Re: the war on trailing whitespace
From: Peter Williams @ 2006-02-27 23:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Andrew Morton, git
In-Reply-To: <7vhd6kxuea.fsf@assigned-by-dhcp.cox.net>
Junio C Hamano wrote:
> Andrew Morton <akpm@osdl.org> writes:
>
>
>>That's not a good reason. People will discover that git has started
>>shouting at them and they'll work out how to make it stop.
>>
>>The problem is getting C users to turn the check on, not in getting python
>>users to turn it off.
>
>
> This whitespace policy should be at least per-project (people
> working on both kernel and other things may have legitimate
> reason to want trailing whitespace in the other project),
I'd be interested to hear these reasons. My experience is that people
don't put trailing white space in deliberately (or even tolerate it if
they notice it) and it's usually there as a side effect of the way their
text editor works (and that's also the reason that they don't usually
notice it). But if my experience is misleading me and there are valid
reasons for having trailing white space I'm genuinely interested in
knowing what they are.
> so we
> would need some configurability; the problem is *both*.
>
> We could do one of two things, at least.
>
> - I modify the git-apply that is in the "next" branch further
> to make --whitespace=error the default, and push it out. You
> convince people who feed things to you to update to *that*
> version or later.
>
> - I already have the added whitespace detection hook (a fixed
> one that actually matches what I use) shipped with git. You
> convince people who feed things to you to update to *that*
> version or later, and to enable that hook.
>
> I think you are arguing for the first one. I am reluctant to do
> so because it would not help by itself *anyway*. In any case
> you need to convince people who feed things to you to do
> something to prevent later changes fed to you from being
> contaminated with trailing whitespaces.
>
> Having said that, I have a third solution, which consists of two
> patches that come on top of what are already in "next" branch:
>
> - apply: squelch excessive errors and --whitespace=error-all
> - apply --whitespace: configuration option.
>
> With these, git-apply used by git-applymbox and git-am would
> refuse to apply a patch that adds trailing whitespaces, when the
> per-repository configuration is set like this:
>
> [apply]
> whitespace = error
>
> (Alternatively,
>
> $ git repo-config apply.whitespace error
>
> would set these lines there for you).
>
> I think there are three kinds of git users.
>
> * Linus, you, and the kernel subsystem maintainers. The
> whitespace policy with this version of git-apply (with the
> configuration option set to apply.whitespace=error) gives
> would help these people by enforcing the SubmittingPatches
> and your "perfect patch" requirements.
>
> * People who feed patches to the above people. They are helped
> by enabling the pre-commit hook that comes with git to
> conform to the kernel whitespace policy -- they need to be
> educated to do so.
>
> * People outside of kernel community, using git in projects to
> which the kernel whitespace policy does not have any
> relevance.
>
> While I do consider the kernel folks a lot more important
> customers than other users, I have to take flak from the third
> kind of users, and to them, authority by Linus or you does not
> weigh as much as the first two classes of people. Making the
> default to --whitespace=error means that you are making me
> justify this kernel project policy as something applicable to
> projects outside the kernel. That is simply not fair to me.
>
> You have to convince people you work with to update to at least
> to this version anyway, so I do not think it is too much to ask
> from you, while you are at it, to tell the higher echelon folks
> to do:
>
> $ git repo-config apply.whitespace error
>
> in their repositories (and/or set that in their templates so new
> repositories created with git-init-db would inherit it).
>
> -
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Peter Williams pwil3058@bigpond.net.au
"Learning, n. The kind of ignorance distinguishing the studious."
-- Ambrose Bierce
^ permalink raw reply
* Re: the war on trailing whitespace
From: Junio C Hamano @ 2006-02-27 23:18 UTC (permalink / raw)
To: Andrew Morton; +Cc: git
In-Reply-To: <20060227011832.78359f0a.akpm@osdl.org>
Andrew Morton <akpm@osdl.org> writes:
> That's not a good reason. People will discover that git has started
> shouting at them and they'll work out how to make it stop.
>
> The problem is getting C users to turn the check on, not in getting python
> users to turn it off.
This whitespace policy should be at least per-project (people
working on both kernel and other things may have legitimate
reason to want trailing whitespace in the other project), so we
would need some configurability; the problem is *both*.
We could do one of two things, at least.
- I modify the git-apply that is in the "next" branch further
to make --whitespace=error the default, and push it out. You
convince people who feed things to you to update to *that*
version or later.
- I already have the added whitespace detection hook (a fixed
one that actually matches what I use) shipped with git. You
convince people who feed things to you to update to *that*
version or later, and to enable that hook.
I think you are arguing for the first one. I am reluctant to do
so because it would not help by itself *anyway*. In any case
you need to convince people who feed things to you to do
something to prevent later changes fed to you from being
contaminated with trailing whitespaces.
Having said that, I have a third solution, which consists of two
patches that come on top of what are already in "next" branch:
- apply: squelch excessive errors and --whitespace=error-all
- apply --whitespace: configuration option.
With these, git-apply used by git-applymbox and git-am would
refuse to apply a patch that adds trailing whitespaces, when the
per-repository configuration is set like this:
[apply]
whitespace = error
(Alternatively,
$ git repo-config apply.whitespace error
would set these lines there for you).
I think there are three kinds of git users.
* Linus, you, and the kernel subsystem maintainers. The
whitespace policy with this version of git-apply (with the
configuration option set to apply.whitespace=error) gives
would help these people by enforcing the SubmittingPatches
and your "perfect patch" requirements.
* People who feed patches to the above people. They are helped
by enabling the pre-commit hook that comes with git to
conform to the kernel whitespace policy -- they need to be
educated to do so.
* People outside of kernel community, using git in projects to
which the kernel whitespace policy does not have any
relevance.
While I do consider the kernel folks a lot more important
customers than other users, I have to take flak from the third
kind of users, and to them, authority by Linus or you does not
weigh as much as the first two classes of people. Making the
default to --whitespace=error means that you are making me
justify this kernel project policy as something applicable to
projects outside the kernel. That is simply not fair to me.
You have to convince people you work with to update to at least
to this version anyway, so I do not think it is too much to ask
from you, while you are at it, to tell the higher echelon folks
to do:
$ git repo-config apply.whitespace error
in their repositories (and/or set that in their templates so new
repositories created with git-init-db would inherit it).
^ permalink raw reply
* Re: bug?: stgit creates (unneccessary?) conflicts when pulling
From: Shawn Pearce @ 2006-02-27 22:26 UTC (permalink / raw)
To: Karl Hasselström; +Cc: git, Catalin Marinas
In-Reply-To: <20060227204252.GA31836@diana.vm.bytemark.co.uk>
Karl Hasselstr?m <kha@treskal.com> wrote:
> If I make a patch series where more than one patch touches the same
> line, I get a lot of merge errors when upstream has accepted them and
> I try to merge them back.
>
> Suppose a line contained the string "0". Patch p1 changes that to "1",
> patch p2 further changes that to "2". Upstream accept the patches, and
> I later "stg pull" them. When reapplying p1 after the pull, stg
> generates a merge conflict since upstream changed the "0" to "2" and
> p1 changes the "0" to "1".
You should look at pg:
http://www.spearce.org/2006/02/pg-version-0111-released.html
It has some of the features of StGIT and it (at least partially)
solves this problem.
Assume you had a patch stack of:
PatchA : Change 0 to a 1
PatchB : Change 1 to a 2
PatchC : Change 2 to a 3
with each affecting the same line of the same file.
When pg grabs its (possibly remote) parent ("stg pull" aka pg-rebase)
we try to push down PatchA. If PatchA fails to push cleanly we'll
pop it off and try to push PatchA + PatchB. If that pushes cleanly
then we fold the content of PatchA into PatchB, effectively making
PatchA part of PatchB. If PatchA + PatchB failed to push down
cleanly then we pop both and retry pushing PatchA + PatchB + PatchC.
If that pushes down cleanly then we make PatchA and PatchB officially
part of PatchC.
This required some ``hacks'' in pg-push. Basically if we are
doing a non-fastforward push (as there are new commits we must
merge on top of) we run an external diff/patch to apply the change.
If that fails we reset the tree back to a clean state and try again.
Whenever patch rejects a hunk we examine the hunk to see if it is
already applied to the target file; if it is already applied we
skip the hunk. If all hunks applied cleanly and/or are already
applied we accept the patch (or combination of patches) as merging
cleanly. Yea, its not very pretty. But it works!
Where it falls down is if the upstream accepts all three of your
patches but then changes the line from a 3 to a 4. pg won't look
at the GIT commit history to detect that the upstream accepted your
final change (PatchC) but then overwrite it with a newer change
(3->4). I have thought about trying to implement this but the
current environment where I am using pg wouldn't benefit from it,
so it has not been high on my priority list.
--
Shawn.
^ 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