* 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
* 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 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 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: [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 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: [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
* [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
* [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 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 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 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 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
* BUG: git-p4: can't add files with special chars
From: Luke Diamand @ 2011-12-12 20:48 UTC (permalink / raw)
To: Git List
I just noticed this today. You can't add a file from git to perforce
that contains a p4 special character (@,#,% or *).
There is code to cope going the other way round (p4 file with special
character in it) but if you create a file in git and then try to git-p4
submit, it fails.
I've just tried a quick and simple fix, and it turns out that it's not
that easy as the special characters get expanded to %40, %2A and so-on.
The % seems to get further expanded by python...
Luke
^ permalink raw reply
* Re: [PATCH 1/3] test-terminal: give the child an empty stdin TTY
From: Junio C Hamano @ 2011-12-12 20:38 UTC (permalink / raw)
To: Jeff King; +Cc: Thomas Rast, git, Jonathan Nieder, Michael Haggerty
In-Reply-To: <20111212191602.GA14061@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Is this right place to do it?
>
> It doesn't catch "cd t && make"....
That's just an illustration. I know my audiences who will give us real
tested patches are intelligent ;-).
And I tend to agree that at the test-lib.sh level might be a bit too low
for convenience of debugging, but it is probably a good start. An obvious
alternative would be a patch similar to the illustration patch applied to
t/Makefile.
As I said in a separate post, I think this is orthogonal to the
test-terminal patch under discussion. Being able to give the tested
programs an environment that mimics an interactive tty session better is a
good thing to do regardless of the "test should not read from make's stdin"
issue.
> happen in t/Makefile. But I actually wonder if it should be in
> test-lib.sh, as it is as much about cleaning up the test script's
> environment as it is about protecting people running "make test" in a
> loop. I.e., something like:
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index bdd9513..5a38505 100644
> --- 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
>
> One downside of that approach is that it makes it harder to insert
> questionable debugging statements into test scripts. E.g., sometimes I
> will temporarily throw a "gdb" or even a "bash" invocation into a test
> script to investigate a failure. But that would still be possible by
> redirecting from "<6".
>
> -Peff
^ permalink raw reply
* Re: [PATCH 1/3] test-terminal: give the child an empty stdin TTY
From: Junio C Hamano @ 2011-12-12 20:06 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Thomas Rast, git, Jeff King, Michael Haggerty,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20111212190553.GJ31793@elie.hsd1.il.comcast.net>
Jonathan Nieder <jrnieder@gmail.com> writes:
> Sorry, I was thinking narrowly about the "git log" tests in
> t7006-pager.sh. I was saying that there, the fact that
> lib-terminal.sh creates an environment in which stdin is not
> guaranteed to be a terminal is a feature, not a bug, since it improves
> the test coverage (and I tend to find the "stdin not a tty" case more
> interesting).
I agree with Thomas's objective of giving ttys to all the streams of
process being tested by default to emulate the usage better, but I also
think being able to test some of the streams redirected to non-tty a good
feature to have in test-terminal driver. And I do not think these two have
to be either-or proposition.
I do not think "Run 'git log' as if the user is on an interactive
terminal, but has plugged /dev/null to its standard input" can be spelled
like this:
test-terminal git log </dev/null
but test-terminal could learn an equivalent feature perhaps from the
command line, no? Perhaps like
test-terminal --stdin=/dev/null git log
test-terminal --stdout=actual --stderr=error git shortlog
or somesuch?
^ permalink raw reply
* Re: [PATCH 5/5] mv: be quiet about overwriting
From: Junio C Hamano @ 2011-12-12 19:59 UTC (permalink / raw)
To: Jeff King; +Cc: Jari Aalto, git
In-Reply-To: <20111212075407.GE17532@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> 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: destination exists (will overwrite), source=one, destination=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>
> ---
> You could perhaps argue that it is useful in the case of moving multiple
> files into a directory (since it tells you _which_ files were
> overwritten). We could turn the warning on in that case, but I'm
> inclined to leave it. If the user cares about this information, they can
> use "-v" along with "-f".
Makes sense, but I also think even under verbose mode we should avoid the
future tense. I.e. something like this:
$ git mv -v -f one two
warning: overwriting two
$ git mv -v -f one two three
warning: overwriting three/one
> builtin/mv.c | 5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/mv.c b/builtin/mv.c
> index c9ecb03..b6e7e4f 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -177,8 +177,9 @@ 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), source=%s, destination=%s"),
> - bad, src, dst);
> + if (verbose)
> + warning(_("%s (will overwrite), source=%s, destination=%s"),
> + bad, src, dst);
> bad = NULL;
> } else
> bad = _("Cannot overwrite");
^ permalink raw reply
* Re: [PATCH 4/5] mv: improve overwrite warning
From: Junio C Hamano @ 2011-12-12 19:57 UTC (permalink / raw)
To: Jeff King; +Cc: Jari Aalto, git
In-Reply-To: <20111212075227.GD17532@sigill.intra.peff.net>
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.
> builtin/mv.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/builtin/mv.c b/builtin/mv.c
> index ae6c30c..c9ecb03 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(_("%s; will overwrite!"), bad);
> + warning(_("%s (will overwrite), source=%s, destination=%s"),
> + bad, src, dst);
> bad = NULL;
> } else
> bad = _("Cannot overwrite");
^ permalink raw reply
* Re: [PATCH 3/5] mv: make non-directory destination error more clear
From: Junio C Hamano @ 2011-12-12 19:55 UTC (permalink / raw)
To: Jeff King; +Cc: Jari Aalto, git
In-Reply-To: <20111212075136.GC17532@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Instead, let's show an error message like:
>
> $ git mv one two three
> fatal: destination 'three' is not a directory
Makes perfect sense.
> We could leave the usage message in place, too, but it
> doesn't actually help here. It contains no hints that there
> are two forms, nor that multi-file form requires that the
> endpoint be a directory. So it just becomes useless noise
> that distracts from the real error.
>
> 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 11abaf5..ae6c30c 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -94,7 +94,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
> destination = copy_pathspec(dest_path[0], argv, argc, 1);
> } else {
> if (argc != 1)
> - usage_with_options(builtin_mv_usage, builtin_mv_options);
> + die("destination '%s' is not a directory", dest_path[0]);
> destination = dest_path;
> }
^ permalink raw reply
* Re: [PATCH 2/5] mv: honor --verbose flag
From: Junio C Hamano @ 2011-12-12 19:53 UTC (permalink / raw)
To: Jeff King; +Cc: Jari Aalto, git
In-Reply-To: <20111212075124.GB17532@sigill.intra.peff.net>
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?
> Documentation/git-mv.txt | 8 ++++++--
> builtin/mv.c | 1 +
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-mv.txt b/Documentation/git-mv.txt
> index 4be7a71..e3c8448 100644
> --- a/Documentation/git-mv.txt
> +++ b/Documentation/git-mv.txt
> @@ -15,8 +15,8 @@ DESCRIPTION
> -----------
> This script is used to move or rename a file, directory or symlink.
>
> - git mv [-f] [-n] [-k] <source> <destination>
> - git mv [-f] [-n] [-k] <source> ... <destination directory>
> + git mv [-v] [-f] [-n] [-k] <source> <destination>
> + git mv [-v] [-f] [-n] [-k] <source> ... <destination directory>
>
> In the first form, it renames <source>, which must exist and be either
> a file, symlink or directory, to <destination>.
> @@ -40,6 +40,10 @@ OPTIONS
> --dry-run::
> Do nothing; only show what would happen
>
> +-v::
> +--verbose::
> + Report the names of files as they are moved.
> +
> GIT
> ---
> Part of the linkgit:git[1] suite
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 5efe6c5..11abaf5 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -59,6 +59,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
> int i, newfd;
> int verbose = 0, show_only = 0, force = 0, ignore_errors = 0;
> struct option builtin_mv_options[] = {
> + OPT__VERBOSE(&verbose, "be verbose"),
> OPT__DRY_RUN(&show_only, "dry run"),
> OPT__FORCE(&force, "force move/rename even if target exists"),
> OPT_BOOLEAN('k', NULL, &ignore_errors, "skip move/rename errors"),
^ permalink raw reply
* Re: [PATCH 1/5] docs: mention "-k" for both forms of "git mv"
From: Junio C Hamano @ 2011-12-12 19:52 UTC (permalink / raw)
To: Jeff King; +Cc: Jari Aalto, git
In-Reply-To: <20111212075031.GA17532@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I actually would rather just see:
>
> git mv [options] <source> <destination>
> git mv [options] <source>... <destination>
>
> but if we are going to go that route, we should probably decide on a
> style and convert all of the descriptions at the same time.
Also most commands when they use these multi-line synopsis style to
differenciate different invocation contexts do take different set of
options for different contexts, so we would need to update the option
descriptions to say "this option only makes sense in this context", etc.
> Documentation/git-mv.txt | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/Documentation/git-mv.txt b/Documentation/git-mv.txt
> index b8db373..4be7a71 100644
> --- a/Documentation/git-mv.txt
> +++ b/Documentation/git-mv.txt
> @@ -15,7 +15,7 @@ DESCRIPTION
> -----------
> This script is used to move or rename a file, directory or symlink.
>
> - git mv [-f] [-n] <source> <destination>
> + git mv [-f] [-n] [-k] <source> <destination>
> git mv [-f] [-n] [-k] <source> ... <destination directory>
>
> In the first form, it renames <source>, which must exist and be either
^ permalink raw reply
* Re: [PATCH] gitk: make "git describe" output clickable, too
From: Junio C Hamano @ 2011-12-12 19:48 UTC (permalink / raw)
To: Paul Mackerras; +Cc: git list, Jim Meyering
In-Reply-To: <87mxb0foqe.fsf@rho.meyering.net>
Jim Meyering <jim@meyering.net> writes:
> I noticed that automake's contribution guidelines suggest using
> "git describe" output in commit logs to reference previous commits.
> By contrast, in coreutils, I had acquired the habit of using a bare SHA1
> prefix (8 hex digits), since gitk creates clickable links for that, and
> not for "git describe" output.
>
> I prefer the readability of the full "git describe" output, yet want to
> retain the gitk links, so wrote the following that renders as clickable
> not just SHA1-like strings, but also an SHA1-like string that is
> prefixed by "-g".
>
> Signed-off-by: Jim Meyering <meyering@redhat.com>
> ---
> This is relative to master.
> Think of this as mere proof-of-concept:
Paul, I think this makes tons of sense. Comments?
> Ideally, the string preceding the -g would be used to disambiguate
> the SHA1 prefix, but that would require more code.
>
> I confess that I haven't looked to see if documentation needs
> to be updated or if this would merit test suite additions.
>
> gitk-git/gitk | 6 +++++-
> 1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/gitk-git/gitk b/gitk-git/gitk
> index 4cde0c4..f8eb613 100755
> --- a/gitk-git/gitk
> +++ b/gitk-git/gitk
> @@ -6688,7 +6688,7 @@ proc appendwithlinks {text tags} {
>
> set start [$ctext index "end - 1c"]
> $ctext insert end $text $tags
> - set links [regexp -indices -all -inline {\m[0-9a-f]{6,40}\M} $text]
> + set links [regexp -indices -all -inline {(?:\m|-g)[0-9a-f]{6,40}\M} $text]
> foreach l $links {
> set s [lindex $l 0]
> set e [lindex $l 1]
> @@ -6704,6 +6704,10 @@ proc appendwithlinks {text tags} {
> proc setlink {id lk} {
> global curview ctext pendinglinks
>
> + if {[string range $id 0 1] eq "-g"} {
> + set id [string range $id 2 end]
> + }
> +
> set known 0
> if {[string length $id] < 40} {
> set matches [longid $id]
> --
> 1.7.8.163.g9859a
^ permalink raw reply
* Re: [RFC/PATCH] add update to branch support for "floating submodules"
From: Junio C Hamano @ 2011-12-12 19:36 UTC (permalink / raw)
To: Leif Gruenwoldt; +Cc: git
In-Reply-To: <CALFF=ZQKRgx_AodBQH17T9cSe_JFtoKie7DoMMfkTXCyCFospw@mail.gmail.com>
Leif Gruenwoldt <leifer@gmail.com> writes:
> On Sat, Dec 10, 2011 at 1:30 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> So that use case does not sound like a good rationale to require addition
>> of floating submodules.
>
> Ok I will try another scenario :)
>
> Imagine again products A, B and C and a common library. The products are in
> a stable state of development and track a stable branch of the common lib.
> Then imagine an important security fix gets made to the common library. On
> the next pull of products A, B, and C they get this fix for free
> because they were
> floating. They didn't need to communicate with the maintainer of the common
> repo to know this. In fact they don't really care. They just want the
> latest stable
> code for that release branch.
>
> This is how package management on many linux systems works. Dependencies
> get updated and all products reap the benefit (or catastrophe) automatically.
Distro package dependency tracking is a poor analogy for many reasons, but
I'll only touch a few.
If you have a common library L and application packages A, B and C, first
of all, you do not build distro package of A from the sources of A and
L. Instead, package A has a build dependency on package L-devel (in other
words, "in order to build A from the source, you need L-devel package
installed in your build environment"), build A from its source, link it
against L's binary without having the source of L. So the source code
arrangement is very different from the typical submodule case, in that you
do not even need to have A and L appear in the same working tree, let
alone bound in a same superproject as two submodules.
A library L may have two API versions, and application A and B may be
written for its v1.0 and v2.0 API, respectively. Distro packaging makes
the binary package A and B _know_ about their own dependency requirements
by recording "A depends on L (v1.0<=,<v2.0)", "B depends on L (v2.0<=)",
etc.
Naively, one might think that two branches, branch-1.0 and branch-2.0, can
be defined in the repository of L, tell somebody (like "superproject that
covers all the packages in the distro") that A wants branch-1.0 and B
wants branch-2.0 of L respectively, to emulate this, but if one thinks
further, one would realize that it is insufficient. For one thing, it is
unclear what should happen when both A and B are asked to be checked out,
but more importantly, in dependency requirements on real distro packaging,
the application C could say "I want v1.0 API but v1.4 is broken and not
compatible with me", which won't fit on the two-branches model. A
workaround to add more branches to L could be devised but any workaround
cannot be a good solution that allows a random application C among 47
others to dictate how the branch structure of L project should look like.
Fortunately, the dependency management is a solved problem by distro
package management and build systems, and they do so without using
anything from submodules. There is no point reinventing these logic in git
submodules and emulating poorly.
The only remotely plausible analogy around distro packaging would be a
superproject full of all the packages in a distro as its submodules, and
records exact versions of each and every package that goes on a release
CD (or DVD). In that case, you do want to have a central registry that
records what exact version of each package is used to cut the CD and the
mother of all modules superproject could be one way to implement it. But
that is not an example of floating, but is a direct opposite.
This exchange convinced me further that anybody who wishes to use
"floating" is better off either by doing one or both of the following:
- using "exact" but not updating religiously, as the interdepency
requirement in their project is not strict; or
- not using submodules at all, but merely keeping these unrelated A, B, C
and L as standalone repositories next to each other in the directory
structure.
^ permalink raw reply
* Re: [PATCH 1/3] test-terminal: give the child an empty stdin TTY
From: Jeff King @ 2011-12-12 19:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Thomas Rast, git, Jonathan Nieder, Michael Haggerty
In-Reply-To: <7vfwgp4niu.fsf@alter.siamese.dyndns.org>
On Mon, Dec 12, 2011 at 11:07:21AM -0800, Junio C Hamano wrote:
> > Because of the latter, t7006.58ff cause unexpected results if you do:
> >
> > git rev-list <range> |
> > while read sha; do
> > git checkout sha
> > make test
> > done
>
> In the above, lack of dollar-sign in "git checkout $sha" is obvious ;-)
> but I think it is a bug that you are not running make with its stdin
> redirected from /dev/null in the first place.
>
> Perhaps "make test" should do that for all tests, not just this terminal
> related one? Doing it this way we do not have to worry about other tests
> reading from the standard input by mistake.
That was my thought, as well. We want the test environment to be as
sterile and predictable as possible, so connecting stdin to /dev/null
seems like a sensible thing.
> diff --git a/Makefile b/Makefile
> index ed82320..7a85237 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2239,7 +2239,7 @@ export NO_SVN_TESTS
> ### Testing rules
>
> test: all
> - $(MAKE) -C t/ all
> + $(MAKE) -C t/ all </dev/null
Is this right place to do it?
It doesn't catch "cd t && make". I would expect at the least for it to
happen in t/Makefile. But I actually wonder if it should be in
test-lib.sh, as it is as much about cleaning up the test script's
environment as it is about protecting people running "make test" in a
loop. I.e., something like:
diff --git a/t/test-lib.sh b/t/test-lib.sh
index bdd9513..5a38505 100644
--- 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
One downside of that approach is that it makes it harder to insert
questionable debugging statements into test scripts. E.g., sometimes I
will temporarily throw a "gdb" or even a "bash" invocation into a test
script to investigate a failure. But that would still be possible by
redirecting from "<6".
-Peff
^ permalink raw reply related
* Re: [PATCH 1/3] test-terminal: give the child an empty stdin TTY
From: Thomas Rast @ 2011-12-12 19:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, Jonathan Nieder, Michael Haggerty
In-Reply-To: <7vfwgp4niu.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
>
> Perhaps "make test" should do that for all tests, not just this terminal
> related one? Doing it this way we do not have to worry about other tests
> reading from the standard input by mistake.
[...]
> --- a/Makefile
> +++ b/Makefile
> @@ -2239,7 +2239,7 @@ export NO_SVN_TESTS
> ### Testing rules
>
> test: all
> - $(MAKE) -C t/ all
> + $(MAKE) -C t/ all </dev/null
But now you haven't fixed (in t/) 'make prove', 'make valgrind', or
./tXXXX-foo.sh. If anything, this angle of attack should go into
test-lib.sh...
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ 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