* Re: GitWeb: Adding fork visualization based on objects/info/alternates
From: Petr Baudis @ 2007-05-28 13:00 UTC (permalink / raw)
To: Marius Storm-Olsen; +Cc: Git Mailing List
In-Reply-To: <46587CE5.1030501@trolltech.com>
Hi,
On Sat, May 26, 2007 at 08:31:01PM CEST, Marius Storm-Olsen wrote:
> In an effort to tidy up the project list, and given that the
> current fork support in GitWeb doesn't exactly fit my
> structure (where forks are in subdirs of parent repo, and has
> to be first level down; alas no seek unlike when not showing
> forks), I set out to add fork support based on
> objects/info/alternates
> of each repo. The result can be seen here:
> http://chaos.troll.no/~marius/git/forks_collapsed.png
> http://chaos.troll.no/~marius/git/forks_expanded.png
>
> I doubt you'd want it in the mainline, as you can't turn the
> feature off; it's just like another column which you can sort
> by. If you do not sort by the 'fork' column, the forks will
> still be colored with the first level color, but not ordered
> under it's parent repo.
>
> Just wanted to feed back what I did so others can get
> inspiration, tweak it, and add it to their own gitwebs.
>
> It cleans up the project list real nicely though.
I think this patch is quite an interesting idea, and I kind of like
the possibility to show forks in the summary project list, if it's
optional (I actually don't think if I would enable this say at
repo.or.cz - some projects have quite a lot of forks and it would
clutter it up a lot e.g. for text-mode browsers that don't grok
enough javascript). OTOH, taking fork info from alternates is more
controversial; I don't want to force the repo.or.cz forks model on
everyone but having $proj in alternates may not mean it is a fork... but
if more people think it's a reasonable heuristic, we might optionally
support it too.
If you would be willing to split this patch to two and make both
features optional (and sign off the patch), I think at least the
forks-in-summary part might be quite a worthy addition.
> @@ -1909,8 +1919,9 @@ EOF
> print qq(<link rel="shortcut icon" href="$favicon" type="image/png"/>\n);
> }
>
> + print "<script src=\"gitweb_scripts.js\" type=\"text/javascript\"></script>\n";
> print "</head>\n" .
> - "<body>\n";
> + "<body onload=\"collapseAllRows();\">\n";
>
> if (-f $site_header) {
> open (my $fd, $site_header);
Would it be more reasonable to just set the style for these rows by
default to display: none?
> --- /dev/null
> +++ b/gitweb/gitweb_scripts.js
We have other patch introducing blame.js, so maybe call this forks.js?
Or do we have a strong reason to prefer a single .js file with all the
lot?
--
Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
Ever try. Ever fail. No matter. // Try again. Fail again. Fail better.
-- Samuel Beckett
^ permalink raw reply
* Re: [PATCH] gitweb: Update copyright information
From: Petr Baudis @ 2007-05-28 13:01 UTC (permalink / raw)
To: Jan-Benedict Glaw; +Cc: Jakub Narebski, git
In-Reply-To: <20070528102743.GH27871@lug-owl.de>
On Mon, May 28, 2007 at 12:27:43PM CEST, Jan-Benedict Glaw wrote:
> On Mon, 2007-05-28 11:58:46 +0200, Jakub Narebski <jnareb@gmail.com> wrote:
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -2,6 +2,7 @@
> >
> > # gitweb - simple web interface to track changes in git repositories
> > #
> > +# (C) 2006-2007, Git Development Community <git@vger.kernel.org>
> > # (C) 2005-2006, Kay Sievers <kay.sievers@vrfy.org>
> > # (C) 2005, Christian Gierke
> > #
>
> Am I the only person that doesn't think that there's a community based
> copyright? Isn't it a matter of people (or a company in case of paid
> work by an employee)???
I'm wondering about it too, this kind of (C) makes no sense to me. But
IANAL...
--
Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
Ever try. Ever fail. No matter. // Try again. Fail again. Fail better.
-- Samuel Beckett
^ permalink raw reply
* Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.
From: Jim Meyering @ 2007-05-28 13:14 UTC (permalink / raw)
To: Linus Torvalds; +Cc: git
In-Reply-To: <alpine.LFD.0.98.0705270904240.26602@woody.linux-foundation.org>
Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Sun, 27 May 2007, Jim Meyering wrote:
>>
>> I have to disagree. There may be precedent for hiding EPIPE errors,
>> but that is not the norm among command line tools wrt piped stdout.
>
> .. and this is a PROBLEM. Which is why I think your patch was really
> wrong.
>
> I don't know how many people remember all the _stupid_ problems we had
> exactly because many versions of bash are crap, crap, crap, and people
> (including you) don't realize that EPIPE is _different_ from other write
> errors.
>
> Just do a google search for
>
> "broken pipe" bash
>
> and not only will you see a lot of complaints, but the #5 entry is a
> complaint for a git issue that we had tons of problems with. See for
> example
>
> http://www.gelato.unsw.edu.au/archives/git/0504/2602.html
Whether bash should print a diagnostic when it kills a process with
SIGPIPE is _different_ from whether the writing process should diagnose
its own write failure arising from a handled SIGPIPE.
I suspect that git's special treatment of EPIPE was a shoot-the-messenger
reaction to the work-around (trap '' PIPE) required to avoid diagnostics
from porcelain being interpreted by what would now be a 2-year-old
version of bash. It is time to remove that work-around, because it
can obscure real errors, and removing it will be largely unnoticed.
...
>> Do you really want git-log to continue to do this?
>>
>> $ (trap '' PIPE; git-log; echo $? >&2 ) | :
>> 0
>>
>> With my patch, it does this:
>>
>> $ (trap '' PIPE; ./git-log; echo $? >&2 ) | :
>> fatal: write failure on standard output: Broken pipe
>> 128
>
> That error return is fine. The annoying error report, however, is NOT.
That error message serves to indicate a REAL FLAW, some of the time.
In such cases, the diagnostic is likely to be welcome, not annoying.
Which is more important: avoiding annoyance in some now-very-unusual
circumstances, or allowing robust porcelain to diagnose real errors?
> And your arguments that "others do it wrong, so we can too" is so broken
> as to be really really sad. If you cannot see the serious problem with
> that argument, I don't know what to tell you.
Questionable debate tactic: misrepresent an opponent's argument with a
statement that is obviously foolish, then proceed to argue that anyone
who doesn't recognise the silliness of the restated argument leaves
you speechless.
My argument is "If so many other tools do it RIGHT,
why can't git do it right, too?".
> Try this:
>
> trap '' PIPE; ./git-log | head
>
> and dammit, if you get an error message from that, your program is BROKEN.
>
> And if you cannot understand that, then I don't even know what to say.
Of course error messages are annoying when your short-pipe-read is
_deliberate_ (tho, most real uses of git tools will actually get no
message to be annoyed about[*]), but what if there really *is* a mistake?
Try this:
# You want to force git to ignore the error.
$ trap '' PIPE; git-rev-list HEAD | sync
$
# I want to diagnose the error (the exit 128 is from zsh, bash gets 0):
$ trap '' PIPE; git-rev-list HEAD | sync
fatal: write failure on standard output: Broken pipe
[Exit 128]
The use of "sync" above is obviously a mistake.
With the existing git tools, most write-to-stdout errors are ignored,
including EPIPE, so this erroneous command completes successfully, just as
it would when writing to a full or corrupt disk. Even if you use bash's
"set -o pipefail" option, there is no indication of the failure. With my
patch, write errors are reported, even EPIPE, so that in this case, the
user of the above will get an error message. And with bash's pipefail
option, the git-rev-list write failure can propagate "out" to the script.
Since using "sync" is contrived, for a little more realism, imagine the
SHA1 strings are being written to a FIFO, and you don't have access to
the program running on the other side. The sender should have a way to
detect when the other end closes the pipe prematurely. Exempting EPIPE,
it CANNOT detect failure:
$ mkfifo F && head -1 F > /dev/null & sleep 1
$ trap '' PIPE; git-rev-list HEAD > F && echo bad: ignored write failure
bad: ignored write failure
Handling EPIPE like other write errors, it CAN detect failure:
$ mkfifo F && head -1 F > /dev/null & sleep 1
$ trap '' PIPE; ./git-rev-list HEAD > F || echo ok
fatal: write failure on standard output: Broken pipe
ok
> But _exiting_ is fine. It's the bogus error reporting that isn't. The
> above command like should NOT cause the user to have to skip stderr -
> because no error happened!
Don't worry about the diagnostic. It probably won't show up much at all
[*] these days, since most shells now let SIGPIPE kill the writer (silently).
And if the message does appear once in a while, there are cleaner ways
to suppress it than hamstringing all of the git plumbing for everyone.
In fact, in this vein, the existing EPIPE exceptions in write_or_die.c
and merge-recursive.c should be removed. Here's a revised patch to do
that, too. Junio, if you see fit to accept any part of this, I'll be
happy to write test case additions demonstrating before/after improvements.
========================================================================
Subject: [PATCH] Don't ignore write failure from git-diff, git-log, etc.
Currently, when git-diff writes to a full device or gets an I/O error,
it fails to detect the write error:
$ git-diff |wc -c
3984
$ git-diff > /dev/full && echo ignored write failure
ignored write failure
git-log does the same thing:
$ git-log -n1 > /dev/full && echo ignored write failure
ignored write failure
Each and every git command should report such a failure.
Some already do, but with the patch below, they all do, and we
won't have to rely on code in each command's implementation to
perform the right incantation.
$ ./git-log -n1 > /dev/full
fatal: write failure on standard output: No space left on device
[Exit 128]
$ ./git-diff > /dev/full
fatal: write failure on standard output: No space left on device
[Exit 128]
You can demonstrate this with git's own --version output, too:
(but git --help detects the failure without this patch)
$ ./git --version > /dev/full
fatal: write failure on standard output: No space left on device
[Exit 128]
Note that the fcntl test (for whether the fileno may be closed) is
required in order to avoid EBADF upon closing an already-closed stdout,
as would happen for each git command that already closes stdout; I think
update-index was the one I noticed in the failure of t5400, before I
added that test.
Also, to be consistent, don't ignore EPIPE write failures.
Signed-off-by: Jim Meyering <jim@meyering.net>
---
git.c | 11 ++++++++++-
merge-recursive.c | 3 ---
write_or_die.c | 4 ----
3 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/git.c b/git.c
index 29b55a1..e176ab4 100644
--- a/git.c
+++ b/git.c
@@ -308,6 +308,7 @@ static void handle_internal_command(int argc, const char **argv, char **envp)
for (i = 0; i < ARRAY_SIZE(commands); i++) {
struct cmd_struct *p = commands+i;
const char *prefix;
+ int status;
if (strcmp(p->cmd, cmd))
continue;
@@ -321,7 +322,15 @@ static void handle_internal_command(int argc, const char **argv, char **envp)
die("%s must be run in a work tree", cmd);
trace_argv_printf(argv, argc, "trace: built-in: git");
- exit(p->fn(argc, argv, prefix));
+ status = p->fn(argc, argv, prefix);
+
+ /* Close stdout if necessary, and diagnose any failure. */
+ if (fcntl(fileno (stdout), F_GETFD) >= 0)
+ && (ferror(stdout) || fclose(stdout)))
+ die("write failure on standard output: %s",
+ strerror(errno));
+
+ exit(status);
}
}
diff --git a/merge-recursive.c b/merge-recursive.c
index 8f72b2c..bfa4335 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -523,9 +523,6 @@ static void flush_buffer(int fd, const char *buf, unsigned long size)
while (size > 0) {
long ret = write_in_full(fd, buf, size);
if (ret < 0) {
- /* Ignore epipe */
- if (errno == EPIPE)
- break;
die("merge-recursive: %s", strerror(errno));
} else if (!ret) {
die("merge-recursive: disk full?");
diff --git a/write_or_die.c b/write_or_die.c
index 5c4bc85..fadfcaa 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -41,8 +41,6 @@ int write_in_full(int fd, const void *buf, size_t count)
void write_or_die(int fd, const void *buf, size_t count)
{
if (write_in_full(fd, buf, count) < 0) {
- if (errno == EPIPE)
- exit(0);
die("write error (%s)", strerror(errno));
}
}
@@ -50,8 +48,6 @@ void write_or_die(int fd, const void *buf, size_t count)
int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg)
{
if (write_in_full(fd, buf, count) < 0) {
- if (errno == EPIPE)
- exit(0);
fprintf(stderr, "%s: write error (%s)\n",
msg, strerror(errno));
return 0;
^ permalink raw reply related
* Re: [RFC/PATCH] gitweb: Create special from-file/to-file header for combined diff
From: Petr Baudis @ 2007-05-28 13:29 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Junio C Hamano, git
In-Reply-To: <200705280850.52323.jnareb@gmail.com>
On Mon, May 28, 2007 at 08:50:51AM CEST, Jakub Narebski wrote:
> Junio C Hamano wrote:
> > Jakub Narebski <jnareb@gmail.com> writes:
> >
> >> Instead of using default, diff(1) like from-file/to-file header for
> >> combined diff (for a merge commit), which looks like:
> >>
> >> --- a/git-gui/git-gui.sh
> >> +++ b/_git-gui/git-gui.sh_
> >>
> >> (where _link_ denotes [hidden] hyperlink), create from-file(n)/to-file
> >> header, using n/file for each or parents, e.g.:
> >>
> >> --- 1/_git-gui/git-gui.sh_
> >> --- 2/_git-gui.sh_
> >> +++ b/_git-gui/git-gui.sh_
> >
> > Sounds quite straightforward to implement, and diff with 1/
> > would be useful to recreate what the person who did the merge
> > pulled in, for most of the time. I suspect diff with 2/ is
> > almost always uninteresting, though.
I like it too.
> Errr... it _is_ implemented in this patch, although code is not perfect
> and has some unnecessary repetitions.
It just shows links to older versions of the blob, doesn't it? Links to
diffs themselves would be useful too.
--
Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
Ever try. Ever fail. No matter. // Try again. Fail again. Fail better.
-- Samuel Beckett
^ permalink raw reply
* Re: [RFC/PATCH] gitweb: Create special from-file/to-file header for combined diff
From: Jakub Narebski @ 2007-05-28 14:22 UTC (permalink / raw)
To: Petr Baudis; +Cc: Junio C Hamano, git
In-Reply-To: <20070528132959.GT4489@pasky.or.cz>
On Mon, 28 May 2007, Petr Baudis wrote:
> On Mon, May 28, 2007 at 08:50:51AM CEST, Jakub Narebski wrote:
>> Junio C Hamano wrote:
>>> Jakub Narebski <jnareb@gmail.com> writes:
>>>
>>>> Instead of using default, diff(1) like from-file/to-file header for
>>>> combined diff (for a merge commit), which looks like:
>>>>
>>>> --- a/git-gui/git-gui.sh
>>>> +++ b/_git-gui/git-gui.sh_
>>>>
>>>> (where _link_ denotes [hidden] hyperlink), create from-file(n)/to-file
>>>> header, using n/file for each or parents, e.g.:
>>>>
>>>> --- 1/_git-gui/git-gui.sh_
>>>> --- 2/_git-gui.sh_
>>>> +++ b/_git-gui/git-gui.sh_
>>>
>>> Sounds quite straightforward to implement, and diff with 1/
>>> would be useful to recreate what the person who did the merge
>>> pulled in, for most of the time. I suspect diff with 2/ is
>>> almost always uninteresting, though.
>
> I like it too.
>
>> Errr... it _is_ implemented in this patch, although code is not perfect
>> and has some unnecessary repetitions.
>
> It just shows links to older versions of the blob, doesn't it? Links to
> diffs themselves would be useful too.
For me most important were those from names (or /dev/null) in the case
of rename or copying (or file which was not present in some branches).
The link is to older version of blob; the links to appropriate blobdiffs
(and with earlier PATCH/RFC "gitweb: Provide links to individual
commitdiffs in difftree for merges" also to appropriate commitdiffs)
are in the difftree / whatchanged table.
Where to put link to diff? Should name be link to diff instead of
link to blob (to version at parent)?
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCH 1/3] Lazily open pack index files on demand
From: Linus Torvalds @ 2007-05-28 16:22 UTC (permalink / raw)
To: Nicolas Pitre; +Cc: Shawn O. Pearce, Dana How, Junio C Hamano, git
In-Reply-To: <alpine.LFD.0.99.0705271929480.3366@xanadu.home>
On Sun, 27 May 2007, Nicolas Pitre wrote:
>
> It helps irrespective of the number of pack files. With the current
> binary search the lookup cost is O(log n). With a Newton method this
> cost is almost O(1).
This is not true.
First off, when comparing O(logn) to O(1), with "n" being less than a
billion, they are pretty much exactly the same. Think of it this way:
O(logn) == O(9) == O(1), if you know that n < 10**9.
Secondly, the cost of Newton isn't "almost O(1)". I don't know _what_ it
is (the rule of thumb with Newton-Raphson should be that the number of
significant correct digits in the answer doubles with each iteration: I
think that probably means that it should approximate O(loglog(n)), but I
haven't thought deeply about it.
But regardless, we end up testing "a few" values. Both for the binary
search and for Newton-Raphson.
So every time you talk about O-notation, you should also consider the
constant costs, especially if the function in question is a slow-changing
one (ie when we start talking O(N**3) we can start ignoring the constants.
With O(logn), you sure as hell cannot!)
And the thing is, Newton-Raphson didn't actually speed anything up in my
tests. Sometimes it was better, sometimes it was worse, most of the time
it was in the noise.
Now, I'm sure the thing could be tweaked. Maybe I didn't do a very good
job at my initial implementation. It was a quick hack. But before somebody
makes a better one and actually shows better performance, I'd say that the
jury is still out.
Linus
^ permalink raw reply
* Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.
From: Marco Roeland @ 2007-05-28 15:46 UTC (permalink / raw)
To: Jim Meyering; +Cc: Linus Torvalds, git
In-Reply-To: <87sl9hw0o0.fsf@rho.meyering.net>
On monday May 28th 2007 at 15:14 Jim Meyering wrote:
> > I don't know how many people remember all the _stupid_ problems we had
> > exactly because many versions of bash are crap, crap, crap, and people
> > (including you) don't realize that EPIPE is _different_ from other write
> > errors.
> >
> > Just do a google search for
> >
> > "broken pipe" bash
> >
> > and not only will you see a lot of complaints, but the #5 entry is a
> > complaint for a git issue that we had tons of problems with. See for
> > example
> >
> > http://www.gelato.unsw.edu.au/archives/git/0504/2602.html
>
> Whether bash should print a diagnostic when it kills a process with
> SIGPIPE is _different_ from whether the writing process should diagnose
> its own write failure arising from a handled SIGPIPE.
>
> I suspect that git's special treatment of EPIPE was a shoot-the-messenger
> reaction to the work-around (trap '' PIPE) required to avoid diagnostics
> from porcelain being interpreted by what would now be a 2-year-old
> version of bash. It is time to remove that work-around, because it
> can obscure real errors, and removing it will be largely unnoticed.
Good point. But also notice that when you are stuck with a shell that
does complain about SIGPIPE it is _really_ annoying!
On current Debian 'sid' with your patch this does not seem to be the
case.
The second chunk of your patch (to git.c) contains a small copy-paste
accident methinks:
> @@ -321,7 +322,15 @@ static void handle_internal_command(int argc, const char **argv, char **envp)
> die("%s must be run in a work tree", cmd);
> trace_argv_printf(argv, argc, "trace: built-in: git");
>
> - exit(p->fn(argc, argv, prefix));
> + status = p->fn(argc, argv, prefix);
> +
> + /* Close stdout if necessary, and diagnose any failure. */
> + if (fcntl(fileno (stdout), F_GETFD) >= 0)
> + && (ferror(stdout) || fclose(stdout)))
> + die("write failure on standard output: %s",
> + strerror(errno));
> +
> + exit(status);
The if statement with 'fcntl' is missing a brace, it should be:
+ if ((fcntl(fileno (stdout), F_GETFD) >= 0)
+ && (ferror(stdout) || fclose(stdout)))
--
Marco Roeland
^ permalink raw reply
* Re: [PATCH 00/15] git-note: A mechanisim for providing free-form after-the-fact annotations on commits
From: Linus Torvalds @ 2007-05-28 16:28 UTC (permalink / raw)
To: Johan Herland; +Cc: git, Junio C Hamano
In-Reply-To: <200705281254.23297.johan@herland.net>
On Mon, 28 May 2007, Johan Herland wrote:
>
> > - This will probably scale horribly badly if you have tens of thousands
> > of notes, even when they are packed. Do we care?
>
> I can't see why the current implementation would scale any worse than an
> equivalent number of (annotated/signed) tags. But then again, the tag
> system might not have been designed with tens of thousands of tag objects
> in mind. :)
Right. I was more thinking that this "notes" thing could potentially be a
very useful thing for some random workflow - using notes to indicate that
some commit has been vetted by somebody, for example (ie adding things
like "Acked-by:" after-the-fact, which happens for the kernel).
And once you start using notes for something like that, I think you're
going to end up with a set of notes that grows with history, and
potentially grows quite quickly.
So I can see people having thousands of tags, but usually you only tag
releases. In contrast, I can see notes being used not as a "per release"
thing, but closer to a "per commit" thing. And that kind of worries me, I
can see workflows where you end up having tons and tons of notes.
But hey, maybe I just worry unnecessarily.
Linus
^ permalink raw reply
* Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.
From: Linus Torvalds @ 2007-05-28 16:32 UTC (permalink / raw)
To: Jim Meyering; +Cc: git
In-Reply-To: <87sl9hw0o0.fsf@rho.meyering.net>
On Mon, 28 May 2007, Jim Meyering wrote:
>
> I suspect that git's special treatment of EPIPE was a shoot-the-messenger
> reaction to the work-around (trap '' PIPE) required to avoid diagnostics
> from porcelain being interpreted by what would now be a 2-year-old
> version of bash.
No. You don't seem to realize. That was the *default* behaviour of bash.
For all I know, it might _still_ be the default behaviour.
The only reason not everybody ever even noticed, was that most
distributions were clueful enough to have figured out that it was broken,
and configured bash separately. But "most" does not mean "all", and I had
this problem on powerpc, and others had it on Debian, I htink (might have
been slackware). I think RH and SuSE had both fixed it explicitly.
> diff --git a/write_or_die.c b/write_or_die.c
> index 5c4bc85..fadfcaa 100644
> --- a/write_or_die.c
> +++ b/write_or_die.c
> @@ -41,8 +41,6 @@ int write_in_full(int fd, const void *buf, size_t count)
> void write_or_die(int fd, const void *buf, size_t count)
> {
> if (write_in_full(fd, buf, count) < 0) {
> - if (errno == EPIPE)
> - exit(0);
> die("write error (%s)", strerror(errno));
Nack. Nack. NACK.
I think this patch is fundamentally WRONG. This fragment is just a prime
example of why the whole patch is crap. The old code was correct, and you
broke it.
Linus
^ permalink raw reply
* Re: [PATCH 00/15] git-note: A mechanisim for providing free-form after-the-fact annotations on commits
From: Johan Herland @ 2007-05-28 16:40 UTC (permalink / raw)
To: Linus Torvalds; +Cc: git, Junio C Hamano
In-Reply-To: <alpine.LFD.0.98.0705280924410.26602@woody.linux-foundation.org>
On Monday 28 May 2007, Linus Torvalds wrote:
> > I can't see why the current implementation would scale any worse than an
> > equivalent number of (annotated/signed) tags. But then again, the tag
> > system might not have been designed with tens of thousands of tag objects
> > in mind. :)
>
> Right. I was more thinking that this "notes" thing could potentially be a
> very useful thing for some random workflow - using notes to indicate that
> some commit has been vetted by somebody, for example (ie adding things
> like "Acked-by:" after-the-fact, which happens for the kernel).
>
> And once you start using notes for something like that, I think you're
> going to end up with a set of notes that grows with history, and
> potentially grows quite quickly.
>
> So I can see people having thousands of tags, but usually you only tag
> releases. In contrast, I can see notes being used not as a "per release"
> thing, but closer to a "per commit" thing. And that kind of worries me, I
> can see workflows where you end up having tons and tons of notes.
>
> But hey, maybe I just worry unnecessarily.
I still don't see what makes note objects inherently more expensive than
commit objects. Except for the refs, of course, but we're getting rid
of those (at least replacing them with a more efficient reverse mapping).
And even if we _do_ end up with 10 notes per commit, we could always
design some kind of "supernote" that lets "git-gc" pack all the notes
related to a commit into _one_ object.
Have fun!
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply
* Re: [PATCH 1/3] Lazily open pack index files on demand
From: Karl Hasselström @ 2007-05-28 17:40 UTC (permalink / raw)
To: Linus Torvalds
Cc: Nicolas Pitre, Shawn O. Pearce, Dana How, Junio C Hamano, git
In-Reply-To: <alpine.LFD.0.98.0705280907290.26602@woody.linux-foundation.org>
On 2007-05-28 09:22:02 -0700, Linus Torvalds wrote:
> Secondly, the cost of Newton isn't "almost O(1)". I don't know
> _what_ it is (the rule of thumb with Newton-Raphson should be that
> the number of significant correct digits in the answer doubles with
> each iteration: I think that probably means that it should
> approximate O(loglog(n)), but I haven't thought deeply about it.
With binary search, the number of correct bits in the index increases
by 1 for each iteration. With Newton iteration, the number of correct
bits doubles for each iteration. So the number of Newton iteration
should be the log of the number of binary search iterations, i.e. log
log n like you say.
But for non-astronomical values of n, I agree that this kind of big-O
analysis is much inferior to benchmarking.
--
Karl Hasselström, kha@treskal.com
www.treskal.com/kalle
^ permalink raw reply
* Re: [PATCH 00/15] git-note: A mechanisim for providing free-form after-the-fact annotations on commits
From: Linus Torvalds @ 2007-05-28 16:58 UTC (permalink / raw)
To: Johan Herland; +Cc: git, Junio C Hamano
In-Reply-To: <200705281840.50814.johan@herland.net>
On Mon, 28 May 2007, Johan Herland wrote:
>
> I still don't see what makes note objects inherently more expensive than
> commit objects. Except for the refs, of course, but we're getting rid
> of those (at least replacing them with a more efficient reverse mapping).
It's exactly the refs that I worry about.
Anything that needs to read in all notes at startup is going to be _slow_.
In contrast, commits we read when (and only when) we need them.
Linus
^ permalink raw reply
* Re: [PATCH 1/3] Lazily open pack index files on demand
From: Nicolas Pitre @ 2007-05-28 17:13 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Shawn O. Pearce, Dana How, Junio C Hamano, git
In-Reply-To: <alpine.LFD.0.98.0705280907290.26602@woody.linux-foundation.org>
On Mon, 28 May 2007, Linus Torvalds wrote:
> On Sun, 27 May 2007, Nicolas Pitre wrote:
> >
> > It helps irrespective of the number of pack files. With the current
> > binary search the lookup cost is O(log n). With a Newton method this
> > cost is almost O(1).
>
> This is not true.
>
> First off, when comparing O(logn) to O(1), with "n" being less than a
> billion, they are pretty much exactly the same. Think of it this way:
> O(logn) == O(9) == O(1), if you know that n < 10**9.
Agreed.
> Secondly, the cost of Newton isn't "almost O(1)". I don't know _what_ it
> is (the rule of thumb with Newton-Raphson should be that the number of
> significant correct digits in the answer doubles with each iteration: I
> think that probably means that it should approximate O(loglog(n)), but I
> haven't thought deeply about it.
Sure. But given the reasoning you gave above for O(log N) being about
the same as O(1) for a small enough N, I think that O(log log N) is even
closer to O(1) in that regard.
> And the thing is, Newton-Raphson didn't actually speed anything up in my
> tests. Sometimes it was better, sometimes it was worse, most of the time
> it was in the noise.
OK I didn't remember the difference was so unconclusive.
The constant cost is certainly a big factor in the equation.
Maybe we didn't test with a big enough repo yet (big in the sense of
number of objects) to see the variable cost actually dominate.
Did you try with the KDE repo at the time? It has over 4M objects.
The binary search would require 14 itterations while the Newton search
should take around 3 or 4 itterations if the log O(log N) esttimate is
right. Of course the evaluation of the next mid point is more costly
with the Newton method.
Nicolas
^ permalink raw reply
* Re: GIT on MinGW problem
From: Nix @ 2007-05-28 16:54 UTC (permalink / raw)
To: hanwen; +Cc: git, Johannes Sixt
In-Reply-To: <4659BA07.4080307@xs4all.nl>
On 27 May 2007, Han-Wen Nienhuys said:
> Johannes Schindelin escreveu:
>
>>> ((Tcl_Obj **) objv) += (async + 3);
>>
>> Ah yes, I was using MinGW's own GCC, which is GCC 3.something.
>>
>> It is a new "feature" of GCC 4.x to disallow constructs like these.
>> (Probably because GCC people think that other people are not intelligent
>> enough to understand such constructs, and therefore prohibit their use.)
>
> I very much doubt that. GCC uses type information to determine whether
> pointers might be aliased. I think disallowing such constructs helps with
> compiler optimization.
Actually it was simply too difficult to maintain this extension over the
C and C++ parser rewrites in the early 4.x timeline. (Also, IIRC, it had
some *nasty* ambiguities, especially in C++.)
--
`On a scale of one to ten of usefulness, BBC BASIC was several points ahead
of the competition, scoring a relatively respectable zero.' --- Peter Corlett
^ permalink raw reply
* Re: [PATCH 00/15] git-note: A mechanisim for providing free-form after-the-fact annotations on commits
From: Michael S. Tsirkin @ 2007-05-28 17:29 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Johan Herland, git, Junio C Hamano
In-Reply-To: <alpine.LFD.0.98.0705272131370.26602@woody.linux-foundation.org>
> Quoting Linus Torvalds <torvalds@linux-foundation.org>:
> Subject: Re: [PATCH 00/15] git-note: A mechanisim for providing free-form after-the-fact annotations on commits
>
>
>
> On Sun, 27 May 2007, Johan Herland wrote:
> >
> > I've been working on combining tag objects and --decorate into a useful
> > proof-of-concept that provides the after-the-fact commit annotations I
> > requested above, and here's the result:
>
> Ok, looks fine to me. I do have a few questions:
> - why don't you just let people name their notes, the same way we name
> tags (and then actually using it as the note name?)
>
> Putting them in the refs/notes/ filesystem by their SHA1 seems a bit
> wasteful, and it would seem that it could be quite nice to name the
> notes some way?
>
> - This will probably scale horribly badly if you have tens of thousands
> of notes, even when they are packed. Do we care?
Maybe note names could include the object they point to?
Would this help with the scalability too (i.e. only get
names for objects we want)?
--
MST
^ permalink raw reply
* Re: [PATCH 00/15] git-note: A mechanisim for providing free-form after-the-fact annotations on commits
From: Michael S. Tsirkin @ 2007-05-28 17:42 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Linus Torvalds, Johan Herland, git, Junio C Hamano
In-Reply-To: <20070528172949.GA7295@mellanox.co.il>
> Quoting Michael S. Tsirkin <mst@dev.mellanox.co.il>:
> Subject: Re: [PATCH 00/15] git-note: A mechanisim for providing free-form after-the-fact annotations on commits
>
> > Quoting Linus Torvalds <torvalds@linux-foundation.org>:
> > Subject: Re: [PATCH 00/15] git-note: A mechanisim for providing free-form after-the-fact annotations on commits
> >
> >
> >
> > On Sun, 27 May 2007, Johan Herland wrote:
> > >
> > > I've been working on combining tag objects and --decorate into a useful
> > > proof-of-concept that provides the after-the-fact commit annotations I
> > > requested above, and here's the result:
> >
> > Ok, looks fine to me. I do have a few questions:
> > - why don't you just let people name their notes, the same way we name
> > tags (and then actually using it as the note name?)
> >
> > Putting them in the refs/notes/ filesystem by their SHA1 seems a bit
> > wasteful, and it would seem that it could be quite nice to name the
> > notes some way?
> >
> > - This will probably scale horribly badly if you have tens of thousands
> > of notes, even when they are packed. Do we care?
>
> Maybe note names could include the object they point to?
> Would this help with the scalability too (i.e. only get
> names for objects we want)?
Hmm, I just noticed there's refs/notes/{object-sha}/{note-sha}
that does exactly that. So - this seems to be clear to everyone
else, but I seem to miss the reason why does this not scale well?
We can just get the notes for objects we pull, right?
--
MST
^ permalink raw reply
* Re: [PATCH 00/15] git-note: A mechanisim for providing free-form after-the-fact annotations on commits
From: Johan Herland @ 2007-05-28 17:48 UTC (permalink / raw)
To: Linus Torvalds; +Cc: git, Junio C Hamano
In-Reply-To: <alpine.LFD.0.98.0705280957070.26602@woody.linux-foundation.org>
On Monday 28 May 2007, Linus Torvalds wrote:
>
> On Mon, 28 May 2007, Johan Herland wrote:
> >
> > I still don't see what makes note objects inherently more expensive than
> > commit objects. Except for the refs, of course, but we're getting rid
> > of those (at least replacing them with a more efficient reverse mapping).
>
> It's exactly the refs that I worry about.
>
> Anything that needs to read in all notes at startup is going to be _slow_.
>
> In contrast, commits we read when (and only when) we need them.
Ok. But the reverse mapping will help with this, won't it?
We'll look up the interesting commits and find their associated
note objects directly.
BTW, if you use "git-note -l <commit>" it'll go directly to <commit>'s
subdir in refs/notes and never look at any other notes.
Of course, this currently does not help the plumbing much, which uses
for_each_note_ref() instead...
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply
* Re: [PATCH 00/15] git-note: A mechanisim for providing free-form after-the-fact annotations on commits
From: Johan Herland @ 2007-05-28 17:58 UTC (permalink / raw)
To: git, Michael S. Tsirkin; +Cc: Linus Torvalds, Junio C Hamano
In-Reply-To: <20070528174205.GB7295@mellanox.co.il>
On Monday 28 May 2007, Michael S. Tsirkin wrote:
> Quoting Michael S. Tsirkin <mst@dev.mellanox.co.il>:
> > Maybe note names could include the object they point to?
> > Would this help with the scalability too (i.e. only get
> > names for objects we want)?
>
> Hmm, I just noticed there's refs/notes/{object-sha}/{note-sha}
> that does exactly that. So - this seems to be clear to everyone
> else, but I seem to miss the reason why does this not scale well?
> We can just get the notes for objects we pull, right?
Currently, only "git-note -l" (and by extension gitk) uses the shortcut
through refs/notes/{object-sha}/...
The --decorate code and everybody else goes through for_each_note_ref()
which looks up every note ref in the repo.
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply
* Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.
From: Jim Meyering @ 2007-05-28 18:19 UTC (permalink / raw)
To: Marco Roeland; +Cc: git
In-Reply-To: <20070528154630.GA9176@fiberbit.xs4all.nl>
Marco Roeland <marco.roeland@xs4all.nl> wrote:
> The if statement with 'fcntl' is missing a brace, it should be:
>
> + if ((fcntl(fileno (stdout), F_GETFD) >= 0)
> + && (ferror(stdout) || fclose(stdout)))
Thank you.
That was a stale patch (from before I compiled/tested, obviously).
I intended this:
if (fcntl(fileno (stdout), F_GETFD) >= 0
&& (ferror(stdout) || fclose(stdout)))
Here's the correct one:
Subject: [PATCH] Don't ignore write failure from git-diff, git-log, etc.
Currently, when git-diff writes to a full device or gets an I/O error,
it fails to detect the write error:
$ git-diff |wc -c
3984
$ git-diff > /dev/full && echo ignored write failure
ignored write failure
git-log does the same thing:
$ git-log -n1 > /dev/full && echo ignored write failure
ignored write failure
Each and every git command should report such a failure.
Some already do, but with the patch below, they all do, and we
won't have to rely on code in each command's implementation to
perform the right incantation.
$ ./git-log -n1 > /dev/full
fatal: write failure on standard output: No space left on device
[Exit 128]
$ ./git-diff > /dev/full
fatal: write failure on standard output: No space left on device
[Exit 128]
You can demonstrate this with git's own --version output, too:
(but git --help detects the failure without this patch)
$ ./git --version > /dev/full
fatal: write failure on standard output: No space left on device
[Exit 128]
Note that the fcntl test (for whether the fileno may be closed) is
required in order to avoid EBADF upon closing an already-closed stdout,
as would happen for each git command that already closes stdout; I think
update-index was the one I noticed in the failure of t5400, before I
added that test.
Also, to be consistent, don't ignore EPIPE write failures.
Signed-off-by: Jim Meyering <jim@meyering.net>
---
git.c | 11 ++++++++++-
merge-recursive.c | 3 ---
write_or_die.c | 4 ----
3 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/git.c b/git.c
index 29b55a1..7b45a73 100644
--- a/git.c
+++ b/git.c
@@ -308,6 +308,7 @@ static void handle_internal_command(int argc, const char **argv, char **envp)
for (i = 0; i < ARRAY_SIZE(commands); i++) {
struct cmd_struct *p = commands+i;
const char *prefix;
+ int status;
if (strcmp(p->cmd, cmd))
continue;
@@ -321,7 +322,15 @@ static void handle_internal_command(int argc, const char **argv, char **envp)
die("%s must be run in a work tree", cmd);
trace_argv_printf(argv, argc, "trace: built-in: git");
- exit(p->fn(argc, argv, prefix));
+ status = p->fn(argc, argv, prefix);
+
+ /* Close stdout if necessary, and diagnose any failure. */
+ if (fcntl(fileno (stdout), F_GETFD) >= 0
+ && (ferror(stdout) || fclose(stdout)))
+ die("write failure on standard output: %s",
+ strerror(errno));
+
+ exit(status);
}
}
diff --git a/merge-recursive.c b/merge-recursive.c
index 8f72b2c..bfa4335 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -523,9 +523,6 @@ static void flush_buffer(int fd, const char *buf, unsigned long size)
while (size > 0) {
long ret = write_in_full(fd, buf, size);
if (ret < 0) {
- /* Ignore epipe */
- if (errno == EPIPE)
- break;
die("merge-recursive: %s", strerror(errno));
} else if (!ret) {
die("merge-recursive: disk full?");
diff --git a/write_or_die.c b/write_or_die.c
index 5c4bc85..fadfcaa 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -41,8 +41,6 @@ int write_in_full(int fd, const void *buf, size_t count)
void write_or_die(int fd, const void *buf, size_t count)
{
if (write_in_full(fd, buf, count) < 0) {
- if (errno == EPIPE)
- exit(0);
die("write error (%s)", strerror(errno));
}
}
@@ -50,8 +48,6 @@ void write_or_die(int fd, const void *buf, size_t count)
int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg)
{
if (write_in_full(fd, buf, count) < 0) {
- if (errno == EPIPE)
- exit(0);
fprintf(stderr, "%s: write error (%s)\n",
msg, strerror(errno));
return 0;
^ permalink raw reply related
* Re: [PATCH 1/3] Lazily open pack index files on demand
From: Nicolas Pitre @ 2007-05-28 18:31 UTC (permalink / raw)
To: Dana How; +Cc: Shawn O. Pearce, Junio C Hamano, git
In-Reply-To: <56b7f5510705271835m5a375324p3a908fe766fdf902@mail.gmail.com>
On Sun, 27 May 2007, Dana How wrote:
> Perhaps I have stumbled on another issue related to
> including SHA-1s in packfiles. This is completely independent
> of the handling of megablobs (my current focus), but the presence
> of large blobs do make this issue more apparent.
>
> Some history of what I've been doing with git:
> First I simply had to import the repo,
> which led to split packs (this was before index v2).
> Then maintaining the repo led to the unfinished maxblobsize stuff.
> Distributing the repo included users pulling (usually) from the central repo,
> which would be trivial since it was also an alternate.
> Local repacking would avoid heavy load on it.
>
> Now I've started looking into how to push back into the
> central repo from a user's repo (not everything will be central;
> some pulling between users will occur
> otherwise I wouldn't be as interested).
>
> It looks like the entire sequence is:
> A. git add file [compute SHA-1 & compress file into objects/xx]
> B. git commit [write some small objects locally]
> C. git push {using PROTO_LOCAL}:
> 1. read & uncompress objects
> 2. recompress objects into a pack and send through a pipe
> 3. read pack on other end of pipe and uncompress each object
> 4. compute SHA-1 for each object and compress file into objects/xx
>
> So, after creating an object in the local working tree,
> to get it into the central repo, we must:
> compress -> uncompress -> compress -> uncompress -> compress.
> In responsiveness this won't compare very well to Perforce,
> which has only one compress step.
>
> The sequence above could be somewhat different currently in git.
> The user might have repacked their repo before pushing,
> but this just moves C1 and C2 back earlier in time,
> it doesn't remove the need for them. Besides, the blobs in
> a push are more likely to be recent and hence unpacked.
>
> Also, C3 and C4 might not happen if more than 100 blobs get pushed.
> But this seems very unusual; only 0.3% of commits in the history
> had 100+ new files/file contents. If the 100 level is reduced,
> then the central repo fills up with packfiles and their index files,
> reducing performance for everybody (using the central repo as an alternate).
But it makes repacking on the central repo much easier, easily regaining
performance for everybody, even more so than if objects were left loose.
You could even use one of GIT's many hooks to asynchronously launch a
repack whenever a push has been completed to keep the number of server
side packs low.
Also, the treshold of 100 is for all objects, not only files. So if you
do multiple commits before pushing (a workflow that GIT strongly
encourage) then you need only 12 commits modifying only 3 file each
with those files located in a subdirectory of their own to create 96
objects already. It grows even faster if the modified files are more
deeply nested.
The 100 object treshold was arbitrarily determined and no performance
tests were ever done to confirm if this is the best value.
> Thus there really is 5X more compression activity going on
> compared to Perforce. How can this be reduced?
>
> One way is to restore the ability to write the "new" loose object format.
> Then C1, C2, and C4 disappear. C3 must remain because we need
> to uncompress the object to compute its SHA-1; we don't need
> to recompress since we were already given the compressed form.
C4 isn't mandatory if you keep object packed on the remote end like I
suggest above.
Note that C1 is unavoidable when locally repacking. It is the only way
to make sure the new pack won't blindly reuse data that might have been
corrupted on disk (think random bit flip), and wrap it with a valid SHA1
that includes the corrupted data which is even worse. And yes it
happened at least once with a report on this list. This is why we are a
bit paranoid about data integrity. And even if the server currently
recompute object SHA1 upon reception of a pack, I think C1 during a push
is a good thing too just so you don't bother the server if your own repo
is locally corrupted.
And yet you need C1 anyway for deltification in most normal cases.
As for C2, you should see it as helping the server repack objects. If
C2 is distributed amongst all client (including C1 and deltification) it
is then much less visible and the server doesn't need to do that part
when repacking.
> And that final sentence is why I sent this email: if the packfile
> contained the SHA-1s, either at the beginning or before each object,
> then they wouldn't need to be recomputed at the receiving end
> and the extra decompression could be skipped as well. This would
> make the total zlib effort the same as Perforce.
This cannot be done. GIT, being distributed, means that you should not
trust the remote end. The notion of validation and trust comes from the
fact that GIT actively validate the SHA1 of what it is fed so a rogue
server (or even client) cannot just substitute an object for another
(with or without malice) and pretend everything is fine.
> The fact that a loose object is never overwritten would still be retained.
> Is that sufficient security? Or does the SHA-1 always need to be
> recomputed on the receiving end? Could that be skipped just for
> specific connections and/or protocols (presumably "trusted" ones)?
Even trusted connections can sometimes transmit bad bits. Corruption
_will_ happen someday to someone. It already happened to people on this
list. Hardware is not immune to faults, and the bigger is your repo,
the more likely you're susceptible to such things. If SHA1 is validated
on each hop then any corruption won't be able to propagate.
> Shawn: I think you mentioned something related to this a few days ago.
> Also, you didn't like split packs putting SHA-1s at the end because
> that messed up streaming for transport, but packs are not split for transport.
Like I stated elsewhere the SHA1 table has no advantage being at the end
anyway since it cannot be trimmed down even for split packs.
Nicolas
^ permalink raw reply
* Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.
From: Marco Roeland @ 2007-05-28 19:05 UTC (permalink / raw)
To: Jim Meyering; +Cc: git
In-Reply-To: <87646cx13d.fsf@rho.meyering.net>
On monday May 28th 2007 at 20:19 Jim Meyering wrote:
> ...
>
> Also, to be consistent, don't ignore EPIPE write failures.
In practice I agree with someone else on this thread that EPIPE _is_
different. In a way the responsibility doesn't lie with the writer but
with the reader.
But just out of curiosity is there an easy way to test the EPIPE
behaviour? I cite a piece of the "changelog.Debian" file from the
Debian version of the bash shell. In Debian, as earlier in many other
distributions, the annoying EPIPE error was "fixed" in version 2.0.3-3
from 19 dec 1999.
========================================================================
* Define DONT_REPORT_SIGPIPE: We don't want to see `Broken pipe' messages
when a job like `cat jobs.c | exit 1' is executed. Fixes part of #7047,
#10259, #10433 and #10494. Comment from the upstream author: "The default
bash behavior with respect to the exit status of a pipeline will not
change. Changing it as suggested in the discussion of #10494 would render
bash incompatible with every other shell out there.". Closed these reports.
-- Matthias Klose <doko@debian.org> Sun, 19 Dec 1999 15:58:43 +0100
========================================================================
The mentioned "test-case" as used in "git log -n1 | exit 1" doesn't
produce an error in my Debian 'sid' bash, either with or without your
patch, so it doesn't seem to have any effect there? Whereas probably in
a "default" bash (don't know if upstream has changed it's mind already!)
with your patch (i.e. the EPIPE special casing removal) it will again
probably introduce these annoying (for interactive use) errors.
Thanks for your patch anyway, the "fcntl" diagnosis is a really useful
technique to know, and IMVHO also useful for git; although perhaps not
very portable for all platforms.
--
Marco Roeland
^ permalink raw reply
* Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.
From: Jim Meyering @ 2007-05-28 20:04 UTC (permalink / raw)
To: Linus Torvalds; +Cc: git
In-Reply-To: <alpine.LFD.0.98.0705280929140.26602@woody.linux-foundation.org>
Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Mon, 28 May 2007, Jim Meyering wrote:
>>
>> I suspect that git's special treatment of EPIPE was a shoot-the-messenger
>> reaction to the work-around (trap '' PIPE) required to avoid diagnostics
>> from porcelain being interpreted by what would now be a 2-year-old
>> version of bash.
Hi Linus!
Thanks for all the encouragement.
> No. You don't seem to realize. That was the *default* behaviour of bash.
Of course it was the default. Because it was the default, it
provoked contortions like using `trap '' PIPE' in shell scripts,
which in turn provoked EPIPE diagnostics from git, which
prompted the EPIPE-ignoring changes in git plumbing.
And those changes can now OBSCURE REAL ERRORS, as I've shown.
Note: it was the SIGPIPE-ignoring work-around that caused the trouble.
The bash bug didn't cause trouble with git _directly_.
If anyone can find a mainstream distro (I didn't) on which my
patch causes trouble, please let us all know.
Bash changed its default first in bash-3.1-alpha1.
The next stable release was bash-3.1, in Dec 2005:
r. By default, the shell no longer reports processes dying from SIGPIPE.
It looks like most major distros had fixed it long before.
The latest stable release is bash-3.2 from October, 2006.
> For all I know, it might _still_ be the default behaviour.
It's not. See above.
Easy to test: run this: seq 99999|head -1
if all you see is a single line with "1" on it, and an exit status of 0,
then there's no problem.
However, the version of bash you use is IRRELEVANT to the question of
EPIPE. SIGPIPE has always been delivered by default. The only difference
lay in whether bash _reported_ the delivery. It's only the work-around
ignoring of SIGPIPE that used to provoke EPIPE "broken pipe" errors.
Now, all of the git porcelain shell code that did that appears to be gone,
probably converted to perl or C.
You can get an EPIPE diagnostic with my patch any time the affected
plumbing is invoked from an environment in which SIGPIPE is ignored.
That environment could be your shell (if you put "trap '' PIPE" in a
start-up file -- though no one does *that*), or porcelain that does that,
or the perlish $SIG{PIPE} = 'IGNORE'. The following are the only parts
of git I've found that ignore SIGPIPE:
git-archimport.perl
git-cvsimport.perl
git-svn.perl
git-svnimport.perl
And nothing in cogito does.
So, now, I *really* don't see why there's any fuss about EPIPE.
> The only reason not everybody ever even noticed, was that most
> distributions were clueful enough to have figured out that it was broken,
> and configured bash separately. But "most" does not mean "all", and I had
> this problem on powerpc, and others had it on Debian, I htink (might have
> been slackware). I think RH and SuSE had both fixed it explicitly.
Precisely. That behavior in bash was so annoying that people were
motivated to fix it quickly. But all of that was resolved long ago.
...
> Nack. Nack. NACK.
>
> I think this patch is fundamentally WRONG. This fragment is just a prime
> example of why the whole patch is crap. The old code was correct, and you
> broke it.
Um... maybe you've forgotten that this patch fixes a hole in the
"old code" (git.c). Many git tools ignore write (ENOSPC) failures.
Compared to that aspect of the fix, I would have thought EPIPE-
handling would be a minor detail. But now, the whole patch has
become "crap"?
Consider the EPIPE-related risks/choice:
1) Continue to ignore EPIPE write failure: can obscure real errors.
BTW, Linus, don't you agree? You never commented on this point.
2) Remove the EPIPE exclusion: *might* make git give a "broken pipe"
diagnostic, if you run git in a SIGPIPE-ignoring environment.
#2 seems to be the lesser of two evils, considering that we can fix or
work around the occasional "broken pipe" error, but we can't work around
an unconditionally-ignored EPIPE.
Jim
^ permalink raw reply
* Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.
From: Jim Meyering @ 2007-05-28 20:23 UTC (permalink / raw)
To: Marco Roeland; +Cc: git
In-Reply-To: <20070528190529.GA10656@fiberbit.xs4all.nl>
Marco Roeland <marco.roeland@xs4all.nl> wrote:
> On monday May 28th 2007 at 20:19 Jim Meyering wrote:
>> Also, to be consistent, don't ignore EPIPE write failures.
>
> In practice I agree with someone else on this thread that EPIPE _is_
> different. In a way the responsibility doesn't lie with the writer but
> with the reader.
Do you think it's ok for git-rev-list _not_ to diagnose an erroneous
command like this (i.e., to exit(0)):
git-rev-list HEAD | sync
where "sync" could be any command that exits successfully
without reading any input?
Is it ok that it is currently *impossible* to diagnose that
failure by looking at exit codes?
> But just out of curiosity is there an easy way to test the EPIPE
> behaviour? I cite a piece of the "changelog.Debian" file from the
There are some examples here:
http://thread.gmane.org/gmane.comp.version-control.git/48469/focus=48617
...
> The mentioned "test-case" as used in "git log -n1 | exit 1" doesn't
> produce an error in my Debian 'sid' bash, either with or without your
> patch, so it doesn't seem to have any effect there? Whereas probably in
> a "default" bash (don't know if upstream has changed it's mind already!)
> with your patch (i.e. the EPIPE special casing removal) it will again
> probably introduce these annoying (for interactive use) errors.
As I just said in reply to Linus, the EPIPE handling difference
is independent of what version of bash you use.
> Thanks for your patch anyway, the "fcntl" diagnosis is a really useful
> technique to know, and IMVHO also useful for git; although perhaps not
> very portable for all platforms.
It appears to be portable enough. fcntl/F_GETFD support is required
by POSIX, and has been around for ages. FWIW, it's also used in git's
daemon.c and sha1_file.c.
^ permalink raw reply
* Working with git-svn to export changes to Subversion
From: Art Haas @ 2007-05-28 20:27 UTC (permalink / raw)
To: git
Hi.
A few days ago I posted a message to this list regarding my attempts to
convert the PythonCAD Subversion repository into git. My first efforts
failed, but after several more attempts and some editing of the
git-svnimport code I've been able to convert the entire repo, tags and
all, seemingly successfully.
Having now converted my repo, I'm now tackling the second-half of my
task, which is to figure out how to use 'git-svn' to export my changes
to a Subversion repository. The documentation for 'git-svn' is set up
to guide someone using git to track a project kept in Subversion, where
in my case the code will be kept in git and exported out into
Subversion. I plan on pushing my changes solely to 'trunk', and when
I make a release I want to tag it and push the tag into the Subversion
repository as well.
I'm writing today to see if others using 'git-svn' are currently using
it in the form I'm trying to do, and if so could these people give me a
couple of pointers regarding any tips, tricks, and/or pitfalls that I
may encounter. Thanks in advance for any info you can send my direction.
My thanks also go to everyone that has contributed to the 'git-svnimport'
code, and git itself as well.
Art Haas
--
Man once surrendering his reason, has no remaining guard against absurdities
the most monstrous, and like a ship without rudder, is the sport of every wind.
-Thomas Jefferson to James Smith, 1822
^ permalink raw reply
* Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.
From: Junio C Hamano @ 2007-05-28 20:44 UTC (permalink / raw)
To: Jim Meyering; +Cc: Marco Roeland, git
In-Reply-To: <87646cx13d.fsf@rho.meyering.net>
Jim Meyering <jim@meyering.net> writes:
> Subject: [PATCH] Don't ignore write failure from git-diff, git-log, etc.
> ...
> You can demonstrate this with git's own --version output, too:
> (but git --help detects the failure without this patch)
>
> $ ./git --version > /dev/full
> fatal: write failure on standard output: No space left on device
> [Exit 128]
>
> Note that the fcntl test (for whether the fileno may be closed) is
> required in order to avoid EBADF upon closing an already-closed stdout,
> as would happen for each git command that already closes stdout; I think
> update-index was the one I noticed in the failure of t5400, before I
> added that test.
>
> Also, to be consistent, don't ignore EPIPE write failures.
>
> Signed-off-by: Jim Meyering <jim@meyering.net>
I do not think anybody has much objection about the change to
handle_internal_command() in git.c you made. Earlier we relied
on exit(3) to close still open filehandles (while ignoring
errors), and you made the close explicit in order to detect
errors.
But "to be consistent" above is not a very good justification.
In the presense of shells that give "annoying" behaviour (we
have to remember that not everybody have enough privilege,
expertise, or motivation to update /bin/sh or install their own
copy in $HOME/bin/sh), "EPIPE is different from other errors" is
a more practical point of view, I'd have to say. IOW, it is not
clear if it is a good thing "to be consistent" to begin with.
I would have appreciated if this were two separate patches. I
think the EPIPE one is an independent issue. We could even make
it a configuration option to ignore EPIPE ;-)
^ 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