* whitespace-stripping
From: J. Bruce Fields @ 2007-09-16 22:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
The following patches fix one (probably rare) bug in
git apply --whitespace=strip
and then teach it to also complain about initial consecutive spaces that
could be tabs.
The latter is the standard for the kernel, but may not be appropriate
for other projects. I'd like to make the whitespace code handle the
kernel style completely first, then consider configuration to handle
other styles if people complain. But maybe the change of behavior would
be an unpleasant surprise for someone with apply.whitespace=strip and a
project that always uses spaces for indents.
--b.
^ permalink raw reply
* [PATCH 1/3] git-apply: fix whitespace stripping
From: J. Bruce Fields @ 2007-09-16 22:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, J. Bruce Fields
In-Reply-To: <11899829424040-git-send-email-bfields@citi.umich.edu>
The algorithm isn't right here: it accumulates any set of 8 spaces into
tabs even if they're separated by tabs, so
<four spaces><tab><four spaces><tab>
is converted to
<tab><tab><tab>
when it should be just
<tab><tab>
So teach git-apply that a tab hides any group of less than 8 previous
spaces in a row.
Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
builtin-apply.c | 13 ++++++++++---
1 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/builtin-apply.c b/builtin-apply.c
index 976ec77..70359c1 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1642,15 +1642,22 @@ static int apply_line(char *output, const char *patch, int plen)
buf = output;
if (need_fix_leading_space) {
+ int consecutive_spaces = 0;
/* between patch[1..last_tab_in_indent] strip the
* funny spaces, updating them to tab as needed.
*/
for (i = 1; i < last_tab_in_indent; i++, plen--) {
char ch = patch[i];
- if (ch != ' ')
+ if (ch != ' ') {
+ consecutive_spaces = 0;
*output++ = ch;
- else if ((i % 8) == 0)
- *output++ = '\t';
+ } else {
+ consecutive_spaces++;
+ if (consecutive_spaces == 8) {
+ *output++ = '\t';
+ consecutive_spaces = 0;
+ }
+ }
}
fixed = 1;
i = last_tab_in_indent;
--
1.5.3.1.42.gfe5df
^ permalink raw reply related
* [PATCH 3/3] git-apply: add tests for stripping of leading and trailing whitespace
From: J. Bruce Fields @ 2007-09-16 22:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, J. Bruce Fields
In-Reply-To: <1189982942187-git-send-email-bfields@citi.umich.edu>
Add tests to make sure we strip leading and trailing whitespace correctly.
Of the four tests, the first two should always have passed, the third
requires the "fix whitespace stripping" patch, and the fourth requires
the "complain about >= 8 consecutive spaces in initial indent" patch.
Note that this patch itself adds leading and trailing whitespace.
Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
t/t4124-apply-whitespace-strip.sh | 43 +++++++++++++++++++++++++++++++++++++
t/t4124/1-after | 3 ++
t/t4124/1-before | 3 ++
t/t4124/2-after | 3 ++
t/t4124/2-before | 3 ++
t/t4124/3-after | 1 +
t/t4124/3-before | 1 +
t/t4124/4-after | 5 ++++
t/t4124/4-before | 5 ++++
9 files changed, 67 insertions(+), 0 deletions(-)
create mode 100644 t/t4124-apply-whitespace-strip.sh
create mode 100644 t/t4124/1-after
create mode 100644 t/t4124/1-before
create mode 100644 t/t4124/2-after
create mode 100644 t/t4124/2-before
create mode 100644 t/t4124/3-after
create mode 100644 t/t4124/3-before
create mode 100644 t/t4124/4-after
create mode 100644 t/t4124/4-before
diff --git a/t/t4124-apply-whitespace-strip.sh b/t/t4124-apply-whitespace-strip.sh
new file mode 100644
index 0000000..3b5f58b
--- /dev/null
+++ b/t/t4124-apply-whitespace-strip.sh
@@ -0,0 +1,43 @@
+#!/bin/sh
+
+test_description='handle space and tab combinations with --whitespace=strip'
+
+. ./test-lib.sh
+
+# The directory t4124/ contains pairs of files "n-before" and "n-after",
+# identicaly except leading and trailing whitespace are stripped from
+# the latter.
+#
+# Check that we strip whitespace correctly by checking that the diff
+# between the two files, applied to the first (with --whitespace=strip)
+# produces the second.
+
+mkpatch () {
+ cp "$1" foo
+ git diff /dev/null foo >patch
+ rm foo
+}
+
+checkstrip () {
+ mkpatch "../t4124/$1-before"
+ git apply --whitespace=strip patch
+ git diff foo "../t4124/$1-after"
+}
+
+test_expect_success \
+ 'trailing tabs and spaces' \
+ 'checkstrip 1'
+
+test_expect_success \
+ 'spaces before tabs' \
+ 'checkstrip 2'
+
+test_expect_success \
+ '8 or more non-consecutive initial spaces' \
+ 'checkstrip 3'
+
+test_expect_success \
+ '8 or more consecutive initial spaces' \
+ 'checkstrip 4'
+
+test_done
diff --git a/t/t4124/1-after b/t/t4124/1-after
new file mode 100644
index 0000000..cf5dfce
--- /dev/null
+++ b/t/t4124/1-after
@@ -0,0 +1,3 @@
+trailing space
+trailing tab
+trailing spaces and tabs
diff --git a/t/t4124/1-before b/t/t4124/1-before
new file mode 100644
index 0000000..1f2505b
--- /dev/null
+++ b/t/t4124/1-before
@@ -0,0 +1,3 @@
+trailing space
+trailing tab
+trailing spaces and tabs
diff --git a/t/t4124/2-after b/t/t4124/2-after
new file mode 100644
index 0000000..f198144
--- /dev/null
+++ b/t/t4124/2-after
@@ -0,0 +1,3 @@
+ space tab
+ space space tab
+ tab space tab
diff --git a/t/t4124/2-before b/t/t4124/2-before
new file mode 100644
index 0000000..8fc35bb
--- /dev/null
+++ b/t/t4124/2-before
@@ -0,0 +1,3 @@
+ space tab
+ space space tab
+ tab space tab
diff --git a/t/t4124/3-after b/t/t4124/3-after
new file mode 100644
index 0000000..4db0e80
--- /dev/null
+++ b/t/t4124/3-after
@@ -0,0 +1 @@
+ 4 spaces, tab, 4 spaces, tab
diff --git a/t/t4124/3-before b/t/t4124/3-before
new file mode 100644
index 0000000..f0e2b9c
--- /dev/null
+++ b/t/t4124/3-before
@@ -0,0 +1 @@
+ 4 spaces, tab, 4 spaces, tab
diff --git a/t/t4124/4-after b/t/t4124/4-after
new file mode 100644
index 0000000..a9b8cf6
--- /dev/null
+++ b/t/t4124/4-after
@@ -0,0 +1,5 @@
+ 7 spaces
+ 8 spaces
+ 9 spaces
+ tab 8 spaces
+ tab 9 spaces
diff --git a/t/t4124/4-before b/t/t4124/4-before
new file mode 100644
index 0000000..a35b624
--- /dev/null
+++ b/t/t4124/4-before
@@ -0,0 +1,5 @@
+ 7 spaces
+ 8 spaces
+ 9 spaces
+ tab 8 spaces
+ tab 9 spaces
--
1.5.3.1.42.gfe5df
^ permalink raw reply related
* [PATCH 2/3] git-apply: complain about >=8 consecutive spaces in initial indent
From: J. Bruce Fields @ 2007-09-16 22:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, J. Bruce Fields
In-Reply-To: <11899829424173-git-send-email-bfields@citi.umich.edu>
Complain if we find 8 spaces or more in a row as part of the initial
whitespace on a line, and (with --whitespace=stripspace) replace such by
a tab.
Well, linux's checkpatch.pl complains about this sort of thing.
Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
builtin-apply.c | 34 +++++++++++++++++++++++++++-------
1 files changed, 27 insertions(+), 7 deletions(-)
diff --git a/builtin-apply.c b/builtin-apply.c
index 70359c1..fb63089 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -918,6 +918,7 @@ static void check_whitespace(const char *line, int len)
{
const char *err = "Adds trailing whitespace";
int seen_space = 0;
+ int consecutive_spaces = 0;
int i;
/*
@@ -944,6 +945,18 @@ static void check_whitespace(const char *line, int len)
else
break;
}
+
+ err = "Initial indent contains eight or more spaces in a row";
+ for (i = 1; i < len; i++) {
+ if (line[i] == ' ')
+ consecutive_spaces++;
+ else if (line[i] == '\t')
+ consecutive_spaces = 0;
+ else
+ break;
+ if (consecutive_spaces == 8)
+ goto error;
+ }
return;
error:
@@ -1607,9 +1620,10 @@ static int apply_line(char *output, const char *patch, int plen)
int i;
int add_nl_to_tail = 0;
int fixed = 0;
- int last_tab_in_indent = -1;
+ int after_indent = -1;
int last_space_in_indent = -1;
int need_fix_leading_space = 0;
+ int consecutive_spaces = 0;
char *buf;
if ((new_whitespace != strip_whitespace) || !whitespace_error ||
@@ -1630,23 +1644,27 @@ static int apply_line(char *output, const char *patch, int plen)
for (i = 1; i < plen; i++) {
char ch = patch[i];
if (ch == '\t') {
- last_tab_in_indent = i;
+ consecutive_spaces = 0;
if (0 <= last_space_in_indent)
need_fix_leading_space = 1;
}
- else if (ch == ' ')
+ else if (ch == ' ') {
+ consecutive_spaces++;
last_space_in_indent = i;
- else
+ } else
break;
+ if (consecutive_spaces == 8)
+ need_fix_leading_space = 1;
}
+ after_indent=i;
buf = output;
if (need_fix_leading_space) {
- int consecutive_spaces = 0;
+ consecutive_spaces = 0;
/* between patch[1..last_tab_in_indent] strip the
* funny spaces, updating them to tab as needed.
*/
- for (i = 1; i < last_tab_in_indent; i++, plen--) {
+ for (i = 1; i < after_indent; i++, plen--) {
char ch = patch[i];
if (ch != ' ') {
consecutive_spaces = 0;
@@ -1660,7 +1678,9 @@ static int apply_line(char *output, const char *patch, int plen)
}
}
fixed = 1;
- i = last_tab_in_indent;
+ i = after_indent;
+ i -= consecutive_spaces;
+ plen += consecutive_spaces;
}
else
i = 1;
--
1.5.3.1.42.gfe5df
^ permalink raw reply related
* git-archive not working correctly ?
From: Niki Guldbrand @ 2007-09-16 22:50 UTC (permalink / raw)
To: git
Hi.
I have been playing with git-archive for and hour or so, but can't get
it to pass "extra" options to tar as it's documented that it should, or
am i reading the docs wrong ?
git-archive --format=<fmt> [--list] [--prefix=<prefix>/] [<extra>]
[--remote=<repo>] <tree-ish> [path…]
<extra>
This can be any options that the archiver backend understand.
See next section.
I want to git tar the "--exclude=option", but i can't get it through.
Is this option only valid for the zip format with the options "-0" and
"-9" ?
Kind regards
Niki Guldbrand
^ permalink raw reply
* Re: metastore
From: david @ 2007-09-16 22:52 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, Daniel Barkalow, martin f krafft, git,
Thomas Harning Jr., Francis Moreau, Nicolas Vilz,
David Härdeman
In-Reply-To: <7v7imq5ki0.fsf@gitster.siamese.dyndns.org>
On Sun, 16 Sep 2007, Junio C Hamano wrote:
> david@lang.hm writes:
>
>>> I'd rather not implement it at such a low level where a true
>>> "checkout" happens. For one thing, I am afraid that the special
>>> casing will affect the normal codepath too much and would make
>>> it into a maintenance nightmare.
>>
>> as I understand it, at this point you already choose between three
>> options.
>>
>> 1. write to a file (and set the write bit if needed)
>> 2. write to stdout
>> 3. write to a pager program
>>
>> I am suggesting adding
>> ...
>> or am I missing something major here?
>
> I do not think we are choosing any option in the codepath at
> all.
>
> What I mean by the normal "checkout" is what checkout_entry in
> entry.c does. There is no other option than (1) above. I would
> want to see an extremely good justification if you need to touch
> that codepath to implement this fringe use case.
>
> I do not think there is nothing that writes file contents to
> stdout/pager other than "git cat-file" or "git show"; I do not
> think they are what you have in mind when talking about managing
> the files under /etc. So unfortunately I do not understand the
> rest of the discussion you made in your message.
Ok, I thought that there was common code for these different uses. could
you re-read the rest of the logic based on the change being done in
checkout_entry?
if you are unwilling to have any changes made to the checkout_entry code
then the only remaing question is what you think of Daniel's suggestion to
have a hook to replace check_updates()?
if it's not acceptable either then we are down to doing a post-checkout
trigger.
one concern I have with that approach is how to deal with partial
checkouts. if a user checks out one file how can the post-checkout trigger
know if it's looking at the correct permissions file as opposed to one
left over from something else? can/should it go and read the file from the
index instead of reading the file on the filesystem? (I don't like this
becouse it leads to non-obvious behavior), or can/should there be a config
option to say that whenever any file is checked out the permissions file
needs to be checked out as well.
a post checkout trigger is useful in enough different situations that the
answers to the above questions don't eliminate the usefulness of the
trigger, they just map out the pitfalls of useing it.
David Lang
^ permalink raw reply
* Re: [RFC] strbuf's in builtin-apply
From: Junio C Hamano @ 2007-09-16 22:54 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: git
In-Reply-To: <20070916172833.GB26457@artemis.corp>
Pierre Habouzit <madcoder@debian.org> writes:
> The builtin-apply part:
> [1/6] New strbuf APIs: splice and attach.
> [2/6] Rewrite convert_to_{git,working_tree} to use strbuf's.
> [3/6] Now that cache.h needs strbuf.h, remove useless includes.
> [4/6] builtin-apply: use strbuf's instead of buffer_desc's.
>
> And the two somehow more independant patches (need 1/6 still):
> [5/6] Refactor replace_encoding_header.
> [6/6] Remove preemptive allocations.
Quite nice. Thanks, will queue.
^ permalink raw reply
* Re: [PATCH 2/3] git-apply: complain about >=8 consecutive spaces in initial indent
From: Martin Langhoff @ 2007-09-16 23:24 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Junio C Hamano, git
In-Reply-To: <1189982942187-git-send-email-bfields@citi.umich.edu>
On 9/17/07, J. Bruce Fields <bfields@citi.umich.edu> wrote:
> Complain if we find 8 spaces or more in a row as part of the initial
> whitespace on a line, and (with --whitespace=stripspace) replace such by
> a tab.
I do quite a bit of hacking on "spaces-for-indentation" projects and
still use stripspace to cleanup my patches. So no, thanks.
Perhaps split it off to a separate option? I'm not opposed to the
functionality per-se, but don't put together with
trailing-space-trimming. It's a different beast. Everyone agrees
trimming trailing spaces as much as everyone disagrees on
tabs-vs-spaces.
cheers,
m
^ permalink raw reply
* Re: [PATCH 2/3] git-apply: complain about >=8 consecutive spaces in initial indent
From: Junio C Hamano @ 2007-09-17 0:24 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: git
In-Reply-To: <1189982942187-git-send-email-bfields@citi.umich.edu>
"J. Bruce Fields" <bfields@citi.umich.edu> writes:
> Complain if we find 8 spaces or more in a row as part of the initial
> whitespace on a line, and (with --whitespace=stripspace) replace such by
> a tab.
>
> Well, linux's checkpatch.pl complains about this sort of thing.
Some people program in Python, so I am afraid that this needs to
be a separate option.
Maybe it is time to redo the --whitespace options as bitmasks so
that we can say --whitespace-fix=tab,tail,lines to pick and
choose which kinds of breakage to fix?
^ permalink raw reply
* [PATCH] apply --index-info: fall back to current index for mode changes
From: Johannes Schindelin @ 2007-09-17 0:24 UTC (permalink / raw)
To: Chris Shoemaker, gitster, git
"git diff" does not record index lines for pure mode changes (i.e. no
lines changed). Therefore, apply --index-info would call out a bogus
error.
Instead, fall back to reading the info from the current index.
Incidentally, this fixes an error where git-rebase would not rebase a
commit including a pure mode change, and changes requiring a threeway
merge.
Noticed by Chris Shoemaker.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin-apply.c | 26 ++++++++++++++++++++++++--
t/t3400-rebase.sh | 15 +++++++++++++++
2 files changed, 39 insertions(+), 2 deletions(-)
diff --git a/builtin-apply.c b/builtin-apply.c
index 7057d0d..bae4413 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -2215,6 +2215,20 @@ static int check_patch_list(struct patch *patch)
return err;
}
+/* This function tries to read the sha1 from the current index */
+static int get_current_sha1(const char *path, unsigned char *sha1)
+{
+ int pos;
+
+ if (read_cache() < 0)
+ return -1;
+ pos = cache_name_pos(path, strlen(path));
+ if (pos < 0)
+ return -1;
+ hashcpy(sha1, active_cache[pos]->sha1);
+ return 0;
+}
+
static void show_index_list(struct patch *list)
{
struct patch *patch;
@@ -2231,8 +2245,16 @@ static void show_index_list(struct patch *list)
if (0 < patch->is_new)
sha1_ptr = null_sha1;
else if (get_sha1(patch->old_sha1_prefix, sha1))
- die("sha1 information is lacking or useless (%s).",
- name);
+ /* git diff has no index line for mode/type changes */
+ if (!patch->lines_added && !patch->lines_deleted) {
+ if (get_current_sha1(patch->new_name, sha1) ||
+ get_current_sha1(patch->old_name, sha1))
+ die("mode change for %s, which is not "
+ "in current HEAD", name);
+ sha1_ptr = sha1;
+ } else
+ die("sha1 information is lacking or useless "
+ "(%s).", name);
else
sha1_ptr = sha1;
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 62205b2..95e33b5 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -68,4 +68,19 @@ test_expect_success \
test 3 = $(git rev-list master.. | wc -l)
'
+test_expect_success 'rebase a single mode change' '
+ git checkout master &&
+ echo 1 > X &&
+ git add X &&
+ test_tick &&
+ git commit -m prepare &&
+ git checkout -b modechange HEAD^ &&
+ echo 1 > X &&
+ git add X &&
+ chmod a+x A &&
+ test_tick &&
+ git commit -m modechange A X &&
+ GIT_TRACE=1 git rebase master
+'
+
test_done
--
1.5.3.1.949.g98c3
^ permalink raw reply related
* Re: git-archive not working correctly ?
From: Frank Lichtenheld @ 2007-09-17 0:31 UTC (permalink / raw)
To: Niki Guldbrand; +Cc: git
In-Reply-To: <1189983026.22727.61.camel@niki2.guldbrand.net>
On Mon, Sep 17, 2007 at 12:50:26AM +0200, Niki Guldbrand wrote:
> I have been playing with git-archive for and hour or so, but can't get
> it to pass "extra" options to tar as it's documented that it should, or
> am i reading the docs wrong ?
>
> git-archive --format=<fmt> [--list] [--prefix=<prefix>/] [<extra>]
> [--remote=<repo>] <tree-ish> [path…]
>
> <extra>
> This can be any options that the archiver backend understand.
> See next section.
>
> I want to git tar the "--exclude=option", but i can't get it through.
> Is this option only valid for the zip format with the options "-0" and
> "-9" ?
Yes. The tar backend currently doesn't support any extra options.
Gruesse,
--
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/
^ permalink raw reply
* Re: git-archive not working correctly ?
From: Junio C Hamano @ 2007-09-17 0:35 UTC (permalink / raw)
To: Niki Guldbrand; +Cc: git
In-Reply-To: <1189983026.22727.61.camel@niki2.guldbrand.net>
Niki Guldbrand <niki.guldbrand@gmail.com> writes:
> git-archive --format=<fmt> [--list] [--prefix=<prefix>/] [<extra>]
> [--remote=<repo>] <tree-ish> [path...]
>
> <extra>
> This can be any options that the archiver backend understand.
> See next section.
>
> I want to git tar the "--exclude=option", but i can't get it through.
> Is this option only valid for the zip format with the options "-0" and
> "-9" ?
The "next section" that sentence refers to is "BACKEND EXTRA
OPTIONS" section, which lists -0 and -9 for zip backend. There
is no --exclude=option in either tar or zip backend.
Note that we do not use GNU tar or zip as archiver backends.
^ permalink raw reply
* Re: [PATCH] apply --index-info: fall back to current index for mode changes
From: Chris Shoemaker @ 2007-09-17 0:46 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: gitster, git
In-Reply-To: <Pine.LNX.4.64.0709170119270.28586@racer.site>
On Mon, Sep 17, 2007 at 01:24:57AM +0100, Johannes Schindelin wrote:
>
> "git diff" does not record index lines for pure mode changes (i.e. no
> lines changed). Therefore, apply --index-info would call out a bogus
> error.
>
> Instead, fall back to reading the info from the current index.
>
> Incidentally, this fixes an error where git-rebase would not rebase a
> commit including a pure mode change, and changes requiring a threeway
> merge.
>
> Noticed by Chris Shoemaker.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Works for me. Thanks.
Acked-by: Chris Shoemaker <chris.shoemaker@cox.net>
^ permalink raw reply
* Re: metastore
From: Junio C Hamano @ 2007-09-17 0:58 UTC (permalink / raw)
To: david
Cc: Johannes Schindelin, Daniel Barkalow, martin f krafft, git,
Thomas Harning Jr., Francis Moreau, Nicolas Vilz,
David Härdeman
In-Reply-To: <Pine.LNX.4.64.0709161541330.24221@asgard.lang.hm>
david@lang.hm writes:
> On Sun, 16 Sep 2007, Junio C Hamano wrote:
>>
>> I do not think there is nothing that writes file contents to
>> stdout/pager other than "git cat-file" or "git show"; I do not
>> think they are what you have in mind when talking about managing
>> the files under /etc. So unfortunately I do not understand the
>> rest of the discussion you made in your message.
>
> Ok, I thought that there was common code for these different
> uses. could you re-read the rest of the logic based on the change
> being done in checkout_entry?
>
> if you are unwilling to have any changes made to the checkout_entry
> code then the only remaing question is what you think of Daniel's
> suggestion to have a hook to replace check_updates()?
>
> if it's not acceptable either then we are down to doing a
> post-checkout trigger.
Post-checkout trigger is something I can say I can live with
without looking at the actual patch, but that does not mean it
would be a better approach at all.
I would not be able to answer the first question right now; that
needs a patch to prove that it can be done with a well contained
set of changes that results in a maintainable code.
I haven't tried to assess the potential extent of damage needed
to checkout_entry(), and I have never been interested in this
"keeping track of /etc in place" topic myself. It is unlikely
I'll try to come up with such a patch on my own to support it at
such a low level near the core. Somebody who cares about that
feature needs to take the initiative of doing that work before
we can discuss and decide, although older-times including myself
can help spot potential issues.
So while I admit I am skeptical, consider me neither willing nor
unwilling at this point.
^ permalink raw reply
* Re: metastore
From: david @ 2007-09-17 2:31 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, Daniel Barkalow, martin f krafft, git,
Thomas Harning Jr., Francis Moreau, Nicolas Vilz,
David Härdeman
In-Reply-To: <7vk5qq3y76.fsf@gitster.siamese.dyndns.org>
On Sun, 16 Sep 2007, Junio C Hamano wrote:
> david@lang.hm writes:
>
>> On Sun, 16 Sep 2007, Junio C Hamano wrote:
>>>
>>> I do not think there is nothing that writes file contents to
>>> stdout/pager other than "git cat-file" or "git show"; I do not
>>> think they are what you have in mind when talking about managing
>>> the files under /etc. So unfortunately I do not understand the
>>> rest of the discussion you made in your message.
>>
>> Ok, I thought that there was common code for these different
>> uses. could you re-read the rest of the logic based on the change
>> being done in checkout_entry?
>>
>> if you are unwilling to have any changes made to the checkout_entry
>> code then the only remaing question is what you think of Daniel's
>> suggestion to have a hook to replace check_updates()?
>>
>> if it's not acceptable either then we are down to doing a
>> post-checkout trigger.
>
> Post-checkout trigger is something I can say I can live with
> without looking at the actual patch, but that does not mean it
> would be a better approach at all.
we agree on this much at least :-)
> I would not be able to answer the first question right now; that
> needs a patch to prove that it can be done with a well contained
> set of changes that results in a maintainable code.
you cannot answer the question in the affirmitive, but you could say that
any changes in that area would be completely unacceptable to you (and for
a while it sounded like you were saying exactly that). in which case any
effort put into preparing patches would be a waste of time
> I haven't tried to assess the potential extent of damage needed
> to checkout_entry(), and I have never been interested in this
> "keeping track of /etc in place" topic myself. It is unlikely
> I'll try to come up with such a patch on my own to support it at
> such a low level near the core. Somebody who cares about that
> feature needs to take the initiative of doing that work before
> we can discuss and decide, although older-times including myself
> can help spot potential issues.
>
> So while I admit I am skeptical, consider me neither willing nor
> unwilling at this point.
this is reasonable. thanks for pointing me so clearly at the routine that
needs to be modified.
David Lang
^ permalink raw reply
* Re: [PATCH 2/3] git-apply: complain about >=8 consecutive spaces in initial indent
From: J. Bruce Fields @ 2007-09-17 2:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vy7f63zr4.fsf@gitster.siamese.dyndns.org>
On Sun, Sep 16, 2007 at 05:24:31PM -0700, Junio C Hamano wrote:
> "J. Bruce Fields" <bfields@citi.umich.edu> writes:
>
> > Complain if we find 8 spaces or more in a row as part of the initial
> > whitespace on a line, and (with --whitespace=stripspace) replace such by
> > a tab.
> >
> > Well, linux's checkpatch.pl complains about this sort of thing.
>
> Some people program in Python, so I am afraid that this needs to
> be a separate option.
OK.
> Maybe it is time to redo the --whitespace options as bitmasks so
> that we can say --whitespace-fix=tab,tail,lines to pick and
> choose which kinds of breakage to fix?
OK. Or maybe keep the current commandline options and have a
whitespace-style config option someplace?
I'm afraid I won't get to either anytime soon, though, so that project's
up for grabs....
--b.
^ permalink raw reply
* Re: [PATCH 2/3] git-apply: complain about >=8 consecutive spaces in initial indent
From: J. Bruce Fields @ 2007-09-17 2:45 UTC (permalink / raw)
To: Martin Langhoff; +Cc: Junio C Hamano, git
In-Reply-To: <46a038f90709161624j6eb55de6m61aab9e585e22a05@mail.gmail.com>
On Mon, Sep 17, 2007 at 11:24:12AM +1200, Martin Langhoff wrote:
> On 9/17/07, J. Bruce Fields <bfields@citi.umich.edu> wrote:
> > Complain if we find 8 spaces or more in a row as part of the initial
> > whitespace on a line, and (with --whitespace=stripspace) replace such by
> > a tab.
>
> I do quite a bit of hacking on "spaces-for-indentation" projects and
> still use stripspace to cleanup my patches. So no, thanks.
OK, fair enough.
--b.
^ permalink raw reply
* Re: [PATCH 1/3] rev-list --bisect: Move finding bisection into do_find_bisection.
From: Christian Couder @ 2007-09-17 3:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vk5qr575a.fsf@gitster.siamese.dyndns.org>
Le dimanche 16 septembre 2007, Junio C Hamano a écrit :
> > +static struct commit_list *do_find_bisection(struct commit_list *list,
> > + int nr, int *weights);
> > +
> > /*
> > * zero or positive weight is the number of interesting commits it can
> > * reach, including itself. Especially, weight = 0 means it does not
>
> The comment whose top part we can see here talks about the magic
> values -1 and -2 used while do_find_bisection() after the
> refactoring does its work, and these magic values are never
> visible to the calling function. You should move the comment to
> the top of do_find_bisection() as well.
>
> Also this forward declaration is unwarranted. A bottom-up
> sequence to define do_find_bisection() first, then to define its
> sole caller find_bisection() next is easier to read at least for
> me.
>
> The latter comment also applies to your other patch.
All right, I will send new patchs with these changes.
Thanks,
Christian.
^ permalink raw reply
* Re: git-gui i18n status?
From: Shawn O. Pearce @ 2007-09-17 3:20 UTC (permalink / raw)
To: Christian Stimming; +Cc: Johannes Schindelin, Junio C Hamano, git
In-Reply-To: <200709161403.50780.stimming@tuhh.de>
Christian Stimming <stimming@tuhh.de> wrote:
> One question came up when seeing the i18n code really in git-gui.git: How are
> translators supposed to submit new or updated translations? Is
> git-gui-i18n.git of any use anymore? This doesn't seem so. Should updated
> translations just be submitted by email to git@vger? In any case, the
> instructions in po/README should probably be updated to explain the
> recommended way of submitting translation updates.
I was sort of hoping Dscho would be able to answer that. ;-)
I can play patch-monkey and apply things people send to the mailing
list. I'm also willing to pull from a tree if the commit history
is clean and mergable. Since each language more or less stands
on its own in its own .po file translators may find it easier to
email patches. I dunno, I'm not a translator.
> Oh, and po/git-gui.pot should probably be updated to reflect the latest string
> additions and changes.
Yes. Dscho was looking at creating a custom diff filter for git
that would better handle showing diffs here. I was sort of waiting
for progress from that (if any) before doing the pot update. I also
have a lot of UI work that I wanted to do in the 0.9.x series and
those are likely to create/change the sets of messages we need
to translate.
--
Shawn.
^ permalink raw reply
* [PATCH 1/3] rev-list --bisect: Move finding bisection into do_find_bisection.
From: Christian Couder @ 2007-09-17 3:28 UTC (permalink / raw)
To: Junio Hamano; +Cc: git
This factorises some code and make a big function smaller.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
builtin-rev-list.c | 90 +++++++++++++++++++++++++++------------------------
1 files changed, 48 insertions(+), 42 deletions(-)
This patch series is a resend with the changes Junio asked for.
diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index ac551d5..2dae287 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -268,39 +268,12 @@ static void show_list(const char *debug, int counted, int nr,
* unknown. After running count_distance() first, they will get zero
* or positive distance.
*/
-
-static struct commit_list *find_bisection(struct commit_list *list,
- int *reaches, int *all)
+static struct commit_list *do_find_bisection(struct commit_list *list,
+ int nr, int *weights)
{
- int n, nr, on_list, counted, distance;
- struct commit_list *p, *best, *next, *last;
- int *weights;
-
- show_list("bisection 2 entry", 0, 0, list);
-
- /*
- * Count the number of total and tree-changing items on the
- * list, while reversing the list.
- */
- for (nr = on_list = 0, last = NULL, p = list;
- p;
- p = next) {
- unsigned flags = p->item->object.flags;
+ int n, counted, distance;
+ struct commit_list *p, *best;
- next = p->next;
- if (flags & UNINTERESTING)
- continue;
- p->next = last;
- last = p;
- if (!revs.prune_fn || (flags & TREECHANGE))
- nr++;
- on_list++;
- }
- list = last;
- show_list("bisection 2 sorted", 0, nr, list);
-
- *all = nr;
- weights = xcalloc(on_list, sizeof(*weights));
counted = 0;
for (n = 0, p = list; p; p = p->next) {
@@ -357,12 +330,8 @@ static struct commit_list *find_bisection(struct commit_list *list,
weight_set(p, distance);
/* Does it happen to be at exactly half-way? */
- if (halfway(p, distance, nr)) {
- p->next = NULL;
- *reaches = distance;
- free(weights);
+ if (halfway(p, distance, nr))
return p;
- }
counted++;
}
@@ -400,12 +369,8 @@ static struct commit_list *find_bisection(struct commit_list *list,
/* Does it happen to be at exactly half-way? */
distance = weight(p);
- if (halfway(p, distance, nr)) {
- p->next = NULL;
- *reaches = distance;
- free(weights);
+ if (halfway(p, distance, nr))
return p;
- }
}
}
@@ -425,12 +390,53 @@ static struct commit_list *find_bisection(struct commit_list *list,
if (distance > counted) {
best = p;
counted = distance;
- *reaches = weight(p);
}
}
+ return best;
+}
+
+static struct commit_list *find_bisection(struct commit_list *list,
+ int *reaches, int *all)
+{
+ int nr, on_list;
+ struct commit_list *p, *best, *next, *last;
+ int *weights;
+
+ show_list("bisection 2 entry", 0, 0, list);
+
+ /*
+ * Count the number of total and tree-changing items on the
+ * list, while reversing the list.
+ */
+ for (nr = on_list = 0, last = NULL, p = list;
+ p;
+ p = next) {
+ unsigned flags = p->item->object.flags;
+
+ next = p->next;
+ if (flags & UNINTERESTING)
+ continue;
+ p->next = last;
+ last = p;
+ if (!revs.prune_fn || (flags & TREECHANGE))
+ nr++;
+ on_list++;
+ }
+ list = last;
+ show_list("bisection 2 sorted", 0, nr, list);
+
+ *all = nr;
+ weights = xcalloc(on_list, sizeof(*weights));
+
+ /* Do the real work of finding bisection commit. */
+ best = do_find_bisection(list, nr, weights);
+
if (best)
best->next = NULL;
+
+ *reaches = weight(best);
free(weights);
+
return best;
}
--
1.5.3.1.59.g93705
^ permalink raw reply related
* [PATCH 2/3] rev-list --bisect: Move some bisection code into best_bisection.
From: Christian Couder @ 2007-09-17 3:28 UTC (permalink / raw)
To: Junio Hamano; +Cc: git
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
builtin-rev-list.c | 43 ++++++++++++++++++++++++++-----------------
1 files changed, 26 insertions(+), 17 deletions(-)
diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index 2dae287..8c9635a 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -255,6 +255,30 @@ static void show_list(const char *debug, int counted, int nr,
}
#endif /* DEBUG_BISECT */
+static struct commit_list *best_bisection(struct commit_list *list, int nr)
+{
+ struct commit_list *p, *best;
+ int best_distance = -1;
+
+ best = list;
+ for (p = list; p; p = p->next) {
+ int distance;
+ unsigned flags = p->item->object.flags;
+
+ if (revs.prune_fn && !(flags & TREECHANGE))
+ continue;
+ distance = weight(p);
+ if (nr - distance < distance)
+ distance = nr - distance;
+ if (distance > best_distance) {
+ best = p;
+ best_distance = distance;
+ }
+ }
+
+ return best;
+}
+
/*
* zero or positive weight is the number of interesting commits it can
* reach, including itself. Especially, weight = 0 means it does not
@@ -272,7 +296,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
int nr, int *weights)
{
int n, counted, distance;
- struct commit_list *p, *best;
+ struct commit_list *p;
counted = 0;
@@ -377,22 +401,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
show_list("bisection 2 counted all", counted, nr, list);
/* Then find the best one */
- counted = -1;
- best = list;
- for (p = list; p; p = p->next) {
- unsigned flags = p->item->object.flags;
-
- if (revs.prune_fn && !(flags & TREECHANGE))
- continue;
- distance = weight(p);
- if (nr - distance < distance)
- distance = nr - distance;
- if (distance > counted) {
- best = p;
- counted = distance;
- }
- }
- return best;
+ return best_bisection(list, nr);
}
static struct commit_list *find_bisection(struct commit_list *list,
--
1.5.3.1.59.g93705
^ permalink raw reply related
* [PATCH 3/3] rev-list --bisect: Bisection "distance" clean up.
From: Christian Couder @ 2007-09-17 3:28 UTC (permalink / raw)
To: Junio Hamano; +Cc: git
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
builtin-rev-list.c | 18 +++++++-----------
1 files changed, 7 insertions(+), 11 deletions(-)
diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index 8c9635a..899a31d 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -189,7 +189,7 @@ static int count_interesting_parents(struct commit *commit)
return count;
}
-static inline int halfway(struct commit_list *p, int distance, int nr)
+static inline int halfway(struct commit_list *p, int nr)
{
/*
* Don't short-cut something we are not going to return!
@@ -202,8 +202,7 @@ static inline int halfway(struct commit_list *p, int distance, int nr)
* 2 and 3 are halfway of 5.
* 3 is halfway of 6 but 2 and 4 are not.
*/
- distance *= 2;
- switch (distance - nr) {
+ switch (2 * weight(p) - nr) {
case -1: case 0: case 1:
return 1;
default:
@@ -295,7 +294,7 @@ static struct commit_list *best_bisection(struct commit_list *list, int nr)
static struct commit_list *do_find_bisection(struct commit_list *list,
int nr, int *weights)
{
- int n, counted, distance;
+ int n, counted;
struct commit_list *p;
counted = 0;
@@ -346,15 +345,13 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
for (p = list; p; p = p->next) {
if (p->item->object.flags & UNINTERESTING)
continue;
- n = weight(p);
- if (n != -2)
+ if (weight(p) != -2)
continue;
- distance = count_distance(p);
+ weight_set(p, count_distance(p));
clear_distance(list);
- weight_set(p, distance);
/* Does it happen to be at exactly half-way? */
- if (halfway(p, distance, nr))
+ if (halfway(p, nr))
return p;
counted++;
}
@@ -392,8 +389,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
weight_set(p, weight(q));
/* Does it happen to be at exactly half-way? */
- distance = weight(p);
- if (halfway(p, distance, nr))
+ if (halfway(p, nr))
return p;
}
}
--
1.5.3.1.59.g93705
^ permalink raw reply related
* rename detection limit checking, cherry picking, and git am -3
From: Mark Levedahl @ 2007-09-17 3:32 UTC (permalink / raw)
To: Git Mailing List
Linus' recent patch to invoke limiting on rename detection broke my
ability to use cherry-picking on one project. This project has about
4300 files on one branch (a), 2500 on a later branch (b), 226 commits in
total between the two branches, and a convoluted history of how branch a
morphed into branch b. About 50 files were renamed in the transition,
and we need to migrate patches from the still maintained branch a onto
the new branch b.
Prior to Linus' recent patch to limit rename detection (0024a549),
cherry picking a patch from a to b, where the patch affected just one
file, often took about 45 seconds on a 3 GHz pentium 4 with the CPU
pegged at 100% for the duration. The cherry picking always succeeded and
correctly followed renames, but was very slow.
Following Linus' patch, the cherry picking fails with a merge conflict
(almost instantly), complaining the file has been deleted on b but
modified on a, i.e., the rename detection does not work. I tried raising
diff.renameLimit to 100000, that seems to have no effect whatsoever on
cherry-pick (the process aborts with a conflict almost immediately).
Curiously, using "git format-patch x..y --stdout | git am -3" succeeds
in this case, and runs in well less than a second. This performance
seems unchanged by the rename detection limit patch.
So, the rename limit patch "broke" git for this usage, though one could
reasonably argue the previous code was so slow as to be broken anyway.
The curious thing to me is the vast superiority of whatever
git-format-patch|git-am -3 does, and I wonder if that isn't a
fundementally better design for cherry picking than git-cherry-pick
implements (it obviously is for this case).
Mark
^ permalink raw reply
* Re: rename detection limit checking, cherry picking, and git am -3
From: Shawn O. Pearce @ 2007-09-17 3:47 UTC (permalink / raw)
To: Mark Levedahl; +Cc: Git Mailing List
In-Reply-To: <46EDF54F.5030503@gmail.com>
Mark Levedahl <mlevedahl@gmail.com> wrote:
> The curious thing to me is the vast superiority of whatever
> git-format-patch|git-am -3 does, and I wonder if that isn't a
> fundementally better design for cherry picking than git-cherry-pick
> implements (it obviously is for this case).
In this case `git am -3` creates a tree object containing only
the files modified by the patch and then feeds that tree into
git-merge-recursive. Now if you go study git-revert's code you'll
see it actually just calls git-merge-recursive on three trees,
but these are three complete trees.
So what's probably happening here is there's less candidates on one
side in the `am -3` case, so we spend a lot less time generating
the rename matrix, searching for a match, and we get better changes
of finding a match.
I actually don't see why cherry-pick can't be defined in terms
of `format-patch|am -3`. It probably would be faster in almost
all cases.
--
Shawn.
^ permalink raw reply
* Re: metastore
From: Junio C Hamano @ 2007-09-17 4:23 UTC (permalink / raw)
To: david
Cc: Johannes Schindelin, Daniel Barkalow, martin f krafft, git,
Thomas Harning Jr., Francis Moreau, Nicolas Vilz,
David Härdeman
In-Reply-To: <Pine.LNX.4.64.0709161925000.24221@asgard.lang.hm>
david@lang.hm writes:
>> Post-checkout trigger is something I can say I can live with
>> without looking at the actual patch, but that does not mean it
>> would be a better approach at all.
>
> we agree on this much at least :-)
>
>> I would not be able to answer the first question right now; that
>> needs a patch to prove that it can be done with a well contained
>> set of changes that results in a maintainable code.
>
> you cannot answer the question in the affirmitive, but you could say
> that any changes in that area would be completely unacceptable to you
> (and for a while it sounded like you were saying exactly that). in
> which case any effort put into preparing patches would be a waste of
> time
I tend to disagree. It's far from a waste of time. While, as I
said, I am skeptical that such a patch would be small impact, if
it helps people's needs, somebody will pick it up and carry
forward, even if that somebody is not me. It can then mature
out of tree and later could be merged. We simply do not know
unless somebody tries. And I am quite happy that you seem to be
motivated enough to see how it goes.
On the other hand, the experiment could fail and you may end up
with a patch that is too messy to be acceptable, in which case
you might feel it a waste of time, but I do not think it is a
waste even in such a case. We would learn what works and what
doesn't, and we can bury "keeping track of /etc" topic to rest.
I also need to rant here a bit.
Fortunately we haven't had this problem too many times on this
list, but sometimes people say "Here is my patch. If this is
accepted I'll add documentation and tests". I rarely reply to
such patches without sugarcoating my response, but my internal
reaction is, "Don't you, as the person who proposes that change,
believe in your patch deeply enough to be willing to perfect it,
in order to make it suitable for consumption by the general
public, whether it is included in my tree or not? A change that
even you do not believe in yourself has very little chance of
benefitting the general public, so thanks but no thanks, I'll
pass."
^ 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