* Re: [PATCH 2/4] Convert resolve_ref+xstrdup to new resolve_refdup function
From: Junio C Hamano @ 2011-12-12 18:07 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Jonathan Nieder
In-Reply-To: <1323688832-32016-2-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Thanks.
The patch looks correct but I have a slight maintainability concern and a
suggestion for possible improvement.
> ...
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index b7c6302..a66d3eb 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -696,17 +696,14 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
> {
> int ret = 0;
> struct branch_info old;
> + char *path;
> unsigned char rev[20];
> int flag;
> memset(&old, 0, sizeof(old));
> - old.path = resolve_ref("HEAD", rev, 0, &flag);
> - if (old.path)
> - old.path = xstrdup(old.path);
> + old.path = path = resolve_refdup("HEAD", rev, 0, &flag);
This uses "one 'const char *' pointer that is used for reading data from
and an extra 'char *' pointer that is used only for freeing" approach,
which has two advantages and one disadvantage:
+ Obviously, we do not have to cast away constness when freeing.
+ A caller that follows the pattern could move the "for-data" pointer
without having to worry about affecting "for-freeing" pointer. It could
even do something like this:
char *for_freeing;
const char *for_data;
for_data = for_freeing = resolve_refdup(...);
if (prefixcmp(for_data, "refs/heads/"))
for_data = skip_prefix(for_data, "refs/heads/");
... do other things using for_data pointer ...
free(for_freeing);
- If the for-freeing and for-data pointers are named too similarly, the
code becomes harder to read. It is easy for a person who is new to the
codebase to start using the for-freeing pointer to manipulate the data
(mostly harmless) or even advance the pointer by mistake (bad).
A handful of places in the existing codebase use this "two pointers"
approach primarily for the second advantage. The way they avoid the
disadvantage is by naming the other pointer "$something_to_free" (and the
"$something_" part is optional if there is only one such variable in the
context) to make it clear that the variable is not meant to be looked at
in the code and its primary purpose is for cleaning up at the end.
Perhaps renaming this "path" to "to_free" (or "path_to_free" if you want
to hint the "path-ness" to the readers, but see below) would make it less
likely that future tweakers mistakenly look at, modify the string thru, or
worse yet move the pointer itself.
> diff --git a/builtin/merge.c b/builtin/merge.c
> index a1c8534..6437db2 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -1096,6 +1096,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
> struct commit_list *common = NULL;
> const char *best_strategy = NULL, *wt_strategy = NULL;
> struct commit_list **remotes = &remoteheads;
> + char *branch_ref;
>
> if (argc == 2 && !strcmp(argv[1], "-h"))
> usage_with_options(builtin_merge_usage, builtin_merge_options);
> @@ -1104,12 +1105,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
> * Check if we are _not_ on a detached HEAD, i.e. if there is a
> * current branch.
> */
> - branch = resolve_ref("HEAD", head_sha1, 0, &flag);
> - if (branch) {
> - if (!prefixcmp(branch, "refs/heads/"))
> - branch += 11;
> - branch = xstrdup(branch);
> - }
> + branch = branch_ref = resolve_refdup("HEAD", head_sha1, 0, &flag);
Without s/branch_ref/to_free/ this part is unreadable and unmaintainable,
as it invites the "which variable should I use?" question.
It is more important to clarify that the "branch" variable is what the
code should look at and manipulate by *not* calling this for-freeing
pointer after what it contains (i.e. "branch").
When naming a "for-freeing" pointer variable, the kind of data the area of
memory happens to contain is of secondary importance. Being deliberately
vague about what the area of memory may contain is a good thing, because
it actively discourages the program from looking at the area via the
pointer if the variable is named "to_free" or something that does not
specify what it contains.
Other places in this patch that call the for-freeing variable "ref" share
the same issue but they are less specific than their for-data variable
counterpart, which makes it slightly less risky than this instance.
^ permalink raw reply
* [PATCH 1/3] test-terminal: give the child an empty stdin TTY
From: Thomas Rast @ 2011-12-12 18:09 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Jonathan Nieder, Michael Haggerty
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
If t7006 runs during any 'make test' run, the next 'read sha' will
fail (git-shortlog ate all of stdin already) and the while loop stops
immediately. In particular, this loop will only ever successfully
test *one* revision.
To fix this, change test-terminal.perl to open a third PTY for stdin,
send an EOF (Ctrl-D) immediately and close it later. Since this may
not be portable, we use POSIX::Termios to set it to Ctrl-D.
Noticed-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
t/test-terminal.perl | 22 +++++++++++++++++++---
1 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/t/test-terminal.perl b/t/test-terminal.perl
index ee01eb9..87b5a8c 100755
--- a/t/test-terminal.perl
+++ b/t/test-terminal.perl
@@ -4,14 +4,16 @@
use warnings;
use IO::Pty;
use File::Copy;
+use POSIX ();
-# 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.
sub start_child {
- my ($argv, $out, $err) = @_;
+ my ($argv, $in, $out, $err) = @_;
my $pid = fork;
if (not defined $pid) {
die "fork failed: $!"
} elsif ($pid == 0) {
+ open STDIN, "<&", $in;
open STDOUT, ">&", $out;
open STDERR, ">&", $err;
close $out;
@@ -64,13 +66,27 @@ sub copy_stdio {
or exit 1;
}
+sub set_default_eof_char {
+ my $fd = fileno shift;
+ my $termios = POSIX::Termios->new;
+ $termios->getattr($fd);
+ $termios->setcc(&POSIX::VEOF, 4);
+ $termios->setattr($fd, &POSIX::TCSANOW)
+ or die "Termios::setattr failed: $!";
+}
+
if ($#ARGV < 1) {
die "usage: test-terminal program args";
}
+my $master_in = new IO::Pty;
my $master_out = new IO::Pty;
my $master_err = new IO::Pty;
-my $pid = start_child(\@ARGV, $master_out->slave, $master_err->slave);
+set_default_eof_char($master_in->slave);
+my $pid = start_child(\@ARGV, $master_in->slave, $master_out->slave, $master_err->slave);
+close $master_in->slave;
close $master_out->slave;
close $master_err->slave;
+print $master_in "\cD";
copy_stdio($master_out, $master_err);
+close $master_in;
exit(finish_child($pid));
--
1.7.8.431.g2abf2
^ permalink raw reply related
* [PATCH 2/3] test-terminal: set output terminals to raw mode
From: Thomas Rast @ 2011-12-12 18:09 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Jonathan Nieder, Michael Haggerty
In-Reply-To: <b97d14da67bd6097e5b04f6ae3057c0f1d536a0b.1323713230.git.trast@student.ethz.ch>
Not setting them to raw mode causes funny things to happen, such as
\n -> \r\n translation:
./test-terminal.perl echo foo | xxd
0000000: 666f 6f0d 0a foo..
(Notice the added 0d.)
To avoid this, set the (pseudo)terminal to raw mode. Note that the
IO::Pty docs recommend doing it on both master and slave.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
t/test-terminal.perl | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/t/test-terminal.perl b/t/test-terminal.perl
index 87b5a8c..a2bfbe5 100755
--- a/t/test-terminal.perl
+++ b/t/test-terminal.perl
@@ -81,6 +81,10 @@ sub set_default_eof_char {
my $master_in = new IO::Pty;
my $master_out = new IO::Pty;
my $master_err = new IO::Pty;
+$master_out->set_raw();
+$master_err->set_raw();
+$master_out->slave->set_raw();
+$master_err->slave->set_raw();
set_default_eof_char($master_in->slave);
my $pid = start_child(\@ARGV, $master_in->slave, $master_out->slave, $master_err->slave);
close $master_in->slave;
--
1.7.8.431.g2abf2
^ permalink raw reply related
* [PATCH 3/3] t/lib-terminal: test test-terminal's sanity
From: Thomas Rast @ 2011-12-12 18:09 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Jonathan Nieder, Michael Haggerty
In-Reply-To: <b97d14da67bd6097e5b04f6ae3057c0f1d536a0b.1323713230.git.trast@student.ethz.ch>
The previous two commits show that getting test-terminal.perl right is
not trivial. Let lib-terminal.sh run a simple test that ensures it
actually opens TTYs for std{in,out,err} and that it does not let stdin
pass through.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
t/lib-terminal.sh | 19 +++++++++++++++++++
1 files changed, 19 insertions(+), 0 deletions(-)
diff --git a/t/lib-terminal.sh b/t/lib-terminal.sh
index 58d911d..9e4ff9c 100644
--- a/t/lib-terminal.sh
+++ b/t/lib-terminal.sh
@@ -33,3 +33,22 @@ test_expect_success PERL 'set up terminal for tests' '
}
fi
'
+
+cat >expect1 <<EOF
+stdin: 1
+stdout: 1
+stderr: 1
+EOF
+: >expect2
+
+test_expect_success TTY 'test-terminal.perl is sane' '
+ test_terminal perl -e "
+ use POSIX qw(isatty);
+ print \"stdin: \", isatty(STDIN), \"\\n\";
+ print \"stdout: \", isatty(STDOUT), \"\\n\";
+ print \"stderr: \", isatty(STDERR), \"\\n\";
+ " >actual1 &&
+ test_cmp expect1 actual1 &&
+ echo foo | test_terminal cat - >actual2 &&
+ test_cmp expect2 actual2
+'
--
1.7.8.431.g2abf2
^ permalink raw reply related
* Re: [PATCH 1/3] test-terminal: give the child an empty stdin TTY
From: Jonathan Nieder @ 2011-12-12 18:19 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Junio C Hamano, Jeff King, Michael Haggerty
In-Reply-To: <b97d14da67bd6097e5b04f6ae3057c0f1d536a0b.1323713230.git.trast@student.ethz.ch>
Hi,
Thomas Rast wrote:
> --- a/t/test-terminal.perl
> +++ b/t/test-terminal.perl
> @@ -4,14 +4,16 @@
> use warnings;
> use IO::Pty;
> use File::Copy;
> +use POSIX ();
>
> -# 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.
Just musing,
Jonathan
^ permalink raw reply
* Re: [PATCH 2/3] test-terminal: set output terminals to raw mode
From: Jonathan Nieder @ 2011-12-12 18:23 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Junio C Hamano, Jeff King, Michael Haggerty
In-Reply-To: <04a77f48dd5d5afebbe625ed25ebecd57b6a8840.1323713230.git.trast@student.ethz.ch>
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)?
^ permalink raw reply
* Re: [PATCH 3/3] t/lib-terminal: test test-terminal's sanity
From: Jonathan Nieder @ 2011-12-12 18:25 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Junio C Hamano, Jeff King, Michael Haggerty
In-Reply-To: <44602402ecfd0e49c202f03e87540934aa23bce0.1323713230.git.trast@student.ethz.ch>
Thomas Rast wrote:
> The previous two commits show that getting test-terminal.perl right is
> not trivial. Let lib-terminal.sh run a simple test that ensures it
> actually opens TTYs for std{in,out,err} and that it does not let stdin
> pass through.
This test would get run again for each lib-terminal.sh user. If we
want to "test the test machinery" like this, I suppose it would make
the most sense to run the check somewhere in the t00* series.
^ 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
* 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 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: [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 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: [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: 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: [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: [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] 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: [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 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 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 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 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 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 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
* 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
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