* builtin-apply whitespace
From: J. Bruce Fields @ 2007-12-16 17:58 UTC (permalink / raw)
To: Wincent Colaiuta; +Cc: git, Junio C Hamano
In-Reply-To: <1197822702-5262-7-git-send-email-bfields@citi.umich.edu>
Apologies, I just realized I also forgot to do the corresponding fix on
the apply side. Here they are. These are also available at
git://linux-nfs.org/~bfields/git.git master
--b.
^ permalink raw reply
* Re: [PATCH] whitespace: fix initial-indent checking
From: Jakub Narebski @ 2007-12-16 18:16 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Wincent Colaiuta, Git Mailing List, Junio Hamano
In-Reply-To: <20071216162637.GA3934@fieldses.org>
J. Bruce Fields wrote:
> On Sun, Dec 16, 2007 at 11:00:55AM +0100, Wincent Colaiuta wrote:
>> El 16/12/2007, a las 10:08, Jakub Narebski escribió:
>>
>>> J. Bruce Fields wrote:
>>>
>>>> This allows catching initial indents like '\t ' (a tab followed
>>>> by 8 spaces), while previously indent-with-non-tab caught only indents
>>>> that consisted entirely of spaces.
>>>
>>> I prefer to use tabs for indent, but _spaces_ for align. While previous,
>>> less strict version of check catches indent using spaces, this one also
>>> catches _align_ using spaces.
>
> No, the previous version didn't work for the align-with-spaces case
> either. Consider, for example,
>
> struct widget *find_widget_by_color(struct color *color,
> int nth_match, unsigned long flags)
>
> If following a "indent-with-tabs, align-with-spaces" policy, then the
> initial whitespaace on the second line should be purely spaces
> (otherwise adjusting the tab stops would ruin the alignment). But
> indent-with-non-tab would flag this as incorrect even before my fix.
Yes, this is (if we want "indent with tab, align with spaces") false
positive even with current version of indent-with-non-tab policy, but
it is _rare_ false positive.
It is useful because it catches quite common "indent with spaces only",
for example if MTA or editor replaces tabs with spaces, or if editor
preserves whitespace but it uses spaces for indent.
So for me this version is a good compromise between false positives
and catching real indent whitespace errors. The version proposed has
IMHO too many false positive, while I guess not catching much more
errors in practice.
>> I'd say that Jakub's is a fairly common use case (it's used in many places
>> in the Git codebase too, I think) so it would be a bad thing to change the
>> behaviour of "indent-with-non-tab".
>>
>> If you also want to check for "align-with-non-tab" then it really should be
>> a separate, optional class of whitespace error.
>
> I would agree with you if it were not for the fact that if you're using
> an "indent-with-tabs, align-with-spaces" policy then the only indent
> whitespace problems that you can flag automatically are space-before-tab
> problems; anything else requires knowledge of the language syntax.
Unfortunately quite true (by the way, doesn't new version of
"align-with-non-tab" do not work for Python sources?)
Perhaps it should be called "no-8spaces" os something like that: is the
width (in columns) of a tab character configurable, by the way?
> So indent-with-non-tab has only ever been useful for projects that
> insist on tabs for all sequences of 8 spaces in the initial whitespace.
IMVVHO the new version of "indent-with-non-tab" (aka "no-8-spaces") is
useful _only_ for such project, while old version not only (see comment
above).
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCH] whitespace: fix initial-indent checking
From: Jakub Narebski @ 2007-12-16 18:24 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Wincent Colaiuta, Git Mailing List, Junio Hamano
In-Reply-To: <200712161916.44715.jnareb@gmail.com>
By the way, I have just [a beginning of] an idea: what if we look at
neighbour lines to try to detect whitespace errors? For example when
using tabs for indent, spaces for align, I think it is an error if
spaces sequence after tab begins earlier than tab sequence in neighbour
line ends.
What do you think about it?
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCH] threaded pack-objects: Use condition variables for thread communication.
From: Johannes Sixt @ 2007-12-16 18:41 UTC (permalink / raw)
To: Peter Baumann; +Cc: git, Nicolas Pitre, Junio C Hamano
In-Reply-To: <20071216120558.GA4999@xp.machine.xx>
On Sunday 16 December 2007 13:05, Peter Baumann wrote:
> On Sun, Dec 16, 2007 at 12:18:53AM +0100, Johannes Sixt wrote:
> > +
> > + progress_lock();
> > + me->working = 0;
> > + progress_unlock();
> > + pthread_cond_signal(&progress_cond);
>
> Shouldn't the pthread_cond_signal be inside the lock?
> e.g. swap progress_unlock() with pthread_cond_signal(&progress_cond)
No, that's not necessary. Both ways are correct, but if it's outside the lock
there is less contention on the mutex (because the waiting thread must
acquire the mutex lock before it can return from pthread_cond_wait).
-- Hannes
^ permalink raw reply
* Re: [PATCH] threaded pack-objects: Use condition variables for thread communication.
From: Peter Baumann @ 2007-12-16 19:00 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git, Nicolas Pitre, Junio C Hamano
In-Reply-To: <200712161941.38109.johannes.sixt@telecom.at>
On Sun, Dec 16, 2007 at 07:41:37PM +0100, Johannes Sixt wrote:
> On Sunday 16 December 2007 13:05, Peter Baumann wrote:
> > On Sun, Dec 16, 2007 at 12:18:53AM +0100, Johannes Sixt wrote:
> > > +
> > > + progress_lock();
> > > + me->working = 0;
> > > + progress_unlock();
> > > + pthread_cond_signal(&progress_cond);
> >
> > Shouldn't the pthread_cond_signal be inside the lock?
> > e.g. swap progress_unlock() with pthread_cond_signal(&progress_cond)
>
> No, that's not necessary. Both ways are correct, but if it's outside the lock
> there is less contention on the mutex (because the waiting thread must
> acquire the mutex lock before it can return from pthread_cond_wait).
>
At least I was told otherwise and [1] backs my knowledge up. Are you
really sure?
-Peter
http://docs.sun.com/app/docs/doc/806-5257/6je9h032r?a=view#sync-53686
^ permalink raw reply
* Re: [PATCH] threaded pack-objects: Use condition variables for thread communication.
From: David Brown @ 2007-12-16 19:18 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Peter Baumann, git, Nicolas Pitre, Junio C Hamano
In-Reply-To: <200712161941.38109.johannes.sixt@telecom.at>
On Sun, Dec 16, 2007 at 07:41:37PM +0100, Johannes Sixt wrote:
>On Sunday 16 December 2007 13:05, Peter Baumann wrote:
>> On Sun, Dec 16, 2007 at 12:18:53AM +0100, Johannes Sixt wrote:
>> > +
>> > + progress_lock();
>> > + me->working = 0;
>> > + progress_unlock();
>> > + pthread_cond_signal(&progress_cond);
>>
>> Shouldn't the pthread_cond_signal be inside the lock?
>> e.g. swap progress_unlock() with pthread_cond_signal(&progress_cond)
>
>No, that's not necessary. Both ways are correct, but if it's outside the lock
>there is less contention on the mutex (because the waiting thread must
>acquire the mutex lock before it can return from pthread_cond_wait).
The signal needs to be before the unlock if is more than one signaller.
Since normal usage has the signal inside of the lock, most implementations
should do this efficiently.
Dave
^ permalink raw reply
* Re: [PATCH 1/3] Revert changes and extend diff option documentation
From: Junio C Hamano @ 2007-12-16 19:43 UTC (permalink / raw)
To: Wincent Colaiuta; +Cc: git
In-Reply-To: <1197631424-52586-2-git-send-email-win@wincent.com>
Wincent Colaiuta <win@wincent.com> writes:
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index 9ecc1d7..54207f0 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -92,10 +92,10 @@ endif::git-format-patch[]
> file gives the default to do so.
>
> --check::
> - Warn if changes introduce trailing whitespace
> - or an indent that uses a space before a tab. Exits with
> - non-zero status if problems are found. Not compatible with
> - --exit-code.
> + Warn if changes introduce whitespace problems (such as
> + trailing whitespace). Configuration and per-path attributes
> + control what git classifies as a whitespace problem (see
> + gitlink:git-config[1] and gitlink:gitattributes[5]).
This is not quite right, is it? The command still exits with exit code.
It is just that the calling process does not see it if you let it spawn
the pager.
> @@ -197,8 +197,9 @@ endif::git-format-patch[]
>
> --exit-code::
> Make the program exit with codes similar to diff(1).
> - That is, it exits with 1 if there were differences and
> - 0 means no differences.
> + That is, it exits with 0 if there were no differences
> + and 1 if there were. If --check is used and the
> + differences introduce whitespace problems exits with 3.
This side is correct.
^ permalink raw reply
* Re: trim_common_tail bug?
From: Junio C Hamano @ 2007-12-16 19:43 UTC (permalink / raw)
To: Jeff King; +Cc: Linus Torvalds, git
In-Reply-To: <20071216070614.GA5072@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Sat, Dec 15, 2007 at 03:02:02PM -0500, Jeff King wrote:
>
>> > for (i = 0, recovered = 0; recovered < trimmed && i <= ctx; i++) {
>> > while (recovered < trimmed && ap[recovered++] != '\n')
>> > ; /* nothing */
>> > }
>> >
>> > then (warning: I haven't had my coffee yet)?
>>
>> Yes, I think that is equivalent. My sleep-deprived brain keeps thinking
>> there must be a more clear way of writing this whole loop, but it
>> escapes me at the moment.
>
> And this came to me in a dream. :) It fixes the bug, and I think it is a
> bit simpler to see the termination conditions in a single loop. But
> please double-check correctness, and that you agree that it is more
> readable.
I wanted to recover to the end of the line that includes the cut-off
point, even when (ctx == 0), to be extra safer, but I do not think it
was necessary.
> ...
> bp -= blk;
> }
>
> + while (recovered < trimmed && ctx)
> + if (ap[recovered++] == '\n')
> + ctx--;
> a->size -= (trimmed - recovered);
> b->size -= (trimmed - recovered);
> }
^ permalink raw reply
* Re: [PATCH] whitespace: fix initial-indent checking
From: Junio C Hamano @ 2007-12-16 19:43 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Wincent Colaiuta, Jakub Narebski, Git Mailing List
In-Reply-To: <20071216162637.GA3934@fieldses.org>
"J. Bruce Fields" <bfields@fieldses.org> writes:
>>> I prefer to use tabs for indent, but _spaces_ for align. While previous,
>>> less strict version of check catches indent using spaces, this one also
>>> catches _align_ using spaces.
>
> No, the previous version didn't work for the align-with-spaces case
> either.
When indent-with-non-tab is in effect, it should tabify spaces in indent
as much as possible. The error definition is deliberately incompatible
with "align with spaces" policy.
The reason I made indent-with-non-tab configurable was to cater to these
people. They can turn it off if they do not want it.
^ permalink raw reply
* [PATCH v2] threaded pack-objects: Use condition variables for thread communication.
From: Johannes Sixt @ 2007-12-16 19:45 UTC (permalink / raw)
To: Peter Baumann; +Cc: git, Nicolas Pitre, Junio C Hamano
In-Reply-To: <20071216190016.GC4999@xp.machine.xx>
In the threaded pack-objects code the main thread and the worker threads
must mutually signal that they have assigned a new pack of work or have
completed their work, respectively. Previously, the code used mutexes that
were locked in one thread and unlocked from a different thread, which is
bogus (and happens to work on Linux).
Here we rectify the implementation by using condition variables: There is
one condition variable on which the main thread waits until a thread
requests new work; and each worker thread has its own condition variable
on which it waits until it is assigned new work or signaled to terminate.
As a cleanup, the worker threads are spawned only after the initial work
packages have been assigned.
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
On Sunday 16 December 2007 20:00, Peter Baumann wrote:
> On Sun, Dec 16, 2007 at 07:41:37PM +0100, Johannes Sixt wrote:
> > On Sunday 16 December 2007 13:05, Peter Baumann wrote:
> > > On Sun, Dec 16, 2007 at 12:18:53AM +0100, Johannes Sixt wrote:
> > > > +
> > > > + progress_lock();
> > > > + me->working = 0;
> > > > + progress_unlock();
> > > > + pthread_cond_signal(&progress_cond);
> > >
> > > Shouldn't the pthread_cond_signal be inside the lock?
> > > e.g. swap progress_unlock() with pthread_cond_signal(&progress_cond)
> >
> > No, that's not necessary. Both ways are correct, but if it's outside the
> > lock there is less contention on the mutex (because the waiting thread
> > must acquire the mutex lock before it can return from pthread_cond_wait).
>
> At least I was told otherwise and [1] backs my knowledge up. Are you
> really sure?
>
> -Peter
>
> http://docs.sun.com/app/docs/doc/806-5257/6je9h032r?a=view#sync-53686
Uh, yes, that's a very compelling reason. I'm wrong. Here's an updated patch.
-- Hannes
builtin-pack-objects.c | 129 +++++++++++++++++++++++++++++------------------
1 files changed, 79 insertions(+), 50 deletions(-)
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 7dd0d7f..4715d5f 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1594,6 +1594,15 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,
#ifdef THREADED_DELTA_SEARCH
+/*
+ * The main thread waits on the condition that (at least) one of the workers
+ * has stopped working (which is indicated in the .working member of
+ * struct thread_params).
+ * When a work thread has completed its work, it sets .working to 0 and
+ * signals the main thread and waits on the condition that .data_ready
+ * becomes 1.
+ */
+
struct thread_params {
pthread_t thread;
struct object_entry **list;
@@ -1601,37 +1610,50 @@ struct thread_params {
unsigned remaining;
int window;
int depth;
+ int working;
+ int data_ready;
+ pthread_mutex_t mutex;
+ pthread_cond_t cond;
unsigned *processed;
};
-static pthread_mutex_t data_request = PTHREAD_MUTEX_INITIALIZER;
-static pthread_mutex_t data_ready = PTHREAD_MUTEX_INITIALIZER;
-static pthread_mutex_t data_provider = PTHREAD_MUTEX_INITIALIZER;
-static struct thread_params *data_requester;
+static pthread_cond_t progress_cond = PTHREAD_COND_INITIALIZER;
static void *threaded_find_deltas(void *arg)
{
struct thread_params *me = arg;
- for (;;) {
- pthread_mutex_lock(&data_request);
- data_requester = me;
- pthread_mutex_unlock(&data_provider);
- pthread_mutex_lock(&data_ready);
- pthread_mutex_unlock(&data_request);
-
- if (!me->remaining)
- return NULL;
-
+ while (me->remaining) {
find_deltas(me->list, &me->remaining,
me->window, me->depth, me->processed);
+
+ progress_lock();
+ me->working = 0;
+ pthread_cond_signal(&progress_cond);
+ progress_unlock();
+
+ /*
+ * We must not set ->data_ready before we wait on the
+ * condition because the main thread may have set it to 1
+ * before we get here. In order to be sure that new
+ * work is available if we see 1 in ->data_ready, it
+ * was initialized to 0 before this thread was spawned
+ * and we reset it to 0 right away.
+ */
+ pthread_mutex_lock(&me->mutex);
+ while (!me->data_ready)
+ pthread_cond_wait(&me->cond, &me->mutex);
+ me->data_ready = 0;
+ pthread_mutex_unlock(&me->mutex);
}
+ /* leave ->working 1 so that this doesn't get more work assigned */
+ return NULL;
}
static void ll_find_deltas(struct object_entry **list, unsigned list_size,
int window, int depth, unsigned *processed)
{
- struct thread_params *target, p[delta_search_threads];
+ struct thread_params p[delta_search_threads];
int i, ret, active_threads = 0;
if (delta_search_threads <= 1) {
@@ -1639,49 +1661,42 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,
return;
}
- pthread_mutex_lock(&data_provider);
- pthread_mutex_lock(&data_ready);
-
- /* Start work threads. */
+ /* Partition the work amongst work threads. */
for (i = 0; i < delta_search_threads; i++) {
+ unsigned sub_size = list_size / (delta_search_threads - i);
+
p[i].window = window;
p[i].depth = depth;
p[i].processed = processed;
- p[i].remaining = 0;
- ret = pthread_create(&p[i].thread, NULL,
- threaded_find_deltas, &p[i]);
- if (ret)
- die("unable to create thread: %s", strerror(ret));
- active_threads++;
- }
-
- /* Then partition the work amongst them. */
- for (i = 0; i < delta_search_threads; i++) {
- unsigned sub_size = list_size / (delta_search_threads - i);
-
- pthread_mutex_lock(&data_provider);
- target = data_requester;
- if (!sub_size) {
- pthread_mutex_unlock(&data_ready);
- pthread_join(target->thread, NULL);
- active_threads--;
- continue;
- }
+ p[i].working = 1;
+ p[i].data_ready = 0;
+ pthread_mutex_init(&p[i].mutex, NULL);
+ pthread_cond_init(&p[i].cond, NULL);
/* try to split chunks on "path" boundaries */
while (sub_size < list_size && list[sub_size]->hash &&
list[sub_size]->hash == list[sub_size-1]->hash)
sub_size++;
- target->list = list;
- target->list_size = sub_size;
- target->remaining = sub_size;
- pthread_mutex_unlock(&data_ready);
+ p[i].list = list;
+ p[i].list_size = sub_size;
+ p[i].remaining = sub_size;
list += sub_size;
list_size -= sub_size;
}
+ /* Start work threads. */
+ for (i = 0; i < delta_search_threads; i++) {
+ if (!p[i].list_size)
+ continue;
+ ret = pthread_create(&p[i].thread, NULL,
+ threaded_find_deltas, &p[i]);
+ if (ret)
+ die("unable to create thread: %s", strerror(ret));
+ active_threads++;
+ }
+
/*
* Now let's wait for work completion. Each time a thread is done
* with its work, we steal half of the remaining work from the
@@ -1690,13 +1705,21 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,
* until the remaining object list segments are simply too short
* to be worth splitting anymore.
*/
- do {
+ while (active_threads) {
+ struct thread_params *target = NULL;
struct thread_params *victim = NULL;
unsigned sub_size = 0;
- pthread_mutex_lock(&data_provider);
- target = data_requester;
progress_lock();
+ for (;;) {
+ for (i = 0; !target && i < delta_search_threads; i++)
+ if (!p[i].working)
+ target = &p[i];
+ if (target)
+ break;
+ pthread_cond_wait(&progress_cond, &progress_mutex);
+ };
+
for (i = 0; i < delta_search_threads; i++)
if (p[i].remaining > 2*window &&
(!victim || victim->remaining < p[i].remaining))
@@ -1723,17 +1746,23 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,
victim->list_size -= sub_size;
victim->remaining -= sub_size;
}
- progress_unlock();
-
target->list_size = sub_size;
target->remaining = sub_size;
- pthread_mutex_unlock(&data_ready);
+ target->working = 1;
+ progress_unlock();
+
+ pthread_mutex_lock(&target->mutex);
+ target->data_ready = 1;
+ pthread_cond_signal(&target->cond);
+ pthread_mutex_unlock(&target->mutex);
if (!sub_size) {
pthread_join(target->thread, NULL);
+ pthread_cond_destroy(&target->cond);
+ pthread_mutex_destroy(&target->mutex);
active_threads--;
}
- } while (active_threads);
+ }
}
#else
--
1.5.3.6.954.g0c63
^ permalink raw reply related
* Re: [PATCH] whitespace: fix initial-indent checking
From: J. Bruce Fields @ 2007-12-16 19:59 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Wincent Colaiuta, Git Mailing List, Junio Hamano
In-Reply-To: <200712161916.44715.jnareb@gmail.com>
On Sun, Dec 16, 2007 at 07:16:44PM +0100, Jakub Narebski wrote:
> J. Bruce Fields wrote:
> > No, the previous version didn't work for the align-with-spaces case
> > either. Consider, for example,
> >
> > struct widget *find_widget_by_color(struct color *color,
> > int nth_match, unsigned long flags)
> >
> > If following a "indent-with-tabs, align-with-spaces" policy, then the
> > initial whitespaace on the second line should be purely spaces
> > (otherwise adjusting the tab stops would ruin the alignment). But
> > indent-with-non-tab would flag this as incorrect even before my fix.
>
> Yes, this is (if we want "indent with tab, align with spaces") false
> positive even with current version of indent-with-non-tab policy, but
> it is _rare_ false positive.
You can find examples like the above all over the git source, even in C
where the top-level code is indented in main().
> It is useful because it catches quite common "indent with spaces only",
> for example if MTA or editor replaces tabs with spaces, or if editor
> preserves whitespace but it uses spaces for indent.
>
> So for me this version is a good compromise between false positives
> and catching real indent whitespace errors. The version proposed has
> IMHO too many false positive, while I guess not catching much more
> errors in practice.
Unfortunately, this compromise wouldn't solve my problem.
Which is: I do get the occasional kernel patch with whitespace problems
uncaught by git's existing checks. It's annoying to have to fix them up
manually (but wastes Andrew Morton's time if I don't). This shouldn't
be necessary, because the kernel has a simple policy for initial
whitespace that is completely automatable.
If we've got to define a fourth whitespace policy for this, well, OK,
I'll live--tell me what I need to do. I haven't seen a convincing
argument for that yet, though.
--b.
^ permalink raw reply
* Re: [PATCH] Add an "-i" option to git-reset, to confirm a reset.
From: Junio C Hamano @ 2007-12-16 20:09 UTC (permalink / raw)
To: Kelvie Wong; +Cc: Johannes Schindelin, git
In-Reply-To: <94ccbe710712151946u22f02a8fkbc3c4cbc96ee22f5@mail.gmail.com>
"Kelvie Wong" <kelvie@ieee.org> writes:
> On Dec 15, 2007 7:35 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> ..
>> I am slightly negative on this patch. Not only do I think that it is both
>> easier and more natural to run diff/status/an-alias to see what a reset
>> would do, but the patch only handles the index_file part (missing the --
>> <file> part AFAICT).
I am in principle very negative on additional option that does the same
thing as what the users on odd occasions can run a separate command
themselves to achieve, and I think "reset -i" falls into that category.
And I am negative on this "-i" not just because I think that would be
only in "odd occasions" (i.e. rare), but because I think it would not
help much. Either you are sure about resetting, in which case you would
not even use "-i" option (and not get this safety), or you are unsure,
in which case you can do "git status" or whatever commands that are
already available.
> w.r.t. the style, you were referring to just the array initializers
> right? Or was there something else I did that doesn't look right?
I spotted only two classes.
+
+ /* Show the user what is about to be reset, and in more detail, if they
+ * like. */
/*
* Show the user what is about to be reset, and in more detail,
* if they like.
*/
+ if(confirm_reset) {
if (confirm_reset) {
^ permalink raw reply
* Re: [PATCH] whitespace: fix initial-indent checking
From: Jakub Narebski @ 2007-12-16 20:31 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Wincent Colaiuta, Git Mailing List, Junio Hamano
In-Reply-To: <20071216195956.GA14676@fieldses.org>
On Sun, 16 Dec 2007, J. Bruce Fields wrote:
> If we've got to define a fourth whitespace policy for this, well, OK,
> I'll live--tell me what I need to do. I haven't seen a convincing
> argument for that yet, though.
Perhaps the new version of policy should be called indent-with-non-tab,
and we keep old version under name indent-with-spaces, hmmm...?
By the way, what about check for diff3/rcsmerge conflict markers?
This is not "whitespace" error per se, but...
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCH] whitespace: fix initial-indent checking
From: Junio C Hamano @ 2007-12-16 20:40 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: git
In-Reply-To: <20071216035440.GM14377@fieldses.org>
"J. Bruce Fields" <bfields@fieldses.org> writes:
> One slightly weird thing about this: I'd expect indent-with-non-tab to
> catch any sequence of 8 or more contiguous spaces, not just such
> sequences at the end of the indent. This doesn't quite do that.
+-------+-------+-------+-------+-------+-------+-------+-------+-------+
Personally, I would hate that. That would muck with two spaces I
deliberately typed after the full stop before this sentence). Please
don't.
Emacs "M-x tabify" tends to do this and I found it unsuitable especially
for code (I am not complaining, it probably was invented for other
purposes and not reformatting code):
If you have original (the run of '>>..>>' is a single tab, '.' is a SP)
dcba....123
fedcba..123
gfedcba.123
and "tabify" the region, you would get:
dcba>>>>123
fedcba>>123
gfedcba.123
That is fine if you are shooting for minimum number of bytes, but often
it is not what you want in your code, especially when the part that
conains the whitespace "..cba 123.." is inside a string constant.
^ permalink raw reply
* Re: [PATCH] whitespace: fix initial-indent checking
From: Junio C Hamano @ 2007-12-16 21:00 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Jakub Narebski, Wincent Colaiuta, Git Mailing List
In-Reply-To: <20071216195956.GA14676@fieldses.org>
"J. Bruce Fields" <bfields@fieldses.org> writes:
> Which is: I do get the occasional kernel patch with whitespace problems
> uncaught by git's existing checks. It's annoying to have to fix them up
> manually (but wastes Andrew Morton's time if I don't). This shouldn't
> be necessary, because the kernel has a simple policy for initial
> whitespace that is completely automatable.
>
> If we've got to define a fourth whitespace policy for this, well, OK,
> I'll live--tell me what I need to do. I haven't seen a convincing
> argument for that yet, though.
There is one "fix" you earlier implemented but there is no way to check
in the current infrastructure of checking one line at a time. A run of
blank lines at the end of file.
I personallyy find it much more interesting and useful than the "align
with space" discussion.
^ permalink raw reply
* Re: [PATCH] whitespace: reorganize initial-indent check
From: Junio C Hamano @ 2007-12-16 21:06 UTC (permalink / raw)
To: Wincent Colaiuta; +Cc: J. Bruce Fields, Git Mailing List
In-Reply-To: <B54C9483-90BE-4B45-A3B7-39FACF0E9F62@wincent.com>
Wincent Colaiuta <win@wincent.com> writes:
> El 16/12/2007, a las 4:48, J. Bruce Fields escribi> Reorganize to emphasize the most complicated part of the code (the tab
>> case).
>
> Any chance of either squashing this series into one patch seeing as
> its all churning over the same part of the code, or resending it with
> numbering? The patches seemed to arrive out of order in my mailbox and
> I don't really know what order they're supposed to be applied in and
> it's a bit hard to review.
I do not think squashing is necessary or a good idea in this case. As
far as I can tell, the series does not have "oops, this fixes the
earlier problem I introduced in the series", but it is purely a logical
progression. The first one fixes a bug introduced by the 0-base
conversion which can and should stand on its own.
The mails were properly threaded with in-reply-to so numbering is not
strictly necessary either, but would have helped readers with MUA that
do not pay attention to that header.
I already see JBF resent the series, which is very nice of him.
^ permalink raw reply
* Re: trim_common_tail bug?
From: Junio C Hamano @ 2007-12-16 21:16 UTC (permalink / raw)
To: Jeff King; +Cc: Linus Torvalds, git
In-Reply-To: <7v8x3ul927.fsf@gitster.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Jeff King <peff@peff.net> writes:
>
>> On Sat, Dec 15, 2007 at 03:02:02PM -0500, Jeff King wrote:
>>
>>> > for (i = 0, recovered = 0; recovered < trimmed && i <= ctx; i++) {
>>> > while (recovered < trimmed && ap[recovered++] != '\n')
>>> > ; /* nothing */
>>> > }
>>> >
>>> > then (warning: I haven't had my coffee yet)?
>>>
>>> Yes, I think that is equivalent. My sleep-deprived brain keeps thinking
>>> there must be a more clear way of writing this whole loop, but it
>>> escapes me at the moment.
>>
>> And this came to me in a dream. :) It fixes the bug, and I think it is a
>> bit simpler to see the termination conditions in a single loop. But
>> please double-check correctness, and that you agree that it is more
>> readable.
>
> I wanted to recover to the end of the line that includes the cut-off
> point, even when (ctx == 0), to be extra safer, but I do not think it
> was necessary.
I have to wonder what happens if the last line which is incomplete has
more than block bytes that are identical at the end? We need to recover
the whole thing to show the correct diff, don't we?
^ permalink raw reply
* Re: [PATCH] whitespace: fix initial-indent checking
From: J. Bruce Fields @ 2007-12-16 21:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vodcqjrtw.fsf@gitster.siamese.dyndns.org>
On Sun, Dec 16, 2007 at 12:40:59PM -0800, Junio C Hamano wrote:
> "J. Bruce Fields" <bfields@fieldses.org> writes:
>
> > One slightly weird thing about this: I'd expect indent-with-non-tab to
> > catch any sequence of 8 or more contiguous spaces, not just such
> > sequences at the end of the indent. This doesn't quite do that.
>
> +-------+-------+-------+-------+-------+-------+-------+-------+-------+
> Personally, I would hate that. That would muck with two spaces I
> deliberately typed after the full stop before this sentence). Please
> don't.
Right, I was only thinking of literal sequences of 8 contiguous spaces,
and only in the initial indent. So it's the failure to flag
"^\t \t"
that I find a little unexpected. Whatever--I don't think it's
particularly important. Like I say, I think there's really only two
cases anyone really cares about:
1. The kernel or git style, where all initial whitespace is
tabbed as much as possible.
2. Various other styles which may be harder to check completely,
but which are likely to share the "no spaces before tabs in
initial indent" rule as a least common denominator.
> Emacs "M-x tabify" tends to do this and I found it unsuitable especially
> for code (I am not complaining, it probably was invented for other
> purposes and not reformatting code):
>
> If you have original (the run of '>>..>>' is a single tab, '.' is a SP)
>
> dcba....123
> fedcba..123
> gfedcba.123
>
> and "tabify" the region, you would get:
>
> dcba>>>>123
> fedcba>>123
> gfedcba.123
>
> That is fine if you are shooting for minimum number of bytes, but often
> it is not what you want in your code, especially when the part that
> conains the whitespace "..cba 123.." is inside a string constant.
Yeah, that sounds pretty irritating.
--b.
^ permalink raw reply
* Re: trim_common_tail bug?
From: Jeff King @ 2007-12-16 21:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, git
In-Reply-To: <7v7ijejq6j.fsf@gitster.siamese.dyndns.org>
On Sun, Dec 16, 2007 at 01:16:36PM -0800, Junio C Hamano wrote:
> > I wanted to recover to the end of the line that includes the cut-off
> > point, even when (ctx == 0), to be extra safer, but I do not think it
> > was necessary.
>
> I have to wonder what happens if the last line which is incomplete has
> more than block bytes that are identical at the end? We need to recover
> the whole thing to show the correct diff, don't we?
No, I think it's right as-is. We forget about the blocks during the
recovery section of the code. IOW, we just keep reading forward until we
find all of the context lines, or we run out of trimmed content. In the
first case, we are fine (we restored the right number of context lines).
In the latter case, we are also fine, because we end up trimming nothing
(IOW, there _weren't_ enough context lines in the first place).
-Peff
^ permalink raw reply
* Re: [PATCH] Add an "-i" option to git-reset, to confirm a reset.
From: Kelvie Wong @ 2007-12-16 21:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git
In-Reply-To: <7vwsrejta3.fsf@gitster.siamese.dyndns.org>
On Dec 16, 2007 12:09 PM, Junio C Hamano <gitster@pobox.com> wrote:
> "Kelvie Wong" <kelvie@ieee.org> writes:
>
> > On Dec 15, 2007 7:35 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > ..
> >> I am slightly negative on this patch. Not only do I think that it is both
> >> easier and more natural to run diff/status/an-alias to see what a reset
> >> would do, but the patch only handles the index_file part (missing the --
> >> <file> part AFAICT).
>
> I am in principle very negative on additional option that does the same
> thing as what the users on odd occasions can run a separate command
> themselves to achieve, and I think "reset -i" falls into that category.
>
> And I am negative on this "-i" not just because I think that would be
> only in "odd occasions" (i.e. rare), but because I think it would not
> help much. Either you are sure about resetting, in which case you would
> not even use "-i" option (and not get this safety), or you are unsure,
> in which case you can do "git status" or whatever commands that are
> already available.
>
> > w.r.t. the style, you were referring to just the array initializers
> > right? Or was there something else I did that doesn't look right?
>
> I spotted only two classes.
>
> +
> + /* Show the user what is about to be reset, and in more detail, if they
> + * like. */
>
> /*
> * Show the user what is about to be reset, and in more detail,
> * if they like.
> */
>
> + if(confirm_reset) {
>
> if (confirm_reset) {
>
I guess this is just for people like me who are used to the "M-x
vc-revert" function in emacs, which shows you exactly what changes get
reverted before doing so.
But I guess it is quite trivial to make an alias to do the same when
invoking git-reset (or checkout) directly.
--
Kelvie Wong
^ permalink raw reply
* [PATCH] Re-re-re-fix common tail optimization
From: Junio C Hamano @ 2007-12-16 21:49 UTC (permalink / raw)
To: Jeff King; +Cc: Linus Torvalds, git
In-Reply-To: <20071216212104.GA32307@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> No, I think it's right as-is. We forget about the blocks during the
> recovery section of the code. IOW, we just keep reading forward until we
> find all of the context lines, or we run out of trimmed content. In the
> first case, we are fine (we restored the right number of context lines).
> In the latter case, we are also fine, because we end up trimming nothing
> (IOW, there _weren't_ enough context lines in the first place).
Kind'a embarrassing that both of us cannot get this right without so
many rounds, isn't it?
-- >8 --
Subject: [PATCH] Re-re-re-fix common tail optimization
We need to be extra careful recovering the removed common section, so
that we do not break context nor the changed incomplete line (i.e. the
last line that does not end with LF).
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/t4024-diff-optimize-common.sh | 69 +++++++++++++++++++++++++++++++++++++++
xdiff-interface.c | 2 +-
2 files changed, 70 insertions(+), 1 deletions(-)
diff --git a/t/t4024-diff-optimize-common.sh b/t/t4024-diff-optimize-common.sh
new file mode 100755
index 0000000..10405f1
--- /dev/null
+++ b/t/t4024-diff-optimize-common.sh
@@ -0,0 +1,69 @@
+#!/bin/sh
+
+test_description='common tail optimization'
+
+. ./test-lib.sh
+
+z=zzzzzzzz ;# 8
+z="$z$z$z$z$z$z$z$z" ;# 64
+z="$z$z$z$z$z$z$z$z" ;# 512
+z="$z$z$z$z" ;# 2048
+z2047=$(expr "$z" : '.\(.*\)') ; #2047
+
+test_expect_success setup '
+
+ echo "a$z2047" >file-a &&
+ echo "b" >file-b &&
+ echo "$z2047" >>file-b &&
+ echo "c$z2047" | tr -d "\012" >file-c &&
+ echo "d" >file-d &&
+ echo "$z2047" | tr -d "\012" >>file-d &&
+
+ git add file-a file-b file-c file-d &&
+
+ echo "A$z2047" >file-a &&
+ echo "B" >file-b &&
+ echo "$z2047" >>file-b &&
+ echo "C$z2047" | tr -d "\012" >file-c &&
+ echo "D" >file-d &&
+ echo "$z2047" | tr -d "\012" >>file-d
+
+'
+
+echo >expect <<\EOF
+diff --git a/file-a b/file-a
+--- a/file-a
++++ b/file-a
+@@ -1 +1 @@
+-aZ
++AZ
+diff --git a/file-b b/file-b
+--- a/file-b
++++ b/file-b
+@@ -1 +1 @@
+-b
++B
+diff --git a/file-c b/file-c
+--- a/file-c
++++ b/file-c
+@@ -1 +1 @@
+-cZ
+\ No newline at end of file
++CZ
+\ No newline at end of file
+diff --git a/file-d b/file-d
+--- a/file-d
++++ b/file-d
+@@ -1 +1 @@
+-d
++D
+EOF
+
+test_expect_success 'diff -U0' '
+
+ git diff -U0 | sed -e "/^index/d" -e "s/$z2047/Z/g" >actual &&
+ diff -u expect actual
+
+'
+
+test_done
diff --git a/xdiff-interface.c b/xdiff-interface.c
index 98b02ed..9ee877c 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -121,7 +121,7 @@ static void trim_common_tail(mmfile_t *a, mmfile_t *b, long ctx)
bp -= blk;
}
- while (recovered < trimmed && ctx)
+ while (recovered < trimmed && 0 <= ctx)
if (ap[recovered++] == '\n')
ctx--;
a->size -= (trimmed - recovered);
^ permalink raw reply related
* Re: [PATCH v2] threaded pack-objects: Use condition variables for thread communication.
From: Junio C Hamano @ 2007-12-16 22:12 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Peter Baumann, git, Nicolas Pitre
In-Reply-To: <200712162045.34456.johannes.sixt@telecom.at>
Looks nice and I think the clean-up makes the logic easier to follow as
well.
^ permalink raw reply
* Re: [PATCH] Re-re-re-fix common tail optimization
From: Jeff King @ 2007-12-16 22:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, git
In-Reply-To: <7v3au2joo2.fsf_-_@gitster.siamese.dyndns.org>
On Sun, Dec 16, 2007 at 01:49:17PM -0800, Junio C Hamano wrote:
> Kind'a embarrassing that both of us cannot get this right without so
> many rounds, isn't it?
Good thing we can just delete the emails and nobody will be the wiser...
> +echo >expect <<\EOF
This would probably work better as 'cat'.
> +test_expect_success 'diff -U0' '
> +
> + git diff -U0 | sed -e "/^index/d" -e "s/$z2047/Z/g" >actual &&
> + diff -u expect actual
Aren't we using "git diff" for the second diff there nowadays?
> - while (recovered < trimmed && ctx)
> + while (recovered < trimmed && 0 <= ctx)
> if (ap[recovered++] == '\n')
> ctx--;
> a->size -= (trimmed - recovered);
Oops (I think maybe I misunderstood what you were asking in the last
email). This fix is correct, though the code is now kind of subtle. I
think it would be more obvious as:
/* finish off any changed line we are in */
while (recovered < trimmed && ap[recovered++] != '\n')
/* nothing */;
/* recover context lines */
while (recovered < trimmed && ctx)
if (ap[recovered++] == '\n')
ctx--;
Your loop does both actions in the same loop, which is correct, but took
me 10 minutes of thinking and staring to realize what was going on.
-Peff
^ permalink raw reply
* Re: [PATCH] Re-re-re-fix common tail optimization
From: Junio C Hamano @ 2007-12-16 22:23 UTC (permalink / raw)
To: Jeff King; +Cc: Linus Torvalds, git
In-Reply-To: <20071216221545.GA32596@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> This would probably work better as 'cat'.
Yeah, I amended it without adding another "re-" to the title ;-) The
result has been already pushed out.
>> +test_expect_success 'diff -U0' '
>> +
>> + git diff -U0 | sed -e "/^index/d" -e "s/$z2047/Z/g" >actual &&
>> + diff -u expect actual
>
> Aren't we using "git diff" for the second diff there nowadays?
Some people seem to think that is a good idea, but I generally do not
like using "git diff" between expect and actual (both untracked) inside
tests. The last "diff" is about validating what git does and using "git
diff" there would make the test meaningless when "git diff" itself is
broken.
This is especially so because comparison between untracked files is a
bolted-on afterthought and I am least confident about among the
codepaths in the whole "git diff" (it is not even my nor Linus's code).
^ permalink raw reply
* Re: Windows binaries for qgit 2.0
From: Marco Costalba @ 2007-12-16 22:26 UTC (permalink / raw)
To: Abdelrazak Younes; +Cc: git
In-Reply-To: <fk3153$rtb$1@ger.gmane.org>
On Dec 16, 2007 12:11 PM, Abdelrazak Younes <younes.a@free.fr> wrote:
>
> Actually you might prefer to just use the LyX dependencies package that
> we provide for Windows developers, it contains Qt. I paste here the
> relevant part of our 'INSTALL.Win32':
>
Thanks, I've tried it but without success because I need MSVC 2005
installed, and currently is not.
Anyhow for now I have produced a version with mingw that seems more or
less to work, when I have a bit of time I will install MSVC 2005 and
try if with that compiler is better...
Thanks
Marco
^ 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