Git development
 help / color / mirror / Atom feed
* [PATCH 1/2] Makefile: Windows lacks /dev/tty
From: Johannes Sixt @ 2011-12-12 21:10 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20111210104130.GI16648@sigill.intra.peff.net>

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
These two patches go on top of jk/credentials.

 Makefile |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index 2c506b3..5b7f6a8 100644
--- a/Makefile
+++ b/Makefile
@@ -1139,6 +1139,7 @@ ifeq ($(uname_S),Windows)
 	NO_CURL = YesPlease
 	NO_PYTHON = YesPlease
 	BLK_SHA1 = YesPlease
+	NO_DEV_TTY = YesPlease
 	NO_POSIX_GOODIES = UnfortunatelyYes
 	NATIVE_CRLF = YesPlease
 
@@ -1232,6 +1233,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	ETAGS_TARGET = ETAGS
 	NO_INET_PTON = YesPlease
 	NO_INET_NTOP = YesPlease
+	NO_DEV_TTY = YesPlease
 	NO_POSIX_GOODIES = UnfortunatelyYes
 	COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/win32
 	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
-- 
1.7.8.216.g2e426

^ permalink raw reply related

* [PATCH 2/2] Makefile: optionally exclude code that needs Unix sockets
From: Johannes Sixt @ 2011-12-12 21:12 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <4EE66DAB.5070407@kdbg.org>

Introduce a configuration option NO_UNIX_SOCKETS to exclude code that
depends on Unix sockets and use it in MSVC and MinGW builds.

Notice that unix-socket.h was missing from LIB_H before; fix that, too.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
All or most of the tests introduced by the series fail on Windows.
What is the preferred way to exclude them? Mark the all with a
prerequisite? Or exit early at the beginning of the test file?

 Makefile |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 5b7f6a8..77c85db 100644
--- a/Makefile
+++ b/Makefile
@@ -143,6 +143,8 @@ all::
 #
 # Define NO_IPV6 if you lack IPv6 support and getaddrinfo().
 #
+# Define NO_UNIX_SOCKETS if your system does not offer unix sockets.
+#
 # Define NO_SOCKADDR_STORAGE if your platform does not have struct
 # sockaddr_storage.
 #
@@ -430,8 +432,6 @@ PROGRAM_OBJS += show-index.o
 PROGRAM_OBJS += upload-pack.o
 PROGRAM_OBJS += http-backend.o
 PROGRAM_OBJS += sh-i18n--envsubst.o
-PROGRAM_OBJS += credential-cache.o
-PROGRAM_OBJS += credential-cache--daemon.o
 PROGRAM_OBJS += credential-store.o
 
 PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
@@ -709,7 +709,6 @@ LIB_OBJS += transport-helper.o
 LIB_OBJS += tree-diff.o
 LIB_OBJS += tree.o
 LIB_OBJS += tree-walk.o
-LIB_OBJS += unix-socket.o
 LIB_OBJS += unpack-trees.o
 LIB_OBJS += url.o
 LIB_OBJS += usage.o
@@ -1112,6 +1111,7 @@ ifeq ($(uname_S),Windows)
 	NO_SYS_POLL_H = YesPlease
 	NO_SYMLINK_HEAD = YesPlease
 	NO_IPV6 = YesPlease
+	NO_UNIX_SOCKETS = YesPlease
 	NO_SETENV = YesPlease
 	NO_UNSETENV = YesPlease
 	NO_STRCASESTR = YesPlease
@@ -1206,6 +1206,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	NO_LIBGEN_H = YesPlease
 	NO_SYS_POLL_H = YesPlease
 	NO_SYMLINK_HEAD = YesPlease
+	NO_UNIX_SOCKETS = YesPlease
 	NO_SETENV = YesPlease
 	NO_UNSETENV = YesPlease
 	NO_STRCASESTR = YesPlease
@@ -1584,6 +1585,12 @@ ifdef NO_INET_PTON
 	LIB_OBJS += compat/inet_pton.o
 	BASIC_CFLAGS += -DNO_INET_PTON
 endif
+ifndef NO_UNIX_SOCKETS
+	LIB_OBJS += unix-socket.o
+	LIB_H += unix-socket.h
+	PROGRAM_OBJS += credential-cache.o
+	PROGRAM_OBJS += credential-cache--daemon.o
+endif
 
 ifdef NO_ICONV
 	BASIC_CFLAGS += -DNO_ICONV
-- 
1.7.8.216.g2e426

^ permalink raw reply related

* [PATCH v3 1/3] grep: load funcname patterns for -W
From: Thomas Rast @ 2011-12-12 21:16 UTC (permalink / raw)
  To: git, Junio C Hamano; +Cc: René Scharfe, Jeff King, Eric Herman
In-Reply-To: <cover.1323723759.git.trast@student.ethz.ch>

git-grep avoids loading the funcname patterns unless they are needed.
ba8ea74 (grep: add option to show whole function as context,
2011-08-01) forgot to extend this test also to the new funcbody
feature.  Do so.

The catch is that we also have to disable threading when using
userdiff, as explained in grep_threads_ok().  So we must be careful to
introduce the same test there.
---
 grep.c          |    7 ++++---
 t/t7810-grep.sh |   14 ++++++++++++++
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/grep.c b/grep.c
index b29d09c..7a070e9 100644
--- a/grep.c
+++ b/grep.c
@@ -948,8 +948,8 @@ int grep_threads_ok(const struct grep_opt *opt)
 	 * machinery in grep_buffer_1. The attribute code is not
 	 * thread safe, so we disable the use of threads.
 	 */
-	if (opt->funcname && !opt->unmatch_name_only && !opt->status_only &&
-	    !opt->name_only)
+	if ((opt->funcname || opt->funcbody)
+	    && !opt->unmatch_name_only && !opt->status_only && !opt->name_only)
 		return 0;
 
 	return 1;
@@ -1008,7 +1008,8 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
 	}
 
 	memset(&xecfg, 0, sizeof(xecfg));
-	if (opt->funcname && !opt->unmatch_name_only && !opt->status_only &&
+	if ((opt->funcname || opt->funcbody)
+	    && !opt->unmatch_name_only && !opt->status_only &&
 	    !opt->name_only && !binary_match_only && !collect_hits) {
 		struct userdiff_driver *drv = userdiff_find_by_path(name);
 		if (drv && drv->funcname.pattern) {
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 81263b7..7ba5b16 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -523,6 +523,20 @@ test_expect_success 'grep -W' '
 	test_cmp expected actual
 '
 
+cat >expected <<EOF
+hello.c=	printf("Hello world.\n");
+hello.c:	return 0;
+hello.c-	/* char ?? */
+EOF
+
+test_expect_success 'grep -W with userdiff' '
+	test_when_finished "rm -f .gitattributes" &&
+	git config diff.custom.xfuncname "(printf.*|})$" &&
+	echo "hello.c diff=custom" >.gitattributes &&
+	git grep -W return >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'grep from a subdirectory to search wider area (1)' '
 	mkdir -p s &&
 	(
-- 
1.7.8.431.g2abf2

^ permalink raw reply related

* [PATCH v3 0/3] grep attributes and multithreading
From: Thomas Rast @ 2011-12-12 21:16 UTC (permalink / raw)
  To: git, Junio C Hamano; +Cc: René Scharfe, Jeff King, Eric Herman
In-Reply-To: <4ED8F9AE.8030605@lsrfire.ath.cx>

I think we should finish up these three patches for the next release.
The first two are unchanged from last time; nobody seemed to have any
objections.

On the third one I'm following René's argument:

> Am 02.12.2011 14:07, schrieb Thomas Rast:
> > So disable threading entirely when not scanning the worktree, to get
> > the NO_PTHREADS performance in that case.  This obsoletes all code
> > related to grep_sha1_async.  The thread startup must be delayed until
> > after all arguments have been parsed, but this does not have a
> > measurable effect.
> 
> This is a bit radical.  I think the underlying issue that 
> read_sha1_file() is not thread-safe can be solved eventually and then 
> we'd need to readd that code.

I already posted a bunch of POC patches, but doing the timing manually
has been getting on my nerves lately.  So I would first like to
formalize some of the performance testing, perhaps along lines already
drawn up by Michael Hagger, perhaps not.  Then we can revisit the
issue of grep performance.  But I would prefer not to block the -W fix
and two easy and confirmed speedups on that.

I dropped this part entirely:

> How about adding a parameter to control the number of threads 
> (--threads?) instead that defaults to eight (or five) for the worktree 
> and one for the rest?  That would also make benchmarking easier.

It does make testing a lot easier, but the interface is IMHO not fit
for users and I have a feeling that the "right" for-debugging
interface will end up falling out of the performance testing work
(probably an environment variable).  The end-user option should be a
config setting, if any.


Thomas Rast (3):
  grep: load funcname patterns for -W
  grep: enable threading with -p and -W using lazy attribute lookup
  grep: disable threading in non-worktree case

 builtin/grep.c  |   34 +++++++++++++++----------
 grep.c          |   73 ++++++++++++++++++++++++++++++++++---------------------
 grep.h          |    7 +++++
 t/t7810-grep.sh |   14 ++++++++++
 4 files changed, 86 insertions(+), 42 deletions(-)

-- 
1.7.8.431.g2abf2

^ permalink raw reply

* [PATCH v3 3/3] grep: disable threading in non-worktree case
From: Thomas Rast @ 2011-12-12 21:16 UTC (permalink / raw)
  To: git, Junio C Hamano; +Cc: René Scharfe, Jeff King, Eric Herman
In-Reply-To: <cover.1323723759.git.trast@student.ethz.ch>

Measurements by various people have shown that grepping in parallel is
not beneficial when the object store is involved.  For example, with a
simple regex:

  Threads     | --cached case            | worktree case
  ----------------------------------------------------------------
  8 (default) | 2.88u 0.21s 0:02.94real  | 0.19u 0.32s 0:00.16real
  4           | 2.89u 0.29s 0:02.99real  | 0.16u 0.34s 0:00.17real
  2           | 2.83u 0.36s 0:02.87real  | 0.18u 0.32s 0:00.26real
  NO_PTHREADS | 2.16u 0.08s 0:02.25real  | 0.12u 0.17s 0:00.31real

This happens because all the threads contend on read_sha1_mutex almost
all of the time.  A more complex regex allows the threads to do more
work in parallel, but as Jeff King found out, the "super boost" (much
higher clock when only one core is active) feature of recent CPUs
still causes the unthreaded case to win by a large margin.

So until the pack machinery allows unthreaded access, we disable
grep's threading in all but the worktree case.

Helped-by: René Scharfe <rene.scharfe@lsrfire.ath.cx>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 builtin/grep.c |   36 ++++++++++++++++++------------------
 1 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index bc23c3c..7affbda 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1003,24 +1003,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	if (!opt.fixed && opt.ignore_case)
 		opt.regflags |= REG_ICASE;
 
-#ifndef NO_PTHREADS
-	if (online_cpus() == 1)
-		use_threads = 0;
-#else
-	use_threads = 0;
-#endif
-
-	opt.use_threads = use_threads;
-
-#ifndef NO_PTHREADS
-	if (use_threads) {
-		if (opt.pre_context || opt.post_context || opt.file_break ||
-		    opt.funcbody)
-			skip_first_line = 1;
-		start_threads(&opt);
-	}
-#endif
-
 	compile_grep_patterns(&opt);
 
 	/* Check revs and then paths */
@@ -1042,6 +1024,24 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		break;
 	}
 
+#ifndef NO_PTHREADS
+	if (list.nr || cached || online_cpus() == 1)
+		use_threads = 0;
+#else
+	use_threads = 0;
+#endif
+
+	opt.use_threads = use_threads;
+
+#ifndef NO_PTHREADS
+	if (use_threads) {
+		if (opt.pre_context || opt.post_context || opt.file_break ||
+		    opt.funcbody)
+			skip_first_line = 1;
+		start_threads(&opt);
+	}
+#endif
+
 	/* The rest are paths */
 	if (!seen_dashdash) {
 		int j;
-- 
1.7.8.431.g2abf2

^ permalink raw reply related

* [PATCH v3 2/3] grep: enable threading with -p and -W using lazy attribute lookup
From: Thomas Rast @ 2011-12-12 21:16 UTC (permalink / raw)
  To: git, Junio C Hamano; +Cc: René Scharfe, Jeff King, Eric Herman
In-Reply-To: <cover.1323723759.git.trast@student.ethz.ch>

Lazily load the userdiff attributes in match_funcname().  Use a
separate mutex around this loading to protect the (not thread-safe)
attributes machinery.  This lets us re-enable threading with -p and
-W while reducing the overhead caused by looking up attributes.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 builtin/grep.c |   12 +++++++--
 grep.c         |   74 ++++++++++++++++++++++++++++++++++----------------------
 grep.h         |    7 +++++
 3 files changed, 61 insertions(+), 32 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 988ea1d..bc23c3c 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -256,6 +256,7 @@ static void start_threads(struct grep_opt *opt)
 
 	pthread_mutex_init(&grep_mutex, NULL);
 	pthread_mutex_init(&read_sha1_mutex, NULL);
+	pthread_mutex_init(&grep_attr_mutex, NULL);
 	pthread_cond_init(&cond_add, NULL);
 	pthread_cond_init(&cond_write, NULL);
 	pthread_cond_init(&cond_result, NULL);
@@ -303,6 +304,7 @@ static int wait_all(void)
 
 	pthread_mutex_destroy(&grep_mutex);
 	pthread_mutex_destroy(&read_sha1_mutex);
+	pthread_mutex_destroy(&grep_attr_mutex);
 	pthread_cond_destroy(&cond_add);
 	pthread_cond_destroy(&cond_write);
 	pthread_cond_destroy(&cond_result);
@@ -1002,17 +1004,21 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		opt.regflags |= REG_ICASE;
 
 #ifndef NO_PTHREADS
-	if (online_cpus() == 1 || !grep_threads_ok(&opt))
+	if (online_cpus() == 1)
 		use_threads = 0;
+#else
+	use_threads = 0;
+#endif
+
+	opt.use_threads = use_threads;
 
+#ifndef NO_PTHREADS
 	if (use_threads) {
 		if (opt.pre_context || opt.post_context || opt.file_break ||
 		    opt.funcbody)
 			skip_first_line = 1;
 		start_threads(&opt);
 	}
-#else
-	use_threads = 0;
 #endif
 
 	compile_grep_patterns(&opt);
diff --git a/grep.c b/grep.c
index 7a070e9..4dd7da2 100644
--- a/grep.c
+++ b/grep.c
@@ -2,6 +2,7 @@
 #include "grep.h"
 #include "userdiff.h"
 #include "xdiff-interface.h"
+#include "thread-utils.h"
 
 void append_header_grep_pattern(struct grep_opt *opt, enum grep_header_field field, const char *pat)
 {
@@ -806,10 +807,46 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
 	opt->output(opt, "\n", 1);
 }
 
-static int match_funcname(struct grep_opt *opt, char *bol, char *eol)
+#ifndef NO_PTHREADS
+/*
+ * This lock protects access to the gitattributes machinery, which is
+ * not thread-safe.
+ */
+pthread_mutex_t grep_attr_mutex;
+
+static inline void grep_attr_lock(struct grep_opt *opt)
+{
+	if (opt->use_threads)
+		pthread_mutex_lock(&grep_attr_mutex);
+}
+
+static inline void grep_attr_unlock(struct grep_opt *opt)
+{
+	if (opt->use_threads)
+		pthread_mutex_unlock(&grep_attr_mutex);
+}
+#else
+#define grep_attr_lock(opt)
+#define grep_attr_unlock(opt)
+#endif
+
+static int match_funcname(struct grep_opt *opt, const char *name, char *bol, char *eol)
 {
 	xdemitconf_t *xecfg = opt->priv;
-	if (xecfg && xecfg->find_func) {
+	if (xecfg && !xecfg->find_func) {
+		struct userdiff_driver *drv;
+		grep_attr_lock(opt);
+		drv = userdiff_find_by_path(name);
+		grep_attr_unlock(opt);
+		if (drv && drv->funcname.pattern) {
+			const struct userdiff_funcname *pe = &drv->funcname;
+			xdiff_set_find_func(xecfg, pe->pattern, pe->cflags);
+		} else {
+			xecfg = opt->priv = NULL;
+		}
+	}
+
+	if (xecfg) {
 		char buf[1];
 		return xecfg->find_func(bol, eol - bol, buf, 1,
 					xecfg->find_func_priv) >= 0;
@@ -835,7 +872,7 @@ static void show_funcname_line(struct grep_opt *opt, const char *name,
 		if (lno <= opt->last_shown)
 			break;
 
-		if (match_funcname(opt, bol, eol)) {
+		if (match_funcname(opt, name, bol, eol)) {
 			show_line(opt, bol, eol, name, lno, '=');
 			break;
 		}
@@ -848,7 +885,7 @@ static void show_pre_context(struct grep_opt *opt, const char *name, char *buf,
 	unsigned cur = lno, from = 1, funcname_lno = 0;
 	int funcname_needed = !!opt->funcname;
 
-	if (opt->funcbody && !match_funcname(opt, bol, end))
+	if (opt->funcbody && !match_funcname(opt, name, bol, end))
 		funcname_needed = 2;
 
 	if (opt->pre_context < lno)
@@ -864,7 +901,7 @@ static void show_pre_context(struct grep_opt *opt, const char *name, char *buf,
 		while (bol > buf && bol[-1] != '\n')
 			bol--;
 		cur--;
-		if (funcname_needed && match_funcname(opt, bol, eol)) {
+		if (funcname_needed && match_funcname(opt, name, bol, eol)) {
 			funcname_lno = cur;
 			funcname_needed = 0;
 		}
@@ -942,19 +979,6 @@ static int look_ahead(struct grep_opt *opt,
 	return 0;
 }
 
-int grep_threads_ok(const struct grep_opt *opt)
-{
-	/* If this condition is true, then we may use the attribute
-	 * machinery in grep_buffer_1. The attribute code is not
-	 * thread safe, so we disable the use of threads.
-	 */
-	if ((opt->funcname || opt->funcbody)
-	    && !opt->unmatch_name_only && !opt->status_only && !opt->name_only)
-		return 0;
-
-	return 1;
-}
-
 static void std_output(struct grep_opt *opt, const void *buf, size_t size)
 {
 	fwrite(buf, size, 1, stdout);
@@ -1008,16 +1032,8 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
 	}
 
 	memset(&xecfg, 0, sizeof(xecfg));
-	if ((opt->funcname || opt->funcbody)
-	    && !opt->unmatch_name_only && !opt->status_only &&
-	    !opt->name_only && !binary_match_only && !collect_hits) {
-		struct userdiff_driver *drv = userdiff_find_by_path(name);
-		if (drv && drv->funcname.pattern) {
-			const struct userdiff_funcname *pe = &drv->funcname;
-			xdiff_set_find_func(&xecfg, pe->pattern, pe->cflags);
-			opt->priv = &xecfg;
-		}
-	}
+	opt->priv = &xecfg;
+
 	try_lookahead = should_lookahead(opt);
 
 	while (left) {
@@ -1093,7 +1109,7 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
 				show_function = 1;
 			goto next_line;
 		}
-		if (show_function && match_funcname(opt, bol, eol))
+		if (show_function && match_funcname(opt, name, bol, eol))
 			show_function = 0;
 		if (show_function ||
 		    (last_hit && lno <= last_hit + opt->post_context)) {
diff --git a/grep.h b/grep.h
index a652800..15d227c 100644
--- a/grep.h
+++ b/grep.h
@@ -115,6 +115,7 @@ struct grep_opt {
 	int show_hunk_mark;
 	int file_break;
 	int heading;
+	int use_threads;
 	void *priv;
 
 	void (*output)(struct grep_opt *opt, const void *data, size_t size);
@@ -131,4 +132,10 @@ struct grep_opt {
 extern struct grep_opt *grep_opt_dup(const struct grep_opt *opt);
 extern int grep_threads_ok(const struct grep_opt *opt);
 
+#ifndef NO_PTHREADS
+/* Mutex used around access to the attributes machinery if
+ * opt->use_threads.  Must be initialized/destroyed by callers! */
+extern pthread_mutex_t grep_attr_mutex;
+#endif
+
 #endif
-- 
1.7.8.431.g2abf2

^ permalink raw reply related

* Re: [PATCH v4 2/2] push: teach --recurse-submodules the on-demand option
From: Phil Hord @ 2011-12-12 21:16 UTC (permalink / raw)
  To: Jens Lehmann; +Cc: Junio C Hamano, Heiko Voigt, Fredrik Gustafsson, git
In-Reply-To: <4E9DE883.9050105@web.de>

On Tue, Oct 18, 2011 at 4:58 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
> Am 18.10.2011 00:33, schrieb Junio C Hamano:
>> The very first step for floating submodules support would be relatively
>> simple. We could declare that an entry in the .gitmodules file in the
>> superproject can optionally specify which branch needs to be checked out
>> with something like:
>>
>>       [submodule "libfoo"]
>>               branch = master
>>                 path = include/foo
>>                 url = git://foo.com/git/lib.git
>>
>> and when such an entry is defined, a command at the superproject level
>> would largely ignore what is at include/foo in the tree object recorded in
>> the superproject commit and in the index. When we show "git status" in the
>> superproject, instead of using the commit bound to the superproject, we
>> would use include/foo/.git/HEAD as the basis for detecting "local" changes
>> to the submodule.
>
> Yup. And the presence of the "branch" config could tell "git submodule
> update" to fetch and advance that branch to the tip every time it is run.
> And it could tell the diff machinery (which is also used by status) to
> ignore the differences between a submodule's HEAD and the SHA-1 in the
> superproject (while still allowing to silence the presence of untracked
> and/or modified files by using the diff.ignoreSubmodules option) and
> fetch would just stop doing any on-demand action for such submodules.
> Anything I missed?
>
>> We could even declare that the gitlink for such a
>> submodule should record 0{40} SHA-1 in the superproject, but I do not
>> think that is necessary.
>
> Me neither, e.g. the SHA-1 which was the submodules HEAD when it was added
> should do nicely. And that would avoid referencing a non-existing commit
> in case you later want to turn a floating submodule into an exact one.


I'm sorry I missed this comment before.

I hope we can allow storing the actual gitlink in the superproject for
each commit even when we're using floating submodules.  I
thought-experimented with this a bit last year and came to the
conclusion that I should be able to 'float' to tips (developer
convenience) and also to store the SHA-1 of each gitlink through
history (automated maybe; as-needed).

The problem with "float-only" is that it loses history so, for
example, git-bisect doesn't work.

The problem with "float + gitlinks", of course, is that it looks like
"not floating" to the developers (git-status is dirty unless
overridden, etc.)

Is there a deeper reason this wouldn't be possible?

Phil

^ permalink raw reply

* Re: [PATCH 1/2] Makefile: Windows lacks /dev/tty
From: Jeff King @ 2011-12-12 21:18 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Junio C Hamano, git
In-Reply-To: <4EE66DAB.5070407@kdbg.org>

On Mon, Dec 12, 2011 at 10:10:03PM +0100, Johannes Sixt wrote:

> diff --git a/Makefile b/Makefile
> index 2c506b3..5b7f6a8 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1139,6 +1139,7 @@ ifeq ($(uname_S),Windows)
>  	NO_CURL = YesPlease
>  	NO_PYTHON = YesPlease
>  	BLK_SHA1 = YesPlease
> +	NO_DEV_TTY = YesPlease
>  	NO_POSIX_GOODIES = UnfortunatelyYes
>  	NATIVE_CRLF = YesPlease
>  
> @@ -1232,6 +1233,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
>  	ETAGS_TARGET = ETAGS
>  	NO_INET_PTON = YesPlease
>  	NO_INET_NTOP = YesPlease
> +	NO_DEV_TTY = YesPlease
>  	NO_POSIX_GOODIES = UnfortunatelyYes
>  	COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/win32
>  	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"

Unless I've bungled something, these should be no-ops, shouldn't they?
The most recent version of the prompt series has platforms opting into
the replacement with HAVE_DEV_TTY.

-Peff

^ permalink raw reply

* Re: [RFC/PATCH 0/7] some sequencer loose ends (Re: Fix revert --abort on Windows)
From: Junio C Hamano @ 2011-12-12 21:31 UTC (permalink / raw)
  To: Jonathan Nieder
  Cc: Johannes Sixt, Ramkumar Ramachandra, git, Christian Couder,
	Martin von Zweigbergk, Jay Soffian
In-Reply-To: <20111210124644.GA22035@elie.hsd1.il.comcast.net>

Overall, I think this series is a more correct and faithful implementation
of the design we discussed earlier in the thread

 http://thread.gmane.org/gmane.comp.version-control.git/179304/focus=179625

I saw a few minor nits in the log messages but otherwise nothing
objectionable jumped at me from my initial reading of the series.

Thanks.

^ permalink raw reply

* Re: [PATCH 2/2] Makefile: optionally exclude code that needs Unix sockets
From: Jeff King @ 2011-12-12 21:39 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Junio C Hamano, git
In-Reply-To: <4EE66E58.6040404@kdbg.org>

On Mon, Dec 12, 2011 at 10:12:56PM +0100, Johannes Sixt wrote:

> Introduce a configuration option NO_UNIX_SOCKETS to exclude code that
> depends on Unix sockets and use it in MSVC and MinGW builds.

Makes sense, thanks.

> Notice that unix-socket.h was missing from LIB_H before; fix that, too.

Oops, thanks.

> All or most of the tests introduced by the series fail on Windows.
> What is the preferred way to exclude them? Mark the all with a
> prerequisite? Or exit early at the beginning of the test file?

The cache daemon is the only thing that uses sockets. I would suggest
just exiting early from its script (patch below).

The rest of the tests should pass under Windows. If they don't, then
they should be fixed (I'm happy to work on that, with the limitation
that I don't actually have a Windows box, and if at all possible I'd
like to keep it that way).

In theory we should also disable the documentation for credential-cache.
But that means surgery not only to Documentation/Makefile, but figuring
out how to pass the flags down to the actual asciidoc process (since
gitcredentials(7) mentions it in the text). Certainly possible, but I
don't know if it's worth the effort or not.

---
diff --git a/Makefile b/Makefile
index e3ee884..6e7e190 100644
--- a/Makefile
+++ b/Makefile
@@ -2220,6 +2220,7 @@ ifdef GIT_TEST_CMP_USE_COPIED_CONTEXT
 	@echo GIT_TEST_CMP_USE_COPIED_CONTEXT=YesPlease >>$@
 endif
 	@echo GETTEXT_POISON=\''$(subst ','\'',$(subst ','\'',$(GETTEXT_POISON)))'\' >>$@
+	@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst ','\'',$(NO_UNIX_SOCKETS)))'\' >>$@
 
 ### Detect Tck/Tk interpreter path changes
 ifndef NO_TCLTK
diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
index 3a65f99..82c8411 100755
--- a/t/t0301-credential-cache.sh
+++ b/t/t0301-credential-cache.sh
@@ -4,6 +4,11 @@ test_description='credential-cache tests'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-credential.sh
 
+test -z "$NO_UNIX_SOCKETS" || {
+	skip_all='skipping credential-cache tests, unix sockets not available'
+	test_done
+}
+
 # don't leave a stale daemon running
 trap 'code=$?; git credential-cache exit; (exit $code); die' EXIT
 

^ permalink raw reply related

* Re: [PATCH 2/5] mv: honor --verbose flag
From: Jeff King @ 2011-12-12 21:45 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jari Aalto, git
In-Reply-To: <7vwra136tj.fsf@alter.siamese.dyndns.org>

On Mon, Dec 12, 2011 at 11:53:28AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > The code for a verbose flag has been here since "git mv" was
> > converted to C many years ago, but actually getting the "-v"
> > flag from the command line was accidentally lost in the
> > transition.
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> > This has been broken since 2006, so I guess nobody really cares. But
> > it's simple to fix.
> 
> Heh. It means nobody exercised the codepaths that are inside "if (verbose)",
> so it may uncover old bugs, no?

I thought that at first, too, but actually there is only one code path
currently enabled by "verbose", and it is to print "Renaming ...". You
can also exercise that code path with "--dry-run" (and the whole path
consists of only a single printf, so hopefully we didn't manage to
squeeze any bugs in there).

Once upon a time, the verbose flag was passed on to add_file_to_index,
but that was dropped when the code switched to using
rename_index_entry_at in 81dc230 (git-mv: Keep moved index entries
inact, 2008-07-21).

-Peff

^ permalink raw reply

* Re: [PATCH 1/2] Makefile: Windows lacks /dev/tty
From: Johannes Sixt @ 2011-12-12 21:52 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20111212211801.GA9754@sigill.intra.peff.net>

Am 12.12.2011 22:18, schrieb Jeff King:
> The most recent version of the prompt series has platforms opting into
> the replacement with HAVE_DEV_TTY.

I see. Obviously, I've tested an earlier iteration. This patch can be
dropped then.

-- Hannes

^ permalink raw reply

* Re: [PATCH 4/5] mv: improve overwrite warning
From: Jeff King @ 2011-12-12 21:52 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jari Aalto, git
In-Reply-To: <7vobvd36ms.fsf@alter.siamese.dyndns.org>

On Mon, Dec 12, 2011 at 11:57:31AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > This message looks overly long to me, but I wanted to match the existing
> > messages. Another option would be just:
> >
> >   warning: overwriting 'three/one'
> 
> Yes, I think it makes perfect sense to drop the ugly "source=one destination=two"
> cruft, both for single-source and multiple-source cases.

Yeah, the more I look at the message in the patch I sent, the uglier it
gets. Here's a re-rolled 4 and 5 with the nicer format.

-Peff

^ permalink raw reply

* [PATCHv2 4/5] mv: improve overwrite warning
From: Jeff King @ 2011-12-12 21:54 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jari Aalto, git
In-Reply-To: <20111212215238.GD9754@sigill.intra.peff.net>

When we try to "git mv" over an existing file, the error
message is fairly informative:

  $ git mv one two
  fatal: destination exists, source=one, destination=two

When the user forces the overwrite, we give a warning:

  $ git mv -f one two
  warning: destination exists; will overwrite!

This is less informative, but still sufficient in the simple
rename case, as there is only one rename happening.

But when moving files from one directory to another, it
becomes useless:

  $ mkdir three
  $ touch one two three/one
  $ git add .
  $ git mv one two three
  fatal: destination exists, source=one, destination=three/one
  $ git mv -f one two three
  warning: destination exists; will overwrite!

The first message is helpful, but the second one gives us no
clue about what was overwritten. Let's mention the name of
the destination file:

  $ git mv -f one two three
  warning: overwriting 'three/one'

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/mv.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index ae6c30c..8dd5a45 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -177,7 +177,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 				 * check both source and destination
 				 */
 				if (S_ISREG(st.st_mode) || S_ISLNK(st.st_mode)) {
-					warning(_("%s; will overwrite!"), bad);
+					warning(_("overwriting '%s'"), dst);
 					bad = NULL;
 				} else
 					bad = _("Cannot overwrite");
-- 
1.7.8.13.g74677

^ permalink raw reply related

* [PATCHv2 5/5] mv: be quiet about overwriting
From: Jeff King @ 2011-12-12 21:54 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jari Aalto, git
In-Reply-To: <20111212215238.GD9754@sigill.intra.peff.net>

When a user asks us to force a mv and overwrite the
destination, we print a warning. However, since a typical
use would be:

  $ git mv one two
  fatal: destination exists, source=one, destination=two
  $ git mv -f one two
  warning: overwriting 'two'

this warning is just noise. We already know we're
overwriting; that's why we gave -f!

This patch silences the warning unless "--verbose" is given.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/mv.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 8dd5a45..2a144b0 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -177,7 +177,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 				 * check both source and destination
 				 */
 				if (S_ISREG(st.st_mode) || S_ISLNK(st.st_mode)) {
-					warning(_("overwriting '%s'"), dst);
+					if (verbose)
+						warning(_("overwriting '%s'"), dst);
 					bad = NULL;
 				} else
 					bad = _("Cannot overwrite");
-- 
1.7.8.13.g74677

^ permalink raw reply related

* Re: Breakage (?) in configure and git_vsnprintf()
From: Jonathan Nieder @ 2011-12-12 21:56 UTC (permalink / raw)
  To: Jeff King
  Cc: Michael Haggerty, Junio C Hamano, git discussion list,
	Michal Rokos, Brandon Casey
In-Reply-To: <20111212064305.GA16511@sigill.intra.peff.net>

Jeff King wrote:

> I'll leave the issue of "-std=c89" triggering SNPRINTF_RETURNS_BOGUS to
> people who know and care about autoconf. My gut is to say "don't do
> that". Git is not actually pure c89. We typically target systems that
> are _at least_ c89, but it's more important to match and run well on
> real-world systems than what was defined in the standard. So we don't
> depend on c99, but we do depend on quirks and features that were
> prominent in mid-90's Unix variants.

Using vsnprintf isn't even wrong from the standards-pedant point of
view --- vsnprintf has been in POSIX for a long time.  The problem is
that the configure script is not setting appropriate feature test
macros such as _XOPEN_SOURCE (like git-compat-util.h does) during its
feature tests.

Maybe it would be possible to hook the appropriate magic into
AC_LANG_PROGRAM.

^ permalink raw reply

* Re: Question about commit message wrapping
From: Frans Klaver @ 2011-12-12 22:16 UTC (permalink / raw)
  To: Holger Hellmuth
  Cc: Andrew Ardill, Jakub Narebski, Sidney San Martín, git
In-Reply-To: <4EE62DB9.8030406@ira.uka.de>

On Mon, 12 Dec 2011 17:37:13 +0100, Holger Hellmuth <hellmuth@ira.uka.de>  
wrote:

>> I am starting to think that we need to somehow keep the current
>> behavior, but override at smaller widths. Maybe even use format=flowed
>> in format-patch.
>
> Could you explain what overriding at smaller widths would achieve?  
> Supporting format=flowed would be nice though.

I specifically meant trying to detect pre-wrapped text, removing the  
new-lines where we think it is because of wrapping at 80 characters. So  
the result would be: perfect on screens up to 80 characters wide, and ok  
on anything wider.

The implementation of that however could affect code in the mail if it  
isn't done properly.

>
>> On the other hand, the fundamental use with git is to
>> communicate code, and I'm not sure how that [cw]ould be handled.
>
> I prefer wrapped code to code that is cut of at a specific column.  
> Wrapped code has much less possibility for misinterpretation. Python  
> programmers might disagree?

Wrapped code as in auto-wrapped? Or as in manually wrapped? Python  
programmers have significant white space, but you can still hard wrap  
stuff, as long as the next statement is properly indented.

>
> I see your proposal mainly as an improvement to the display of texts,  
> not code.

Me too.


-- 
Using Opera's revolutionary e-mail client: http://www.opera.com/mail/

^ permalink raw reply

* [PATCH v2] Update documentation for stripspace
From: Conrad Irwin @ 2011-12-12 22:28 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Conrad Irwin
In-Reply-To: <7vy5ui5h0k.fsf@alter.siamese.dyndns.org>

On Sun, Dec 11, 2011 at 10:41 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Conrad Irwin <conrad.irwin@gmail.com> writes:
>
> The original says "remove" and new one says "normalize*s*". I think we
> tend to say things in imperative mood (i.e. without the trailing "s").

Good point, fixed.

>
> I do not think 'user-provided metadata' is a good wording. This is just a
> simple text clean-up filter and you can use it to clean your text files
> that you mean to store in the repository as well.

Changed just to "text", as that seems simpler. I've left the example
uses as is, they were the prime reason for that sentence existing.

>
> The last one is a bit funny, though.
>
> By definition, you cannot end the last line with more than one '\n' (upon
> seeing the second '\n', you would realize immediately that the line you
> saw was _not_ the last line). I think you meant the file does not end with
> an incomplete line, i.e. "ensures the output does not end with an
> incomplete line by adding '\n' at the end if needed".

Hmm, I'm not sure that's the best way of describing it — I've gone with:
"add a missing '\n' to the last line if necessary.".

>
>> +In the case where the input consists entirely of whitespace characters, no
>> +output will be produced.
>> +
>> +*NOTE*: This is intended for cleaning metadata, prefer the `--whitespace=fix`
>> +mode of linkgit:git-apply[1] for correcting whitespace of patches or files in
>> +the repository.
>
> I can tell that these three lines were the _primary_ thing you wanted to
> add with this patch, having never seen anybody got confused between the
> whitespace breakage fix and text cleaning, I wonder if this is adding
> clarity or giving users an impression that git can do too many things than
> they can wrap their mind around and forcing them to wonder if they have to
> learn everything git can do for them.

The motivation for this patch was an old post to the Cairo mailing list
[1]. There they were using (previously undocumented) behaviour of
git stripspace, instead of git apply --whitespace=fix (it may be that
--whitespace=fix didn't exist at that point?).

I've moved the *NOTE* into a SEE ALSO section where I think it reads
less opinionatedly — is that better?

>
>>  OPTIONS
>>  -------
>>  -s::
>>  --strip-comments::
>> -     In addition to empty lines, also strip lines starting with '#'.
>> +     Also remove all lines starting with '#'.
[snip]
>
> If I were touching this description, I probably would say something like
> "Treat lines starting with a '#' as if they are empty lines".
>

If only it were that simple! If you have a commented line between two
non-commented lines, then no empty line results. I've added an example
to the re-rolled patch that show-cases the behaviour of comment
stripping in both cases. I went with "skip and remove" as the verb, to
imply that the lines are ignored by the previous transformations.

Thanks for the detailed feedback.

Conrad

[1] http://lists.freedesktop.org/archives/cairo/2006-June/007062.html

----8<----

Tell the user what this command is intended for, and expand the
description of what it does.

Signed-off-by: Conrad Irwin <conrad.irwin@gmail.com>
---
 Documentation/git-stripspace.txt |   69 ++++++++++++++++++++++++++++++++++---
 builtin/stripspace.c             |    2 +-
 2 files changed, 64 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-stripspace.txt b/Documentation/git-stripspace.txt
index b78f031..a0a6ea4 100644
--- a/Documentation/git-stripspace.txt
+++ b/Documentation/git-stripspace.txt
@@ -3,26 +3,83 @@ git-stripspace(1)
 
 NAME
 ----
-git-stripspace - Filter out empty lines
+git-stripspace - Remove unnecessary whitespace
 
 
 SYNOPSIS
 --------
 [verse]
-'git stripspace' [-s | --strip-comments] < <stream>
+'git stripspace' [-s | --strip-comments] < input
 
 DESCRIPTION
 -----------
-Remove multiple empty lines, and empty lines at beginning and end.
+
+Clean the input in the manner used by 'git' for text such as commit
+messages, notes, tags and branch descriptions.
+
+With no arguments, this will:
+
+- remove trailing whitespace from all lines
+- collapse multiple consecutive empty lines into one empty line
+- remove blank lines from the beginning and end of the input
+- add a missing '\n' to the last line if necessary.
+
+In the case where the input consists entirely of whitespace characters, no
+output will be produced.
 
 OPTIONS
 -------
 -s::
 --strip-comments::
-	In addition to empty lines, also strip lines starting with '#'.
+	Skip and remove all lines starting with '#'.
+
+EXAMPLES
+--------
+
+Given the following noisy input with '$' indicating the end of a line:
+
+--------
+|A brief introduction   $
+|   $
+|$
+|A new paragraph$
+|# with a commented-out line    $
+|explaining lots of stuff.$
+|$
+|# An old paragraph, also commented-out. $
+|      $
+|The end.$
+|  $
+---------
+
+Use 'git stripspace' with no arguments to obtain:
+
+--------
+|A brief introduction$
+|$
+|A new paragraph$
+|# with a commented-out line$
+|explaining lots of stuff.$
+|$
+|# An old paragraph, also commented-out.$
+|$
+|The end.$
+---------
 
-<stream>::
-	Byte stream to act on.
+Use 'git stripspace --strip-comments' to obtain:
+
+--------
+|A brief introduction$
+|$
+|A new paragraph$
+|explaining lots of stuff.$
+|$
+|The end.$
+---------
+
+SEE ALSO
+--------
+The `--whitespace=fix` mode of linkgit:git-apply[1].
 
 GIT
 ---
diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index 1288ffc..f16986c 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -75,7 +75,7 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
 				!strcmp(argv[1], "--strip-comments")))
 		strip_comments = 1;
 	else if (argc > 1)
-		usage("git stripspace [-s | --strip-comments] < <stream>");
+		usage("git stripspace [-s | --strip-comments] < input");
 
 	if (strbuf_read(&buf, 0, 1024) < 0)
 		die_errno("could not read the input");
-- 
1.7.8.164.g00d7e

^ permalink raw reply related

* Re: [PATCH v4 2/2] push: teach --recurse-submodules the on-demand option
From: Jens Lehmann @ 2011-12-12 22:29 UTC (permalink / raw)
  To: Phil Hord; +Cc: Junio C Hamano, Heiko Voigt, Fredrik Gustafsson, git
In-Reply-To: <CABURp0okOmsk4JV9Ku5pHJb5vT-kr_fmweNNBKZ_OoRyfZan=Q@mail.gmail.com>

Am 12.12.2011 22:16, schrieb Phil Hord:
> On Tue, Oct 18, 2011 at 4:58 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>> Am 18.10.2011 00:33, schrieb Junio C Hamano:
>>> We could even declare that the gitlink for such a
>>> submodule should record 0{40} SHA-1 in the superproject, but I do not
>>> think that is necessary.
>>
>> Me neither, e.g. the SHA-1 which was the submodules HEAD when it was added
>> should do nicely. And that would avoid referencing a non-existing commit
>> in case you later want to turn a floating submodule into an exact one.
> 
> 
> I'm sorry I missed this comment before.
> 
> I hope we can allow storing the actual gitlink in the superproject for
> each commit even when we're using floating submodules.

I think you misread my statement, I was just talking about the initial
commit containing the newly added submodule, not any subsequent ones.
Floating makes differences between the original SHA-1 and the current
tip of the branch invisible, so there is nothing to commit.

>  I thought-experimented with this a bit last year and came to the
> conclusion that I should be able to 'float' to tips (developer
> convenience) and also to store the SHA-1 of each gitlink through
> history (automated maybe; as-needed).

Which means that after "git submodule update" floated a submodule branch
further, you would have to commit that in the superproject.

> The problem with "float-only" is that it loses history so, for
> example, git-bisect doesn't work.

Yep. And different developers can have the same superproject commit
checked out but their submodules can be quite different.

> The problem with "float + gitlinks", of course, is that it looks like
> "not floating" to the developers (git-status is dirty unless
> overridden, etc.)

Yeah. But what if each "git submodule update" would update the tip of
the submodule branch and add that to the superproject? You could follow
a tip but still produce reproducible trees.

^ permalink raw reply

* Re: [RFC/PATCH] add update to branch support for "floating submodules"
From: Jens Lehmann @ 2011-12-12 22:31 UTC (permalink / raw)
  To: Leif Gruenwoldt; +Cc: Andreas T.Auer, Junio C Hamano, git
In-Reply-To: <CALFF=ZRB7qjj7VMhzr12ySdHmZsySoqceu5brFht8rX1+W3NPg@mail.gmail.com>

Am 12.12.2011 20:13, schrieb Leif Gruenwoldt:
> On Mon, Dec 12, 2011 at 1:42 PM, Andreas T.Auer
> <andreas.t.auer_gtml_37453@ursus.ath.cx> wrote:
> 
>> The next question is: Wouldn't you like to have the new stable branch only
>> pulled in, when the projectX (as the superproject) is currently on that new
>> development branch (maybe master)?
>>
>> But if you checkout that fixed released version 1.2.9.8, wouldn't it be
>> better that in that case the gitlinked version of the submodule is checked
>> out instead of some unrelated new version? I mean, when the gitlinks are
>> tracked with the projectX commits, this should work well.
>>
>> And what about a maintenance branch, which is not a fixed version but a
>> quite stable branch which should only have bugfixes. Shouldn't the auto-pull
>> be disabled in that case, too?
>>
>> I think the "auto-pull" behavior should depend on the currently checked out
>> branch. So the configuration options should allow the definition of one or
>> more mappings.
> 
> Yes. I think you nailed it. The floating behaviour would best be
> configured per branch.

Why not use .gitmodules and make the "branch" setting work without having to
sync it?

> An aside. Would this mean a "git pull" on the product repo would
> automatically do a pull (git submodule update) on the submodule too?

Me thinks that only "git submodule update" should do that. Or what should
happen for people not using pull but just doing fetch and checkout?

^ permalink raw reply

* Re: [PATCH v3 0/3] grep attributes and multithreading
From: René Scharfe @ 2011-12-12 22:37 UTC (permalink / raw)
  To: Thomas Rast; +Cc: git, Junio C Hamano, Jeff King, Eric Herman
In-Reply-To: <cover.1323723759.git.trast@student.ethz.ch>

Am 12.12.2011 22:16, schrieb Thomas Rast:
> I think we should finish up these three patches for the next release.

> I already posted a bunch of POC patches, but doing the timing manually
> has been getting on my nerves lately.  So I would first like to
> formalize some of the performance testing, perhaps along lines already
> drawn up by Michael Hagger, perhaps not.  Then we can revisit the
> issue of grep performance.  But I would prefer not to block the -W fix
> and two easy and confirmed speedups on that.

Yes, that's a good idea.  The three patches are uncontroversial 
incremental improvements.

> I dropped this part entirely:
>
>> How about adding a parameter to control the number of threads
>> (--threads?) instead that defaults to eight (or five) for the worktree
>> and one for the rest?  That would also make benchmarking easier.
>
> It does make testing a lot easier, but the interface is IMHO not fit
> for users and I have a feeling that the "right" for-debugging
> interface will end up falling out of the performance testing work
> (probably an environment variable).  The end-user option should be a
> config setting, if any.

Agreed; users shouldn't need to specify such a parameter -- our 
heuristic should be good enough.

René

^ permalink raw reply

* Re: [PATCH 4/2] grep: turn off threading for non-worktree
From: René Scharfe @ 2011-12-12 22:37 UTC (permalink / raw)
  To: Pete Wyckoff; +Cc: Thomas Rast, git, Eric Herman, Junio C Hamano
In-Reply-To: <20111210131305.GA13344@arf.padd.com>

Am 10.12.2011 14:13, schrieb Pete Wyckoff:
> rene.scharfe@lsrfire.ath.cx wrote on Wed, 07 Dec 2011 18:00 +0100:
>> Am 07.12.2011 09:12, schrieb Thomas Rast:
>>> René Scharfe wrote:
>>>> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
>>>> index 47ac188..e981a9b 100644
>>>> --- a/Documentation/git-grep.txt
>>>> +++ b/Documentation/git-grep.txt
>>>> @@ -228,8 +228,9 @@ OPTIONS
>>>>   	there is a match and with non-zero status when there isn't.
>>>>
>>>>   --threads<n>::
>>>> +	Run<n>  search threads in parallel.  Default is 8 when searching
>>>> +	the worktree and 0 otherwise.  This option is ignored if git was
>>>> +	built without support for POSIX threads.
>>> [...]
>>>> -		nr_threads = (online_cpus()>  1) ? THREADS : 0;
>>>> +		nr_threads = (online_cpus()>  1&&  !list.nr) ? THREADS : 0;
>>>
>>> It would be more consistent to stick to the pack.threads convention
>>> where 0 means "all of my cores", so to disable threading the user
>>> would set the number of threads to 1.  Or were you trying to measure
>>> the contention between the worker thread and the add_work() thread?
>>
>> Yes, indeed, the cost for the threading overhead does interest me.  The
>> documentation should perhaps mention --no-threads explicitly to avoid
>> confusion.
>>
>> Currently there is no way to specify "as many threads as there are
>> cores" here.  Previous measurements indicated that it wasn't too useful,
>> however, because I/O parallelism was beneficial even for machines with
>> less than eight cores and more threads didn't pay off.
>
> Right.  Even for single CPU machines this is true, so the
> nr_threads calculation above should still use all 8 THREADS
> regardless of the number of online_cpus().

That makes sense.  However, in a quick test with a simple regex against 
a cache-warm Linux repo threading increased the runtime of git grep by 
30% on a single-core virtual machine.  Let's keep that check until we 
understand this better..

René

^ permalink raw reply

* Re: Auto update submodules after merge and reset
From: Jens Lehmann @ 2011-12-12 22:39 UTC (permalink / raw)
  To: Andreas T.Auer; +Cc: Phil Hord, git
In-Reply-To: <4EE51D7B.7020806@ursus.ath.cx>

Am 11.12.2011 22:15, schrieb Andreas T.Auer:
> 
> 
> On 10.12.2011 00:57 Phil Hord wrote:
>>  On Thu, Dec 8, 2011 at 4:13 AM,
>>  <andreas.t.auer_gtml_37453@ursus.ath.cx> wrote:
>>
>>  Yes, but maybe you can update this information in the .gitmodules
>>  file easily with a command.  Maybe it could be something simpler
>>  than "git sync-gitmodules-branches", but that is essentially what it
>>  would do: it would save the current branch in each submodule as the
>>  "tracked" branch in the .gitmodules file.
> 
> Ok, I have read a better description of the "floating submodule" model now, so it is a different use case and somehow it makes sense. In that case there are probably just a few branches that you would like to follow, maybe an "unstable" for the newest development or "stable" for the current release or some maintenance branches.
> 
>>  Now this makes sense.  I want the same thing.  I want to preserve
>>  history on "old" commits, but I want to "advance to tip" on "new"
>>  commits.
>>
>>  The trouble, I think, is in telling the difference between "old" and
>>   "new".  I think it means there is a switch, like --auto-follow (or
>>  --no-auto-follow for the alternate if core.auto_follow is set).  But
>>   having a config option as the default is likely to break lots of
>>  scripts.
> 
> In my use case the branches on the submodules follow the superproject and (mostly) versions that are committed there, it just adds the possibility to keep on working without checking out a branch after an update and without colliding with existing branchnames in the submodule.

Using superproject branch names in the submodules make no sense for a
lot of use cases.

> The other use case wants to follow the commits of that other submodule without checking the corresponding gitlinks into the superproject. But wouldn't it also make sense here to define actually a mapping in the .gitmodule that says "if the branch 'develop' is checkedout in the supermodule then with every submodule update automatically pull the newest 'unstable' commit from the submodule"? Or for "master" follow "stable" or for the "maint" branch follow updates in the "bugfixes" branch.
> 
> For example
> 
> [submodule "commonlib"]
>     update = heads develop:unstable master:stable maint:bugfixes

Having that configured with "branch=unstable", "branch=stable" etc. in
.gitmodules on the superprojects branches would be simpler and achieve
the same functionality.

> So whenever a defined branch is checked out in the superproject the mapped branch will be checked out in the submodule ("new" commit), but if a (e.g. tagged) commit is checked out ("old" commit) then the gitlink in the superproject is used to check out the referenced commit in the submodule.

I think checkout should only use the submodule commit recorded in the
superproject and a subsequent "git submodule update" should be needed
to update the submodule to tip. Otherwise you record SHA-1 but still
won't be able to bisect ...

> In http://thread.gmane.org/gmane.comp.version-control.git/183837 was discussed whether the gitlink in the superproject should be set to all-zero if updates follow the tip or maybe use the SHA1 of the commit when the submodule was added. I think the gitlink should be updated everytime when a new commit in the superproject is created.

Nope, only when "git submodule update" is run. Otherwise you'll spray the
history with submodule updates totally unrelated to the commits in the
superproject, which is rather confusing.

^ permalink raw reply

* Re: [RFC/PATCH] add update to branch support for "floating submodules"
From: Phil Hord @ 2011-12-12 22:56 UTC (permalink / raw)
  To: Leif Gruenwoldt; +Cc: Andreas T.Auer, Junio C Hamano, git
In-Reply-To: <CALFF=ZRB7qjj7VMhzr12ySdHmZsySoqceu5brFht8rX1+W3NPg@mail.gmail.com>

On Mon, Dec 12, 2011 at 2:13 PM, Leif Gruenwoldt <leifer@gmail.com> wrote:
> On Mon, Dec 12, 2011 at 1:42 PM, Andreas T.Auer
> <andreas.t.auer_gtml_37453@ursus.ath.cx> wrote:
>
>> The next question is: Wouldn't you like to have the new stable branch only
>> pulled in, when the projectX (as the superproject) is currently on that new
>> development branch (maybe master)?
>>
>> But if you checkout that fixed released version 1.2.9.8, wouldn't it be
>> better that in that case the gitlinked version of the submodule is checked
>> out instead of some unrelated new version? I mean, when the gitlinks are
>> tracked with the projectX commits, this should work well.
>>
>> And what about a maintenance branch, which is not a fixed version but a
>> quite stable branch which should only have bugfixes. Shouldn't the auto-pull
>> be disabled in that case, too?
>>
>> I think the "auto-pull" behavior should depend on the currently checked out
>> branch. So the configuration options should allow the definition of one or
>> more mappings.
>
> Yes. I think you nailed it. The floating behaviour would best be
> configured per branch.

Yes, I think you nailed it too.  I've been thinking the same thing for
a while now, but I didn't know how to express it completely.  Some of
the discussion on here last week gelled the last bits in my mind.

To wit, I think I would want something like this in my project:

Use gitlinks when the superproject HEAD is one of these:
    refs/heads/maint/*
    refs/heads/svn/*     (historic branches)
    refs/tags/*
    <SHA1> (detached)

Float on the rest, using the branch given in .gitmodules (which may be
* to mean "use the same branch as the superproject".)

But maybe it is foolish of me to keep branches where I really want
lightweight tags.  If so, I could get away with this:

   Float if .git/HEAD begins with "refs/heads"
   Else, use the SHA1.


> An aside. Would this mean a "git pull" on the product repo would
> automatically do a pull (git submodule update) on the submodule too?

Good question.

I want to say "eventually, but not yet."  But someone else may
disagree.  "git pull --recurse-submodules=yes" does not do this yet.
A separate git-submodule-update is still required.  But I think this
is a separate issue from floating submodules.

Phil

^ permalink raw reply

* Re: [PATCH 1/3] test-terminal: give the child an empty stdin TTY
From: Jonathan Nieder @ 2011-12-12 23:25 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, Thomas Rast, git, Michael Haggerty
In-Reply-To: <20111212191602.GA14061@sigill.intra.peff.net>

Jeff King wrote:

> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -198,6 +198,9 @@ else
>  	exec 4>/dev/null 3>/dev/null
>  fi
>
> +exec 6<&0
> +exec 0</dev/null
> +
>  test_failure=0
>  test_count=0
>  test_fixed=0

Independently of the changes to explicitly pass HEAD to "git shortlog"
in t7006 (we should) and make test_terminal make stdin into a tty, too
(if tests still make sure "git log --stdin" launches a pager with
stdin not a tty, then it should be a fine change), this looks good to
me.

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox