Re: [PATCH] Add ALL_LDFLAGS to the git target.

Jason Riedy [off-list ref] writes:

For some reason, I need ALL_LDFLAGS in the git target only on
AIX.

I wonder what the dependency is, since ALL_LDFLAGS is not
modified on AIX, but you are right.  That is the only binary
that does not link with ALL_LDFLAGS which can include whatever
user passes via LDFLAGS.

Once it builds, only one test "fails" on AIX 5.1 with 
1.3.0.rc1, t5500-fetch-pack.sh, but it looks like it's some
odd tool problem in the tester + my setup and not a real bug.

Curious and would appreciate more details.

And Junio C Hamano writes:
 - I wonder what the dependency is, since ALL_LDFLAGS is not
 - modified on AIX, [...]

Specifically, -lcrypto.  Mine is in a funny place, so I need
LDFLAGS passed in.

 - > Once it builds, only one test "fails" on AIX 5.1 with 
 - > 1.3.0.rc1, t5500-fetch-pack.sh, but it looks like it's some
 - > odd tool problem in the tester + my setup and not a real bug.
 - 
 - Curious and would appreciate more details.

I just found it.  The progress meter stuff in pack-objects
splats all over the output.  So trash/client/log.txt is
completely mangled.  Everything functions correctly, but
the textual output is garbage.  If I set progress to 0 in 
pack-objects.c, everthing's happy.

There's no way to pass -q through fetch-pack to upload-pack...
Gee, look, a comment that says "Yeah, yeah, fixme."  I have
no real desire to add an args argument and propagate that
change through all the connect routines.  An alternative is
to add a "quiet" command to the protocol.  Another would be 
to dup all three file descriptors.  yech.  Preference?

(I haven't updated git in a while on this platform.  
Recompiling and testing takes a while on a 375 MHz Power3.)

Jason

Jason Riedy [off-list ref] writes:

And Junio C Hamano writes:
 - I wonder what the dependency is, since ALL_LDFLAGS is not
 - modified on AIX, [...]

Specifically, -lcrypto.  Mine is in a funny place, so I need
LDFLAGS passed in.

Thanks.  That is the right fix, then.

 - > Once it builds, only one test "fails" on AIX 5.1 with 
 - > 1.3.0.rc1, t5500-fetch-pack.sh, but it looks like it's some
 - > odd tool problem in the tester + my setup and not a real bug.
 - 
 - Curious and would appreciate more details.

I just found it.  The progress meter stuff in pack-objects
splats all over the output.  So trash/client/log.txt is
completely mangled.  Everything functions correctly, but
the textual output is garbage.  If I set progress to 0 in 
pack-objects.c, everthing's happy.

Hmph.  We do fprintf(stderr, "blah\r") to draw them.  The
standard says that "standard error stream is not fully
buffered", but I guess it does not necessarily mean it is
unbuffered, so we probably need to fflush(3) there.  Would
something like this help?

-- >8 --

diff --git a/fetch-clone.c b/fetch-clone.c
index da1b3ff..252e5ec 100644
--- a/fetch-clone.c
+++ b/fetch-clone.c

@@ -230,6 +230,7 @@
 					total >> 20,
 					1000*((total >> 10) & 1023)>>10,
 					avg_bytes / avg_time );
+				fflush(stderr);
 			}
 		}
 	}

diff --git a/imap-send.c b/imap-send.c
index e33c78b..dcfa8d8 100644
--- a/imap-send.c
+++ b/imap-send.c

@@ -1345,6 +1345,7 @@
 	while (1) {
 		unsigned percent = n * 100 / total;
 		fprintf( stderr, "%4u%% (%d/%d) done\r", percent, n, total );
+		fflush(stderr);
 		if (!split_msg( &all_msgs, &msg, &ofs ))
 			break;
 		r = imap_store_msg( ctx, &msg, &uid );

diff --git a/pack-objects.c b/pack-objects.c
index 49357c6..7c85348 100644
--- a/pack-objects.c
+++ b/pack-objects.c

@@ -360,6 +360,7 @@
 			if (progress_update || percent != last_percent) {
 				fprintf(stderr, "%4u%% (%u/%u) done\r",
 					percent, written, nr_result);
+				fflush(stderr);
 				progress_update = 0;
 				last_percent = percent;
 			}

@@ -570,6 +571,7 @@
  already_added:
 	if (progress_update) {
 		fprintf(stderr, "Counting objects...%d\r", nr_objects);
+		fflush(stderr);
 		progress_update = 0;
 	}
 	if (exclude)

@@ -912,6 +914,7 @@
 			if (percent != last_percent || progress_update) {
 				fprintf(stderr, "%4u%% (%u/%u) done\r",
 					percent, processed, nr_result);
+				fflush(stderr);
 				progress_update = 0;
 				last_percent = percent;
 			}

diff --git a/read-tree.c b/read-tree.c
index eaff444..6a2aa16 100644
--- a/read-tree.c
+++ b/read-tree.c

@@ -325,6 +325,7 @@
 				    progress_update) {
 					fprintf(stderr, "%4u%% (%u/%u) done\r",
 						percent, cnt, total);
+					fflush(stderr);
 					last_percent = percent;
 				}
 			}

diff --git a/unpack-objects.c b/unpack-objects.c
index 815a1b3..8596f9b 100644
--- a/unpack-objects.c
+++ b/unpack-objects.c

@@ -220,6 +220,7 @@
 			last_sec = now.tv_sec;
 			last_percent = percentage;
 			fprintf(stderr, "%4u%% (%u/%u) done\r", percentage, nr, total);
+			fflush(stderr);
 		}
 	}
 	switch (type) {

And Junio C Hamano writes:
 - Hmph.  We do fprintf(stderr, "blah\r") to draw them.  The
 - standard says that "standard error stream is not fully
 - buffered", but I guess it does not necessarily mean it is
 - unbuffered, so we probably need to fflush(3) there.  Would
 - something like this help?

I suppose I should have mentioned that I tried flushing 
stderr.  Your more comprehensive flushing also does not 
fix it, giving outputs like:

Unpacking Total 3333 objects
, written 33 (delta 1), reused 0 (delta 0)

The problem is that stderr from a child is not tied to any 
stream of its parent.  Generally, as far as I know, you 
cannot make any assumptions about how pipes from separate 
processes are interleaved in the output.  Some standard may 
say something, but I have no idea what or if anyone listens.
And this particular system is a busy SMP node, making the
problem worse.

Line-buffered streams like stdout tend to work, but not 
unbuffered streams like stderr.  We can't make stderr line-
buffered without breaking the status indicator...

If I add a third fd to all the pipes and dup it to stderr,
the tests work.  I never read from that fd, so I never get
the status output...  Progress needs to be part of the 
protocol so front ends can handle it cleanly rather than 
using stderr tricks.

So some possibilities:
  1) Add the ability to pass options through the whole
     connect system.  Then pass -q in the tester.
  2) Add a specific "quiet" command to the protocol for
     just passing -q from git-fetch-pack.  Pass -q in the 
     tester.
  3) Add an option to pack-objects that dumps progress
     output to stdout in a special packet format.  Then
     update everyone who talks through upload-pack to
     expect another phase of informational messages after
     negotiating object differences and before the pack
     data.

The first two are cosmetic fixes only, and #2 is a cheap,
ugly, but easy hack.

This problem is (to me) low priority.  It unfortunately 
breaks a test case on AIX, but I can live with it for now.
If others here start to listen to the gospel of git, well,
I'll need to fix it.  (But I once recommended Arch, and
people stopped listening after they tried it.)

Folks using moderately-loaded SMPs may experience similar 
problems.  But if they're fetching large packs, the problem
likely won't appear at all.

Jason

P.S. For the whole finding-a-function-name business, some of 
us are using git on fixed-format Fortran.  Every non-comment
line begins with whitespace...  ;)  And in free format, many
people don't add that first indentation within subroutines.

Jason Riedy [off-list ref] wrote:

P.S. For the whole finding-a-function-name business, some of 
us are using git on fixed-format Fortran.  Every non-comment
line begins with whitespace...  ;)  And in free format, many
people don't add that first indentation within subroutines.

Urgh.  So, which regex library do people want to use? ;-)  (My vote's
for pcre.)

-- [mdw]

On Tue, 28 Mar 2006, Mark Wooding wrote:

Jason Riedy [off-list ref] wrote:

quoted

P.S. For the whole finding-a-function-name business, some of 
us are using git on fixed-format Fortran.  Every non-comment
line begins with whitespace...  ;)  And in free format, many
people don't add that first indentation within subroutines.

Urgh.  So, which regex library do people want to use? ;-)  (My vote's
for pcre.)

I'd really just prefer to make the "-p" switch configurable, the way it 
was before. No regexps, just the same rules as for GNU diff, perhaps with 
the difference being that it would be on by default.

Another possible approach is to say
 - if the first line of the real diff matches the rules, do NOT add 
   another line that matches the rule at the @@-line.

since the simple @@-line rule really doesn't make sense for any file that 
is "dense" (ie where most lines start with non-whitespace).

		Linus

Linus Torvalds [off-list ref] wrote:

I'd really just prefer to make the "-p" switch configurable, the way
it was before. No regexps, just the same rules as for GNU diff,

The rules for GNU diff aren't actually good enough if you can't
configure them.  We used to be able to put runes in GIT_DIFF_OPTS.

perhaps with the difference being that it would be on by default.

I thought it /was/ on by default:

: static const char *diff_opts = "-pu";

(killed in cebff98db).

Another possible approach is to say
 - if the first line of the real diff matches the rules, do NOT add 
   another line that matches the rule at the @@-line.

since the simple @@-line rule really doesn't make sense for any file that 
is "dense" (ie where most lines start with non-whitespace).

It's true, and that's an easy fix.  But it doesn't do any actual harm.

-- [mdw]

Dear diary, on Wed, Mar 29, 2006 at 01:03:05AM CEST, I got a letter
where Linus Torvalds [off-list ref] said that...

On Tue, 28 Mar 2006, Mark Wooding wrote:

quoted

Urgh.  So, which regex library do people want to use? ;-)  (My vote's
for pcre.)

... No regexps, ...

To toss a random feature idea around, in the recent days I've found
myself thinking about regexp pickaxe several times.

And while already tossing stuff, what about a naive proof-of-concept
patch?  A silly example:

	git-whatchanged --pickaxe-regex -p -S' +$' | less -p '^[-+ ].* +$'

Then keep hitting 'n'. Good that most of the matches are deletions. :)
(Or commit messages.)

---

git-diff-* --pickaxe-regex will change the -S pickaxe to match
POSIX extended regular expressions instead of fixed strings.

The regex.h library is a rather stupid interface and I like pcre too, but
with any luck it will be everywhere we will want to run Git on, it being
POSIX.2 and all. I'm not sure if we can expect platforms like AIX to
conform to POSIX.2 or if win32 has regex.h. We might add a flag to
Makefile if there is a portability trouble potential.

Signed-off-by: Petr Baudis <redacted>
---

 Documentation/diff-options.txt |    4 ++
 diff.c                         |    2 +
 diff.h                         |    1 +
 diffcore-pickaxe.c             |   68 ++++++++++++++++++++++++++++++----------
 4 files changed, 58 insertions(+), 17 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 2a0275e..ec6811c 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt

@@ -69,6 +69,10 @@
 	changeset, not just the files that contain the change
 	in <string>.
 
+--pickaxe-regex::
+	Make the <string> not a plain string but an extended POSIX
+	regex to match.
+
 -O<orderfile>::
 	Output the patch in the order specified in the
 	<orderfile>, which has one shell glob pattern per line.

diff --git a/diff.c b/diff.c
index 8b37477..e006adb 100644
--- a/diff.c
+++ b/diff.c

@@ -883,6 +883,8 @@ int diff_opt_parse(struct diff_options *
 		options->filter = arg + 14;
 	else if (!strcmp(arg, "--pickaxe-all"))
 		options->pickaxe_opts = DIFF_PICKAXE_ALL;
+	else if (!strcmp(arg, "--pickaxe-regex"))
+		options->pickaxe_opts = DIFF_PICKAXE_REGEX;
 	else if (!strncmp(arg, "-B", 2)) {
 		if ((options->break_opt =
 		     diff_scoreopt_parse(arg)) == -1)

diff --git a/diff.h b/diff.h
index 8fac465..564c94f 100644
--- a/diff.h
+++ b/diff.h

@@ -112,6 +112,7 @@ #define DIFF_DETECT_RENAME	1
 #define DIFF_DETECT_COPY	2
 
 #define DIFF_PICKAXE_ALL	1
+#define DIFF_PICKAXE_REGEX	2
 
 extern void diffcore_std(struct diff_options *);
 

diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 50e46ab..d89f314 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c

@@ -1,12 +1,15 @@
 /*
  * Copyright (C) 2005 Junio C Hamano
  */
+#include <regex.h>
+
 #include "cache.h"
 #include "diff.h"
 #include "diffcore.h"
 
 static unsigned int contains(struct diff_filespec *one,
-			     const char *needle, unsigned long len)
+			     const char *needle, unsigned long len,
+			     regex_t *regexp)
 {
 	unsigned int cnt;
 	unsigned long offset, sz;

@@ -17,15 +20,28 @@ static unsigned int contains(struct diff
 	sz = one->size;
 	data = one->data;
 	cnt = 0;
-
-	/* Yes, I've heard of strstr(), but the thing is *data may
-	 * not be NUL terminated.  Sue me.
-	 */
-	for (offset = 0; offset + len <= sz; offset++) {
-		/* we count non-overlapping occurrences of needle */
-		if (!memcmp(needle, data + offset, len)) {
-			offset += len - 1;
+
+	if (regexp) {
+		regmatch_t regmatch;
+		int flags = 0;
+
+		while (*data && !regexec(regexp, data, 1, &regmatch, flags)) {
+			flags |= REG_NOTBOL;
+			data += regmatch.rm_so;
+			if (*data) data++;
 			cnt++;
+		}
+
+	} else { /* Classic exact string match */
+		/* Yes, I've heard of strstr(), but the thing is *data may
+		 * not be NUL terminated.  Sue me.
+		 */
+		for (offset = 0; offset + len <= sz; offset++) {
+			/* we count non-overlapping occurrences of needle */
+			if (!memcmp(needle, data + offset, len)) {
+				offset += len - 1;
+				cnt++;
+			}
 		}
 	}
 	return cnt;

@@ -36,10 +52,24 @@ void diffcore_pickaxe(const char *needle
 	struct diff_queue_struct *q = &diff_queued_diff;
 	unsigned long len = strlen(needle);
 	int i, has_changes;
+	regex_t regex, *regexp = NULL;
 	struct diff_queue_struct outq;
 	outq.queue = NULL;
 	outq.nr = outq.alloc = 0;
 
+	if (opts & DIFF_PICKAXE_REGEX) {
+		int err;
+		err = regcomp(&regex, needle, REG_EXTENDED | REG_NEWLINE);
+		if (err) {
+			/* The POSIX.2 people are surely sick */
+			char errbuf[1024];
+			regerror(err, &regex, errbuf, 1024);
+			regfree(&regex);
+			die("invalid pickaxe regex: %s", errbuf);
+		}
+		regexp = &regex;
+	}
+
 	if (opts & DIFF_PICKAXE_ALL) {
 		/* Showing the whole changeset if needle exists */
 		for (i = has_changes = 0; !has_changes && i < q->nr; i++) {

@@ -48,16 +78,16 @@ void diffcore_pickaxe(const char *needle
 				if (!DIFF_FILE_VALID(p->two))
 					continue; /* ignore unmerged */
 				/* created */
-				if (contains(p->two, needle, len))
+				if (contains(p->two, needle, len, regexp))
 					has_changes++;
 			}
 			else if (!DIFF_FILE_VALID(p->two)) {
-				if (contains(p->one, needle, len))
+				if (contains(p->one, needle, len, regexp))
 					has_changes++;
 			}
 			else if (!diff_unmodified_pair(p) &&
-				 contains(p->one, needle, len) !=
-				 contains(p->two, needle, len))
+				 contains(p->one, needle, len, regexp) !=
+				 contains(p->two, needle, len, regexp))
 				has_changes++;
 		}
 		if (has_changes)

@@ -80,16 +110,16 @@ void diffcore_pickaxe(const char *needle
 				if (!DIFF_FILE_VALID(p->two))
 					; /* ignore unmerged */
 				/* created */
-				else if (contains(p->two, needle, len))
+				else if (contains(p->two, needle, len, regexp))
 					has_changes = 1;
 			}
 			else if (!DIFF_FILE_VALID(p->two)) {
-				if (contains(p->one, needle, len))
+				if (contains(p->one, needle, len, regexp))
 					has_changes = 1;
 			}
 			else if (!diff_unmodified_pair(p) &&
-				 contains(p->one, needle, len) !=
-				 contains(p->two, needle, len))
+				 contains(p->one, needle, len, regexp) !=
+				 contains(p->two, needle, len, regexp))
 				has_changes = 1;
 
 			if (has_changes)

@@ -97,6 +127,10 @@ void diffcore_pickaxe(const char *needle
 			else
 				diff_free_filepair(p);
 		}
+
+	if (opts & DIFF_PICKAXE_REGEX) {
+		regfree(&regex);
+	}
 
 	free(q->queue);
 	*q = outq;

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
Right now I am having amnesia and deja-vu at the same time.  I think
I have forgotten this before.

Hi,

On Tue, 28 Mar 2006, Mark Wooding wrote:

Jason Riedy [off-list ref] wrote:

quoted

P.S. For the whole finding-a-function-name business, some of 
us are using git on fixed-format Fortran.  Every non-comment
line begins with whitespace...  ;)  And in free format, many
people don't add that first indentation within subroutines.

Urgh.  So, which regex library do people want to use? ;-)  (My vote's
for pcre.)

My vote is against adding such a dependency for so little gain. We already 
use regex.h (probably my fault).

Ciao,
Dscho