* Re: [RFC/PATCH] show tracking branches with their associated branch names
From: Santhosh @ 2011-12-12 14:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Vincent van Ravesteijn, git
In-Reply-To: <7vsjkq5h0i.fsf@alter.siamese.dyndns.org>
On Mon, Dec 12, 2011 at 12:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> It makes sense to make the various information available, but it does not
> mean it makes sense to add it by default for everybody at all. Given that
> against all common sense, many newbie web-tips and third-party documents
> suggest "git branch | sed -ne 's/^\* //p'" as a way to find the current
> branch in scripts, I am sure such a change will cause trouble to many
> while only helping a few.
>
You certainly have a point.
> I wouldn't mind a new option --verbose-format=... that takes various
> formatting letters similar to how --pretty=format:... does, though.
>
I will explore this option and see if it is worth the trouble.
Probably I can accomplish this locally with an alias.
~Santhosh.
^ permalink raw reply
* Re: Breakage (?) in configure and git_vsnprintf()
From: Jeff King @ 2011-12-12 14:25 UTC (permalink / raw)
To: Andreas Schwab
Cc: Michael Haggerty, Junio C Hamano, git discussion list,
Michal Rokos, Brandon Casey
In-Reply-To: <m262hmgmrw.fsf@igel.home>
On Mon, Dec 12, 2011 at 10:30:27AM +0100, Andreas Schwab wrote:
> Jeff King <peff@peff.net> writes:
>
> > if (maxsize > 0) {
> > - ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, ap);
> > + va_copy(cp, ap);
> > + ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, cp);
>
> You also need to call va_end on the copy.
Silly me. Thanks for catching.
Junio, here's a corrected version.
-- >8 --
Subject: [PATCH] compat/snprintf: don't look at va_list twice
If you define SNPRINTF_RETURNS_BOGUS, we use a special
git_vsnprintf wrapper assumes that vsnprintf returns "-1"
instead of the number of characters that you would need to
store the result.
To do this, it invokes vsnprintf multiple times, growing a
heap buffer until we have enough space to hold the result.
However, this means we evaluate the va_list parameter
multiple times, which is generally a bad thing (it may be
modified by calls to vsnprintf, yielding undefined
behavior).
Instead, we must va_copy it and hand the copy to vsnprintf,
so we always have a pristine va_list.
Signed-off-by: Jeff King <peff@peff.net>
---
compat/snprintf.c | 9 +++++++--
1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/compat/snprintf.c b/compat/snprintf.c
index e1e0e75..42ea1ac 100644
--- a/compat/snprintf.c
+++ b/compat/snprintf.c
@@ -19,11 +19,14 @@
#undef vsnprintf
int git_vsnprintf(char *str, size_t maxsize, const char *format, va_list ap)
{
+ va_list cp;
char *s;
int ret = -1;
if (maxsize > 0) {
- ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, ap);
+ va_copy(cp, ap);
+ ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, cp);
+ va_end(cp);
if (ret == maxsize-1)
ret = -1;
/* Windows does not NUL-terminate if result fills buffer */
@@ -42,7 +45,9 @@ int git_vsnprintf(char *str, size_t maxsize, const char *format, va_list ap)
if (! str)
break;
s = str;
- ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, ap);
+ va_copy(cp, ap);
+ ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, cp);
+ va_end(cp);
if (ret == maxsize-1)
ret = -1;
}
--
1.7.8.13.g74677
^ permalink raw reply related
* Git blame only current branch
From: Stephen Bash @ 2011-12-12 15:24 UTC (permalink / raw)
To: git discussion list
In-Reply-To: <e9e35956-a091-4143-8fd4-3516b54263a6@mail>
Hi all-
I'm curious if there's a method to make git blame merge commits that introduce code to the given branch rather than commits on the original (topic) branch? For example:
A--B--C---M--D master
\ /
1--2--3 topicA
I'd like a mode where 'git blame master ...' shows commit M for lines changed by topicA so I can easily do 'git blame M^ ...' to see changes (on master) prior to the merge of topicA. Unfortunately 'git blame master ^topicA ...' blames all the changes of topicA to 3 (which reading the docs appears to be the "correct" behavior).
Thanks!
Stephen
^ permalink raw reply
* Re: [RFC/PATCH] add update to branch support for "floating submodules"
From: Andreas T.Auer @ 2011-12-12 15:34 UTC (permalink / raw)
To: Leif Gruenwoldt; +Cc: Junio C Hamano, git
In-Reply-To: <CALFF=ZQKRgx_AodBQH17T9cSe_JFtoKie7DoMMfkTXCyCFospw@mail.gmail.com>
On 10.12.2011 16:27 Leif Gruenwoldt wrote:
> 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.
So you don't want to have a stale submodule as Junio suggested, which is
older than the gitlinked commit in the superproject, but you want to
have the newest stable version, which is not yet gitlinked in the
superproject, right?
Wouldn't ( cd commonlib ; git pull stable ) instead of
git submodule update commonlib
work as you want?
To be able to configure this update behavior in .gitmodules for _some_
submodules, could be helpful in this case.
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?
> This is how package management on many linux systems works.
> Dependencies get updated and all products reap the benefit (or
> catastrophe) automatically.
If I have e.g. the Debian testing distro, which is more floating than
the most other Linux distro releases, then I still get only those
versions of the packages that are referenced by this "Debian testing"
superproject, unless I specify a different superproject (e.g. "Debian
unstable") to get a newer version, but they are still tracked in some
superproject. I'm not aware of a way to get the newest version of a
package before it is in some "superproject", except downloading it
explictily somewhere else. But I don't think this is what you want.
^ permalink raw reply
* Re: Question about commit message wrapping
From: Holger Hellmuth @ 2011-12-12 16:37 UTC (permalink / raw)
To: Frans Klaver; +Cc: Andrew Ardill, Jakub Narebski, Sidney San Martín, git
In-Reply-To: <CAH6sp9NwyxZi6KR4U96=sWdiqCseyTLEDoHdw=y9hUx2kHwOpg@mail.gmail.com>
On 12.12.2011 09:41, Frans Klaver wrote:
>> Emails on this list are almost exclusively sent pre-wrapped to 80
>> character line lengths.
>> The result is exactly the kind of ragged output you used in your
>> example. Changing this behaviour may break backwards compatibility,
>> but it is already broken for 'future' compatibility.
I don't really see backwards compatibility broken either. At the moment
commit messages are usually pre-wrapped at 72 columns, which looks
perfect only on 80 column displays, ok on wider displays and bad on
narrow displays.
If the requirement to pre-wrap would fall and either 'git log' or 'less'
doing the wrap, old commit messages would still look perfect on 80
column, ok on wider and bad on narrow displays. Newer commit messages
would look good everywhere.
The only breakage would be that new long commit messages would look bad
on older git versions. Because of that the auto-wrap should be
implemented first and the "requirement" for 72 columns should fall in a
later version.
> I am starting to think that we need to somehow keep the current
> behavior, but override at smaller widths. Maybe even use format=flowed
> in format-patch.
Could you explain what overriding at smaller widths would achieve?
Supporting format=flowed would be nice though.
> On the other hand, the fundamental use with git is to
> communicate code, and I'm not sure how that [cw]ould be handled.
I prefer wrapped code to code that is cut of at a specific column.
Wrapped code has much less possibility for misinterpretation. Python
programmers might disagree?
I see your proposal mainly as an improvement to the display of texts,
not code.
^ permalink raw reply
* Re: Git blame only current branch
From: Jeff King @ 2011-12-12 16:55 UTC (permalink / raw)
To: Stephen Bash; +Cc: git discussion list
In-Reply-To: <d615954f-bed8-482d-a2e3-e1e741d6dd23@mail>
On Mon, Dec 12, 2011 at 10:24:47AM -0500, Stephen Bash wrote:
> I'm curious if there's a method to make git blame merge commits that
> introduce code to the given branch rather than commits on the original
> (topic) branch? For example:
Usually when you are interested in seeing merges like this in git-log,
you would use one of "--first-parent" or "--merges". However, though
"git blame" takes revision arguments, it does its own traversal of the
graph that does not respect those options.
Modifying it to do --first-parent is pretty easy:
diff --git a/builtin/blame.c b/builtin/blame.c
index 80febbe..c19a8cd 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1191,6 +1191,8 @@ static int num_scapegoats(struct rev_info *revs, struct commit *commit)
{
int cnt;
struct commit_list *l = first_scapegoat(revs, commit);
+ if (revs->first_parent_only)
+ return l ? 1 : 0;
for (cnt = 0; l; l = l->next)
cnt++;
return cnt;
With that, "git blame --first-parent" produces reasonable results for
me. But of course I didn't do more than 30 seconds of testing, so it is
entirely possible there are corner cases or unforeseen side effects.
Handling --merges is probably a little trickier, as you need to consider
only some commits as scapegoats, but still traverse through everything
to find the merges.
-Peff
^ permalink raw reply related
* Re: Git blame only current branch
From: Stephen Bash @ 2011-12-12 17:05 UTC (permalink / raw)
To: Jeff King; +Cc: git discussion list
In-Reply-To: <20111212165542.GA4802@sigill.intra.peff.net>
----- Original Message -----
> From: "Jeff King" <peff@peff.net>
> Sent: Monday, December 12, 2011 11:55:42 AM
> Subject: Re: Git blame only current branch
>
> On Mon, Dec 12, 2011 at 10:24:47AM -0500, Stephen Bash wrote:
>
> > I'm curious if there's a method to make git blame merge commits
> > that introduce code to the given branch rather than commits on
> > the original (topic) branch? For example:
>
> Usually when you are interested in seeing merges like this in
> git-log, you would use one of "--first-parent" or "--merges".
> However, though "git blame" takes revision arguments, it does
> its own traversal of the graph that does not respect those
> options.
My first thought was --first-parent, and was disappointed when I didn't find it in the blame documentation :) I think for my purposes --first-parent is better than --merges because there are non-merge commits on the branch(es) of interest (and thus I think the problem would become ill-posed in the --merges case).
> Modifying it to do --first-parent is pretty easy:
> ... snip ...
That's pretty simple... I'll try to do a little testing this afternoon.
Thanks!
Stephen
^ permalink raw reply
* Re: [PATCH v2 08/51] is_dup_ref(): extract function from sort_ref_array()
From: Junio C Hamano @ 2011-12-12 17:14 UTC (permalink / raw)
To: Michael Haggerty
Cc: Jeff King, git, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips
In-Reply-To: <4EE5E918.8090104@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> Extra refs are created locally by other git code that I am not familiar
> with, so duplicate extra refs could only be created by a bug in git.
I think we already discussed this, but just in case you forgot. They are
used to tell git to treat as if objects in histories leading to certain
commits are complete and available, i.e. only their object names matter
and the refname field is primarily for debugging. It would be a grave
regression if you errored out seeing duplicate names in them.
^ permalink raw reply
* Re: [PATCH] tag deletions not rejected with receive.denyDeletes= true
From: Junio C Hamano @ 2011-12-12 17:16 UTC (permalink / raw)
To: Jerome DE VIVIE; +Cc: Junio C Hamano, git
In-Reply-To: <12967682.2821323698766430.JavaMail.root@promailix.prometil.com>
Jerome DE VIVIE <j.devivie@prometil.com> writes:
> Junio C Hamano <gitster@pobox.com> writes :
>> Our documentation can also use some updates, as it dates to the days back
>> when we more liberally used "refs" and "branches" interchangeably.
>
> Ok, I have turned the patch below for documentation.
Err,.. what I meant by "documentation update" is more like this.
Documentation/config.txt | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 8a7d2d4..8eda8e4 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1653,15 +1653,15 @@ receive.unpackLimit::
`transfer.unpackLimit` is used instead.
receive.denyDeletes::
- If set to true, git-receive-pack will deny a ref update that deletes
- the ref. Use this to prevent such a ref deletion via a push.
+ If set to true, git-receive-pack will deny an update that deletes
+ the branch. Use this to prevent a push from deleting a branch.
receive.denyDeleteCurrent::
- If set to true, git-receive-pack will deny a ref update that
+ If set to true, git-receive-pack will deny an update that
deletes the currently checked out branch of a non-bare repository.
receive.denyCurrentBranch::
- If set to true or "refuse", git-receive-pack will deny a ref update
+ If set to true or "refuse", git-receive-pack will deny an update
to the currently checked out branch of a non-bare repository.
Such a push is potentially dangerous because it brings the HEAD
out of sync with the index and working tree. If set to "warn",
^ permalink raw reply related
* Re: Git blame only current branch
From: andreas.t.auer_gtml_37453 @ 2011-12-12 17:19 UTC (permalink / raw)
To: Stephen Bash; +Cc: Jeff King, git discussion list
In-Reply-To: <5e2440c1-8d11-4d92-b42f-14169a62ced1@mail>
On 12.12.2011 18:05 Stephen Bash wrote:
> ----- Original Message -----
> > From: "Jeff King" <peff@peff.net> Sent: Monday, December 12, 2011
> > 11:55:42 AM Subject: Re: Git blame only current branch
> >
> > On Mon, Dec 12, 2011 at 10:24:47AM -0500, Stephen Bash wrote:
> >
> > Usually when you are interested in seeing merges like this in
> > git-log, you would use one of "--first-parent" or "--merges".
> > However, though "git blame" takes revision arguments, it does its
> > own traversal of the graph that does not respect those options.
>
> My first thought was --first-parent, and was disappointed when I
> didn't find it in the blame documentation :) I think for my purposes
> --first-parent is better than --merges because there are non-merge
> commits on the branch(es) of interest (and thus I think the problem
> would become ill-posed in the --merges case).
>
> > Modifying it to do --first-parent is pretty easy: ... snip ...
>
> That's pretty simple... I'll try to do a little testing this
> afternoon.
You might need to consider that if the master branch was first merged
into topicA before topicA was merged back to the master that the master
would only be fast-forwarded and so the first parent of M would be 3 not
C. So depending how the developers merged you might get different results.
^ permalink raw reply
* Re: [RFC/PATCH] add update to branch support for "floating submodules"
From: Leif Gruenwoldt @ 2011-12-12 18:04 UTC (permalink / raw)
To: Andreas T.Auer; +Cc: Junio C Hamano, git
In-Reply-To: <4EE61EED.50604@ursus.ath.cx>
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 have a stale submodule as Junio suggested, which is
> older than the gitlinked commit in the superproject, but you want to have
> the newest stable version, which is not yet gitlinked in the superproject,
> right?
Right.
> Wouldn't ( cd commonlib ; git pull stable ) instead of
> git submodule update commonlib
> work as you want?
Yes that's how we perform business now.
> To be able to configure this update behavior in .gitmodules for _some_
> submodules, could be helpful in this case.
Yes my thoughts exactly.
> 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.
^ permalink raw reply
* 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
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