* Re: [LHF] making t5000 "tar xf" tests more lenient
From: Junio C Hamano @ 2013-01-05 20:11 UTC (permalink / raw)
To: René Scharfe; +Cc: git
In-Reply-To: <50E8722B.8010408@lsrfire.ath.cx>
René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> Anyway, I don't think the pax headers are to blame here.
Hmph, I am reasonably sure I saw a test that created an archive from
a commit (hence with pax header), asked platform tar to either list
the contents or actually extracted to the filesystem, and tried to
ensure nothing but the paths in the repository existed in the
archive. When the platform tar implementation treated the pax
header as an extra file, such a test sees something not in the
repository and fails.
This was on (and I am still on) NetBSD 6.0 not 6.0.1, by the way.
^ permalink raw reply
* Re: [PATCH] run-command: encode signal death as a positive integer
From: Johannes Sixt @ 2013-01-05 19:50 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Bart Trojanowski
In-Reply-To: <20130105144949.GA24479@sigill.intra.peff.net>
Am 05.01.2013 15:49, schrieb Jeff King:
> On Sat, Jan 05, 2013 at 09:03:16AM -0500, Jeff King wrote:
>
>> In fact, I really wonder if this code from wait_or_whine is actually
>> correct:
>>
>> code = WTERMSIG(status);
>> /*
>> * This return value is chosen so that code & 0xff
>> * mimics the exit code that a POSIX shell would report for
>> * a program that died from this signal.
>> */
>> code -= 128;
>
> After looking at it some more, it is correct, but I think we could make
> life slightly easier for callers. See the patch below. I've tried to
> re-state the somewhat rambling argument from my previous email;
> hopefully it makes sense.
>
> -- >8 --
> Subject: [PATCH] run-command: encode signal death as a positive integer
>
> When a sub-command dies due to a signal, we encode the
> signal number into the numeric exit status as "signal -
> 128". This is easy to identify (versus a regular positive
> error code), and when cast to an unsigned integer (e.g., by
> feeding it to exit), matches what a POSIX shell would return
> when reporting a signal death in $? or through its own exit
> code.
>
> So we have a negative value inside the code, but once it
> passes across an exit() barrier, it looks positive (and any
> code we receive from a sub-shell will have the positive
> form). E.g., death by SIGPIPE (signal 13) will look like
> -115 to us in inside git, but will end up as 141 when we
> call exit() with it. And a program killed by SIGPIPE but run
> via the shell will come to us with an exit code of 141.
>
> Unfortunately, this means that when the "use_shell" option
> is set, we need to be on the lookout for _both_ forms. We
> might or might not have actually invoked the shell (because
> we optimize out some useless shell calls). If we didn't invoke
> the shell, we will will see the sub-process's signal death
> directly, and run-command converts it into a negative value.
> But if we did invoke the shell, we will see the shell's
> 128+signal exit status. To be thorough, we would need to
> check both, or cast the value to an unsigned char (after
> checking that it is not -1, which is a magic error value).
>
> Fortunately, most callsites do not care at all whether the
> exit was from a code or from a signal; they merely check for
> a non-zero status, and sometimes propagate the error via
> exit(). But for the callers that do care, we can make life
> slightly easier by just using the consistent positive form.
>
> This actually fixes two minor bugs:
>
> 1. In launch_editor, we check whether the editor died from
> SIGINT or SIGQUIT. But we checked only the negative
> form, meaning that we would fail to notice a signal
> death exit code which was propagated through the shell.
>
> 2. In handle_alias, we assume that a negative return value
> from run_command means that errno tells us something
> interesting (like a fork failure, or ENOENT).
> Otherwise, we simply propagate the exit code. Negative
> signal death codes confuse us, and we print a useless
> "unable to run alias 'foo': Success" message. By
> encoding signal deaths using the positive form, the
> existing code just propagates it as it would a normal
> non-zero exit code.
>
> The downside is that callers of run_command can no longer
> differentiate between a signal received directly by the
> sub-process, and one propagated. However, no caller
> currently cares, and since we already optimize out some
> calls to the shell under the hood, that distinction is not
> something that should be relied upon by callers.
The idea was initially to regard death by signal as an internal error.
But since (1) there are no callers that are really interested in the
difference and (2) we get it wrong in the shell case anyway, this change
makes total sense.
Acked-by: Johannes Sixt <j6t@kdbg.org>
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Documentation/technical/api-run-command.txt | 6 ++----
> editor.c | 2 +-
> run-command.c | 2 +-
> 3 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/technical/api-run-command.txt b/Documentation/technical/api-run-command.txt
> index f18b4f4..5d7d7f2 100644
> --- a/Documentation/technical/api-run-command.txt
> +++ b/Documentation/technical/api-run-command.txt
> @@ -55,10 +55,8 @@ The functions above do the following:
> non-zero.
>
> . If the program terminated due to a signal, then the return value is the
> - signal number - 128, ie. it is negative and so indicates an unusual
> - condition; a diagnostic is printed. This return value can be passed to
> - exit(2), which will report the same code to the parent process that a
> - POSIX shell's $? would report for a program that died from the signal.
> + signal number + 128, ie. the same value that a POSIX shell's $? would
> + report. A diagnostic is printed.
>
>
> `start_async`::
> diff --git a/editor.c b/editor.c
> index 065a7ab..27bdecd 100644
> --- a/editor.c
> +++ b/editor.c
> @@ -51,7 +51,7 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
> sigchain_push(SIGINT, SIG_IGN);
> sigchain_push(SIGQUIT, SIG_IGN);
> ret = finish_command(&p);
> - sig = ret + 128;
> + sig = ret - 128;
> sigchain_pop(SIGINT);
> sigchain_pop(SIGQUIT);
> if (sig == SIGINT || sig == SIGQUIT)
> diff --git a/run-command.c b/run-command.c
> index 757f263..cfb7274 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -249,7 +249,7 @@ static int wait_or_whine(pid_t pid, const char *argv0)
> * mimics the exit code that a POSIX shell would report for
> * a program that died from this signal.
> */
> - code -= 128;
> + code += 128;
> } else if (WIFEXITED(status)) {
> code = WEXITSTATUS(status);
> /*
>
^ permalink raw reply
* Re: [LHF] making t5000 "tar xf" tests more lenient
From: Junio C Hamano @ 2013-01-05 19:43 UTC (permalink / raw)
To: René Scharfe; +Cc: git
In-Reply-To: <50E8722B.8010408@lsrfire.ath.cx>
René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> Anyway, I don't think the pax headers are to blame here. The patch below
> fixes the tar failures for me, but I'm not sure why. There must be
> something special about some (not all!) directory entries with trailing
> slashes after directory names.
>
> Several ZIP tests still fail, however, because NetBSD's unzip doesn't
> seem to support (our flavour of) symlinks and streamed files.
Just FYI, I found that unzip that comes with base distro and the one
you can add via pkg_add (or pkgin) are different, and the latter
seemed to behave better.
^ permalink raw reply
* Re: [PATCH 00/10] push: switch default from "matching" to "simple"
From: Matthieu Moy @ 2013-01-05 19:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <1357368788-28035-1-git-send-email-gitster@pobox.com>
Junio C Hamano <gitster@pobox.com> writes:
> We promised to change the behaviour of lazy "git push [there]" that
> does not say what to push on the command line from "matching" to
> "simple" in Git 2.0. Even though the top-level RelNotes symbolic
> link points at 1.8.2, we can change our mind to tag 2.0 at the end
> of this cycle.
I had a quick look at the series, it sounds straightforward and correct.
Thanks,
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH] gitk: Display the date of a tag in a human friendly way.
From: Junio C Hamano @ 2013-01-05 19:38 UTC (permalink / raw)
To: Anand Kumria; +Cc: Paul Mackerras, git
In-Reply-To: <CAM1C4Gm_ea8DgrVhnp_MHmqaF6pyDe98EDA_BPkjvc8M5AO6FQ@mail.gmail.com>
Anand Kumria <wildfire@progsoc.org> writes:
> Sorry, I didn't know that gitk had been split back out (and
> Documentation/gitk.txt still mentions it is part of the git suite).
It is not "split back" at all, and it won't be. From "git" user's
point of view it is part of the suite.
Gitk however is still a viable freestanding project, so it would be
selfish for me to take a patch to gitk-git/gitk directly to my tree,
as the patch will not be able to flow back to the standalone gitk
project. Hence we always let patches go through Paul's tree and then
I pull from him.
^ permalink raw reply
* Re: [LHF] making t5000 "tar xf" tests more lenient
From: René Scharfe @ 2013-01-05 18:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vwqw7mb09.fsf@alter.siamese.dyndns.org>
Am 24.12.2012 21:49, schrieb Junio C Hamano:
> I've been running testsuite on a few platforms that are unfamiliar
> to me, and was bitten by BSD implementation of tar that do not grok
> the extended pax headers. I've already fixed one in t9502 [*1*]
> where we produce a tarball with "git archive" and then try to
> validate it with the platform implementation of "tar" so that the
> test won't barf when it sees the extended pax header.
>
> t5000 is littered with similar tests that break unnecessarily with
> BSD implementations. I think what it wants to test are:
>
> - "archive" produces tarball, and "tar" can extract from it. If
> your "tar" does not understand pax header, you may get it as an
> extra file that shouldn't be there, but that should not cause the
> test to fail---in real life, people without a pax-aware "tar"
> will just ignore and remove the header and can go on.
>
> - "get-tar-commmit-id" can inspect a tarball produced by "archive"
> to recover the object name of the commit even on a platform
> without a pax-aware "tar".
>
> Perhaps t5000 can be restructured like so:
>
> - create a tarball with the commit-id and test with
> "get-tar-commit-id" to validate it; also create a tarball out of
> a tree to make sure it does not have commit-id and check with
> "get-tar-commit-id". Do this on any and all platform, even on
> the ones without a pax-aware "tar".
>
> - check platform implementation of "tar" early to see if extracting
> a simple output from "git archive" results in an extra pax header
> file. If so, remember this fact and produce any and all tarballs
> used in the remainder of the test by forcing ^{tree}.
>
> so that people on platforms without pax-aware "tar" do not have to
> install GNU tar only to pass this test.
>
> It would be a good exercise during the holiday week for somebody on
> BSD (it seems NetBSD is more troublesome than OpenBSD) to come up
> with a patch to help users on these platforms.
I got around to building a virtual machine with NetBSD 6.0.1, which
involved a bit of cursing because networking only seems to work when I
turn off ACPI and SMP -- and running tests is a lot more pleasant with
more than one CPU.
Anyway, I don't think the pax headers are to blame here. The patch below
fixes the tar failures for me, but I'm not sure why. There must be
something special about some (not all!) directory entries with trailing
slashes after directory names.
Several ZIP tests still fail, however, because NetBSD's unzip doesn't
seem to support (our flavour of) symlinks and streamed files.
I'll take a deeper look into it over the weekend.
René
---
archive-tar.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/archive-tar.c b/archive-tar.c
index 0ba3f25..fd2d3e8 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -215,6 +215,8 @@ static int write_tar_entry(struct archiver_args *args,
if (S_ISDIR(mode) || S_ISGITLINK(mode)) {
*header.typeflag = TYPEFLAG_DIR;
mode = (mode | 0777) & ~tar_umask;
+ if (pathlen && path[pathlen - 1] == '/')
+ pathlen--;
} else if (S_ISLNK(mode)) {
*header.typeflag = TYPEFLAG_LNK;
mode |= 0777;
--
1.7.12
^ permalink raw reply related
* [PATCH] Alphabetize the fast-import options, following a suggestion on the list.
From: Eric S. Raymond @ 2013-01-05 16:44 UTC (permalink / raw)
To: git
---
Documentation/git-fast-import.txt | 94 +++++++++++++++++++--------------------
1 file changed, 45 insertions(+), 49 deletions(-)
diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 68bca1a..d006bcf 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -33,24 +33,9 @@ the frontend program in use.
OPTIONS
-------
---date-format=<fmt>::
- Specify the type of dates the frontend will supply to
- fast-import within `author`, `committer` and `tagger` commands.
- See ``Date Formats'' below for details about which formats
- are supported, and their syntax.
-
--- done::
- Terminate with error if there is no 'done' command at the
- end of the stream.
-
---force::
- Force updating modified existing branches, even if doing
- so would cause commits to be lost (as the new commit does
- not contain the old commit).
-
---max-pack-size=<n>::
- Maximum size of each output packfile.
- The default is unlimited.
+--active-branches=<n>::
+ Maximum number of branches to maintain active at once.
+ See ``Memory Utilization'' below for details. Default is 5.
--big-file-threshold=<n>::
Maximum size of a blob that fast-import will attempt to
@@ -58,13 +43,27 @@ OPTIONS
(512 MiB). Some importers may wish to lower this on systems
with constrained memory.
+--cat-blob-fd=<fd>::
+ Write responses to `cat-blob` and `ls` queries to the
+ file descriptor <fd> instead of `stdout`. Allows `progress`
+ output intended for the end-user to be separated from other
+ output.
+
+--date-format=<fmt>::
+ Specify the type of dates the frontend will supply to
+ fast-import within `author`, `committer` and `tagger` commands.
+ See ``Date Formats'' below for details about which formats
+ are supported, and their syntax.
+
--depth=<n>::
Maximum delta depth, for blob and tree deltification.
Default is 10.
---active-branches=<n>::
- Maximum number of branches to maintain active at once.
- See ``Memory Utilization'' below for details. Default is 5.
+--done::
+ Terminate with error if there is no 'done' command at the end
+ of the stream. This option might be useful for detecting
+ errors that cause the frontend to terminate before it has
+ started to write a stream.
--export-marks=<file>::
Dumps the internal marks table to <file> when complete.
@@ -75,6 +74,20 @@ OPTIONS
at checkpoint (or completion) the same path can also be
safely given to \--import-marks.
+--export-pack-edges=<file>::
+ After creating a packfile, print a line of data to
+ <file> listing the filename of the packfile and the last
+ commit on each branch that was written to that packfile.
+ This information may be useful after importing projects
+ whose total object set exceeds the 4 GiB packfile limit,
+ as these commits can be used as edge points during calls
+ to 'git pack-objects'.
+
+--force::
+ Force updating modified existing branches, even if doing
+ so would cause commits to be lost (as the new commit does
+ not contain the old commit).
+
--import-marks=<file>::
Before processing any input, load the marks specified in
<file>. The input file must exist, must be readable, and
@@ -87,13 +100,9 @@ OPTIONS
Like --import-marks but instead of erroring out, silently
skips the file if it does not exist.
---relative-marks::
- After specifying --relative-marks the paths specified
- with --import-marks= and --export-marks= are relative
- to an internal directory in the current repository.
- In git-fast-import this means that the paths are relative
- to the .git/info/fast-import directory. However, other
- importers may use a different location.
+--max-pack-size=<n>::
+ Maximum size of each output packfile.
+ The default is unlimited.
--no-relative-marks::
Negates a previous --relative-marks. Allows for combining
@@ -101,32 +110,19 @@ OPTIONS
--(no-)-relative-marks with the --(import|export)-marks=
options.
---cat-blob-fd=<fd>::
- Write responses to `cat-blob` and `ls` queries to the
- file descriptor <fd> instead of `stdout`. Allows `progress`
- output intended for the end-user to be separated from other
- output.
-
---done::
- Require a `done` command at the end of the stream.
- This option might be useful for detecting errors that
- cause the frontend to terminate before it has started to
- write a stream.
-
---export-pack-edges=<file>::
- After creating a packfile, print a line of data to
- <file> listing the filename of the packfile and the last
- commit on each branch that was written to that packfile.
- This information may be useful after importing projects
- whose total object set exceeds the 4 GiB packfile limit,
- as these commits can be used as edge points during calls
- to 'git pack-objects'.
-
--quiet::
Disable all non-fatal output, making fast-import silent when it
is successful. This option disables the output shown by
\--stats.
+--relative-marks::
+ After specifying --relative-marks the paths specified
+ with --import-marks= and --export-marks= are relative
+ to an internal directory in the current repository.
+ In git-fast-import this means that the paths are relative
+ to the .git/info/fast-import directory. However, other
+ importers may use a different location.
+
--stats::
Display some basic statistics about the objects fast-import has
created, the packfiles they were stored into, and the
--
1.8.1
--
<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>
"Gun control" is a job-safety program for criminals.
^ permalink raw reply related
* Re: [PATCH] git-fast-import(1): remove duplicate "--done" option
From: Eric S. Raymond @ 2013-01-05 16:26 UTC (permalink / raw)
To: John Keeping; +Cc: git, Jonathan Nieder, Sverre Rabbelier
In-Reply-To: <20130105160652.GE6440@serenity.lan>
John Keeping <john@keeping.me.uk>:
> I'm guessing that the reason the option was documented again (in commit
> 3266de10) is because the options are not in an obvious order. There
> does seem to be some grouping of the options by type, but without
> subheadings I wonder if it would make more sense to just put them all in
> alphabetical order?
+1
This duplication originated with me. I'll apologize with a
reordering patch.
--
<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>
^ permalink raw reply
* Re: Lockless Refs? (Was [PATCH] refs: do not use cached refs in repack_without_ref)
From: Jeff King @ 2013-01-05 16:12 UTC (permalink / raw)
To: Martin Fick; +Cc: Michael Haggerty, Shawn Pearce, git, Junio C Hamano
In-Reply-To: <201212310330.53835.mfick@codeaurora.org>
On Mon, Dec 31, 2012 at 03:30:53AM -0700, Martin Fick wrote:
> The general approach is to setup a transaction and either
> commit or abort it. A transaction can be setup by renaming
> an appropriately setup directory to the "ref.lock" name. If
> the rename succeeds, the transaction is begun. Any actor can
> abort the transaction (up until it is committed) by simply
> deleting the "ref.lock" directory, so it is not at risk of
> going stale.
Deleting a directory is not atomic, as you first have to remove the
contents, putting it into a potentially inconsistent state. I'll assume
you deal with that later...
> One important piece of the transaction is the use of uuids.
> The uuids provide a mechanism to tie the atomic commit pieces
> to the transactions and thus to prevent long sleeping process
> from inadvertently performing actions which could be out of
> date when they wake finally up.
Has this been a problem for you in practice? Avoiding this is one of the
reasons that git does not take out long locks; instead, it takes the
lock only at the moment it is ready to write, and aborts if it has been
updated since the longer-term operation began. This has its own problems
(you might do a lot of work only to have your operation aborted), but I
am not sure that your proposal improves on that.
Your proposal does sound like it could potentially improve robustness
when killing stale transactions (i.e., you know that the transaction
originator will never wake up and think it still has the lock). But
again, is that a problem in practice? Git typically holds ref locks for
a few syscalls. If you are conservative about leaving potentially stale
locks in place (e.g., give them a few minutes to complete before
assuming they are now bogus), you will not run into that problem.
The more conservative you are about treating a lock as stale, of course,
the less performant you will be in the face of stale locks. But since
they're the exception, that isn't a big problem in practice (at least it
has not been for me).
> In each case, the atomic
> commit piece is the renaming of a file. For the create and
> update pieces, a file is renamed from the "ref.lock" dir to
> the "ref" file resulting in an update to the sha for the ref.
I think we've had problems with cross-directory renames on some
filesystems, but I don't recall the details. I know that Coda does not
like cross-directory links, but cross-directory renames are OK (and in
fact we fall back to the latter when the former does not work).
Ah, here we go: 5723fe7 (Avoid cross-directory renames and linking on
object creation, 2008-06-14). Looks like NFS is the culprit.
> In the case of a delete, the actor may verify that "ref"
> currently contains the sha to "prune" if it needs to, and
> then renames the "ref" file to "ref.lock/uuid/delete". On
> success, the ref was deleted.
>
> Whether successful or not, the actor may now simply delete
> the "ref.lock" directory, clearing the way for a new
> transaction. Any other actor may delete this directory at
> any time also, likely either on conflict (if they are
> attempting to initiate a transaction), or after a grace
> period just to cleanup the FS. Any actor may also safely
> cleanup the tmp directories, preferably also after a grace
> period.
Hmm. So what happens to the "delete" file when the ref.lock directory is
being deleted? Presumably deleting the ref.lock directory means doing it
recursively (which is non-atomic). But then why are we keeping the
delete file at all, if we're just about to remove it?
What happens if another process wants to cancel a transaction that is
partially done? That is, the ref.lock directory is created, but it
contains the uuid subdir? It sounds like it's OK to just delete from it,
and the original process will then fail at its rename?
> One neat part about this scheme is that I believe it would be
> backwards compatible with the current locking mechanism since
> the transaction directory will simply appear to be a lock to
> older clients. And the old lock file should continue to lock
> out these newer transactions.
Yeah, I don't see anything that would prevent that. The current code
only cares about open("$ref.lock", O_EXCL). But that should be correct
and atomic with respect to the renamed directories. You are depending on
atomic directory renames. Those aren't used anywhere yet in git, as far
as I know. So that may run into problems (but there's no reason this
system couldn't be optional for filesystems that are more abled, and
other systems could fall back to the straight-file locking).
So in response to your question, no, I don't see any real showstoppers
here. And unlike the "all refs are files in a directory" scheme, it's
confined to writing, which solves the readdir() atomicity questions I
had there.
I still can't say I'm super excited about it, just because it seems like
a solution in search of a problem that I have not experienced myself.
But if it's solving a problem for you, I don't want to discourage you
from pursuing it.
> To be honest, I suspect I missed something obvious because
> this seems almost too simple to work. I am ashamed that it
> took me so long to come up with (of course, I will be even
> more ashamed :( when it is shown to be flawed!) This scheme
> also feels extensible. if there are no obvious flaws in it, I
> will try to post solutions for ref packing and for multiple
> repository/ref transactions also soon.
Fixing the minor remaining races on ref packing would be nice, but I do
not think those are a problem of the on-disk lock representation. The
reason they are not fixed now is from an attempt to keep lock contention
low between unrelated updates (something which we should probably give
up on in favor of correctness; but that is orthogonal to whether we do
it with the existing file locks or with a new system).
A much stronger fix for that would be to record deletes in the loose ref
storage so that we don't have to update the packed-refs file as often
(because it is inherently a lock bottleneck, and because it is wasteful
when you have a very large number of refs). But that means dealing with
directory/file conflicts between deleted and existing branches, which is
a pain.
-Peff
^ permalink raw reply
* [PATCH] Documentation: fix man page dependency on asciidoc.conf
From: John Keeping @ 2013-01-05 16:00 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
When building manual pages, the source text is transformed to XML with
AsciiDoc before the man pages are generated from the XML with xmlto.
Fix the dependency in the Makefile so that the XML files are rebuilt
when asciidoc.conf changes and not just the manual pages from unchanged
XML.
Signed-off-by: John Keeping <john@keeping.me.uk>
---
Documentation/Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/Makefile b/Documentation/Makefile
index e53d333..71d2b4a 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -178,7 +178,7 @@ all: html man
html: $(DOC_HTML)
-$(DOC_HTML) $(DOC_MAN1) $(DOC_MAN5) $(DOC_MAN7): asciidoc.conf
+$(DOC_HTML) $(MAN_XML): asciidoc.conf
man: man1 man5 man7
man1: $(DOC_MAN1)
--
1.8.0.2
^ permalink raw reply related
* [PATCH] git-fast-import(1): remove duplicate "--done" option
From: John Keeping @ 2013-01-05 16:06 UTC (permalink / raw)
To: git; +Cc: Jonathan Nieder, Eric S. Raymond, Sverre Rabbelier
The "--done" option to git-fast-import is documented twice in its manual
page. Combine the best bits of each description, keeping the location
of the instance that was added first.
Signed-off-by: John Keeping <john@keeping.me.uk>
---
I'm guessing that the reason the option was documented again (in commit
3266de10) is because the options are not in an obvious order. There
does seem to be some grouping of the options by type, but without
subheadings I wonder if it would make more sense to just put them all in
alphabetical order?
Documentation/git-fast-import.txt | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 68bca1a..4ef5721 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -39,10 +39,6 @@ OPTIONS
See ``Date Formats'' below for details about which formats
are supported, and their syntax.
--- done::
- Terminate with error if there is no 'done' command at the
- end of the stream.
-
--force::
Force updating modified existing branches, even if doing
so would cause commits to be lost (as the new commit does
@@ -108,7 +104,8 @@ OPTIONS
output.
--done::
- Require a `done` command at the end of the stream.
+ Terminate with error if there is no 'done' command at the
+ end of the stream.
This option might be useful for detecting errors that
cause the frontend to terminate before it has started to
write a stream.
--
1.8.0.2
^ permalink raw reply related
* Re: cvsps, parsecvs, svn2git and the CVS exporter mess
From: Eric S. Raymond @ 2013-01-05 15:58 UTC (permalink / raw)
To: Bart Massey
Cc: Max Horn, Michael Haggerty, Yann Dirson, Heiko Voigt,
Antoine Pelisse, Keith Packard, David Mansfield, git
In-Reply-To: <CAA6gtpky9JxFDdpLM6kY9su-9FWX8RoWHU4uptd_Zk+ZJuhrtA@mail.gmail.com>
Bart Massey <bart@cs.pdx.edu>:
> I don't know what Eric Raymond "officially end-of-life"-ing parsecvs means?
You and Keith handed me the maintainer's baton. If I were to EOL it,
that would be the successor you two designated judging in public that
the code is unsalvageable or has become pointless. If you wanted to
exclude the possibility that a successor would make that call, you
shouldn't have handed it off in a state so broken that I can't even
test it properly.
But I don't in fact think the parsecvs code is pointless. The fact that it
only needs the ,v files is nifty and means it could be used as an RCS
exporter too. The parsing and topo-analysis stages look like really
good work, very crisp and elegant (which is no less than I'd expect
from Keith, actually).
Alas, after wrestling with it I'm beginning to wonder whether the
codebase is salvageable by anyone but Keith himself. The tight coupling
to the git cache mechanism is the biggest problem. So far, I can't
figure out what tree.c is actually doing in enough detail to fix it or pry
it loose - the code is opaque and internal documentation is lacking.
More generally, interfacing to the unstable API of libgit was clearly
a serious mistake, leading directly to the current brokenness. The
tool should have emitted an import stream to begin with. I'm trying
to fix that, but success is looking doubtful.
--
<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>
^ permalink raw reply
* Re: cvsps, parsecvs, svn2git and the CVS exporter mess
From: Eric S. Raymond @ 2013-01-05 15:11 UTC (permalink / raw)
To: Max Horn
Cc: Michael Haggerty, Yann Dirson, Heiko Voigt, Antoine Pelisse,
Bart Massey, Keith Packard, David Mansfield, git
In-Reply-To: <1E7F9F86-F040-42E4-98C4-152B8CCE47CE@quendi.de>
Max Horn <postbox@quendi.de>:
> Hm, you snipped this part of Michael's mail:
>
> >> However, if that is a
> >> problem, it is possible to configure cvs2git to write the blobs inline
> >> with the rest of the dumpfile (this mode is supported because "hg
> >> fast-import" doesn't support detached blobs).
>
> I would call "hg fast-import" a main potential customer, given that there "cvs2hg" is another part of the cvs2svn suite. So I can't quite see how you can come to your conclusion above...
Perhaps I was unclear. I consider the interface design error to
be not in the fact that all the blobs are written first or detached,
but rather that the implementation detail of the two separate journal
files is ever exposed.
I understand why the storage of intermediate results was done this
way, in order to decrease the tool's working set during the run, but
finishing by automatically concatenating the results and streaming
them to stdout would surely have been the right thing here.
The downstream cost of letting the journalling implementation be
exposed, instead, can be seen in this snippet from the new git-cvsimport
I've been working on:
def command(self):
"Emit the command implied by all previous options."
return "(cvs2git --username=git-cvsimport --quiet --quiet --blobfile={0} --dumpfile={1} {2} {3} && cat {0} {1} && rm {0} {1})".format(tempfile.mkstemp()[1], tempfile.mkstemp()[1], self.opts, self.modulepath)
According to the documentation, every caller of csv2git must go
through analogous contortions! This is not the Unix way; if Unix
design principles had been minimally applied, that second line would
just read like this:
return "cvs2git --username=git-cvsimport --quiet --quiet"
If Unix design principles had been thoroughly applied, the "--quiet
--quiet" part would be unnecessary too - well-behaved Unix commands
*default* to being completely quiet unless either (a) they have an
exceptional condition to report, or (b) their expected running time is
so long that tasteful silence would leave users in doubt that they're
working.
(And yes, I do think violating these principles is a lapse of taste when
git tools do it, too.)
Michael Haggerty wants me to trust that cvs2git's analysis stage has
been fixed, but I must say that is a more difficult leap of faith when
two of the most visible things about it are still (a) a conspicuous
instance of interface misdesign, and (b) documentation that is careless and
incomplete.
--
<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>
^ permalink raw reply
* [PATCH] fix compilation with NO_PTHREADS
From: Jeff King @ 2013-01-05 14:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Commit 1327452 cleaned up an unused parameter from
wait_or_whine, but forgot to update a caller that is inside
"#ifdef NO_PTHREADS".
Signed-off-by: Jeff King <peff@peff.net>
---
I happened to notice this while looking at the sigpipe topic. I guess
not many people are building with NO_PTHREADS these days.
run-command.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/run-command.c b/run-command.c
index cfb7274..0471219 100644
--- a/run-command.c
+++ b/run-command.c
@@ -725,7 +725,7 @@ int finish_async(struct async *async)
int finish_async(struct async *async)
{
#ifdef NO_PTHREADS
- return wait_or_whine(async->pid, "child process", 0);
+ return wait_or_whine(async->pid, "child process");
#else
void *ret = (void *)(intptr_t)(-1);
--
1.8.1.rc1.16.g6d46841
^ permalink raw reply related
* Re: [BUG] git submodule update is not fail safe
From: Jens Lehmann @ 2013-01-05 14:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Heiko Voigt, git, Manlio Perillo, W. Trevor King
In-Reply-To: <50E83224.2070701@web.de>
Am 05.01.2013 15:01, schrieb Jens Lehmann:
> Am 04.01.2013 22:51, schrieb Junio C Hamano:
>> Manlio Perillo <manlio.perillo@gmail.com> writes:
>>
>>> $ git submodule update --init
>>> ...
>>> Submodule 'roms/vgabios' (git://git.qemu.org/vgabios.git/) registered
>>> for path 'roms/vgabios'
>>> fatal: unable to connect to anongit.freedesktop.org:
>>> anongit.freedesktop.org[0: 131.252.210.161]: errno=Connection timed out
>>>
>>> Unable to fetch in submodule path 'pixman'
>>>
>>> $ git submodule update --init
>>> fatal: Needed a single revision
>>> Unable to find current revision in submodule path 'pixman'
>>>
>>> The problem is easy to solve: manually remove the pixman directory;
>>> however IMHO git submodule update should not fail this way since it may
>>> confuse the user.
>>
>> Sounds like a reasonable observation. Jens, Heiko, comments?
>
> The reason seems to be that clone leaves a partial initialized .git
> directory in case of connection problems. The next time submodule
> update runs it tries to revive the submodule from .git/modules/<name>
> but fails as there are no objects in it.
>
> This looks like a bug in clone to me, as it takes precautions to clean
> up if something goes wrong but doesn't do that in this case. But while
> glancing over the code I didn't find out what goes wrong here.
I dug a bit deeper here and found the reason: In remove_junk() of
builtin/clone.c the junk_git_dir points to the gitfile in the
submodules work tree, not the .git/modules/<name> directory where
clone was told to put the git directory. Will see if I can come up
with a patch soonish ...
^ permalink raw reply
* Re: [BUG] git submodule update is not fail safe
From: Manlio Perillo @ 2013-01-05 14:49 UTC (permalink / raw)
To: Jens Lehmann; +Cc: Junio C Hamano, Heiko Voigt, git, W. Trevor King
In-Reply-To: <50E83224.2070701@web.de>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Il 05/01/2013 15:01, Jens Lehmann ha scritto:
> [...]
>>> $ git submodule update --init
>>> fatal: Needed a single revision
>>> Unable to find current revision in submodule path 'pixman'
>>>
>>> The problem is easy to solve: manually remove the pixman directory;
>>> however IMHO git submodule update should not fail this way since it may
>>> confuse the user.
>>
>> Sounds like a reasonable observation. Jens, Heiko, comments?
>
> The reason seems to be that clone leaves a partial initialized .git
> directory in case of connection problems. The next time submodule
> update runs it tries to revive the submodule from .git/modules/<name>
> but fails as there are no objects in it.
>
> [...]
>
> If this isn't seen as a bug in clone, we could also remove the
> .git/modules/<name> directory in module_clone() of git-submodule.s
> h when the clone fails. Manilo,
Its Manlio, not Manilo ;-).
> does the following patch remove the
> problems you are seeing (after removing .git/modules/pixman manually)?
>
I don't think I can test it right now, since the problem can only be
reproduced when git clone fails due to network problems.
Without the patch, if I remove the .git/modules/pixman directory,
`git submodule update --init pixamp` fails:
Unable to find current revision in submodule path 'pixman'
fatal: Not a git repository: pixman/../.git/modules/pixman
To reproduce the problem, however, it seems all you need to do is to
send SIGINT signal during `git submodule update` :
$ git submodule update --init pixman
Cloning into 'pixman'...
remote: Counting objects: 10137, done.
^C
$ git submodule update pixman
remote: Counting objects: 10137, done.
^C
$ git submodule update pixman
fatal: Needed a single revision
Unable to find current revision in submodule path 'pixman'
Note that I had to send SIGINT two times, in order to corrupt the module.
I suspect your patch does not fix this (since I don't get the "Clone
failed" error message).
I also noted that If I send SIGINT before git starts counting remote
objects, I get a different count number:
$ git submodule update pixman
Cloning into 'pixman'...
^C
$ git submodule update pixman
remote: Counting objects: 9757, done.
^C
$ git submodule update pixman
fatal: Needed a single revision
Unable to find current revision in submodule path 'pixman'
Note that git is reporting 9757 remote objects, instead of 10137.
P.S.:
sorry for the mail I sent today.
It reported the exact same problem I reported yesterday: this morning I
was rather sure that I got a different error message from submodule
update...
Regards Manlio
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iEYEARECAAYFAlDoPZMACgkQscQJ24LbaUTfNQCdFvhSQwGlJZlvOr+TIHHyDFJY
d8AAn0zuHKjBGIcqr8RH/rftHjomvPtM
=48RN
-----END PGP SIGNATURE-----
^ permalink raw reply
* [PATCH] run-command: encode signal death as a positive integer
From: Jeff King @ 2013-01-05 14:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, git, Bart Trojanowski
In-Reply-To: <20130105140316.GA7272@sigill.intra.peff.net>
On Sat, Jan 05, 2013 at 09:03:16AM -0500, Jeff King wrote:
> In fact, I really wonder if this code from wait_or_whine is actually
> correct:
>
> code = WTERMSIG(status);
> /*
> * This return value is chosen so that code & 0xff
> * mimics the exit code that a POSIX shell would report for
> * a program that died from this signal.
> */
> code -= 128;
After looking at it some more, it is correct, but I think we could make
life slightly easier for callers. See the patch below. I've tried to
re-state the somewhat rambling argument from my previous email;
hopefully it makes sense.
-- >8 --
Subject: [PATCH] run-command: encode signal death as a positive integer
When a sub-command dies due to a signal, we encode the
signal number into the numeric exit status as "signal -
128". This is easy to identify (versus a regular positive
error code), and when cast to an unsigned integer (e.g., by
feeding it to exit), matches what a POSIX shell would return
when reporting a signal death in $? or through its own exit
code.
So we have a negative value inside the code, but once it
passes across an exit() barrier, it looks positive (and any
code we receive from a sub-shell will have the positive
form). E.g., death by SIGPIPE (signal 13) will look like
-115 to us in inside git, but will end up as 141 when we
call exit() with it. And a program killed by SIGPIPE but run
via the shell will come to us with an exit code of 141.
Unfortunately, this means that when the "use_shell" option
is set, we need to be on the lookout for _both_ forms. We
might or might not have actually invoked the shell (because
we optimize out some useless shell calls). If we didn't invoke
the shell, we will will see the sub-process's signal death
directly, and run-command converts it into a negative value.
But if we did invoke the shell, we will see the shell's
128+signal exit status. To be thorough, we would need to
check both, or cast the value to an unsigned char (after
checking that it is not -1, which is a magic error value).
Fortunately, most callsites do not care at all whether the
exit was from a code or from a signal; they merely check for
a non-zero status, and sometimes propagate the error via
exit(). But for the callers that do care, we can make life
slightly easier by just using the consistent positive form.
This actually fixes two minor bugs:
1. In launch_editor, we check whether the editor died from
SIGINT or SIGQUIT. But we checked only the negative
form, meaning that we would fail to notice a signal
death exit code which was propagated through the shell.
2. In handle_alias, we assume that a negative return value
from run_command means that errno tells us something
interesting (like a fork failure, or ENOENT).
Otherwise, we simply propagate the exit code. Negative
signal death codes confuse us, and we print a useless
"unable to run alias 'foo': Success" message. By
encoding signal deaths using the positive form, the
existing code just propagates it as it would a normal
non-zero exit code.
The downside is that callers of run_command can no longer
differentiate between a signal received directly by the
sub-process, and one propagated. However, no caller
currently cares, and since we already optimize out some
calls to the shell under the hood, that distinction is not
something that should be relied upon by callers.
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/technical/api-run-command.txt | 6 ++----
editor.c | 2 +-
run-command.c | 2 +-
3 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/Documentation/technical/api-run-command.txt b/Documentation/technical/api-run-command.txt
index f18b4f4..5d7d7f2 100644
--- a/Documentation/technical/api-run-command.txt
+++ b/Documentation/technical/api-run-command.txt
@@ -55,10 +55,8 @@ The functions above do the following:
non-zero.
. If the program terminated due to a signal, then the return value is the
- signal number - 128, ie. it is negative and so indicates an unusual
- condition; a diagnostic is printed. This return value can be passed to
- exit(2), which will report the same code to the parent process that a
- POSIX shell's $? would report for a program that died from the signal.
+ signal number + 128, ie. the same value that a POSIX shell's $? would
+ report. A diagnostic is printed.
`start_async`::
diff --git a/editor.c b/editor.c
index 065a7ab..27bdecd 100644
--- a/editor.c
+++ b/editor.c
@@ -51,7 +51,7 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
sigchain_push(SIGINT, SIG_IGN);
sigchain_push(SIGQUIT, SIG_IGN);
ret = finish_command(&p);
- sig = ret + 128;
+ sig = ret - 128;
sigchain_pop(SIGINT);
sigchain_pop(SIGQUIT);
if (sig == SIGINT || sig == SIGQUIT)
diff --git a/run-command.c b/run-command.c
index 757f263..cfb7274 100644
--- a/run-command.c
+++ b/run-command.c
@@ -249,7 +249,7 @@ static int wait_or_whine(pid_t pid, const char *argv0)
* mimics the exit code that a POSIX shell would report for
* a program that died from this signal.
*/
- code -= 128;
+ code += 128;
} else if (WIFEXITED(status)) {
code = WEXITSTATUS(status);
/*
--
1.8.1.rc1.16.g6d46841
^ permalink raw reply related
* Re: [BUG] git submodule update is not fail safe
From: Jens Lehmann @ 2013-01-05 14:07 UTC (permalink / raw)
To: Manlio Perillo; +Cc: Junio C Hamano, Heiko Voigt, git, W. Trevor King
In-Reply-To: <50E83001.9000505@gmail.com>
Am 05.01.2013 14:52, schrieb Manlio Perillo:
> Il 04/01/2013 22:51, Junio C Hamano ha scritto:
>> Manlio Perillo <manlio.perillo@gmail.com> writes:
>
>>> $ git submodule update --init
>>> ...
>>> Submodule 'roms/vgabios' (git://git.qemu.org/vgabios.git/) registered
>>> for path 'roms/vgabios'
>>> fatal: unable to connect to anongit.freedesktop.org:
>>> anongit.freedesktop.org[0: 131.252.210.161]: errno=Connection timed out
>>>
>>> Unable to fetch in submodule path 'pixman'
>>>
>>> $ git submodule update --init
>>> fatal: Needed a single revision
>>> Unable to find current revision in submodule path 'pixman'
>>>
>>> The problem is easy to solve: manually remove the pixman directory;
>>> however IMHO git submodule update should not fail this way since it may
>>> confuse the user.
>
>> Sounds like a reasonable observation. Jens, Heiko, comments?
>
> I have found another, related problem.
>
> Today I tried to update qemu submodules again, however the command
> failed with an "obscure" error message:
>
> $ git submodule update pixman
> fatal: Needed a single revision
> Unable to find current revision in submodule path 'pixman'
>
>
> The pixman submodule is the one that I failed to update in the very begin.
> The problem is not with the pixman or qemu repository: if I clone again
> qemu (with the --recursive option), all is ok.
>
> The problem is with the private working copy (in .git/modules/pixman)
> being corrupted:
>
> $git log
> fatal: bad default revision 'HEAD'.
>
> The HEAD file contains "ref: refs/heads/master", but the refs/heads
> directory is empty.
Yep, as I explained in my other email the partially set up
.git/modules/pixman is the reason for the trouble you have.
> By the way: since git submodule is a porcelain command, IMHO it should
> not show to the user these low level error message; at least it should
> give more details.
> As an example, in this case it could say something like:
>
> the local module "pixmap" seems to be corrupted.
> Run xxx to remove the module and yyy to create it again.
>
> The ideal solution is, for submodule update, to never leave an
> incomplete directory; that is: the update command should be atomic.
I agree that submodule update should not leave an inconsistent state.
In that case you wouldn't see any low level error messages (which I
think is ok if something the porcelain didn't expect to happen occurs,
like it did here).
^ permalink raw reply
* Re: [PATCH] gitk: Display the date of a tag in a human friendly way.
From: Anand Kumria @ 2013-01-05 14:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Paul Mackerras, git
In-Reply-To: <7vhamwse2c.fsf@alter.siamese.dyndns.org>
Hi Junio,
On 4 January 2013 23:50, Junio C Hamano <gitster@pobox.com> wrote:
> Anand Kumria <wildfire@progsoc.org> writes:
>
>> By selecting a tag within gitk you can display information about it.
>> This information is output by using the command
>>
>> 'git cat-file tag <tagid>'
>>
>> This outputs the *raw* information from the tag, amongst which is the
>> time - in seconds since the epoch. As useful as that value is, I find it
>> a lot easier to read and process time which it is something like:
>>
>> "Mon Dec 31 14:26:11 2012 -0800"
>>
>> This change will modify the display of tags in gitk like so:
>>
>> @@ -1,7 +1,7 @@
>> object 5d417842efeafb6e109db7574196901c4e95d273
>> type commit
>> tag v1.8.1
>> -tagger Junio C Hamano <gitster@pobox.com> 1356992771 -0800
>> +tagger Junio C Hamano <gitster@pobox.com> Mon Dec 31 14:26:11 2012 -0800
>>
>> Git 1.8.1
>> -----BEGIN PGP SIGNATURE-----
>>
>> Signed-off-by: Anand Kumria <wildfire@progsoc.org>
>> ---
>
> Sounds like a sensible thing to do but I didn't check how else
> (other than purely for displaying) this string is used.
As far as I can tell it is only used for display (cached_tagcontent in
gitk) purposes.
> Paul, the patch is not made against your tree, so if you choose to
> take it you would need to strip the leading directory at the top.
Sorry, I didn't know that gitk had been split back out (and
Documentation/gitk.txt still mentions it is part of the git suite).
Regards,
Anand
^ permalink raw reply
* Re: [RFC/PATCH] avoid SIGPIPE warnings for aliases
From: Jeff King @ 2013-01-05 14:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Bart Trojanowski
In-Reply-To: <7vsj6gsi7v.fsf@alter.siamese.dyndns.org>
On Fri, Jan 04, 2013 at 02:20:52PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > I have two reservations with this patch:
> >
> > 1. We are ignoring SIGPIPE all the time. For an alias that is calling
> > "log", that is fine. But if pack-objects dies on the server side,
> > seeing that it died from SIGPIPE is useful data, and we are
> > squelching that. Maybe callers of run-command should have to pass
> > an "ignore SIGPIPE" flag?
>
> What should this do:
>
> GIT_PAGER='head -n 1' git -p -c alias.o='!cat longfile' o
>
> Should it behave just like
>
> cat longfile | head -n 1
>
> or should it behave differently?
With respect to error messages, I'd think they should behave the same.
But they don't necessarily. The latter does not print any message at
all. But consider this version of the former:
$ cat foo
#!/bin/sh
exec cat longfile
$ git -c alias.o='!./foo' o | head -n 1
error: ./foo died of signal 13
fatal: While expanding alias 'o': './foo': Success
So why don't we see that message more often? There are two reasons.
One reason is that we piped it ourselves here. When git pipes to the
pager, it sends stderr along the same channel. So even if you did:
GIT_PAGER='head -n 1' git -p -c alias.o='!./foo' o
git writes the error, but it goes to the pager, which has already ended
(since that is what caused the SIGPIPE in the first place). But imagine
that your sub-command is actually invoking git itself, and it is the
sub-git which starts the pager. Meaning the outer git wrapper's stderr
is _not_ redirected. Like this:
$ cat foo
#!/bin/sh
exec git log -p
$ GIT_PAGER='head -n 1' git -c alias.o='!./foo' o
error: ./foo died of signal 13
fatal: While expanding alias 'o': './foo': Success
The second reason is that most shells will "eat" the SIGPIPE exit
status, and convert it into a high, positive error code. You can see
that effect here:
$ GIT_PAGER='head -n 1' git log -p
$ echo $?
141
And since we execute aliases via the shell, we end up seeing the
converted exit code (141) instead of the signal death. _Unless_ we
optimize out the shell call (which is why we see it by putting the
command inside "./foo", which git executes directly, but not when we
give the literal "cat longfile", which git will feed to the shell).
Or at least that's _one_ way to see it. Another way is to use a shell
that does not do such conversion. Setting SHELL_PATH to zsh seems to do
so, and I think that is how Bart ran into it (my patch is a followup to
a Google+ posting he made).
> I am having a feeling that whatever external command given as the
> value of alias.$cmd should choose what error status it wants to be
> reported.
I suppose. It means that our "do not run the shell if there are no
meta-characters" optimization is leaky, since the exit code behaves
differently depending on whether we run the shell (and depending on your
exact shell). One solution would be to fix that leakiness, and if
use_shell is in effect for run-command, to convert a signal death into
the value that the shell would otherwise give us.
In fact, I really wonder if this code from wait_or_whine is actually
correct:
code = WTERMSIG(status);
/*
* This return value is chosen so that code & 0xff
* mimics the exit code that a POSIX shell would report for
* a program that died from this signal.
*/
code -= 128;
If we get signal 13, we end up with -115, because "code" is signed. When
the lower 8 bits are taken, and then converted into an unsigned value,
we get 141: the shell value.
But do we really want to return a negative value here? Should this
instead be:
code += 128
which yields the same code when fed to exit, but internally looks like
the shell version to us? So we get a consistent result whether the shell
was actually used or not.
That makes more sense to me, and would mean that whether we converted
the signal number or whether it was done by a subshell, it looks the
same to us. Callers which care about signals (e.g., the recent changes
to launch_editor to detect SIGINT) would have to be adjusted. But I
think it fixes an obscure bug there. Right now launch_editor is actually
checking the whether the _shell_ died from a signal, and will fail to
notice when an editor invoked by the shell is killed by those signals
(this would be pretty rare, though, because typically SIGINT is
delivered to the shell as well as the editor).
This would also fix the code in handle_alias. It looks for a negative
return code from run_command as the sign that there was an internal
error running the command, and that errno would be valid. But right now
a negative return can also come from signal death.
> > 2. The die_errno in handle_alias is definitely wrong. Even if we want
> > to print a message for signal death, showing errno is bogus unless
> > the return value was -1. But is it the right thing to just pass the
> > negative value straight to exit()? It works, but it is depending on
> > the fact that (unsigned char)(ret & 0xff) behaves in a certain way
> > (i.e., that we are on a twos-complement platform, and -13 becomes
> > 141).
>
> Isn't that what POSIX.1 guarantees us, though?
>
> The value of status may be 0, EXIT_SUCCESS, EXIT_FAILURE, or any
> other value, though only the least significant 8 bits (that is,
> status & 0377) shall be available to a waiting parent process.
Sort of. I was worried about:
1. Not-quite-POSIX platforms (i.e., Windows). But JSixt has said that
is fine, because we already have a compatibility wrapper which
masks off only the low byte.
2. We are relying on the specifics of how a negative value is treated
by exit(). The cast I gave above is guaranteed to work in standard
C, but we do not know the implementation details of exit(). Still,
I think that is being overly paranoid. Any sane implementation will
do what we expect.
-Peff
^ permalink raw reply
* Re: [BUG] git submodule update is not fail safe
From: Jens Lehmann @ 2013-01-05 14:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Heiko Voigt, git, Manlio Perillo, W. Trevor King
In-Reply-To: <7vzk0osjli.fsf@alter.siamese.dyndns.org>
Am 04.01.2013 22:51, schrieb Junio C Hamano:
> Manlio Perillo <manlio.perillo@gmail.com> writes:
>
>> $ git submodule update --init
>> ...
>> Submodule 'roms/vgabios' (git://git.qemu.org/vgabios.git/) registered
>> for path 'roms/vgabios'
>> fatal: unable to connect to anongit.freedesktop.org:
>> anongit.freedesktop.org[0: 131.252.210.161]: errno=Connection timed out
>>
>> Unable to fetch in submodule path 'pixman'
>>
>> $ git submodule update --init
>> fatal: Needed a single revision
>> Unable to find current revision in submodule path 'pixman'
>>
>> The problem is easy to solve: manually remove the pixman directory;
>> however IMHO git submodule update should not fail this way since it may
>> confuse the user.
>
> Sounds like a reasonable observation. Jens, Heiko, comments?
The reason seems to be that clone leaves a partial initialized .git
directory in case of connection problems. The next time submodule
update runs it tries to revive the submodule from .git/modules/<name>
but fails as there are no objects in it.
This looks like a bug in clone to me, as it takes precautions to clean
up if something goes wrong but doesn't do that in this case. But while
glancing over the code I didn't find out what goes wrong here.
If this isn't seen as a bug in clone, we could also remove the
.git/modules/<name> directory in module_clone() of git-submodule.s
h when the clone fails. Manilo, does the following patch remove the
problems you are seeing (after removing .git/modules/pixman manually)?
diff --git a/git-submodule.sh b/git-submodule.sh
index 2365149..4098702 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -208,7 +208,10 @@ module_clone()
git clone $quiet -n ${reference:+"$reference"} \
--separate-git-dir "$gitdir" "$url" "$sm_path"
) ||
- die "$(eval_gettext "Clone of '\$url' into submodule path '\$sm_path' failed")"
+ (
+ rm -rf "$gitdir" &&
+ die "$(eval_gettext "Clone of '\$url' into submodule path '\$sm_path' failed")"
+ )
fi
# We already are at the root of the work tree but cd_to_toplevel will
^ permalink raw reply related
* Re: [BUG] git submodule update is not fail safe
From: Manlio Perillo @ 2013-01-05 13:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jens Lehmann, Heiko Voigt, git, W. Trevor King
In-Reply-To: <7vzk0osjli.fsf@alter.siamese.dyndns.org>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Il 04/01/2013 22:51, Junio C Hamano ha scritto:
> Manlio Perillo <manlio.perillo@gmail.com> writes:
>
>> $ git submodule update --init
>> ...
>> Submodule 'roms/vgabios' (git://git.qemu.org/vgabios.git/) registered
>> for path 'roms/vgabios'
>> fatal: unable to connect to anongit.freedesktop.org:
>> anongit.freedesktop.org[0: 131.252.210.161]: errno=Connection timed out
>>
>> Unable to fetch in submodule path 'pixman'
>>
>> $ git submodule update --init
>> fatal: Needed a single revision
>> Unable to find current revision in submodule path 'pixman'
>>
>> The problem is easy to solve: manually remove the pixman directory;
>> however IMHO git submodule update should not fail this way since it may
>> confuse the user.
>
> Sounds like a reasonable observation. Jens, Heiko, comments?
I have found another, related problem.
Today I tried to update qemu submodules again, however the command
failed with an "obscure" error message:
$ git submodule update pixman
fatal: Needed a single revision
Unable to find current revision in submodule path 'pixman'
The pixman submodule is the one that I failed to update in the very begin.
The problem is not with the pixman or qemu repository: if I clone again
qemu (with the --recursive option), all is ok.
The problem is with the private working copy (in .git/modules/pixman)
being corrupted:
$git log
fatal: bad default revision 'HEAD'.
The HEAD file contains "ref: refs/heads/master", but the refs/heads
directory is empty.
By the way: since git submodule is a porcelain command, IMHO it should
not show to the user these low level error message; at least it should
give more details.
As an example, in this case it could say something like:
the local module "pixmap" seems to be corrupted.
Run xxx to remove the module and yyy to create it again.
The ideal solution is, for submodule update, to never leave an
incomplete directory; that is: the update command should be atomic.
Regards Manlio
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iEYEARECAAYFAlDoMAEACgkQscQJ24LbaUQVugCggdl36Hx5JIW/hd1SVXWv+ths
zpYAnR+93BfDLaFhXEiaQvu/TickmDA0
=2Mnw
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [PATCH] Add getenv.so for catching invalid getenv() use via LD_PRELOAD
From: Duy Nguyen @ 2013-01-05 11:37 UTC (permalink / raw)
To: Matt Kraai
Cc: Nguyễn Thái Ngọc, git, Junio C Hamano,
David Michael
In-Reply-To: <20130105103900.GA4200@ftbfs.org>
On Sat, Jan 5, 2013 at 5:39 PM, Matt Kraai <kraai@ftbfs.org> wrote:
> On Sat, Jan 05, 2013 at 03:55:46PM +0700, Nguyễn Thái Ngọc Duy wrote:
>> Perhaps this will help the getenv bug hunting (I assume we do the
>> hunting on Linux platform only). So far it catches this and is stuck
>> at getenv in git_pager().
>
> It seems like a static analysis tool might be able to detect these
> problems. Is there a way to do so using sparse?
That was my first thought. But this may involve flow analysis and I
don't think sparse is up to it. ccc-analyzer is still pretty basic.
And between static analysis and runtime check, I prefer the latter as
it's more reliable as long as you have a good coverage test.
>
>> + n = backtrace(buffer, 100);
>> + symbols = backtrace_symbols(buffer, n);
>> + if (symbols) {
>> + for (i = 0;i < n; i++)
>
> s/;i/; i/
Thanks. I will fix it later if people actually want this.
--
Duy
^ permalink raw reply
* Re: t7061: comments and one failure
From: Antoine Pelisse @ 2013-01-05 11:29 UTC (permalink / raw)
To: Jeff King; +Cc: Torsten Bögershausen, Git Mailing List
In-Reply-To: <20130105112432.GA14666@sigill.intra.peff.net>
> Yeah, I can reproduce the problem here on my OS X test box. It seems to
> be related to core.ignorecase. If you put
>
> git config core.ignorecase false
>
> at the top of t7061, it passes on OS X. Likewise, if you set it to true,
> it will start failing on Linux.
Great, so I have a chance to reproduce and fix it. (and it gives me a
massive hint on where the bug can stand).
^ permalink raw reply
* Re: t7061: comments and one failure
From: Jeff King @ 2013-01-05 11:24 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: apelisse, Git Mailing List
In-Reply-To: <50E8096C.7000501@web.de>
On Sat, Jan 05, 2013 at 12:07:24PM +0100, Torsten Bögershausen wrote:
> TC 9 is failing (Mac OS X 10.6),
Yeah, I can reproduce the problem here on my OS X test box. It seems to
be related to core.ignorecase. If you put
git config core.ignorecase false
at the top of t7061, it passes on OS X. Likewise, if you set it to true,
it will start failing on Linux.
So it looks like a real bug, not a test-portability issue.
> Looking into the code, there are 2 questions:
>
> 1) echo "ignored" >.gitignore &&
> We don't need the quoting of a simple string which does not have space in it.
No, but it does not hurt anything.
> 2) : >untracked/ignored &&
> Do we need the colon here?
No, but it does not hurt anything.
-Peff
^ 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