* Re: [msysGit] Re: [PATCH 0/2] submodules: Use relative paths to gitdir and work tree
From: Johannes Schindelin @ 2012-02-27 21:41 UTC (permalink / raw)
To: Johannes Sixt
Cc: Jens Lehmann, Junio C Hamano, Git Mailing List, Antony Male,
Phil Hord, msysGit
In-Reply-To: <4F4BF357.8020407@kdbg.org>
Hi,
On Mon, 27 Feb 2012, Johannes Sixt wrote:
> Am 26.02.2012 20:58, schrieb Jens Lehmann:
>
> > - gitdir=$(git rev-parse --git-dir)
> > + gitdir=$(git rev-parse --git-dir | sed -e 's,^\([a-z]\):/,/\1/,')
>
> I don't like pipelines of this kind because they fork yet another
> process. But it looks like there are not that many alternatives...
Not many, but some rather straight-forward ones. E.g.
# quoted to preserve tabs & newslines (Windows users are creative)
gitdir="$(git rev-parse --git-dir)"
case "$gitdir" in
[A-Za-z]:*)
gitdir="/${gitdir%%:*}${gitdir#?:}"
;;
esac
Note: untested.
Note2: I did not add error handling, either.
Note3: yes, ${...} is available in bash and it does not fork. Neither does
case ... esac
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH 02/11] Factor out and export large blob writing code to arbitrary file handle
From: Junio C Hamano @ 2012-02-27 21:50 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <7v4nucb2xl.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> So I think the external declaration and the definition should move to a
> more generic place, namely streaming.[ch]. It does not belong to entry.c
> anymore.
>
> Thanks for working on this.
In other words, I think the result should look more like this.
The original logic in entry.c is that the caller should try to get a
filter and call streaming_write_entry(), but either of them is allowed to
return a failure when the blob is not suitable for the streaming codepath
to tell the caller to try their traditional codepath.
We might want to add another helper function for callers to use to decide
if they should use the streaming interface, or the traditional one, before
actually making a call to streaming_write_entry(). With the original (and
current) API, they have to retry even when the streaming codepath truly
failed (e.g. no such blob object), in which case it is very likely that
the traditional codepath in the caller will fail the same way. Retrying is
a wasted effort in such a case.
-- >8 --
Subject: [PATCH] streaming: make streaming-write-entry to be more reusable
The static function in entry.c takes a cache entry and streams its blob
contents to a file in the working tree. Refactor the logic to a new API
function stream_blob_to_fd() that takes an object name and an open file
descriptor, so that it can be reused by other callers.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
entry.c | 53 +++++------------------------------------------------
streaming.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
streaming.h | 2 ++
3 files changed, 62 insertions(+), 48 deletions(-)
diff --git a/entry.c b/entry.c
index 852fea1..17a6bcc 100644
--- a/entry.c
+++ b/entry.c
@@ -120,58 +120,15 @@ static int streaming_write_entry(struct cache_entry *ce, char *path,
const struct checkout *state, int to_tempfile,
int *fstat_done, struct stat *statbuf)
{
- struct git_istream *st;
- enum object_type type;
- unsigned long sz;
int result = -1;
- ssize_t kept = 0;
- int fd = -1;
-
- st = open_istream(ce->sha1, &type, &sz, filter);
- if (!st)
- return -1;
- if (type != OBJ_BLOB)
- goto close_and_exit;
+ int fd;
fd = open_output_fd(path, ce, to_tempfile);
- if (fd < 0)
- goto close_and_exit;
-
- for (;;) {
- char buf[1024 * 16];
- ssize_t wrote, holeto;
- ssize_t readlen = read_istream(st, buf, sizeof(buf));
-
- if (!readlen)
- break;
- if (sizeof(buf) == readlen) {
- for (holeto = 0; holeto < readlen; holeto++)
- if (buf[holeto])
- break;
- if (readlen == holeto) {
- kept += holeto;
- continue;
- }
- }
-
- if (kept && lseek(fd, kept, SEEK_CUR) == (off_t) -1)
- goto close_and_exit;
- else
- kept = 0;
- wrote = write_in_full(fd, buf, readlen);
-
- if (wrote != readlen)
- goto close_and_exit;
- }
- if (kept && (lseek(fd, kept - 1, SEEK_CUR) == (off_t) -1 ||
- write(fd, "", 1) != 1))
- goto close_and_exit;
- *fstat_done = fstat_output(fd, state, statbuf);
-
-close_and_exit:
- close_istream(st);
- if (0 <= fd)
+ if (0 <= fd) {
+ result = stream_blob_to_fd(fd, ce->sha1, filter, 1);
+ *fstat_done = fstat_output(fd, state, statbuf);
result = close(fd);
+ }
if (result && 0 <= fd)
unlink(path);
return result;
diff --git a/streaming.c b/streaming.c
index 71072e1..7e7ee2b 100644
--- a/streaming.c
+++ b/streaming.c
@@ -489,3 +489,58 @@ static open_method_decl(incore)
return st->u.incore.buf ? 0 : -1;
}
+
+
+/****************************************************************
+ * Users of streaming interface
+ ****************************************************************/
+
+int stream_blob_to_fd(int fd, unsigned const char *sha1, struct stream_filter *filter,
+ int can_seek)
+{
+ struct git_istream *st;
+ enum object_type type;
+ unsigned long sz;
+ ssize_t kept = 0;
+ int result = -1;
+
+ st = open_istream(sha1, &type, &sz, filter);
+ if (!st)
+ return result;
+ if (type != OBJ_BLOB)
+ goto close_and_exit;
+ for (;;) {
+ char buf[1024 * 16];
+ ssize_t wrote, holeto;
+ ssize_t readlen = read_istream(st, buf, sizeof(buf));
+
+ if (!readlen)
+ break;
+ if (can_seek && sizeof(buf) == readlen) {
+ for (holeto = 0; holeto < readlen; holeto++)
+ if (buf[holeto])
+ break;
+ if (readlen == holeto) {
+ kept += holeto;
+ continue;
+ }
+ }
+
+ if (kept && lseek(fd, kept, SEEK_CUR) == (off_t) -1)
+ goto close_and_exit;
+ else
+ kept = 0;
+ wrote = write_in_full(fd, buf, readlen);
+
+ if (wrote != readlen)
+ goto close_and_exit;
+ }
+ if (kept && (lseek(fd, kept - 1, SEEK_CUR) == (off_t) -1 ||
+ write(fd, "", 1) != 1))
+ goto close_and_exit;
+ result = 0;
+
+ close_and_exit:
+ close_istream(st);
+ return result;
+}
diff --git a/streaming.h b/streaming.h
index 589e857..3e82770 100644
--- a/streaming.h
+++ b/streaming.h
@@ -12,4 +12,6 @@ extern struct git_istream *open_istream(const unsigned char *, enum object_type
extern int close_istream(struct git_istream *);
extern ssize_t read_istream(struct git_istream *, char *, size_t);
+extern int stream_blob_to_fd(int fd, const unsigned char *, struct stream_filter *, int can_seek);
+
#endif /* STREAMING_H */
--
1.7.9.2.312.g1abc3
^ permalink raw reply related
* Re: [RFC/PATCH] Make git-{pull,rebase} no-tracking message friendlier
From: Matthieu Moy @ 2012-02-27 22:06 UTC (permalink / raw)
To: Carlos Martín Nieto; +Cc: git
In-Reply-To: <1330013115-26355-1-git-send-email-cmn@elego.de>
Carlos Martín Nieto <cmn@elego.de> writes:
> - echo "You asked me to $cmd without telling me which branch you
> -want to $op_type $op_prep, and 'branch.${branch_name#refs/heads/}.merge' in
> -your configuration file does not tell me, either. Please
> -specify which branch you want to use on the command line and
> + echo "You asked me to $cmd without telling me which branch you want to
> +$op_type $op_prep, and there is no tracking information for the current branch.
> +Please specify which branch you want to use on the command line and
> try again (e.g. '$example').
At this point, it may be better to actually give the full command
instead of just this "(e.g. '$example')", i.e. stg like
git $op_type <remote> $example
I also saw users confused by the message (indeed without reading it,
but ...). Giving them something as close as possible to
cut-and-paste-able command should help.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH v6 02/11] Add git-column and column mode parsing
From: Ramsay Jones @ 2012-02-27 20:09 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano
In-Reply-To: <1330170078-29353-3-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy wrote:
[...]
> +static int parse_option(const char *arg, int len,
> + unsigned int *mode, int stdout_is_tty)
> +{
> + struct colopt opts[] = {
> + { ENABLE, "always", 1 },
> + { ENABLE, "never", 0 },
> + { ENABLE, "auto", -1 },
> + };
Hmm, I don't recognise this table from last time ...
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(opts); i++) {
> + int set = 1, arg_len = len, name_len;
set is initialised here (mainly to silence the compiler) in each
loop, but then ...
> + const char *arg_str = arg;
> +
> + if (opts[i].type == OPTION) {
> + if (arg_len > 2 && !strncmp(arg_str, "no", 2)) {
> + arg_str += 2;
> + arg_len -= 2;
> + set = 0;
> + } else {
> + set = 1;
... this else clause is no longer required.
> + }
> + }
> +
> + name_len = strlen(opts[i].name);
> + if (arg_len != name_len ||
> + strncmp(arg_str, opts[i].name, name_len))
> + continue;
> +
> + switch (opts[i].type) {
> + case ENABLE:
> + return set_enable_bit(mode, opts[i].value,
> + stdout_is_tty);
given the above table, can the following case limbs ever be reached?
(the "no" prefix is only applied to the OPTION type, so most of the
above code seems to be useless now ...)
> + case MODE:
> + return set_mode(mode, opts[i].value);
> + case OPTION:
> + return set_option(mode, opts[i].value, set);
> + default:
> + die("BUG: Unknown option type %d", opts[i].type);
> + }
> + }
> +
> + return error("unsupported style '%s'", arg);
> +}
> +
[...]
Note, I only skimmed the patch text, I haven't applied it or tested it ...
ATB,
Ramsay Jones
^ permalink raw reply
* [PATCH 1/2] builtin/symbolic-ref.c: add option to fall back to normal ref
From: Jan Krüger @ 2012-02-27 22:08 UTC (permalink / raw)
To: git
Frequently, people want to determine the current value of HEAD in
scripts. However, there is no tool that can always output it, since "git
symbolic-ref" will fail if HEAD isn't currently a symref, and other
tools (e.g. "git rev-parse --symbolic-full-name") will also fail in
one of HEAD's possible modes.
To resolve this situation, add the new -f option to symbolic-ref that
falls back to outputting the value of HEAD as a normal ref if necessary.
Signed-off-by: Jan Krüger <jk@jk.gs>
---
Documentation/git-symbolic-ref.txt | 7 ++++++-
builtin/symbolic-ref.c | 16 ++++++++++++++--
2 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-symbolic-ref.txt b/Documentation/git-symbolic-ref.txt
index a45d4c4..a05819b 100644
--- a/Documentation/git-symbolic-ref.txt
+++ b/Documentation/git-symbolic-ref.txt
@@ -8,7 +8,7 @@ git-symbolic-ref - Read and modify symbolic refs
SYNOPSIS
--------
[verse]
-'git symbolic-ref' [-q] [-m <reason>] <name> [<ref>]
+'git symbolic-ref' [-q] [-f] [-m <reason>] <name> [<ref>]
DESCRIPTION
-----------
@@ -33,6 +33,11 @@ OPTIONS
symbolic ref but a detached HEAD; instead exit with
non-zero status silently.
+-f::
+ When showing the current value of <name>, do not fail if it is
+ not a symbolic ref; instead output the SHA1 value referenced by
+ <name>.
+
-m::
Update the reflog for <name> with <reason>. This is valid only
when creating or updating a symbolic ref.
diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
index 2ef5962..2e0a86f 100644
--- a/builtin/symbolic-ref.c
+++ b/builtin/symbolic-ref.c
@@ -8,6 +8,8 @@ static const char * const git_symbolic_ref_usage[] = {
NULL
};
+static int fallback_regular_ref;
+
static void check_symref(const char *HEAD, int quiet)
{
unsigned char sha1[20];
@@ -17,10 +19,18 @@ static void check_symref(const char *HEAD, int quiet)
if (!refs_heads_master)
die("No such ref: %s", HEAD);
else if (!(flag & REF_ISSYMREF)) {
- if (!quiet)
+ if (fallback_regular_ref) {
+ char sha1[20];
+ if (!get_sha1(HEAD, sha1))
+ puts(sha1_to_hex(sha1));
+ else
+ die("failed to resolve ref %s", HEAD);
+ return;
+ } else if (!quiet) {
die("ref %s is not a symbolic ref", HEAD);
- else
+ } else {
exit(1);
+ }
}
puts(refs_heads_master);
}
@@ -32,6 +42,8 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
struct option options[] = {
OPT__QUIET(&quiet,
"suppress error message for non-symbolic (detached) refs"),
+ OPT_BOOLEAN('f', NULL, &fallback_regular_ref,
+ "fall back to showing as a regular ref"),
OPT_STRING('m', NULL, &msg, "reason", "reason of the update"),
OPT_END(),
};
--
1.7.9.2.302.g3724c.dirty
From 1fffb746a65aac88d3af9bae785b1cfa58cbf31c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jan=20Kr=C3=BCger?= <jk@jk.gs>
Date: Mon, 27 Feb 2012 22:40:13 +0100
Subject: [PATCH 2/2] builtin/symbolic-ref.c: add option to output shortened
ref
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
In scripts meant to generate user-consumable output, it can be helpful
to resolve a symbolic ref and output the result in a shortened form,
such as for use in shell prompts. Add a new -s option to allow this.
Signed-off-by: Jan Krüger <jk@jk.gs>
---
Documentation/git-symbolic-ref.txt | 6 +++++-
builtin/symbolic-ref.c | 5 +++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-symbolic-ref.txt b/Documentation/git-symbolic-ref.txt
index a05819b..7f108ce 100644
--- a/Documentation/git-symbolic-ref.txt
+++ b/Documentation/git-symbolic-ref.txt
@@ -8,7 +8,7 @@ git-symbolic-ref - Read and modify symbolic refs
SYNOPSIS
--------
[verse]
-'git symbolic-ref' [-q] [-f] [-m <reason>] <name> [<ref>]
+'git symbolic-ref' [-q] [-f] [-s] [-m <reason>] <name> [<ref>]
DESCRIPTION
-----------
@@ -38,6 +38,10 @@ OPTIONS
not a symbolic ref; instead output the SHA1 value referenced by
<name>.
+-s::
+ When showing the value of <name> as a symbolic ref, try to shorten the
+ value, e.g. from `refs/heads/master` to `master`.
+
-m::
Update the reflog for <name> with <reason>. This is valid only
when creating or updating a symbolic ref.
diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
index 2e0a86f..df8da11 100644
--- a/builtin/symbolic-ref.c
+++ b/builtin/symbolic-ref.c
@@ -9,6 +9,7 @@ static const char * const git_symbolic_ref_usage[] = {
};
static int fallback_regular_ref;
+static int shorten;
static void check_symref(const char *HEAD, int quiet)
{
@@ -32,6 +33,9 @@ static void check_symref(const char *HEAD, int quiet)
exit(1);
}
}
+ if (shorten)
+ refs_heads_master = shorten_unambiguous_ref(
+ refs_heads_master, 0);
puts(refs_heads_master);
}
@@ -44,6 +48,7 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
"suppress error message for non-symbolic (detached) refs"),
OPT_BOOLEAN('f', NULL, &fallback_regular_ref,
"fall back to showing as a regular ref"),
+ OPT_BOOLEAN('s', NULL, &shorten, "shorten ref output"),
OPT_STRING('m', NULL, &msg, "reason", "reason of the update"),
OPT_END(),
};
--
1.7.9.2.302.g3724c.dirty
^ permalink raw reply related
* [PATCH 2/2] builtin/symbolic-ref.c: add option to output shortened ref
From: Jan Krüger @ 2012-02-27 22:10 UTC (permalink / raw)
To: git
In scripts meant to generate user-consumable output, it can be helpful
to resolve a symbolic ref and output the result in a shortened form,
such as for use in shell prompts. Add a new -s option to allow this.
Signed-off-by: Jan Krüger <jk@jk.gs>
---
Documentation/git-symbolic-ref.txt | 6 +++++-
builtin/symbolic-ref.c | 5 +++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-symbolic-ref.txt b/Documentation/git-symbolic-ref.txt
index a05819b..7f108ce 100644
--- a/Documentation/git-symbolic-ref.txt
+++ b/Documentation/git-symbolic-ref.txt
@@ -8,7 +8,7 @@ git-symbolic-ref - Read and modify symbolic refs
SYNOPSIS
--------
[verse]
-'git symbolic-ref' [-q] [-f] [-m <reason>] <name> [<ref>]
+'git symbolic-ref' [-q] [-f] [-s] [-m <reason>] <name> [<ref>]
DESCRIPTION
-----------
@@ -38,6 +38,10 @@ OPTIONS
not a symbolic ref; instead output the SHA1 value referenced by
<name>.
+-s::
+ When showing the value of <name> as a symbolic ref, try to shorten the
+ value, e.g. from `refs/heads/master` to `master`.
+
-m::
Update the reflog for <name> with <reason>. This is valid only
when creating or updating a symbolic ref.
diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
index 2e0a86f..df8da11 100644
--- a/builtin/symbolic-ref.c
+++ b/builtin/symbolic-ref.c
@@ -9,6 +9,7 @@ static const char * const git_symbolic_ref_usage[] = {
};
static int fallback_regular_ref;
+static int shorten;
static void check_symref(const char *HEAD, int quiet)
{
@@ -32,6 +33,9 @@ static void check_symref(const char *HEAD, int quiet)
exit(1);
}
}
+ if (shorten)
+ refs_heads_master = shorten_unambiguous_ref(
+ refs_heads_master, 0);
puts(refs_heads_master);
}
@@ -44,6 +48,7 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
"suppress error message for non-symbolic (detached) refs"),
OPT_BOOLEAN('f', NULL, &fallback_regular_ref,
"fall back to showing as a regular ref"),
+ OPT_BOOLEAN('s', NULL, &shorten, "shorten ref output"),
OPT_STRING('m', NULL, &msg, "reason", "reason of the update"),
OPT_END(),
};
--
1.7.9.2.302.g3724c.dirty
^ permalink raw reply related
* Re: [PATCH 1/2] builtin/symbolic-ref.c: add option to fall back to normal ref
From: Junio C Hamano @ 2012-02-27 22:21 UTC (permalink / raw)
To: Jan Krüger; +Cc: git
In-Reply-To: <1330380536-9647-1-git-send-email-jk@jk.gs>
Jan Krüger <jk@jk.gs> writes:
> Frequently, people want to determine the current value of HEAD in
> scripts. However, there is no tool that can always output it, since "git
> symbolic-ref" will fail if HEAD isn't currently a symref, and other
> tools (e.g. "git rev-parse --symbolic-full-name") will also fail in
> one of HEAD's possible modes.
What is "the current value of HEAD"?
The symbolic-ref command is there for people who _care_ about the
distinction between a HEAD that points at a branch and a HEAD that points
directly at a commit. There is no room for the command to "fall back"
anywhere, as that will only introduce an unnecessary ambiguity to the
command whose sole purpose is to be able to tell them apart.
If the caller does not need to know if the HEAD is detached or not, and
wants to know what commit it points at, why is it insufficient to just use
rev-parse, e.g. "git rev-parse --verify HEAD"?
^ permalink raw reply
* Re: [PATCH 3/3] parse-options: remove PARSE_OPT_NEGHELP
From: René Scharfe @ 2012-02-27 22:26 UTC (permalink / raw)
To: Jeff King
Cc: git, Junio C Hamano, Bert Wesarg, Geoffrey Irving,
Johannes Schindelin, Pierre Habouzit
In-Reply-To: <20120227182504.GA1600@sigill.intra.peff.net>
Am 27.02.2012 19:25, schrieb Jeff King:
> On Sat, Feb 25, 2012 at 08:15:56PM +0100, René Scharfe wrote:
>
>> diff --git a/builtin/grep.c b/builtin/grep.c
>> index e4ea900..b151467 100644
>> --- a/builtin/grep.c
>> +++ b/builtin/grep.c
>> @@ -671,7 +671,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>> struct string_list path_list = STRING_LIST_INIT_NODUP;
>> int i;
>> int dummy;
>> - int use_index = 1;
>> + int no_index = 0;
>> enum {
>> pattern_type_unspecified = 0,
>> pattern_type_bre,
>> @@ -684,9 +684,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>> struct option options[] = {
>> OPT_BOOLEAN(0, "cached",&cached,
>> "search in index instead of in the work tree"),
>> - { OPTION_BOOLEAN, 0, "index",&use_index, NULL,
>> - "finds in contents not managed by git",
>> - PARSE_OPT_NOARG | PARSE_OPT_NEGHELP },
>> + OPT_BOOL(0, "no-index",&no_index,
>> + "finds in contents not managed by git"),
>> OPT_BOOLEAN(0, "untracked",&untracked,
>> "search in both tracked and untracked files"),
>> OPT_SET_INT(0, "exclude-standard",&opt_exclude,
>> @@ -851,7 +850,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>> break; /* nothing */
>> }
>>
>> - if (use_index&& !startup_info->have_repository)
>> + if (!no_index&& !startup_info->have_repository)
[Unrelated: The whitespace in the two lines above and before ampersands
in general was damaged by Thunderbird. First time I noticed.]
> Hmm. We usually try to avoid these sorts of double negations in the
> code, as they can often make the logic hard to read. In this case, it is
> not _so_ bad, because out of the 4 uses of use_index/no_index, only one
> is "!no_index", and it is in a relatively simple conditional.
The variable could be named "unmanaged", "external" or similar instead
of "no_index". The latter just matches the option name and thus was the
obvious first choice to me.
> But I do feel like the original was slightly easier to read, and that
> getting rid of NEGHELP is restricting how the developer can express the
> options.
>
> I think your original motivation was that NEGHELP lead to confusion
> where the name of the option does not match its description. Would it be
> better to simply be explicit that an option is a reversed boolean (i.e.,
> what the user specifies on the command line and what is in the code are
> naturally opposites). Like:
>
> OPT_REVERSE_BOOL(0, "no-index",&use_index,
> "finds in contents not managed by git"),
It's better than NEGHELP, but I find your use of two negations (REVERSE
and "no-") confusing. We don't need to invent new OPT_ types for this,
by the way, we can just do this:
OPT_NEGBIT(0, "no-index", &use_index,
"finds in contents not managed by git", 1),
It certainly shortens the patch.
> Using NEGHELP, the "reverse" is between the option name and the
> description, which is very subtle. Here it is between the option name
> and the variable, which is hopefully a little more explicit (especially
> with the big REVERSE in the macro name).
We have precedence for OPT_NEGBIT in grep, although the double negations
for -h and --full-name are required because both turn off bits that
other options turn on, while for --no-index it wouldn't be strictly
needed, as there is no option that overrules it except --index.
I don't care too much either way, though. The changes from patch 2 (the
no no-no one) are not restricted to OPT_BOOL.
> I dunno. Given that there are only two uses of NEGHELP, and that they
> don't come out too badly, I don't care _too_ much. But I have seen some
> really tortured logic with double-negations like this, and I'm concerned
> that a few months down the road somebody is going to want NEGHELP (or
> something similar) in a case where it actually does really impact
> readability.
I'm curious to see a case that can be solved better using NEGHELP, but
we can always add it back if we find such a beast. I'd much rather see
it go until then because of it's non-obvious semantics.
René
^ permalink raw reply
* Re: [PATCH] fsck: do not print dangling objects by default
From: Clemens Buchacher @ 2012-02-27 22:18 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20120227213304.GB19779@sigill.intra.peff.net>
On Mon, Feb 27, 2012 at 04:33:04PM -0500, Jeff King wrote:
> On Mon, Feb 27, 2012 at 10:13:16PM +0100, Clemens Buchacher wrote:
>
> > > Yes, that was certainly part of my pros-and-cons analysis. If you run
> > > "git fsck" without "--no-dangling" without reading the manual, you may
> > > get confused, but that is *not* the primary audience.
> >
> > It is not my only concern that users might be confused. I believe the
> > command prints a lot of useless messages, which is by itself a UI
> > deficiency. But even worse, those numerous messages tend to hide an
> > actual problem in a long scrollback buffer. Sometimes my scrollback
> > buffer is not even large enough and I have to re-run fsck (which is not
> > exactly a fast command), just so I can grep out the dangling blobs.
>
> Yeah, but doesn't adding "--no-dangling" solve that issue?
I can just as well use grep -v ^dangling.
^ permalink raw reply
* Re: [PATCH 2/2] builtin/symbolic-ref.c: add option to output shortened ref
From: Junio C Hamano @ 2012-02-27 22:28 UTC (permalink / raw)
To: Jan Krüger; +Cc: git
In-Reply-To: <1330380638-9738-1-git-send-email-jk@jk.gs>
Jan Krüger <jk@jk.gs> writes:
> In scripts meant to generate user-consumable output, it can be helpful
> to resolve a symbolic ref and output the result in a shortened form,
> such as for use in shell prompts. Add a new -s option to allow this.
I think this one (unlike 1/2) makes sense, but a single letter -s feels a
bit too vague. Always spelling in long option "--short" so that it
matches "%(refname:short)" in for-each-ref might be better, I would think.
Especially given that the expected use case is primarily in scripts not
from the command line, being more explicit and easier to read has value
over being short and easier to (mis)type.
^ permalink raw reply
* Re: [PATCH 1/2] builtin/symbolic-ref.c: add option to fall back to normal ref
From: Jan Krüger @ 2012-02-27 22:40 UTC (permalink / raw)
To: git
In-Reply-To: <7v1upf6hp5.fsf@alter.siamese.dyndns.org>
On 02/27/2012 11:21 PM, Junio C Hamano wrote:
> What is "the current value of HEAD"?
Well, essentially it contains a "type" and a "value". The type is either
"symbolic" or "not symbolic" and the value is a ref if HEAD is currently
symbolic or a SHA1 otherwise.
These definitions don't come from the glossary, obviously. It's just the
way I interpret HEAD. If you think the patch would be better off with a
different wording, I'd be happy for suggestions.
In any case, if you have the "value" without the "type", you can
actually derive the "type" from the "value": if it has the shape of a
SHA1, it was a direct ref; otherwise it was a symbolic ref (unless the
user has a ref that looks like a SHA1 but in that case some other tools
do funny things, too).
> The symbolic-ref command is there for people who _care_ about the
> distinction between a HEAD that points at a branch and a HEAD that points
> directly at a commit. There is no room for the command to "fall back"
> anywhere, as that will only introduce an unnecessary ambiguity to the
> command whose sole purpose is to be able to tell them apart.
That's why it's an optional thing.
> If the caller does not need to know if the HEAD is detached or not, and
> wants to know what commit it points at, why is it insufficient to just use
> rev-parse, e.g. "git rev-parse --verify HEAD"?
The use case here is having one convenient command to remember what is
currently checked out, and being able to go back to that later. "git
checkout" will do the right thing if you just give it "master" or a
complete SHA1, so all we need to remember is that.
"git rev-parse --verify HEAD" will only give us the SHA1, even if HEAD
isn't actually detached.
Incidentally, the -s switch in the second patch makes this even easier:
OLDHEAD=`git symbolic-ref -s -f HEAD`
# do stuff here that involves switching branches or detaching or
# whatever
git checkout $OLDHEAD
Without these patches, the shortest way I can think of to do the same
thing would be:
OLDHEAD=`git rev-parse --symbolic-full-name HEAD`
[ "$OLDHEAD" = "HEAD" ] && OLDHEAD=`git rev-parse HEAD`
# ...
-Jan
^ permalink raw reply
* Re: [PATCH 2/2] builtin/symbolic-ref.c: add option to output shortened ref
From: Jan Krüger @ 2012-02-27 22:46 UTC (permalink / raw)
To: git
In-Reply-To: <7vwr7752s8.fsf@alter.siamese.dyndns.org>
On 02/27/2012 11:28 PM, Junio C Hamano wrote:
> I think this one (unlike 1/2) makes sense, but a single letter -s feels a
> bit too vague. Always spelling in long option "--short" so that it
> matches "%(refname:short)" in for-each-ref might be better, I would think.
>
> Especially given that the expected use case is primarily in scripts not
> from the command line, being more explicit and easier to read has value
> over being short and easier to (mis)type.
Good point. If we can agree on what to do with the first patch, I'll
change that in the next iteration.
-Jan
^ permalink raw reply
* Re: [PATCH 2/2] builtin/symbolic-ref.c: add option to output shortened ref
From: Junio C Hamano @ 2012-02-27 23:04 UTC (permalink / raw)
To: Jan Krüger; +Cc: git
In-Reply-To: <4F4C07AD.3050404@jk.gs>
Jan Krüger <jk@jk.gs> writes:
> On 02/27/2012 11:28 PM, Junio C Hamano wrote:
>> I think this one (unlike 1/2) makes sense, but a single letter -s feels a
>> bit too vague. Always spelling in long option "--short" so that it
>> matches "%(refname:short)" in for-each-ref might be better, I would think.
>>
>> Especially given that the expected use case is primarily in scripts not
>> from the command line, being more explicit and easier to read has value
>> over being short and easier to (mis)type.
>
> Good point. If we can agree on what to do with the first patch, I'll
> change that in the next iteration.
Dropping the first patch would be my preference. I do not think this
change deserves to be taken hostage to it.
^ permalink raw reply
* Re: [PATCH 2/2] builtin/symbolic-ref.c: add option to output shortened ref
From: Junio C Hamano @ 2012-02-27 23:54 UTC (permalink / raw)
To: Jan Krüger; +Cc: git
In-Reply-To: <7vpqcz514d.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> ... I do not think this change deserves to be taken hostage to it.
So here is a trivial rebase of the patch.
The --short option should apply only when querying (-q also shares this, I
think), so I split the synopsis into two.
I was the guity one who named the variable refs_heads_master, hoping that
it would be easier to name it after representative value it would contain
(so that it would be clear for other code that may want to strip the
leading component what to expect), but now it has happened with your
patch, the variable name looks silly.
OPT_BOOLEAN() is deprecated; OPT_BOOL() has a much saner semantics for
most callers' needs and we should consider using it when adding new
options.
Perhaps we would also want some tests?
Documentation/git-symbolic-ref.txt | 7 ++++++-
builtin/symbolic-ref.c | 11 ++++++++---
2 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-symbolic-ref.txt b/Documentation/git-symbolic-ref.txt
index a45d4c4..89e7707 100644
--- a/Documentation/git-symbolic-ref.txt
+++ b/Documentation/git-symbolic-ref.txt
@@ -8,7 +8,8 @@ git-symbolic-ref - Read and modify symbolic refs
SYNOPSIS
--------
[verse]
-'git symbolic-ref' [-q] [-m <reason>] <name> [<ref>]
+'git symbolic-ref' [-m <reason>] <name> <ref>
+'git symbolic-ref' [-q] [--short] <name>
DESCRIPTION
-----------
@@ -33,6 +34,10 @@ OPTIONS
symbolic ref but a detached HEAD; instead exit with
non-zero status silently.
+--short::
+ When showing the value of <name> as a symbolic ref, try to shorten the
+ value, e.g. from `refs/heads/master` to `master`.
+
-m::
Update the reflog for <name> with <reason>. This is valid only
when creating or updating a symbolic ref.
diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
index 2ef5962..801d62e 100644
--- a/builtin/symbolic-ref.c
+++ b/builtin/symbolic-ref.c
@@ -8,13 +8,15 @@ static const char * const git_symbolic_ref_usage[] = {
NULL
};
+static int shorten;
+
static void check_symref(const char *HEAD, int quiet)
{
unsigned char sha1[20];
int flag;
- const char *refs_heads_master = resolve_ref_unsafe(HEAD, sha1, 0, &flag);
+ const char *refname = resolve_ref_unsafe(HEAD, sha1, 0, &flag);
- if (!refs_heads_master)
+ if (!refname)
die("No such ref: %s", HEAD);
else if (!(flag & REF_ISSYMREF)) {
if (!quiet)
@@ -22,7 +24,9 @@ static void check_symref(const char *HEAD, int quiet)
else
exit(1);
}
- puts(refs_heads_master);
+ if (shorten)
+ refname = shorten_unambiguous_ref(refname, 0);
+ puts(refname);
}
int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
@@ -32,6 +36,7 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
struct option options[] = {
OPT__QUIET(&quiet,
"suppress error message for non-symbolic (detached) refs"),
+ OPT_BOOL(0, "short", &shorten, "shorten ref output"),
OPT_STRING('m', NULL, &msg, "reason", "reason of the update"),
OPT_END(),
};
^ permalink raw reply related
* Re: [PATCH 3/3] parse-options: remove PARSE_OPT_NEGHELP
From: Jeff King @ 2012-02-28 0:34 UTC (permalink / raw)
To: René Scharfe
Cc: git, Junio C Hamano, Bert Wesarg, Geoffrey Irving,
Johannes Schindelin, Pierre Habouzit
In-Reply-To: <4F4C0308.2050804@lsrfire.ath.cx>
On Mon, Feb 27, 2012 at 11:26:16PM +0100, René Scharfe wrote:
> > OPT_REVERSE_BOOL(0, "no-index",&use_index,
> > "finds in contents not managed by git"),
>
> It's better than NEGHELP, but I find your use of two negations
> (REVERSE and "no-") confusing. We don't need to invent new OPT_
> types for this, by the way, we can just do this:
>
> OPT_NEGBIT(0, "no-index", &use_index,
> "finds in contents not managed by git", 1),
>
> It certainly shortens the patch.
Right. The point is that the code and the option want to look at the
conditional in two different ways. So you have to have negate it
_somewhere_. Either at point of use (your original patch), between the
option name and the variable name (what I wrote above), or using a
reversed help text (the original code before your patch).
I think we both agree that NEGHELP is the worst one. I have a slight
preference for doing the reversal at the point of the option definition,
if only because we know it happens at a single point, and it lets the
actual code (which is usually the trickier part) be more clear. But
beyond that, I think it is largely a matter of aesthetics.
> >I dunno. Given that there are only two uses of NEGHELP, and that they
> >don't come out too badly, I don't care _too_ much. But I have seen some
> >really tortured logic with double-negations like this, and I'm concerned
> >that a few months down the road somebody is going to want NEGHELP (or
> >something similar) in a case where it actually does really impact
> >readability.
>
> I'm curious to see a case that can be solved better using NEGHELP,
> but we can always add it back if we find such a beast. I'd much
> rather see it go until then because of it's non-obvious semantics.
Yeah, I should have spelled out my "or something similar" more. I'd
rather see something like NEGBIT above than NEGHELP; the latter is a bit
too subtle.
-Peff
^ permalink raw reply
* Re: [PATCH 03/11] cat-file: use streaming interface to print blobs
From: Nguyen Thai Ngoc Duy @ 2012-02-28 1:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vzkc49nnu.fsf@alter.siamese.dyndns.org>
2012/2/28 Junio C Hamano <gitster@pobox.com>:
>> + enum object_type type;
>> + unsigned long size;
>> + char *buffer = read_sha1_file(sha1, &type, &size);
>> + if (memcmp(buffer, "object ", 7) ||
>> + get_sha1_hex(buffer + 7, new_sha1))
>> + die("%s not a valid tag", sha1_to_hex(sha1));
>> + sha1 = new_sha1;
>> + free(buffer);
>> + }
>> +
>> + return streaming_write_sha1(1, 0, sha1, OBJ_BLOB, NULL);
>
> I do not think your previous refactoring added a fall-back codepath to the
> function you are calling here. In the original context, the caller of
> streaming_write_entry() made sure that the blob is suitable for streaming
> write by getting an istream, and called the function only when that is the
> case. Blobs unsuitable for streaming (e.g. an deltified object in a pack)
> were handled by the caller that decided not to call
> streaming_write_entry() with the conventional "read to core and then write
> it out" codepath.
>
> And I do not think your updated caller in cat_one_file() is equipped to do
> so at all.
>
> So it looks to me that this patch totally breaks the cat-file. What am I
> missing?
I think open_istream can deal with unsuitable for streaming objects
too. There's a fallback "incore" backend that does
read_sha1_file_extended.
--
Duy
^ permalink raw reply
* Re: [PATCH 00/11] Large blob fixes
From: Nguyen Thai Ngoc Duy @ 2012-02-28 1:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v7gz89kws.fsf@alter.siamese.dyndns.org>
2012/2/28 Junio C Hamano <gitster@pobox.com>:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> These patches make sure we avoid keeping whole blob in memory, at
>> least in common cases. Blob-only streaming code paths are opened to
>> accomplish that.
>
> Some in the series seem to be unrelated to the above, namely, the
> index-pack ones.
index-pack patches in this series can make "index-pack --verify"
worse, but it's already not so good. Will take the --verify patch out.
I will need better strategy than blindly skipping sha-1 collision test
when --verify is specified.
--
Duy
^ permalink raw reply
* Re: git-subtree Ready #2
From: Jakub Narebski @ 2012-02-28 2:04 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, David A. Greene, Avery Pennarun, git
In-Reply-To: <20120227212157.GA19779@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>
> Yeah, I don't see much point in rewriting. If parts of the history suck,
> then so be it. It's probably not that big to store. And while it's
> sometimes easier to fix bad commit messages when they are recent and in
> your memory (rather than trying to remember later what you meant to
> say), I think it is already too late for that. Any archaeology you do
> now to make good commit messages could probably just as easily be done
> if and when somebody actually needs the commit message later (emphasis
> on the "if" -- it's likely that nobody will care about most of the
> commit messages later at all).
Anyway we already have subtree merges if subsystem with bad error
messages -- see gitweb.
--
Jakub Narebski
^ permalink raw reply
* Re: Question about your comment on the git parable
From: Neal Kreitzinger @ 2012-02-28 2:41 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Federico Galassi, git
In-Reply-To: <201202261606.25599.jnareb@gmail.com>
On 2/26/2012 9:06 AM, Jakub Narebski wrote:
> On Sun, 26 Feb 2012, Federico Galassi wrote:
>> On 26/feb/2012, at 12:29, Jakub Narebski wrote:
>>
>>> Would you mind if this discussion was moved to git mailing
>>> list (git@vger.kernel.org), of course always with copy directly
>>> to you? There are people there that can answer your questions
>>> better.
>>
>> No problem.
>>
>>> On Sun, 26 Feb 2012, Federico Galassi wrote:
>>>> Hello, i think you're the author of these comments:
>>>> http://news.ycombinator.com/item?id=616610
>>>>
>>>> I'm doing educational work on git based on the parable (talks,
>>>> articles, etc..) and i'd like to improve on the real reason
>>>> for a staging area.
>>>>
>>>> My question basically is: why is it really needed for merging?
>>>> I mean, given the fictional git-like system of the parable,
>>>> if I need to merge 2 snapshots i could:
>>>>
>>>> 1) search the commit tree for a base point
> [...]
>>>> 2) compare the diffs between the snapshots and the base point snapshot
>>>> 3) if a conflict happens (change in the same line), just leave
>>>> something in the working dir to mark the conflict. For example,
>>>> keeping it simple, the system could reject a new commit until
>>>> the markers of the conflict are removed from the conflicting file.
>>>>
>>>> Couldn't it just work this way?
>>>
>>> Well, it could; that is how many if not most of other version control
>>> systems work.
>>>
>>>
>>> There are (at least!) three problems with that approach. First, sometimes
>>> it is not possible to "leave something in the working dir to mark the
>>> conflict". Take for example case where binary file (e.g. image) was
>>> changed, and textual 3-way diff file-merge algorithm wouldn't work.
>>>
>>> Second, what to do in the case of *tree-level* conflict, for example
>>> rename/rename conflict, where one side renamed file to different
>>> name (moved to different place) than the other side. There are no
>>> conflict markers for this...
>>>
>>> Third, what about false positives with detecting conflict markers,
>>> i.e. the case where "rejecting new commit until conflict markers are
>>> removed", for example AsciiDoc files can be falsely detected as having
>>> partial conflict markers, and of course test vectors for testing conflict
>>> would have to have conflict markers in them.
>>
>> Ok, it's clear to me that the markers in file approach is just a little
>> bit too simple. Do you see any concrete advantage in the staging area
>> compared to, say, tree conflict metadata in the working dir and maybe
>> a dedicated smart "resolve conflict" command?
>
> First, for such _local_ information working directory isn't the best place.
> What if you accidentally delete this? It is not and should not be
> committed to repository,so there is no way to undelete it, except redoing
> merge and losing all your progress so far in resolving merge conflicts.
> It is much better to put such information somewhere in administrative
> area[1] of repository.
>
> Second, if we have staging area where we store information about which
> files are tracked, and a bunch of per-file metadata like modification time
> for better performance, why not use it also for storing information about
> merge in progress?
>
> [1]: Name taken from "Version Control by Example" (free e-book) by
> Eric Sink.
>
>
> There is also a thing very specific to Git, namely that "git add" adds
> a current content of a file to object database of a repository (though
> with modern git there is also "git add --intent-to-add" which works
> like add-ing file in other version control systems)... and you have to
> store reference to newly created object somewhere so that it doesn't get
> garbage-collected.
>
>>>> Can you mention other situations in which the pattern "files to be added"
>>>> is either mandatory or really helpful?
>>>
>>> Note that any version control system must have a kind of proto-staging
>>> area to know which files are to be added in next commit.
>>>
>>> If you do
>>>
>>> $ scm add file.c
>>>
>>> then version control system must save somewhere that 'file.c' is to be
>>> tracked (to be added in next commit).
>>
>> Yes, the fictional vcs just tracked all the files in the working dir.
>> Being selective on which file to track is of course another interesting
>> feature.
>
> IRL it is a _necessary_ feature. One of more common, if not most common
> application of version control system is to manage source files for a
> computer program. And there you have object files, executables and other
> _generated_ files which shouldn't be put in version control, not to
> mention backups created by your editor / IDE (e.g. "*~" files in Unix
> world, "*.bak" files in MS Windows world).
>
> Not to mention files which you have added to working directory, but are
> not ready to be added to new commit.
>
In the google tech talk "Contributing to Git":
http://www.youtube.com/watch?v=j45cs5_nY2k , at the 44:00 min mark the
index is discussed. It notes that many scm's have an "index" but hide
it from you. Some of the advantages of git giving you access to the
index are also discussed.
v/r,
neal
^ permalink raw reply
* real origin of the name "git"
From: Neal Kreitzinger @ 2012-02-28 3:04 UTC (permalink / raw)
To: git
In regards to debate about the origin of the name "git", here is evidence
that Linus really did name it after himself in his own words:
(excerpt 23:36 - 24:19 minute mark, Computer History Museum presents: Linus
Torvalds, Origins of Linux, September 19, 2001):
http://www.youtube.com/watch?v=WVTWCPoUt8w
v/r,
neal
^ permalink raw reply
* [PATCH 1/2] index-pack: split second pass obj handling into own function
From: Nguyễn Thái Ngọc Duy @ 2012-02-28 4:36 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/index-pack.c | 31 ++++++++++++++++++-------------
1 files changed, 18 insertions(+), 13 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index dd1c5c9..918684f 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -682,6 +682,23 @@ static int compare_delta_entry(const void *a, const void *b)
objects[delta_b->obj_no].type);
}
+/*
+ * Second pass:
+ * - for all non-delta objects, look if it is used as a base for
+ * deltas;
+ * - if used as a base, uncompress the object and apply all deltas,
+ * recursively checking if the resulting object is used as a base
+ * for some more deltas.
+ */
+static void second_pass(struct object_entry *obj)
+{
+ struct base_data *base_obj = alloc_base_data();
+ base_obj->obj = obj;
+ base_obj->data = NULL;
+ find_unresolved_deltas(base_obj);
+ display_progress(progress, nr_resolved_deltas);
+}
+
/* Parse all objects and return the pack content SHA1 hash */
static void parse_pack_objects(unsigned char *sha1)
{
@@ -736,26 +753,14 @@ static void parse_pack_objects(unsigned char *sha1)
qsort(deltas, nr_deltas, sizeof(struct delta_entry),
compare_delta_entry);
- /*
- * Second pass:
- * - for all non-delta objects, look if it is used as a base for
- * deltas;
- * - if used as a base, uncompress the object and apply all deltas,
- * recursively checking if the resulting object is used as a base
- * for some more deltas.
- */
if (verbose)
progress = start_progress("Resolving deltas", nr_deltas);
for (i = 0; i < nr_objects; i++) {
struct object_entry *obj = &objects[i];
- struct base_data *base_obj = alloc_base_data();
if (is_delta_type(obj->type))
continue;
- base_obj->obj = obj;
- base_obj->data = NULL;
- find_unresolved_deltas(base_obj);
- display_progress(progress, nr_resolved_deltas);
+ second_pass(obj);
}
}
--
1.7.3.1.256.g2539c.dirty
^ permalink raw reply related
* [PATCH 2/2] index-pack: support multithreaded delta resolving
From: Nguyễn Thái Ngọc Duy @ 2012-02-28 4:36 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1330403813-2770-1-git-send-email-pclouds@gmail.com>
This puts delta resolving on each base on a separate thread, one base
cache per thread.
An experiment on a 24 core machine with git.git shows that performance
does not increase proportional to the number of cores. So by default,
we use maximum 3 cores.
$ /usr/bin/time ~/t/git index-pack --threads=1 -v --stdin < XXX.pack
Receiving objects: 100% (146564/146564), 53.99 MiB | 17.47 MiB/s, done.
Resolving deltas: 100% (109205/109205), done.
pack d5471e8365717a5812cbc81ec7277cb697a80f08
11.58user 0.37system 0:12.04elapsed 99%CPU (0avgtext+0avgdata 375088maxresident)k
0inputs+118592outputs (0major+56894minor)pagefaults 0swaps
$ ... --threads=2 ...
14.58user 0.47system 0:09.99elapsed 150%CPU (0avgtext+0avgdata 411536maxresident)k
0inputs+118592outputs (0major+79961minor)pagefaults 0swaps
$ ... --threads=3 ...
14.36user 0.64system 0:08.12elapsed 184%CPU (0avgtext+0avgdata 393312maxresident)k
0inputs+118592outputs (0major+50998minor)pagefaults 0swaps
$ ... --threads=4 ...
15.81user 0.71system 0:08.17elapsed 202%CPU (0avgtext+0avgdata 419152maxresident)k
0inputs+118592outputs (0major+54907minor)pagefaults 0swaps
$ ... --threads=5 ...
14.76user 0.72system 0:07.06elapsed 219%CPU (0avgtext+0avgdata 414112maxresident)k
0inputs+118592outputs (0major+59547minor)pagefaults 0swaps
$ ... --threads=8 ...
15.98user 0.81system 0:07.71elapsed 217%CPU (0avgtext+0avgdata 429904maxresident)k
0inputs+118592outputs (0major+66221minor)pagefaults 0swaps
$ ... --threads=12 ...
15.81user 0.74system 0:09.60elapsed 172%CPU (0avgtext+0avgdata 442336maxresident)k
0inputs+118592outputs (0major+61353minor)pagefaults 0swaps
$ ... --threads=16 ...
15.41user 0.57system 0:11.62elapsed 137%CPU (0avgtext+0avgdata 451728maxresident)k
0inputs+118592outputs (0major+63569minor)pagefaults 0swaps
$ ... --threads=24 ...
15.84user 0.63system 0:12.83elapsed 128%CPU (0avgtext+0avgdata 475728maxresident)k
0inputs+118592outputs (0major+58013minor)pagefaults 0swaps
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
On linux-2.6.git, 2 cores, real time from 3m18 went down to around 2m.
Documentation/git-index-pack.txt | 10 ++
Makefile | 2 +-
builtin/index-pack.c | 188 ++++++++++++++++++++++++++++++++++----
3 files changed, 180 insertions(+), 20 deletions(-)
diff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt
index 909687f..39e6d0d 100644
--- a/Documentation/git-index-pack.txt
+++ b/Documentation/git-index-pack.txt
@@ -74,6 +74,16 @@ OPTIONS
--strict::
Die, if the pack contains broken objects or links.
+--threads=<n>::
+ Specifies the number of threads to spawn when resolving
+ deltas. This requires that index-pack be compiled with
+ pthreads otherwise this option is ignored with a warning.
+ This is meant to reduce packing time on multiprocessor
+ machines. The required amount of memory for the delta search
+ window is however multiplied by the number of threads.
+ Specifying 0 will cause git to auto-detect the number of CPU's
+ and use maximum 3 threads.
+
Note
----
diff --git a/Makefile b/Makefile
index 1fb1705..5fae875 100644
--- a/Makefile
+++ b/Makefile
@@ -2159,7 +2159,7 @@ builtin/branch.o builtin/checkout.o builtin/clone.o builtin/reset.o branch.o tra
builtin/bundle.o bundle.o transport.o: bundle.h
builtin/bisect--helper.o builtin/rev-list.o bisect.o: bisect.h
builtin/clone.o builtin/fetch-pack.o transport.o: fetch-pack.h
-builtin/grep.o builtin/pack-objects.o transport-helper.o thread-utils.o: thread-utils.h
+builtin/index-pack.o builtin/grep.o builtin/pack-objects.o transport-helper.o thread-utils.o: thread-utils.h
builtin/send-pack.o transport.o: send-pack.h
builtin/log.o builtin/shortlog.o: shortlog.h
builtin/prune.o builtin/reflog.o reachable.o: reachable.h
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 918684f..e331f23 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -9,6 +9,7 @@
#include "progress.h"
#include "fsck.h"
#include "exec_cmd.h"
+#include "thread-utils.h"
static const char index_pack_usage[] =
"git index-pack [-v] [-o <index-file>] [--keep | --keep=<msg>] [--verify] [--strict] (<pack-file> | --stdin [--fix-thin] [<pack-file>])";
@@ -38,6 +39,15 @@ struct base_data {
int ofs_first, ofs_last;
};
+struct thread_local {
+#ifndef NO_PTHREADS
+ pthread_t thread;
+#endif
+ struct base_data *base_cache;
+ size_t base_cache_used;
+ int nr_resolved_deltas;
+};
+
/*
* Even if sizeof(union delta_base) == 24 on 64-bit archs, we really want
* to memcmp() only the first 20 bytes.
@@ -54,11 +64,12 @@ struct delta_entry {
static struct object_entry *objects;
static struct delta_entry *deltas;
-static struct base_data *base_cache;
-static size_t base_cache_used;
+static struct thread_local *thread_data;
static int nr_objects;
+static int nr_processed;
static int nr_deltas;
static int nr_resolved_deltas;
+static int nr_threads;
static int from_stdin;
static int strict;
@@ -75,6 +86,42 @@ static git_SHA_CTX input_ctx;
static uint32_t input_crc32;
static int input_fd, output_fd, pack_fd;
+#ifndef NO_PTHREADS
+
+static pthread_mutex_t read_mutex;
+#define read_lock() pthread_mutex_lock(&read_mutex)
+#define read_unlock() pthread_mutex_unlock(&read_mutex)
+
+static pthread_mutex_t work_mutex;
+#define work_lock() pthread_mutex_lock(&work_mutex)
+#define work_unlock() pthread_mutex_unlock(&work_mutex)
+
+/*
+ * Mutex and conditional variable can't be statically-initialized on Windows.
+ */
+static void init_thread(void)
+{
+ init_recursive_mutex(&read_mutex);
+ pthread_mutex_init(&work_mutex, NULL);
+}
+
+static void cleanup_thread(void)
+{
+ pthread_mutex_destroy(&read_mutex);
+ pthread_mutex_destroy(&work_mutex);
+}
+
+#else
+
+#define read_lock()
+#define read_unlock()
+
+#define work_lock()
+#define work_unlock()
+
+#endif
+
+
static int mark_link(struct object *obj, int type, void *data)
{
if (!obj)
@@ -223,6 +270,18 @@ static NORETURN void bad_object(unsigned long offset, const char *format, ...)
die("pack has bad object at offset %lu: %s", offset, buf);
}
+static struct thread_local *get_thread_data()
+{
+#ifndef NO_PTHREADS
+ int i;
+ pthread_t self = pthread_self();
+ for (i = 1; i < nr_threads; i++)
+ if (self == thread_data[i].thread)
+ return &thread_data[i];
+#endif
+ return &thread_data[0];
+}
+
static struct base_data *alloc_base_data(void)
{
struct base_data *base = xmalloc(sizeof(struct base_data));
@@ -237,15 +296,16 @@ static void free_base_data(struct base_data *c)
if (c->data) {
free(c->data);
c->data = NULL;
- base_cache_used -= c->size;
+ get_thread_data()->base_cache_used -= c->size;
}
}
static void prune_base_data(struct base_data *retain)
{
struct base_data *b;
- for (b = base_cache;
- base_cache_used > delta_base_cache_limit && b;
+ struct thread_local *data = get_thread_data();
+ for (b = data->base_cache;
+ data->base_cache_used > delta_base_cache_limit && b;
b = b->child) {
if (b->data && b != retain)
free_base_data(b);
@@ -257,22 +317,23 @@ static void link_base_data(struct base_data *base, struct base_data *c)
if (base)
base->child = c;
else
- base_cache = c;
+ get_thread_data()->base_cache = c;
c->base = base;
c->child = NULL;
if (c->data)
- base_cache_used += c->size;
+ get_thread_data()->base_cache_used += c->size;
prune_base_data(c);
}
static void unlink_base_data(struct base_data *c)
{
- struct base_data *base = c->base;
+ struct base_data *base;
+ base = c->base;
if (base)
base->child = NULL;
else
- base_cache = NULL;
+ get_thread_data()->base_cache = NULL;
free_base_data(c);
}
@@ -461,19 +522,24 @@ static void sha1_object(const void *data, unsigned long size,
enum object_type type, unsigned char *sha1)
{
hash_sha1_file(data, size, typename(type), sha1);
+ read_lock();
if (has_sha1_file(sha1)) {
void *has_data;
enum object_type has_type;
unsigned long has_size;
has_data = read_sha1_file(sha1, &has_type, &has_size);
+ read_unlock();
if (!has_data)
die("cannot read existing object %s", sha1_to_hex(sha1));
if (size != has_size || type != has_type ||
memcmp(data, has_data, size) != 0)
die("SHA1 COLLISION FOUND WITH %s !", sha1_to_hex(sha1));
free(has_data);
- }
+ } else
+ read_unlock();
+
if (strict) {
+ read_lock();
if (type == OBJ_BLOB) {
struct blob *blob = lookup_blob(sha1);
if (blob)
@@ -507,6 +573,7 @@ static void sha1_object(const void *data, unsigned long size,
}
obj->flags |= FLAG_CHECKED;
}
+ read_unlock();
}
}
@@ -552,7 +619,7 @@ static void *get_base_data(struct base_data *c)
if (!delta_nr) {
c->data = get_data_from_pack(obj);
c->size = obj->size;
- base_cache_used += c->size;
+ get_thread_data()->base_cache_used += c->size;
prune_base_data(c);
}
for (; delta_nr > 0; delta_nr--) {
@@ -568,7 +635,7 @@ static void *get_base_data(struct base_data *c)
free(raw);
if (!c->data)
bad_object(obj->idx.offset, "failed to apply delta");
- base_cache_used += c->size;
+ get_thread_data()->base_cache_used += c->size;
prune_base_data(c);
}
free(delta);
@@ -596,7 +663,7 @@ static void resolve_delta(struct object_entry *delta_obj,
bad_object(delta_obj->idx.offset, "failed to apply delta");
sha1_object(result->data, result->size, delta_obj->real_type,
delta_obj->idx.sha1);
- nr_resolved_deltas++;
+ get_thread_data()->nr_resolved_deltas++;
}
static struct base_data *find_unresolved_deltas_1(struct base_data *base,
@@ -696,7 +763,35 @@ static void second_pass(struct object_entry *obj)
base_obj->obj = obj;
base_obj->data = NULL;
find_unresolved_deltas(base_obj);
- display_progress(progress, nr_resolved_deltas);
+}
+
+static void *threaded_second_pass(void *arg)
+{
+ struct thread_local *data = get_thread_data();
+ for (;;) {
+ int i, nr = 16;
+ work_lock();
+ nr_resolved_deltas += data->nr_resolved_deltas;
+ display_progress(progress, nr_resolved_deltas);
+ data->nr_resolved_deltas = 0;
+ while (nr_processed < nr_objects &&
+ is_delta_type(objects[nr_processed].type))
+ nr_processed++;
+ if (nr_processed >= nr_objects) {
+ work_unlock();
+ break;
+ }
+ i = nr_processed;
+ nr_processed += nr;
+ work_unlock();
+
+ for (; nr && i < nr_objects; i++, nr--) {
+ if (is_delta_type(objects[i].type))
+ continue;
+ second_pass(&objects[i]);
+ }
+ }
+ return NULL;
}
/* Parse all objects and return the pack content SHA1 hash */
@@ -755,13 +850,30 @@ static void parse_pack_objects(unsigned char *sha1)
if (verbose)
progress = start_progress("Resolving deltas", nr_deltas);
- for (i = 0; i < nr_objects; i++) {
- struct object_entry *obj = &objects[i];
- if (is_delta_type(obj->type))
- continue;
- second_pass(obj);
+ nr_processed = 0;
+#ifndef NO_PTHREADS
+ if (nr_threads > 1) {
+ init_thread();
+ for (i = 1; i < nr_threads; i++) {
+ int ret = pthread_create(&thread_data[i].thread, NULL,
+ threaded_second_pass, NULL);
+ if (ret)
+ die("unable to create thread: %s", strerror(ret));
+ }
+ for (i = 1; i < nr_threads; i++) {
+ pthread_join(thread_data[i].thread, NULL);
+ thread_data[i].thread = 0;
+ }
+ cleanup_thread();
+
+ /* stop get_thread_data() from looking up beyond the
+ first item, when fix_unresolved_deltas() runs */
+ nr_threads = 1;
+ return;
}
+#endif
+ threaded_second_pass(thread_data);
}
static int write_compressed(struct sha1file *f, void *in, unsigned int size)
@@ -967,6 +1079,17 @@ static int git_index_pack_config(const char *k, const char *v, void *cb)
die("bad pack.indexversion=%"PRIu32, opts->version);
return 0;
}
+ if (!strcmp(k, "pack.threads")) {
+ nr_threads = git_config_int(k, v);
+ if (nr_threads < 0)
+ die("invalid number of threads specified (%d)",
+ nr_threads);
+#ifdef NO_PTHREADS
+ if (nr_threads != 1)
+ warning("no threads support, ignoring %s", k);
+#endif
+ return 0;
+ }
return git_default_config(k, v, cb);
}
@@ -1125,6 +1248,16 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
keep_msg = "";
} else if (!prefixcmp(arg, "--keep=")) {
keep_msg = arg + 7;
+ } else if (!prefixcmp(arg, "--threads=")) {
+ char *end;
+ nr_threads = strtoul(arg+10, &end, 0);
+ if (!arg[10] || *end || nr_threads < 0)
+ usage(index_pack_usage);
+#ifdef NO_PTHREADS
+ if (nr_threads != 1)
+ warning("no threads support, "
+ "ignoring %s", arg);
+#endif
} else if (!prefixcmp(arg, "--pack_header=")) {
struct pack_header *hdr;
char *c;
@@ -1196,6 +1329,23 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
if (strict)
opts.flags |= WRITE_IDX_STRICT;
+#ifndef NO_PTHREADS
+ if (!nr_threads) {
+ nr_threads = online_cpus();
+ /* An experiment showed that more threads does not mean faster */
+ if (nr_threads > 3)
+ nr_threads = 3;
+ }
+ /* reserve thread_data[0] for the main thread */
+ if (nr_threads > 1)
+ nr_threads++;
+#else
+ if (nr_threads != 1)
+ warning("no threads support, ignoring --threads");
+ nr_threads = 1;
+#endif
+ thread_data = xcalloc(nr_threads, sizeof(*thread_data));
+
curr_pack = open_pack_file(pack_name);
parse_pack_header();
objects = xcalloc(nr_objects + 1, sizeof(struct object_entry));
--
1.7.3.1.256.g2539c.dirty
^ permalink raw reply related
* Incremental updates to What's cooking
From: Junio C Hamano @ 2012-02-28 6:53 UTC (permalink / raw)
To: git
Cc: Zbigniew Jędrzejewski-Szmek,
Nguyễn Thái Ngọc Duy, René Scharfe,
Jeff King, Clemens Buchacher, Jakub Narebski
It is a bit too much to keep sending the full report every night, so I'll
send out some highlights since issue #09 of this month for tonight.
Short term plans
----------------
Will merge to "master".
+ jb/required-filter 02-17/02-26 #1
+ ph/cherry-pick-advice-refinement 02-22/02-26 #1
+ pj/completion-remote-set-url-branches 02-22/02-26 #2
Will merge to "next"
- cn/maint-branch-with-bad 02-27 #1
- jk/symbolic-ref-short 02-27 #1
Will merge to "next", perhaps?
- jn/gitweb-hilite-regions 02-26 #4
- th/git-diffall 02-27 #1
Notable topics that may need more discussion
--------------------------------------------
I will not list the stalled topics here and harrass owners of them here.
* zj/diff-stat-dyncol (2012-02-27) 11 commits
- diff --stat: add config option to limit graph width
- diff --stat: enable limiting of the graph part
- diff --stat: add a test for output with COLUMNS=40
- diff --stat: use a maximum of 5/8 for the filename part
- merge --stat: use the full terminal width
- log --stat: use the full terminal width
- show --stat: use the full terminal width
- diff --stat: use the full terminal width
- diff --stat: tests for long filenames and big change counts
- t4014: addtional format-patch test vectors
- Merge branches zj/decimal-width, zj/term-columns and jc/diff-stat-scaler
I resurrected the additional tests for format-patch from an earlier round,
as it illustrates the behaviour change brought by "5/8 split" very well.
* nd/columns (2012-02-26) 11 commits
- tag: add --column
- column: support piping stdout to external git-column process
- status: add --column
- branch: add --column
- help: reuse print_columns() for help -a
- column: add column.ui for default column output settings
- column: support columns with different widths
- column: add columnar layout
- Stop starting pager recursively
- Add git-column and column mode parsing
- column: add API to print items in columns
Comments by Ramsay on the parsing code seemed reasonable.
* cb/fsck-squelch-dangling (2012-02-26) 1 commit
- fsck: do not print dangling objects by default
Introduces "fsck --dangling" and removes the output for dangling objects
from the default output.
The first patch to advance this topic should add --[no-]dangling option
and update the documentation to illustrate its use. On top of that, if we
were to change the default not to show the dangling objects, deprecation
patches that span longer timeperiod need to be built, but the follow-on
patches and the default change may not be necessary, given that the
command is "fsck".
* rs/no-no-no-parseopt (2012-02-26) 3 commits
- parse-options: remove PARSE_OPT_NEGHELP
- parse-options: allow positivation of options starting, with no-
- test-parse-options: convert to OPT_BOOL()
Options that use PARSE_OPT_NEGHELP needed to word their help text in
a strange way, and this gets rid of the uses of them.
I tend to agree with Peff that REVERSE_BOOL or NEGBIT based solution may
be more readable in the longer term. As the options that used the funny
NEGHELP are limited, the difference between two approaches may not matter
too much, but then that argues against fixing the funny help messages,
so...
^ permalink raw reply
* [PATCH] commit: allow {--amend|-c foo} when {HEAD|foo} has empty message
From: Thomas Rast @ 2012-02-28 8:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Because --amend (-c foo) internally load the message from HEAD (foo,
resp.) using the same code paths as -C, they erroneously refuse to
work at all when the message of HEAD (foo) is empty.
Remove the corresponding check under --amend and -c.
None of this behavior was ever tested (not even for -C empty_message),
so we add a whole batch of new tests.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
Noticed while helping "starlays" on IRC. It's possible to work around
the bug by first giving HEAD a non-empty commit message from the
command line, as in
git commit --amend -mfoo
git commit --amend
I haven't really checked, but from an irresponsibly quick glance it
looks like this bug has always been there in the C version (that is,
since f5bbc322).
builtin/commit.c | 2 +-
t/t7501-commit.sh | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 67 insertions(+), 1 deletion(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 3714582..45a57af 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -690,7 +690,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
hook_arg1 = "message";
} else if (use_message) {
buffer = strstr(use_message_buffer, "\n\n");
- if (!buffer || buffer[2] == '\0')
+ if (!amend && !edit_message && (!buffer || buffer[2] == '\0'))
die(_("commit has empty message"));
strbuf_add(&sb, buffer + 2, strlen(buffer + 2));
hook_arg1 = "commit";
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 8bb3833..6ab7712 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -473,4 +473,70 @@ test_expect_success 'amend can copy notes' '
'
+test_expect_success 'amend on empty commit message' '
+
+ echo bar > bar &&
+ git add bar &&
+ test_tick &&
+ git commit --allow-empty-message -m "" &&
+ git tag empty_message &&
+ git commit --amend -mnonempty &&
+ git cat-file commit HEAD | grep nonempty
+
+'
+
+test_expect_success 'amend with editor on empty commit message' '
+
+ git reset --hard empty_message &&
+ cat >editor <<-\EOF &&
+ #!/bin/sh
+ echo nonempty_one >"$1"
+ EOF
+ chmod 755 editor &&
+ EDITOR=./editor git commit --amend &&
+ git cat-file commit HEAD | grep nonempty_one
+
+'
+
+test_expect_success '--amend -C empty_message fails' '
+
+ test_commit nonempty &&
+ test_must_fail git commit --amend -C empty_message
+
+'
+
+test_expect_success '-C empty_message fails' '
+
+ echo 1 > bar &&
+ git add bar &&
+ test_must_fail git commit --amend -C empty_message
+
+'
+
+test_expect_success '--amend -c empty_message works' '
+
+ cat >editor <<-\EOF &&
+ #!/bin/sh
+ echo nonempty_two >"$1"
+ EOF
+ chmod 755 editor &&
+ EDITOR=./editor git commit --amend -c empty_message &&
+ git cat-file commit HEAD | grep nonempty_two
+
+'
+
+test_expect_success '-c empty_message works' '
+
+ echo 2 > bar &&
+ git add bar &&
+ cat >editor <<-\EOF &&
+ #!/bin/sh
+ echo nonempty_three >"$1"
+ EOF
+ chmod 755 editor &&
+ EDITOR=./editor git commit -c empty_message &&
+ git cat-file commit HEAD | grep nonempty_three
+
+'
+
test_done
--
1.7.9.2.467.g7fee4
^ permalink raw reply related
* Re: [PATCH] commit: allow {--amend|-c foo} when {HEAD|foo} has empty message
From: Jeff King @ 2012-02-28 9:05 UTC (permalink / raw)
To: Thomas Rast; +Cc: Junio C Hamano, git
In-Reply-To: <8529824c8569a8a0b4c4caf3a562750925758e74.1330419275.git.trast@student.ethz.ch>
On Tue, Feb 28, 2012 at 09:57:05AM +0100, Thomas Rast wrote:
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 3714582..45a57af 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -690,7 +690,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> hook_arg1 = "message";
> } else if (use_message) {
> buffer = strstr(use_message_buffer, "\n\n");
> - if (!buffer || buffer[2] == '\0')
> + if (!amend && !edit_message && (!buffer || buffer[2] == '\0'))
> die(_("commit has empty message"));
Hmm. So "buffer" used to never be NULL (because we would die if it is),
and now we might not die if we are doing an amend, no? And the next line
is:
> strbuf_add(&sb, buffer + 2, strlen(buffer + 2));
Doesn't this need to handle the case of NULL buffer (i.e., when it does
not already have "\n\n" in it)?
-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