* 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
* Re: [RFC/PATCH] add update to branch support for "floating submodules"
From: Leif Gruenwoldt @ 2011-12-12 19:13 UTC (permalink / raw)
To: Andreas T.Auer; +Cc: Junio C Hamano, git
In-Reply-To: <4EE64B04.8080405@ursus.ath.cx>
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.
An aside. Would this mean a "git pull" on the product repo would
automatically do a pull (git submodule update) on the submodule too?
^ permalink raw reply
* Re: [PATCH 1/3] test-terminal: give the child an empty stdin TTY
From: Junio C Hamano @ 2011-12-12 19:07 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Jeff King, Jonathan Nieder, Michael Haggerty
In-Reply-To: <b97d14da67bd6097e5b04f6ae3057c0f1d536a0b.1323713230.git.trast@student.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> So far, test-terminal.perl did not care at all about the stdin (that
> is, leave it as-is). This mostly works well, but git-shortlog is a
> problem:
>
> * It takes decisions based on isatty(0). (No test checks this, but
> compare 'git shortlog </dev/null' with 'git shortlog' in a
> terminal.)
>
> * It reads all of stdin if !isatty(0) and no arguments were passed.
>
> 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.
-- >8 --
Subject: Do not let the tests read from standard input stream
Consider running a loop like this:
git rev-list <range> |
while read commit
do
git checkout $commit && make test || break
done
If any of the test reads from the standard input, we may end up running a
test for just one commit (and while running the test for that commit, the
remainder of the rev-list output is consumed by the test). A workaround
would be to redirect like this:
git checkout $commit && make test </dev/null || break
but the Makefile should be doing that for us instead.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Makefile | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
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
test-ctype$X: ctype.o
^ permalink raw reply related
* Re: [PATCH 1/3] test-terminal: give the child an empty stdin TTY
From: Jonathan Nieder @ 2011-12-12 19:05 UTC (permalink / raw)
To: Thomas Rast
Cc: git, Junio C Hamano, Jeff King, Michael Haggerty,
Nguyễn Thái Ngọc Duy
In-Reply-To: <201112121934.40953.trast@student.ethz.ch>
Thomas Rast wrote:
> I'm not sure I understand what you are arguing for or why. That I
> avoid wasting a Pty, and only replace stdin with /dev/null?
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).
That said, a test for the implicit HEAD behavior of "git shortlog"
would be a welcome addition to t4201-shortlog.sh, and we might need a
helper "test_with_stdin_as_a_terminal" for that.
The tests you mentioned in t7006 are just buggy --- it was just meant
to make sure we don't regress in the change introduced by 773b69bf
("shortlog: run setup_git_directory_gently() sooner"), and I doubt I
was thinking about the "implicit HEAD when stdin is a tty" behavior at
all. So independently of everything else, I believe we should do the
following.
-- >8 --
Subject: test: do not let "git shortlog" DWIM based on tty
In the spirit of v1.7.3.3~14^2 (t4203: do not let "git shortlog" DWIM
based on tty, 2010-10-19), do not leave out the revision argument to
"git shortlog" and rely on the default of HEAD in the usual case that
standard input is not connected to a terminal. Otherwise, tests break
when you do
git rev-list <range> |
while read sha; do
git checkout sha
make test
done
Reported-by: Thomas Rast <trast@student.ethz.ch>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
| 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)
--git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 320e1d1d..6f05b11a 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -409,14 +409,14 @@ test_GIT_PAGER_overrides expect_success test_must_fail 'git -p'
test_doesnt_paginate expect_failure test_must_fail 'git -p nonsense'
-test_pager_choices 'git shortlog'
+test_pager_choices 'git shortlog HEAD'
test_expect_success 'setup: configure shortlog not to paginate' '
git config pager.shortlog false
'
-test_doesnt_paginate expect_success 'git shortlog'
-test_no_local_config_subdir expect_success 'git shortlog'
-test_default_pager expect_success 'git -p shortlog'
-test_core_pager_subdir expect_success 'git -p shortlog'
+test_doesnt_paginate expect_success 'git shortlog HEAD'
+test_no_local_config_subdir expect_success 'git shortlog HEAD'
+test_default_pager expect_success 'git -p shortlog HEAD'
+test_core_pager_subdir expect_success 'git -p shortlog HEAD'
test_core_pager_subdir expect_success test_must_fail \
'git -p apply </dev/null'
--
1.7.8
^ permalink raw reply related
* Re: [PATCH 2/3] test-terminal: set output terminals to raw mode
From: Thomas Rast @ 2011-12-12 19:01 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Junio C Hamano, Jeff King, Michael Haggerty
In-Reply-To: <20111212182318.GE31793@elie.hsd1.il.comcast.net>
Jonathan Nieder wrote:
> Thomas Rast wrote:
>
> > Not setting them to raw mode causes funny things to happen, such as
> > \n -> \r\n translation:
> [...]
> > To avoid this, set the (pseudo)terminal to raw mode. Note that the
> > IO::Pty docs recommend doing it on both master and slave.
>
> Good idea, so for what it's worth,
> Acked-by: Jonathan Nieder <jrnieder@gmail.com>
>
> Does this change the behavior in
> <https://rt.cpan.org/Ticket/Display.html?id=65692> (oh please oh
> please)?
I don't think so. I tested this tweak of the script:
perl -MIO::Pty -MFile::Copy -e '
for (my $i = 0;; $i++) {
my $master = new IO::Pty;
my $slave = $master->slave;
$master->set_raw();
$slave->set_raw();
if (fork == 0) {
close $master or die "close: $!";
open STDOUT, ">&", $slave or die "dup2: $!";
close $slave or die "close: $!";
exec("echo", "hi", $i) or die "exec: $!";
}
close $slave or die "close: $!";
copy($master, \*STDOUT) or die "copy: $!";
close $master or die "close: $!"; wait;
}
'
That's over ssh on
$ uname -a
Darwin mackeller.inf.ethz.ch 11.1.0 Darwin Kernel Version 11.1.0: Tue Jul 26 16:07:11 PDT 2011; root:xnu-1699.22.81~1/RELEASE_X86_64 x86_64
What's odd is that when I was logged in at university (over Gbit
ethernet, but still over ssh), the unmodified version wouldn't even
get off the ground. I may just have been dreaming however.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [RFC/PATCH] add update to branch support for "floating submodules"
From: Andreas T.Auer @ 2011-12-12 18:42 UTC (permalink / raw)
To: Leif Gruenwoldt; +Cc: Junio C Hamano, git
In-Reply-To: <CALFF=ZRYB1LkAY5WSC4Eydu-N0KNnWLLM2CfbSXZji18yO82gw@mail.gmail.com>
On 12.12.2011 19:04 Leif Gruenwoldt wrote:
> On Mon, Dec 12, 2011 at 10:34 AM, Andreas T.Auer
> <andreas.t.auer_gtml_37453@ursus.ath.cx> wrote:
>
> > So you don't want to add a new commit to the products A, B and C
> > repos whenever the stable branch of the submodule changes, but on
> > the other hand when you commit changes to the products it would
> > still make sense to update the gitlink to the current commonlib
> > version together with your changes, too, right?
>
> Hmm I supose that does make sense. If the commonlib version was auto
> recorded during a commit of the product it would be nice. Then
> if/when the user reconfigured the submodule from "floating" to
> "strict" mode it would then have a submodule sha1 reference. I like
> how this sounds.
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.
^ permalink raw reply
* Re: [PATCH 1/3] test-terminal: give the child an empty stdin TTY
From: Thomas Rast @ 2011-12-12 18:34 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Junio C Hamano, Jeff King, Michael Haggerty
In-Reply-To: <20111212181915.GD31793@elie.hsd1.il.comcast.net>
Jonathan Nieder wrote:
> Hi,
>
> Thomas Rast wrote:
> > -# Run @$argv in the background with stdio redirected to $out and $err.
> > +# Run @$argv in the background with stdio redirected from $in and to $out and $err.
>
> I'm not thrilled about this change. The original purpose of
> test_terminal was to test commands like "git log" that need to check
> whether stdout is a tty in order to decide whether to use color and to
> paginate their output. Perhaps whether stdin is a tty _should_ affect
> those decisions, but it currently doesn't (for example, "echo HEAD |
> git log --stdin" works) and that would deserve a separate test, I'd
> think.
>
> The testsuite bug you mentioned sounds like a real one and worth
> fixing, though. Maybe there would be some way for test_terminal to
> give the caller some control over which file descriptors to replace
> with a terminal.
I'm not sure I understand what you are arguing for or why. That I
avoid wasting a Pty, and only replace stdin with /dev/null?
(Because with the current state of the tests, this shouldn't make much
of a difference. I just figured I should go all the way and give
commands an environment that really looks like they'd been called from
the terminal.)
--
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