* 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
* Tilde spec - befuzzled
From: Luke Diamand @ 2012-02-28 9:07 UTC (permalink / raw)
To: Git List
The documentation for caret and tilde specs is making my head hurt, even
though they always _do_ exactly what I want. And I thought I understood
them until I read more carefully.
A suffix '{caret}' to a revision parameter means the first parent of
that commit object. '{caret}<n>' means the <n>th parent (i.e.
'<rev>{caret}'
So far, so good.
'<rev>{tilde}<n>', e.g. 'master{tilde}3'::
A suffix '{tilde}<n>' to a revision parameter means the commit
object that is the <n>th generation grand-parent of the named
commit object, following only the first parents.
Hang on, *grand*-parents?
So HEAD~1 won't give me the *parent* commit of HEAD, but the
*grandparent* commit of HEAD (following only the first parents) ?
How do I get to the *parent* commit of HEAD?
Does that mean that HEAD~ != HEAD^
And why does HEAD~1 always look exactly what I would naively call the
'parent' of HEAD?
I'm pretty sure I'm missing something very obvious. I think it must
confuse other people as well though, as it's quite easy to find webpages
around that claim that tildes give the _parent_ commit.
Thanks
Luke
^ permalink raw reply
* Re: [PATCH] commit: allow {--amend|-c foo} when {HEAD|foo} has empty message
From: Jeff King @ 2012-02-28 9:14 UTC (permalink / raw)
To: Thomas Rast; +Cc: Junio C Hamano, git
In-Reply-To: <20120228090540.GB5757@sigill.intra.peff.net>
On Tue, Feb 28, 2012 at 04:05:40AM -0500, Jeff King wrote:
> > } 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)?
I wrote that after looking at just your patch. Looking at
builtin/commit.c, I think use_message_buffer will always be a re-encoded
commit object. So that strstr should _never_ fail unless the commit
object is corrupt. So the right thing is probably:
buffer = strstr(use_message_buffer, "\n\n");
if (!buffer)
die(_("commit object has invalid format"));
if (!amend && !edit_message && buffer[2] == '\0))
die(_("commit has empty message"));
-Peff
^ permalink raw reply
* Re: [PATCH] commit: allow {--amend|-c foo} when {HEAD|foo} has empty message
From: Thomas Rast @ 2012-02-28 9:20 UTC (permalink / raw)
To: Jeff King; +Cc: Thomas Rast, Junio C Hamano, git
In-Reply-To: <20120228091422.GC5757@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Feb 28, 2012 at 04:05:40AM -0500, Jeff King wrote:
>
>> > } 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)?
>
> I wrote that after looking at just your patch. Looking at
> builtin/commit.c, I think use_message_buffer will always be a re-encoded
> commit object. So that strstr should _never_ fail unless the commit
> object is corrupt. So the right thing is probably:
>
> buffer = strstr(use_message_buffer, "\n\n");
> if (!buffer)
> die(_("commit object has invalid format"));
> if (!amend && !edit_message && buffer[2] == '\0))
> die(_("commit has empty message"));
Interesting. After I got your mail, I started poking around, and it
turns out we're in a funny situation here. I did this:
$ git cat-file commit HEAD
tree 205f6b799e7d5c2524468ca006a0131aa57ecce7
parent c4938d8e6d23e3a8fe10e6466ecd827662c14846
author Thomas Rast <trast@student.ethz.ch> 1330417798 +0100
committer Thomas Rast <trast@student.ethz.ch> 1330417798 +0100
$ git cat-file commit HEAD | grep -v '^$' | git hash-object -w -t commit --stdin
f68fcc1996173a9e04bd45d42abbb7c85c79546d
$ git reset --hard f68fcc1996173a9e04bd45d42abbb7c85c79546d
So now I'm at a commit which does not have that \n\n. And poking around
gives a very confusing picture:
$ git fsck
Checking object directories: 100% (256/256), done.
$ git show
Segmentation fault
$ git log
Segmentation fault
$ git format-patch -1
0001-.txt
$ cat 0001-.txt # no output!
$ git bundle create test.bdl HEAD
error: rev-list died of signal 11
error: rev-list died
$ git rev-list HEAD
f68fcc1996173a9e04bd45d42abbb7c85c79546d
c4938d8e6d23e3a8fe10e6466ecd827662c14846
$ git rev-list --pretty=oneline HEAD
Segmentation fault
So either there's a lot to be fixed, or fsck needs to catch this.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: Tilde spec - befuzzled
From: Andreas Ericsson @ 2012-02-28 9:50 UTC (permalink / raw)
To: Luke Diamand; +Cc: Git List
In-Reply-To: <4F4C995D.9000504@diamand.org>
On 02/28/2012 10:07 AM, Luke Diamand wrote:
> The documentation for caret and tilde specs is making my head hurt, even though they always _do_ exactly what I want. And I thought I understood them until I read more carefully.
>
> A suffix '{caret}' to a revision parameter means the first parent of
> that commit object. '{caret}<n>' means the <n>th parent (i.e.
> '<rev>{caret}'
>
> So far, so good.
>
> '<rev>{tilde}<n>', e.g. 'master{tilde}3'::
> A suffix '{tilde}<n>' to a revision parameter means the commit
> object that is the <n>th generation grand-parent of the named
> commit object, following only the first parents.
>
> Hang on, *grand*-parents?
>
Replace "grand-parent" with "ancestor" and your head might hurt a
little less. Caret only ever describes parent, while tilde can
describe either parent or n'th generation grand-parent. Since parent
and grand-parent are all ancestors, that would be a better term.
I believe this stems from the fact that most git-gurus use a single
caret to specify 'parent' and only go to the tilde when they want
grandparents. It might be because caret was implemented first, or
because of english keyboard layout. On swedish keyboards, using tilde
is far more convenient than using caret, so that's what I mostly use
when I want the parent of a single-parent commit (it requires only
one hand and no thumb-acrobatics even on my laptop).
--
Andreas Ericsson andreas.ericsson@op5.se
OP5 AB www.op5.se
Tel: +46 8-230225 Fax: +46 8-230231
Considering the successes of the wars on alcohol, poverty, drugs and
terror, I think we should give some serious thought to declaring war
on peace.
^ permalink raw reply
* [PATCH v2] commit: allow {--amend|-c foo} when {HEAD|foo} has empty message
From: Thomas Rast @ 2012-02-28 10:36 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20120228091422.GC5757@sigill.intra.peff.net>
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.
Reported-by: Lazar Florentin <florentin.lazar@gmail.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
Like the last version, plus Peff's guard for the invalid commit
format. I also received Lazar (starlay) 's identity for the
attribution.
builtin/commit.c | 4 +++-
t/t7501-commit.sh | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 69 insertions(+), 1 deletion(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 3714582..5e9a832 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -690,7 +690,9 @@ 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 (!buffer)
+ die(_("commit object has invalid format"));
+ if (!amend && !edit_message && 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
* [RFC PATCH 2a] pretty: detect missing \n\n in commit message
From: Thomas Rast @ 2012-02-28 10:37 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <010901fbfffe0f806bb19d556ebc1e512a4697f4.1330425111.git.trast@student.ethz.ch>
get_header()'s exit condition is finding the \n\n that separates the
commit header from its message. If such a double newline is not
present, it segfaults. Catch this case and die().
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
This would be the minimal fix to the pretty machinery so that 'git
rev-list --pretty=something HEAD' works when there are such broken
commits.
If 2b goes in, there isn't really any point as we would never get this
far on such a commit.
pretty.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/pretty.c b/pretty.c
index 8688b8f..b7f097d 100644
--- a/pretty.c
+++ b/pretty.c
@@ -440,7 +440,10 @@ static char *get_header(const struct commit *commit, const char *key)
const char *line = commit->buffer;
for (;;) {
- const char *eol = strchr(line, '\n'), *next;
+ const char *eol, *next;
+ if (!line)
+ die (_("malformed commit object: no separating \\n\\n?"));
+ eol = strchr(line, '\n');
if (line == eol)
return NULL;
--
1.7.9.2.467.g7fee4
^ permalink raw reply related
* [RFC PATCH 2b] parse_commit: refuse to load commit without \n\n separator
From: Thomas Rast @ 2012-02-28 10:37 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <010901fbfffe0f806bb19d556ebc1e512a4697f4.1330425111.git.trast@student.ethz.ch>
A commit object consists of a series of headers, terminated by a blank
line (\n\n), followed by the message.
Refuse to parse a commit which does not contain this \n\n. This also
lets git-fsck detect the breakage.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
This is the more drastic approach compared to 2a. Since the code in
pretty.c was broken given such input, I would favor this. It does,
however, remove the possibility of fixing up such a broken commit with
the tools git provides (other than resorting to git-hash-object and
git-replace).
commit.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/commit.c b/commit.c
index 4b39c19..502675c 100644
--- a/commit.c
+++ b/commit.c
@@ -265,6 +265,9 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s
if (get_sha1_hex(bufptr + 5, parent) < 0)
return error("bad tree pointer in commit %s",
sha1_to_hex(item->object.sha1));
+ if (!memmem(bufptr, size, "\n\n", 2))
+ return error("commit object %s is missing \\n\\n separator",
+ sha1_to_hex(item->object.sha1));
item->tree = lookup_tree(parent);
bufptr += 46; /* "tree " + "hex sha1" + "\n" */
pptr = &item->parents;
--
1.7.9.2.467.g7fee4
^ permalink raw reply related
* Re: [PATCH v6 02/11] Add git-column and column mode parsing
From: Nguyen Thai Ngoc Duy @ 2012-02-28 11:00 UTC (permalink / raw)
To: Ramsay Jones; +Cc: git, Junio C Hamano
In-Reply-To: <4F4BE300.2060302@ramsay1.demon.co.uk>
2012/2/28 Ramsay Jones <ramsay@ramsay1.demon.co.uk>:
> 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 ...
>
>> + }
>> + }
>> +
>> + 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 ...)
Not in this patch, no. The table is extended later on with more modes
and options.
>
>> + 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);
>> +}
>> +
--
Duy
^ 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