* Re: [PATCH 3/3] pretty=format: Avoid some expensive calculations when not needed
From: Junio C Hamano @ 2007-11-05 19:51 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Rene Scharfe, gitster
In-Reply-To: <Pine.LNX.4.64.0711041915290.4362@racer.site>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Unfortunately, we cannot reuse the result of that function, which
> would be cleaner: there are more users than just git log. Most
> notably, git-archive with "$Format:...$" substitution.
That makes sense.
> diff --git a/pretty.c b/pretty.c
> index 490cede..241e91c 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -393,6 +393,7 @@ void format_commit_message(const struct commit *commit,
> int i;
> enum { HEADER, SUBJECT, BODY } state;
> const char *msg = commit->buffer;
> + char *active = interp_find_active(format, table, ARRAY_SIZE(table));
> ...
> + if (active[IHASH])
> + interp_set_entry(table, IHASH,
> + sha1_to_hex(commit->object.sha1));
> + if (active[IHASH_ABBREV])
> + interp_set_entry(table, IHASH_ABBREV,
> find_unique_abbrev(commit->object.sha1,
> DEFAULT_ABBREV));
Instead of allocating a separate array and freeing at the end,
wouldn't it make more sense to have a bitfield that records what
is used by the format string inside the array elements?
^ permalink raw reply
* Re: [PATCH] upload-pack: Use finish_{command,async}() instead of waitpid().
From: Junio C Hamano @ 2007-11-05 20:05 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git
In-Reply-To: <200711042046.48257.johannes.sixt@telecom.at>
Johannes Sixt <johannes.sixt@telecom.at> writes:
> - If pack-objects sees an error, it terminates with failure. Since this
> breaks the pipe to rev-list, rev-list is killed with SIGPIPE.
I was a bit uneasy about this part. We do not explicitly tell
rev-list not to ignore SIGPIPE, so it could spin fruitlessly
listing revs and calling write(2). But the error from that
write should already be handled correctly anyway, so this should
be Ok.
> The test case checks for failures in rev-list (a missing
> object). Any hints how to trigger a failure in pack-objects
> that does not also trigger in rev-list would be welcome.
How about removing a blob from the test repository to corrupt
it? rev-list --objects I think would happily list the blob
because it sees its name in its containing tree without checking
its existence.
^ permalink raw reply
* Re: [PATCH 3/3] pretty=format: Avoid some expensive calculations when not needed
From: René Scharfe @ 2007-11-05 20:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git
In-Reply-To: <7v8x5cqxn0.fsf@gitster.siamese.dyndns.org>
Junio C Hamano schrieb:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> Unfortunately, we cannot reuse the result of that function, which
>> would be cleaner: there are more users than just git log. Most
>> notably, git-archive with "$Format:...$" substitution.
>
> That makes sense.
>
>
>> diff --git a/pretty.c b/pretty.c
>> index 490cede..241e91c 100644
>> --- a/pretty.c
>> +++ b/pretty.c
>> @@ -393,6 +393,7 @@ void format_commit_message(const struct commit *commit,
>> int i;
>> enum { HEADER, SUBJECT, BODY } state;
>> const char *msg = commit->buffer;
>> + char *active = interp_find_active(format, table, ARRAY_SIZE(table));
>> ...
>> + if (active[IHASH])
>> + interp_set_entry(table, IHASH,
>> + sha1_to_hex(commit->object.sha1));
>> + if (active[IHASH_ABBREV])
>> + interp_set_entry(table, IHASH_ABBREV,
>> find_unique_abbrev(commit->object.sha1,
>> DEFAULT_ABBREV));
>
> Instead of allocating a separate array and freeing at the end,
> wouldn't it make more sense to have a bitfield that records what
> is used by the format string inside the array elements?
How about (ab)using the value field? Let interp_find_active() mark
unneeded entries with NULL, and the rest with some cookie. All table
entries with non-NULL values need to be initialized. interp_set_entry()
needs to be aware of this cookie, as it mustn't free() it. The cookie
could be the address of a static char* in interpolate.c.
^ permalink raw reply
* Re: [PATCH 3/2] Enhance --early-output format
From: Junio C Hamano @ 2007-11-05 20:24 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Paul Mackerras, Marco Costalba, Git Mailing List
In-Reply-To: <alpine.LFD.0.999.0711041124050.15101@woody.linux-foundation.org>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> It wasn't totally trivial, but it doesn't seem to be excessively subtle
> either. About half the patch is moving around some code to look at whether
> the commit is interesting or not and rewriting the parents, so that it can
> be shared with the revision walker.
Very nicely done.
> + while (list) {
> + struct commit *commit = list->item;
> + unsigned int flags = commit->object.flags;
> +
> + list = list->next;
> + if (flags & UNINTERESTING)
> + continue;
> + if (rev->prune_fn && rev->dense && !(flags & TREECHANGE)) {
> + if (commit->parents && !commit->parents->next)
> + continue;
> + }
When looking at:
if (A && B && C) {
if (D && E)
continue;
}
an uninitiated might say "Huh? Why use nested 'if'?", but to
somebody who knows how revision traversal works, the above split
is a more logical way to test this condition. Maybe one liner
comment is in order?
> +static void show_early_header(struct rev_info *rev, const char *stage, int nr)
> +{
> + if (rev->shown_one) {
> + rev->shown_one = 0;
> + if (rev->commit_format != CMIT_FMT_ONELINE)
> + putchar(rev->diffopt.line_termination);
> + }
> + printf("Final output: %d %s\n", nr, stage);
> +}
As you noted, this is more like "Partial output" now.
How about painting the bikeshed pink by saying:
Partial output: 20
Partial output: 70
Final output: 70000
> + /* Did we already get enough commits for the early output? */
> + if (!i)
> + return;
> +
> + /*
> + * ..if no, then repeat it twice a second until we
> + * do.
> + *
> + * NOTE! We don't use "it_interval", because if the
> + * reader isn't listening, we want our output to be
> + * throttled by the writing, and not have the timer
> + * trigger every second even if we're blocked on a
> + * reader!
> + */
A comment like this is very much appreciated.
> + early_output_timer.it_value.tv_sec = 0;
> + early_output_timer.it_value.tv_usec = 500000;
> + setitimer(ITIMER_REAL, &early_output_timer, NULL);
> }
^ permalink raw reply
* Re: [PATCH 3/3] pretty=format: Avoid some expensive calculations when not needed
From: Jon Loeliger @ 2007-11-05 20:25 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, Johannes Schindelin, Git List
In-Reply-To: <472F7B2F.4050608@lsrfire.ath.cx>
On Mon, 2007-11-05 at 14:21, René Scharfe wrote:
> How about (ab)using the value field? Let interp_find_active() mark
> unneeded entries with NULL, and the rest with some cookie. All table
> entries with non-NULL values need to be initialized. interp_set_entry()
> needs to be aware of this cookie, as it mustn't free() it. The cookie
> could be the address of a static char* in interpolate.c.
Or adding a "flags/properties" field and having it
note things like static entries, active, etc...?
jdl
^ permalink raw reply
* Re: [PATCH 3/2] Enhance --early-output format
From: Linus Torvalds @ 2007-11-05 20:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Paul Mackerras, Marco Costalba, Git Mailing List
In-Reply-To: <7vsl3kphjp.fsf@gitster.siamese.dyndns.org>
On Mon, 5 Nov 2007, Junio C Hamano wrote:
>
> > + if (rev->prune_fn && rev->dense && !(flags & TREECHANGE)) {
> > + if (commit->parents && !commit->parents->next)
> > + continue;
> > + }
>
> When looking at:
>
> if (A && B && C) {
> if (D && E)
> continue;
> }
>
> an uninitiated might say "Huh? Why use nested 'if'?", but to
> somebody who knows how revision traversal works, the above split
> is a more logical way to test this condition. Maybe one liner
> comment is in order?
I'd almost prefer not to.
If people feel the code is subtle enough that a comment is in order to
explain *what* something does, I would rather introduce helper functions
than add comments. I'm not a big believer in comments in the middle of
code to explain the code, but I *am* a big believer in trying to make the
code easy to read without them.
(I don't dislike comments per se, but I'd *much* rather have the comments
explain what is going on at a higher level. Comments that talk about the
details of the code itself is likely bogus).
So we could add a commit to say what is going on ("ignore commits that
have only one parent and didn't change the tree"), but I'd not want to
explain why that particular layout of code.
That said, I've often found the "TREECHANGE" bit annoying. The fact that
we always have to test it together with testing "rev->prune_fn" and often
also the "rev->dense" flag is just annoying. I'd almost like to just
always set the bit if "rev->prune_fn" isn't set. Alternatively, the code
could be rewritten to just have a few inline helper functions, and it
could perhaps be written as
if (!(flags & TREECHANGE) && revs->dense && single_parent(commit))
continue;
I dunno. I have no really *strong* opinions, although I suspect that
anybody who looks at that particular piece of code had better understand
these issues well enough anyway that it doesn't much matter..
> As you noted, this is more like "Partial output" now.
> How about painting the bikeshed pink by saying:
>
> Partial output: 20
> Partial output: 70
> Final output: 70000
I'm fine with it. The reason I did it the way I did was that this way I
didn't need to change "gitk" - I could just use Pauls original patch
as-is, since that code didn't care *what* came after the "Final output",
and didn't care how many times it showed up.
Linus
^ permalink raw reply
* Re: [bug in next ?] git-fetch/git-push issue
From: Jeff King @ 2007-11-05 21:07 UTC (permalink / raw)
To: Daniel Barkalow; +Cc: Pierre Habouzit, Nicolas Pitre, Git ML
In-Reply-To: <Pine.LNX.4.64.0711051259580.7357@iabervon.org>
On Mon, Nov 05, 2007 at 01:17:14PM -0500, Daniel Barkalow wrote:
> I think this is the bit that's wrong. I blame Jeff, in 334f4831. :)
>
> The issue is that, in the previous version, we'd hit a continue on the
> not-an-ancestor message and not reach the update_tracking_ref() section
> for that ref. In 334f4831, all of the updating is after the loop, and it
> doesn't filter out the refs that didn't actually get pushed.
Nope, that's not the problem. We _only_ update any tracking refs at all
if ret == 0, and if we fail to push, then we are setting ret to -2.
Hrm. Oh wait, it looks like we then totally write over the current value
of 'ret' when we do pack_objects. Oops.
I'm unclear how to fix this, as I'm not really sure what ret is
_supposed_ to be communicating. What does the '-2' mean, as compared to
a '-4'? Should we be doing a 'ret += pack_objects(out, remote_refs)' or
some other bit-masking magic?
-Peff
^ permalink raw reply
* Re: [PATCH] Make git-clean a builtin
From: Junio C Hamano @ 2007-11-05 21:14 UTC (permalink / raw)
To: Shawn Bohrer; +Cc: git
In-Reply-To: <11942029474058-git-send-email-shawn.bohrer@gmail.com>
Shawn Bohrer <shawn.bohrer@gmail.com> writes:
> This replaces git-clean.sh with builtin-clean.c, and moves git-clean.sh to
> the examples.
>
> Signed-off-by: Shawn Bohrer <shawn.bohrer@gmail.com>
> ---
> diff --git a/builtin-clean.c b/builtin-clean.c
> new file mode 100644
> index 0000000..4141eb4
> --- /dev/null
> +++ b/builtin-clean.c
> @@ -0,0 +1,157 @@
> +/*
> + * "git clean" builtin command
> + *
> + * Copyright (C) 2007 Shawn Bohrer
> + *
> + * Based on git-clean.sh by Pavel Roskin
> + */
> +
> +#include "builtin.h"
> +#include "cache.h"
> +#include "dir.h"
> +
> +static int disabled = 1;
This means we are committed to make clean.requireForce default
to true, which is fine by me. I need to warn the users about
this early.
> +static int show_only = 0;
> +static int remove_directories = 0;
> +static int quiet = 0;
> +static int ignored = 0;
> +static int ignored_only = 0;
Please do not explicitly initialize static variables to zero.
^ permalink raw reply
* Re: [PATCH 0/3] more terse push output
From: Junio C Hamano @ 2007-11-05 21:14 UTC (permalink / raw)
To: Jeff King; +Cc: git, Nicolas Pitre, Johannes Schindelin
In-Reply-To: <20071105050517.GA6244@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> - the 'ref is not up-to-date, maybe you need to push' message has gone
> away in favor of the terse '[rejected] ... (non-fast forward)'. I
> know there was some discussion recently of enhancing that message.
> Is this perhaps too terse?
I kind of like this part.
I also like the part that stops mentioning the "pretend fetching
back" action. This would mostly parrot the same set of refs for
people who do use the tracking branches anyway.
^ permalink raw reply
* Re: [PATCH 1/3] Split off the pretty print stuff into its own file
From: Junio C Hamano @ 2007-11-05 21:16 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Rene Scharfe, gitster
In-Reply-To: <Pine.LNX.4.64.0711041914310.4362@racer.site>
I'll take the [1/3] to 'master'. I will see how discussion
goes about the rest on the list for now.
^ permalink raw reply
* Re: [PATCH] Build in ls-remote
From: Junio C Hamano @ 2007-11-05 21:22 UTC (permalink / raw)
To: Daniel Barkalow; +Cc: git
In-Reply-To: <Pine.LNX.4.64.0711041544200.7357@iabervon.org>
Daniel Barkalow <barkalow@iabervon.org> writes:
> This actually replaces peek-remote with ls-remote, since peek-remote
> now handles everything.
Nice logical conclusion of the series ;-)
Will queue.
Thanks.
^ permalink raw reply
* Re: [PATCH] Rearrange git-format-patch synopsis to improve clarity.
From: Junio C Hamano @ 2007-11-05 21:22 UTC (permalink / raw)
To: David Symonds; +Cc: git
In-Reply-To: <119421522591-git-send-email-dsymonds@gmail.com>
Thanks
^ permalink raw reply
* Re: [PATCH] t7501-commit.sh: Add test case for fixing author in amend commit.
From: Junio C Hamano @ 2007-11-05 21:23 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: Johannes Schindelin, gitster, git
In-Reply-To: <1194283047-16565-1-git-send-email-krh@redhat.com>
Kristian Høgsberg <krh@redhat.com> writes:
> Signed-off-by: Kristian Høgsberg <krh@redhat.com>
> ---
>
> How about this?
Looks Ok; will queue.
Thanks.
^ permalink raw reply
* Re: [PATCH 3/2] Enhance --early-output format
From: Linus Torvalds @ 2007-11-05 21:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Paul Mackerras, Marco Costalba, Git Mailing List
In-Reply-To: <alpine.LFD.0.999.0711051233270.15101@woody.linux-foundation.org>
On Mon, 5 Nov 2007, Linus Torvalds wrote:
>
> That said, I've often found the "TREECHANGE" bit annoying. The fact that
> we always have to test it together with testing "rev->prune_fn" and often
> also the "rev->dense" flag is just annoying. I'd almost like to just
> always set the bit if "rev->prune_fn" isn't set. Alternatively, the code
> could be rewritten to just have a few inline helper functions, and it
> could perhaps be written as
>
> if (!(flags & TREECHANGE) && revs->dense && single_parent(commit))
> continue;
>
> I dunno.
Here's a possible cleanup patch. It's on top of the enhanced
--early-output format commit, and in fact fixes a stupid bug in that
commit ("return -1" vs "return NULL"), but that bug-fix is really an
independent thing.
It basically removes the unnecessary indirection of "revs->prune_fn",
since that function is always the same one (or NULL), and there is in fact
not even an abstraction reason to make it a function (ie its not called
from some other file and doesn't allow us to keep the function itself
static or anything like that).
It then just replaces it with a bit that says "prune or not", and if not
pruning, every commit gets TREECHANGE.
That in turn means that
- if (!revs->prune_fn || (flags & TREECHANGE))
- if (revs->prune_fn && !(flags & TREECHANGE))
just become
- if (flags & TREECHANGE)
- if (!(flags & TREECHANGE))
respectively.
Together with adding the "single_parent()" helper function, the "complex"
conditional now becomes
if (!(flags & TREECHANGE) && rev->dense && single_parent(commit))
continue;
Anyway, do with this as you will. I think it's ok, but apart from passing
the test-suite and looking obvious, this has gotten zero testing.
Linus
---
builtin-log.c | 6 ++----
builtin-rev-list.c | 14 +++++++-------
commit.h | 5 +++++
revision.c | 20 ++++++++++++--------
revision.h | 5 +----
5 files changed, 27 insertions(+), 23 deletions(-)
diff --git a/builtin-log.c b/builtin-log.c
index 981f388..76c84e2 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -92,10 +92,8 @@ static int estimate_commit_count(struct rev_info *rev, struct commit_list *list)
list = list->next;
if (flags & UNINTERESTING)
continue;
- if (rev->prune_fn && rev->dense && !(flags & TREECHANGE)) {
- if (commit->parents && !commit->parents->next)
- continue;
- }
+ if (!(flags & TREECHANGE) && rev->dense && single_parent(commit))
+ continue;
n++;
}
return n;
diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index 6970467..2dec887 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -142,7 +142,7 @@ static int count_distance(struct commit_list *entry)
if (commit->object.flags & (UNINTERESTING | COUNTED))
break;
- if (!revs.prune_fn || (commit->object.flags & TREECHANGE))
+ if (commit->object.flags & TREECHANGE)
nr++;
commit->object.flags |= COUNTED;
p = commit->parents;
@@ -198,7 +198,7 @@ static inline int halfway(struct commit_list *p, int nr)
/*
* Don't short-cut something we are not going to return!
*/
- if (revs.prune_fn && !(p->item->object.flags & TREECHANGE))
+ if (!(p->item->object.flags & TREECHANGE))
return 0;
if (DEBUG_BISECT)
return 0;
@@ -268,7 +268,7 @@ static struct commit_list *best_bisection(struct commit_list *list, int nr)
int distance;
unsigned flags = p->item->object.flags;
- if (revs.prune_fn && !(flags & TREECHANGE))
+ if (!(flags & TREECHANGE))
continue;
distance = weight(p);
if (nr - distance < distance)
@@ -308,7 +308,7 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n
int distance;
unsigned flags = p->item->object.flags;
- if (revs.prune_fn && !(flags & TREECHANGE))
+ if (!(flags & TREECHANGE))
continue;
distance = weight(p);
if (nr - distance < distance)
@@ -362,7 +362,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
p->item->util = &weights[n++];
switch (count_interesting_parents(commit)) {
case 0:
- if (!revs.prune_fn || (flags & TREECHANGE)) {
+ if (flags & TREECHANGE) {
weight_set(p, 1);
counted++;
show_list("bisection 2 count one",
@@ -435,7 +435,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
* add one for p itself if p is to be counted,
* otherwise inherit it from q directly.
*/
- if (!revs.prune_fn || (flags & TREECHANGE)) {
+ if (flags & TREECHANGE) {
weight_set(p, weight(q)+1);
counted++;
show_list("bisection 2 count one",
@@ -482,7 +482,7 @@ static struct commit_list *find_bisection(struct commit_list *list,
continue;
p->next = last;
last = p;
- if (!revs.prune_fn || (flags & TREECHANGE))
+ if (flags & TREECHANGE)
nr++;
on_list++;
}
diff --git a/commit.h b/commit.h
index 4ed0c1c..aa67986 100644
--- a/commit.h
+++ b/commit.h
@@ -117,4 +117,9 @@ extern int interactive_add(void);
extern void add_files_to_cache(int verbose, const char *prefix, const char **files);
extern int rerere(void);
+static inline int single_parent(struct commit *commit)
+{
+ return commit->parents && !commit->parents->next;
+}
+
#endif /* COMMIT_H */
diff --git a/revision.c b/revision.c
index 5d6f208..7a1ecba 100644
--- a/revision.c
+++ b/revision.c
@@ -308,6 +308,14 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)
struct commit_list **pp, *parent;
int tree_changed = 0, tree_same = 0;
+ /*
+ * If we don't do pruning, everything is interesting
+ */
+ if (!revs->prune) {
+ commit->object.flags |= TREECHANGE;
+ return;
+ }
+
if (!commit->tree)
return;
@@ -415,8 +423,7 @@ static int add_parents_to_list(struct rev_info *revs, struct commit *commit, str
* simplify the commit history and find the parent
* that has no differences in the path set if one exists.
*/
- if (revs->prune_fn)
- revs->prune_fn(revs, commit);
+ try_to_simplify_commit(revs, commit);
if (revs->no_walk)
return 0;
@@ -684,9 +691,6 @@ void init_revisions(struct rev_info *revs, const char *prefix)
revs->skip_count = -1;
revs->max_count = -1;
- revs->prune_fn = NULL;
- revs->prune_data = NULL;
-
revs->commit_format = CMIT_FMT_DEFAULT;
diff_setup(&revs->diffopt);
@@ -1271,7 +1275,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
diff_tree_setup_paths(revs->prune_data, &revs->pruning);
/* Can't prune commits with rename following: the paths change.. */
if (!revs->diffopt.follow_renames)
- revs->prune_fn = try_to_simplify_commit;
+ revs->prune = 1;
if (!revs->full_diff)
diff_tree_setup_paths(revs->prune_data, &revs->diffopt);
}
@@ -1412,7 +1416,7 @@ enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)
return commit_ignore;
if (!commit_match(commit, revs))
return commit_ignore;
- if (revs->prune_fn && revs->dense) {
+ if (revs->prune && revs->dense) {
/* Commit without changes? */
if (!(commit->object.flags & TREECHANGE)) {
/* drop merges unless we want parenthood */
@@ -1460,7 +1464,7 @@ static struct commit *get_revision_1(struct rev_info *revs)
case commit_ignore:
continue;
case commit_error:
- return -1;
+ return NULL;
default:
return commit;
}
diff --git a/revision.h b/revision.h
index 2232247..a798514 100644
--- a/revision.h
+++ b/revision.h
@@ -15,8 +15,6 @@
struct rev_info;
struct log_info;
-typedef void (prune_fn_t)(struct rev_info *revs, struct commit *commit);
-
struct rev_info {
/* Starting list */
struct commit_list *commits;
@@ -28,12 +26,11 @@ struct rev_info {
/* Basic information */
const char *prefix;
void *prune_data;
- prune_fn_t *prune_fn;
-
unsigned int early_output;
/* Traversal flags */
unsigned int dense:1,
+ prune:1,
no_merges:1,
no_walk:1,
remove_empty_trees:1,
^ permalink raw reply related
* Re: [PATCH] git-mailsplit: with maildirs try to process new/ if cur/ is empty
From: Jeff King @ 2007-11-05 21:26 UTC (permalink / raw)
To: Gerrit Pape; +Cc: Fernando J. Pereda, git, Junio C Hamano
In-Reply-To: <20071105124920.17726.qmail@746e9cce42b49f.315fe32.mid.smarden.org>
On Mon, Nov 05, 2007 at 12:49:20PM +0000, Gerrit Pape wrote:
> On Fri, Oct 26, 2007 at 06:01:18PM +0200, Fernando J. Pereda wrote:
> > By that reasoning, you should make it parse both cur/ and new/.
> Okay.
Isn't the subject line now wrong?
-Peff
^ permalink raw reply
* Re: [PATCH] errors: "strict subset" -> "ancestor"
From: Jeff King @ 2007-11-05 21:28 UTC (permalink / raw)
To: Wincent Colaiuta
Cc: David Symonds, Steffen Prohaska, J. Bruce Fields, Junio C Hamano,
git
In-Reply-To: <E7B0CBAA-BB97-40A0-8CFB-A6F01A047D17@wincent.com>
On Mon, Nov 05, 2007 at 02:06:35PM +0100, Wincent Colaiuta wrote:
> Kind of: it aligns "error:" with "local":
>
> error: remote 'refs/heads/master' is not an ancestor of
> local 'refs/heads/master'.
FYI, in the thread 'more terse push output', there is discussion of
eliminating this message entirely. So I wanted to point readers of this
thread in that direction.
-Peff
^ permalink raw reply
* Re: [PATCH 3/2] Enhance --early-output format
From: Linus Torvalds @ 2007-11-05 21:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Paul Mackerras, Marco Costalba, Git Mailing List
In-Reply-To: <alpine.LFD.0.999.0711051313350.15101@woody.linux-foundation.org>
On Mon, 5 Nov 2007, Linus Torvalds wrote:
>
> Here's a possible cleanup patch. It's on top of the enhanced
> --early-output format commit, and in fact fixes a stupid bug in that
> commit ("return -1" vs "return NULL"), but that bug-fix is really an
> independent thing.
.. and this extends a bit further on the notion.
It basically means that "rev->dense" can now be ignored outside of
revision.c, because we'll just set TREECHANGE automatically when
seeing a non-merge regular commit when --sparse is being used.
So it's not just a simplification, it's a performance optimization too!
Although since nobody sane would ever use --sparse, I guess nobody really
cares.
Linus
---
builtin-log.c | 8 ++------
revision.c | 9 +++++++++
2 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/builtin-log.c b/builtin-log.c
index 76c84e2..d6845bc 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -88,13 +88,9 @@ static int estimate_commit_count(struct rev_info *rev, struct commit_list *list)
while (list) {
struct commit *commit = list->item;
unsigned int flags = commit->object.flags;
-
list = list->next;
- if (flags & UNINTERESTING)
- continue;
- if (!(flags & TREECHANGE) && rev->dense && single_parent(commit))
- continue;
- n++;
+ if ((flags & TREECHANGE) && !(flags & UNINTERESTING))
+ n++;
}
return n;
}
diff --git a/revision.c b/revision.c
index 7a1ecba..02e9241 100644
--- a/revision.c
+++ b/revision.c
@@ -325,6 +325,15 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)
return;
}
+ /*
+ * Normal non-merge commit? If we don't want to make the
+ * history dense, we consider it always to be a change..
+ */
+ if (!revs->dense && !commit->parents->next) {
+ commit->object.flags |= TREECHANGE;
+ return;
+ }
+
pp = &commit->parents;
while ((parent = *pp) != NULL) {
struct commit *p = parent->item;
^ permalink raw reply related
* Re: [PATCH] git-commit.sh: Fix usage checks regarding paths given when they do not make sense
From: Junio C Hamano @ 2007-11-05 21:39 UTC (permalink / raw)
To: Björn Steinbrink; +Cc: paolo.bonzini, krh, git
In-Reply-To: <1194291393-1067-1-git-send-email-B.Steinbrink@gmx.de>
Björn Steinbrink <B.Steinbrink@gmx.de> writes:
> The checks that looked for paths given to git-commit in addition to
> --all or --interactive expected only 3 values, while the case statement
> actually provides 4, so the check was never triggered.
>
> The bug was introduced in 6cbf07efc5702351897dee4742525c9b9f7828ac when
> the case statement was extended to handle --interactive.
Gaah, and thanks.
We really should have "negative" tests to catch this kind of
breakage in our testsuite. People when inventing new features
are eager to write tests to show off the new stuff works, but
not many people are careful enough to add tests to demonstrate
the commands properly catch user errors such as borked command
line options.
^ permalink raw reply
* [PATCH] t5530-upload-pack-error: Check more carefully for failures.
From: Johannes Sixt @ 2007-11-05 21:40 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <7v3avkqwyz.fsf@gitster.siamese.dyndns.org>
Previously, the test only triggered a failure in upload-pack's rev-list
subprocess. Here we also test for a failure of pack-objects, and make sure
that the right process failed.
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
On Monday 05 November 2007 21:05, Junio C Hamano wrote:
> > The test case checks for failures in rev-list (a missing
> > object). Any hints how to trigger a failure in pack-objects
> > that does not also trigger in rev-list would be welcome.
>
> How about removing a blob from the test repository to corrupt
> it? rev-list --objects I think would happily list the blob
> because it sees its name in its containing tree without checking
> its existence.
That does it. This goes on top of my previous patch.
-- Hannes
t/t5530-upload-pack-error.sh | 27 +++++++++++++++++++++++----
1 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh
index 9923ba0..70d4f86 100755
--- a/t/t5530-upload-pack-error.sh
+++ b/t/t5530-upload-pack-error.sh
@@ -15,7 +15,7 @@ test_expect_success 'setup and corrupt repository' '
test_tick &&
echo changed >file &&
git commit -a -m changed &&
- rm -f .git/objects/f7/3f3093ff865c514c6c51f867e35f693487d0d3
+ rm -f .git/objects/5e/a2ed416fbd4a4cbe227b75fe255dd7fa6bd4d6
'
@@ -24,14 +24,33 @@ test_expect_failure 'fsck fails' '
git fsck
'
-test_expect_failure 'upload pack fails due to error in rev-list' '
+test_expect_success 'upload-pack fails due to error in pack-objects' '
- echo "0032want $(git rev-parse HEAD)
+ ! echo "0032want $(git rev-parse HEAD)
00000009done
-0000" | git-upload-pack . > /dev/null
+0000" | git-upload-pack . > /dev/null 2> output.err &&
+ grep "pack-objects died" output.err
+'
+
+test_expect_success 'corrupt repo differently' '
+
+ git hash-object -w file &&
+ rm -f .git/objects/be/c63e37d08c454ad3a60cde90b70f3f7d077852
'
+test_expect_failure 'fsck fails' '
+
+ git fsck
+'
+test_expect_success 'upload-pack fails due to error in rev-list' '
+
+ ! echo "0032want $(git rev-parse HEAD)
+00000009done
+0000" | git-upload-pack . > /dev/null 2> output.err &&
+ grep "waitpid (async) failed" output.err
+'
+
test_expect_success 'create empty repository' '
mkdir foo &&
--
1.5.3.4.315.g2ce38
^ permalink raw reply related
* Re: [bug in next ?] git-fetch/git-push issue
From: Daniel Barkalow @ 2007-11-05 21:41 UTC (permalink / raw)
To: Jeff King; +Cc: Pierre Habouzit, Nicolas Pitre, Git ML
In-Reply-To: <20071105210711.GA9176@sigill.intra.peff.net>
On Mon, 5 Nov 2007, Jeff King wrote:
> On Mon, Nov 05, 2007 at 01:17:14PM -0500, Daniel Barkalow wrote:
>
> > I think this is the bit that's wrong. I blame Jeff, in 334f4831. :)
> >
> > The issue is that, in the previous version, we'd hit a continue on the
> > not-an-ancestor message and not reach the update_tracking_ref() section
> > for that ref. In 334f4831, all of the updating is after the loop, and it
> > doesn't filter out the refs that didn't actually get pushed.
>
> Nope, that's not the problem. We _only_ update any tracking refs at all
> if ret == 0, and if we fail to push, then we are setting ret to -2.
That's an odd combination of behavior: we update some of the refs, leave
the ones that didn't work alone, report success on the ones that worked,
but then we forget that some things worked?
If we're going to refuse to update local tracking refs, whose state
doesn't matter much, we should certainly refuse to update the remote refs,
which are probably public and extremely important. If we just pushed and
we fetch, we should see exclusively changes that somebody else (including
hooks remotely) did, not anything that we ourselves did.
> Hrm. Oh wait, it looks like we then totally write over the current value
> of 'ret' when we do pack_objects. Oops.
>
> I'm unclear how to fix this, as I'm not really sure what ret is
> _supposed_ to be communicating. What does the '-2' mean, as compared to
> a '-4'? Should we be doing a 'ret += pack_objects(out, remote_refs)' or
> some other bit-masking magic?
I'd guess -2 is supposed to indicate that there were some errors but some
things may have worked. If pack_objects() or receive_status() fails, we
shouldn't update anything locally (because it won't have been accepted
remotely); otherwise, we should make local updates of every remote efect
we had, even if we end up returning non-zero to indicate that we didn't do
everything asked.
-Daniel
*This .sig left intentionally blank*
^ permalink raw reply
* Re: [PATCH 2/3] parseopt: introduce OPT_RECURSE to specify shared options
From: Junio C Hamano @ 2007-11-05 21:48 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Pierre Habouzit
In-Reply-To: <Pine.LNX.4.64.0711051340490.4362@racer.site>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> After kicking this around a bit more on IRC, we had another idea. Instead
> of introducing OPT_RECURSE(), do something like OPT__QUIET(), only this
> time in diff.h:
>
> #define OPT__DIFF(opt) \
> OPT_BOOLEAN('p', NULL, &opt.format_patch, "show a patch"), \
> ...
>
> Pierre said this feels a bit "80s", so I'd like to hear other people's
> opinions.
>
> Hmm?
As I am from "80s" ;-) I like the simpler "macro" one much
better. There aren't many things that can go wrong in the
approach.
^ permalink raw reply
* Re: [PATCH] Use parseopts in builtin-push
From: Junio C Hamano @ 2007-11-05 21:50 UTC (permalink / raw)
To: Daniel Barkalow; +Cc: git
In-Reply-To: <Pine.LNX.4.64.0711042235350.7357@iabervon.org>
Thanks; will take this one.
The finalized version for builtin-fetch is appreciated.
^ permalink raw reply
* git pull opinion
From: Aghiles @ 2007-11-05 21:52 UTC (permalink / raw)
To: git
Hello,
I am not sure this is the best place to write about this. Anyway,
we just switched a couple of repositories to git (from svn) here
at work and one thing people find annoying is a pull into
a dirty directory. Before the "stash" feature it was even worse
but now we can type:
git stash
git pull
git stash apply
But isn't that something we should be able to specify to the "pull"
command ? Additionally and if I am not mistakn, those commands will
create "dangling" commits and blobs. So one has to execute:
git prune
Is there an "easier" way to pull into a dirty directory ? I am
asking this to make sure I understand the problem and not
because I find it annoying to type those 4 commands to perform
a pull (although some of my colleagues do find that annoying :).
For now, I am recommanding to my colleagues to commit very often
(even unfinished changes), pull, and then rebase the commits into
a more meaningful commit before pushing. Which seems to be a good
practice anyway,
Thank you for git,
- Aghiles.
ps; if someone is interested to hear what is the general opinion
on switching to git from svn in our company, I could elaborate.
^ permalink raw reply
* Re: [PATCH] git-revert is one of the most misunderstood command in git, help users out.
From: Alejandro Martinez Ruiz @ 2007-11-05 21:54 UTC (permalink / raw)
To: Steven Grimm; +Cc: Pierre Habouzit, Junio C Hamano, git
In-Reply-To: <CD2E6759-9E7E-41E6-8B58-AB6CA9604111@midwinter.com>
On Mon 05 Nov 2007, 11:28, Steven Grimm wrote:
>
> But that suggested command is not going to convince anyone they were wrong
> about git being hard to learn. I wonder if instead of saying, "I know what
> you meant, but I'm going to make you type a different command," we should
> make git revert just do what the user meant.
I think that would just add to confusion. "revert" applies to full
changesets, not single files, plus it creates a new commit, which is
probably not what the user wants. Most of them just want to revert some
local changes to some random files, so teach them what they need, if
anything.
> There is already precedent for that kind of mixed-mode UI:
>
> git checkout my-branch
> vs.
> git checkout my/source/file.c
This is a different case: you're basically performing the same
operation, with the second line applying just to a subset of files.
- Alex
^ permalink raw reply
* Re: [PATCH] git-revert is one of the most misunderstood command in git, help users out.
From: David Kastrup @ 2007-11-05 22:06 UTC (permalink / raw)
To: Steven Grimm; +Cc: Pierre Habouzit, Junio C Hamano, git
In-Reply-To: <20071105215433.GA12827@inspiron>
Alejandro Martinez Ruiz <alex@flawedcode.org> writes:
> On Mon 05 Nov 2007, 11:28, Steven Grimm wrote:
>>
>> But that suggested command is not going to convince anyone they were wrong
>> about git being hard to learn. I wonder if instead of saying, "I know what
>> you meant, but I'm going to make you type a different command," we should
>> make git revert just do what the user meant.
>
> I think that would just add to confusion. "revert" applies to full
> changesets, not single files, plus it creates a new commit, which is
> probably not what the user wants. Most of them just want to revert some
> local changes to some random files, so teach them what they need, if
> anything.
>
>> There is already precedent for that kind of mixed-mode UI:
>>
>> git checkout my-branch
>> vs.
>> git checkout my/source/file.c
>
> This is a different case: you're basically performing the same
> operation, with the second line applying just to a subset of files.
Huh? The first one moves HEAD. The second one doesn't.
--
David Kastrup, Kriemhildstr. 15, 44793 Bochum
^ 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