* Re: [PATCH] fetch-pack: Prepare for a side-band demultiplexer in a thread.
From: Junio C Hamano @ 2007-11-18 0:42 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git
In-Reply-To: <200711172309.28364.johannes.sixt@telecom.at>
Johannes Sixt <johannes.sixt@telecom.at> writes:
> This change again originates from the MinGW port. Since we don't
> have fork(2) on Windows, we must run the sideband demultiplexer
> in a thread.
If the rationale was "running in a thread is more natural on the
platform", I would understand it.
But "_must_ run because there is no fork(2)" solicits a "Huh?
How does Cygwin does it then?" from me.
^ permalink raw reply
* Re: [PATCH 3/3] send-pack: assign remote errors to each ref
From: Junio C Hamano @ 2007-11-18 1:03 UTC (permalink / raw)
To: Jeff King; +Cc: git, Alex Riesen, Pierre Habouzit, Daniel Barkalow
In-Reply-To: <20071117125602.GC23186@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> diff --git a/builtin-send-pack.c b/builtin-send-pack.c
> index c7d07aa..bcf7143 100644
> --- a/builtin-send-pack.c
> +++ b/builtin-send-pack.c
> @@ -146,19 +146,43 @@ static void get_local_heads(void)
> for_each_ref(one_local_ref, NULL);
> }
>
> -static int receive_status(int in)
> +static struct ref *set_ref_error(struct ref *refs, const char *line)
> {
> + struct ref *ref;
> +
> + for (ref = refs; ref; ref = ref->next) {
> + const char *msg;
> + if (prefixcmp(line, ref->name))
> + continue;
> + msg = line + strlen(ref->name);
> + if (*msg++ != ' ')
> + continue;
> + ref->status = REF_STATUS_REMOTE_REJECT;
> + ref->error = xstrdup(msg);
> + ref->error[strlen(ref->error)-1] = '\0';
> + return ref;
> + }
> + return NULL;
> +}
It probably would not matter for sane repositories, but with
thousands of refs, strlen() and prefixcmp() may start to hurt:
struct ref *ref;
int reflen;
const char *msg = strchr(line, ' ');
if (!msg)
return NULL;
reflen = msg - line;
msg++;
for (ref = refs; ref; ref = ref->next) {
if (strncmp(line, ref->name, reflen) || line[reflen] != ' ')
continue;
...
return ref->next;
}
return NULL;
but the "hint" optimization probably make the above
micro-optimization irrelevant.
> +/* a return value of -1 indicates that an error occurred,
> + * but we were able to set individual ref errors. A return
> + * value of -2 means we couldn't even get that far. */
It is preferred to have a multi-line comment like this:
/*
* A return value of -1 ...
* ...
* ... couldn't even get that far.
*/
> +static int receive_status(int in, struct ref *refs)
> ...
> + hint = NULL;
> while (1) {
> len = packet_read_line(in, line, sizeof(line));
> if (!len)
> @@ -171,7 +195,10 @@ static int receive_status(int in)
> }
> if (!memcmp(line, "ok", 2))
> continue;
> - fputs(line, stderr);
> + if (hint)
> + hint = set_ref_error(hint, line + 3);
> + if (!hint)
> + hint = set_ref_error(refs, line + 3);
Clever... taking advantage of the order receive-pack reports to
optimize.
Before receive_status() is called, can the refs already have the
error status and string set?
> @@ -429,12 +463,15 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
> }
> close(out);
>
> - print_push_status(dest, remote_refs);
> -
> if (expect_status_report) {
> - if (receive_status(in))
> + ret = receive_status(in, remote_refs);
> + if (ret == -2)
> return -1;
Hmm. When we did not receive status, we cannot tell what
succeeded or failed, but what we _can_ tell the user is which
refs we attempted to push. I wonder if robbing that information
from the user with this "return -1" is a good idea. Perhaps we
would instead want to set the status of all the refs to error
and call print_push_status() anyway? I dunno.
> }
> + else
> + ret = 0;
> +
> + print_push_status(dest, remote_refs);
>
> if (!args.dry_run && remote) {
> for (ref = remote_refs; ref; ref = ref->next)
^ permalink raw reply
* Re: [PATCH] fetch-pack: Prepare for a side-band demultiplexer in a thread.
From: Alex Riesen @ 2007-11-18 1:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, git
In-Reply-To: <7vtznkz8nw.fsf@gitster.siamese.dyndns.org>
Junio C Hamano, Sun, Nov 18, 2007 01:42:11 +0100:
> Johannes Sixt <johannes.sixt@telecom.at> writes:
>
> > This change again originates from the MinGW port. Since we don't
> > have fork(2) on Windows, we must run the sideband demultiplexer
> > in a thread.
>
> If the rationale was "running in a thread is more natural on the
> platform", I would understand it.
>
> But "_must_ run because there is no fork(2)" solicits a "Huh?
> How does Cygwin does it then?" from me.
>
You wont believe it: they start the currently running program again
and copy parents memory over into the child. Sometimes it fails.
If you want something scary:
http://cygwin.com/cgi-bin/cvsweb.cgi/src/winsup/cygwin/fork.cc?rev=1.193&content-type=text/x-cvsweb-markup&cvsroot=src
^ permalink raw reply
* Re: Cloning empty repositories, was Re: What is the idea for bare repositories?
From: Junio C Hamano @ 2007-11-18 1:06 UTC (permalink / raw)
To: Jeff King
Cc: Sergei Organov, Matthieu Moy, Johannes Schindelin, Bill Lear,
Jan Wielemaker, git
In-Reply-To: <20071118002514.GA4458@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Junio, can I get an ACK or NAK on the patch below?
I do not think it would hurt. Is the "local" case the only
codepath that needs this (iow, we would not need this message if
other transports die more loudly and/or we cannot tell if the
failure is wrong URL or empty remote repository)?
^ permalink raw reply
* Re: [Advance Warning PATCH] Move gitk to its own subdirectory
From: Junio C Hamano @ 2007-11-18 1:18 UTC (permalink / raw)
To: Alex Riesen; +Cc: git
In-Reply-To: <20071117232519.GA7664@steel.home>
Alex Riesen <raa.lkml@gmail.com> writes:
> You made it part of "all" target, which we will have to change if gitk
> is to become a subproject: Makefile better handle absence of the
> Makefile under gitk-git, and continue build.
Not necessarily.
The policy is completely up to the superproject (which is
git.git). Currently, the policy is "make all" and "make
install" in git.git builds and installs what are considered core
parts of the system which includes git-gui and gitk.
gitk nor git-gui should not become useless without being in
git.git (iow, their build procedure better not depend on things
outside their subtree), but I do not think that is a hard
requirement either. Shawn *chose* to keep git-gui buildable and
installable separately as well as as part of git.git. Again I
think that policy is up to individual subprojects.
^ permalink raw reply
* Re: [PATCH 1/3] send-pack: track errors for each ref
From: Junio C Hamano @ 2007-11-18 1:21 UTC (permalink / raw)
To: Jeff King; +Cc: Daniel Barkalow, git, Alex Riesen, Pierre Habouzit
In-Reply-To: <20071118001312.GB4000@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Sat, Nov 17, 2007 at 01:05:35PM -0500, Daniel Barkalow wrote:
>
>> > + if (ref->nonfastforward) {
>> > + strcat(quickref, "...");
>> > + type = '+';
>> > + msg = " (forced update)";
>> > + }
>> > + else {
>>
>> Coding style, IIRC.
>
> Sorry, I don't see the style nit you're mentioning here.
I think Daniel is referring to "Put 'else' on the same line as
the brace that closes the corresponding 'if' opened", iow:
if (...) {
...
} else {
...
}
^ permalink raw reply
* Re: What's cooking in git.git (topics)
From: Junio C Hamano @ 2007-11-18 1:29 UTC (permalink / raw)
To: Alex Riesen; +Cc: git
In-Reply-To: <20071117234240.GB7664@steel.home>
Alex Riesen <raa.lkml@gmail.com> writes:
> Junio C Hamano, Sat, Nov 17, 2007 21:51:04 +0100:
>> * mh/rebase-skip-hard (Thu Nov 8 08:03:06 2007 +0100) 1 commit
>> - Do git reset --hard HEAD when using git rebase --skip
>>
>> Some people on the list may find this debatable. Opinions?
>
> I like it (and didn't like the previous behaviour). Anyway, it is not
> obvious what to do when --skip refuses to continue rebase because of
> dirty index.
True. Let's have it in 'next' then.
^ permalink raw reply
* Re: [PATCH 3/3] send-pack: assign remote errors to each ref
From: Jeff King @ 2007-11-18 2:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Alex Riesen, Pierre Habouzit, Daniel Barkalow
In-Reply-To: <7vir40z7nm.fsf@gitster.siamese.dyndns.org>
On Sat, Nov 17, 2007 at 05:03:57PM -0800, Junio C Hamano wrote:
> > + for (ref = refs; ref; ref = ref->next) {
> > + const char *msg;
> > + if (prefixcmp(line, ref->name))
> > + continue;
>
> It probably would not matter for sane repositories, but with
> thousands of refs, strlen() and prefixcmp() may start to hurt:
It is actually _just_ prefixcmp. Or do you mean the strlen we call in
prefixcmp? If so, I think the right solution is to make prefixcmp
faster. :)
> but the "hint" optimization probably make the above
> micro-optimization irrelevant.
Agreed.
> It is preferred to have a multi-line comment like this:
>
> /*
> * A return value of -1 ...
> * ...
> * ... couldn't even get that far.
> */
OK. Since it is already in next, do you want a style fixup patch?
> Before receive_status() is called, can the refs already have the
> error status and string set?
Nothing else sets the string, so the latter is not possible (perhaps it
should be "remote_error" for clarity). It is less clear that we are not
overwriting another status; however, if you look at do_send_pack, we
only actually send the remote refs that are getting REF_STATUS_OK.
A broken or malicious remote could change the push status of an
arbitrary ref to rejection, but I don't really see the point. We could
explicitly check that we are changing from OK to REMOTE_REJECTED in
set_ref_error.
> > if (expect_status_report) {
> > - if (receive_status(in))
> > + ret = receive_status(in, remote_refs);
> > + if (ret == -2)
> > return -1;
>
> Hmm. When we did not receive status, we cannot tell what
> succeeded or failed, but what we _can_ tell the user is which
> refs we attempted to push. I wonder if robbing that information
> from the user with this "return -1" is a good idea. Perhaps we
> would instead want to set the status of all the refs to error
> and call print_push_status() anyway? I dunno.
That is a reasonable behavior (although they have already seen an
"error: " message, I think). We might also consider returning something
besides "-1" to differentiate "ok, but some refs failed" from "terribly
broken". The old code used to use "-2" and "-4", but I checked and all
of the error checking paths seemed to end up as a boolean.
I can work up a patch if there is consensus.
-Peff
^ permalink raw reply
* Re: [PATCH 1/3] send-pack: track errors for each ref
From: Jeff King @ 2007-11-18 3:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Daniel Barkalow, git, Alex Riesen, Pierre Habouzit
In-Reply-To: <7v6400z6tk.fsf@gitster.siamese.dyndns.org>
On Sat, Nov 17, 2007 at 05:21:59PM -0800, Junio C Hamano wrote:
> > Sorry, I don't see the style nit you're mentioning here.
>
> I think Daniel is referring to "Put 'else' on the same line as
> the brace that closes the corresponding 'if' opened", iow:
>
> if (...) {
> ...
> } else {
> ...
> }
Ah. We are really inconsistent about that (I count about 333 non-cuddled
(but possible to cuddle) versus 499 cuddled). Should we put something in
the CodingGuidelines? I am also happy to roll this change into a style
patch.
For reference, I counted with:
# cuddled:
grep '} *else' *.c | wc -l
# possible but non-cuddled
# when we find an 'else', if the previous line had a '}'
# on it, then it is a hit. There are a few false positives
# when the "} else" is from an outer block scope
# And no, this sed invocation probably isn't totally portable. :)
sed -n '/else/{x; /}/{x;p}}; h' *.c | wc -l
# or to check the accuracy
sed -n '/else/{x; /}/{p;x;p}}; h' *.c
-Peff
^ permalink raw reply
* Re: Cloning empty repositories, was Re: What is the idea for bare repositories?
From: Jeff King @ 2007-11-18 3:25 UTC (permalink / raw)
To: Junio C Hamano
Cc: Sergei Organov, Matthieu Moy, Johannes Schindelin, Bill Lear,
Jan Wielemaker, git
In-Reply-To: <7vejeoz7is.fsf@gitster.siamese.dyndns.org>
On Sat, Nov 17, 2007 at 05:06:51PM -0800, Junio C Hamano wrote:
> > Junio, can I get an ACK or NAK on the patch below?
>
> I do not think it would hurt. Is the "local" case the only
> codepath that needs this (iow, we would not need this message if
> other transports die more loudly and/or we cannot tell if the
> failure is wrong URL or empty remote repository)?
Good question, and the answer is yes, it is the only spot that produces
no useful output. The other errors are:
via ssh, git-daemon, or file:// (all using receive-pack) you get:
$ git-clone localhost:foo bar
Initialized empty Git repository in /home/peff/bar/.git/
fatal: no matching remote head
fetch-pack from 'localhost:foo' failed.
via http, you get:
$ git-clone http://localhost/git/foo bar
Initialized empty Git repository in /home/peff/bar/.git/
Cannot get remote repository information.
Perhaps git-update-server-info needs to be run there?
I didn't try rsync, though I expect it should be the same as http.
-Peff
^ permalink raw reply
* Re: [PATCH v4] user-manual: Add section "Why bisecting merge commits can be harder ..."
From: J. Bruce Fields @ 2007-11-18 3:59 UTC (permalink / raw)
To: Steffen Prohaska
Cc: Junio C Hamano, Benoit Sigoure, Andreas Ericsson, Johannes Sixt,
git
In-Reply-To: <1194702594213-git-send-email-prohaska@zib.de>
On Sat, Nov 10, 2007 at 02:49:54PM +0100, Steffen Prohaska wrote:
> +[[bisect-merges]]
> +Why bisecting merge commits can be harder than bisecting linear history
> +-----------------------------------------------------------------------
> +
> +This section discusses how gitlink:git-bisect[1] plays
> +with differently shaped histories. The text is based upon
> +an email by Junio C. Hamano to the git mailing list
> +(link:http://marc.info/?l=git&m=119403257315527&w=2[link:http://marc.info/?l=git&m=119403257315527&w=2]).
> +It was adapted for the user manual.
This is not the only text that's been taken from someplace else, but if
we attributed them all in the text it would get a little cumbersome....
I think the place for that kind of thing is in the commit message, but
if we really think we need to include it in the main text, we could add
a separate "'acknowledgements" section.
> +
> +Using gitlink:git-bisect[1] on a history with merges can be
> +challenging. Bisecting through merges is not a technical
> +problem. The real problem is what to do when the culprit turns
> +out to be a merge commit. How to spot what really is wrong, and
> +figure out how to fix it. The problem is not for the tool but
> +for the human, and it is real.
I think we can pare that down a little.
> +
> +Imagine this history:
> +
> +................................................
> + ---Z---o---X---...---o---A---C---D
> + \ /
> + o---o---Y---...---o---B
> +................................................
> +
> +Suppose that on the upper development line, the meaning of one
> +of the functions that existed at Z was changed at commit X. The
> +commits from Z leading to A change both the function's
> +implementation and all calling sites that existed at Z, as well
> +as new calling sites they add, to be consistent. There is no
> +bug at A.
> +
> +Suppose that in the meantime the lower development line somebody
> +added a new calling site for that function at commit Y. The
> +commits from Z leading to B all assume the old semantics of that
> +function and the callers and the callee are consistent with each
> +other. There is no bug at B, either.
> +
> +Suppose further that the two development lines were merged at C
> +and there was no textual conflict with this three way merge.
> +The result merged cleanly.
> +
> +Now, during bisect you find that the merge C is broken. You
> +started to bisect, because you found D is bad and you know Z was
> +good. The breakage is understandable, as at C, the new calling
> +site of the function added by the lower branch is not converted
> +to the new semantics, while all the other calling sites that
> +already existed at Z would have been converted by the merge. The
> +new calling site has semantic adjustment needed, but you do not
> +know that yet.
> +
> +You need to find out what is the cause of the breakage by looking
> +at the merge commit C and the history leading to it. How would
> +you do that?
> +
> +Both "git diff A C" and "git diff B C" would be an enormous patch.
> +Each of them essentially shows the whole change on each branch
> +since they diverged. The developers may have well behaved to
> +create good commits that follow the "commit small, commit often,
> +commit well contained units" mantra, and each individual commit
> +leading from Z to A and from Z to B may be easy to review and
> +understand, but looking at these small and easily reviewable
> +steps alone would not let you spot the breakage. You need to
> +have a global picture of what the upper branch did (and
> +among many, one of them is to change the semantics of that
> +particular function) and look first at the huge "diff A C"
> +(which shows the change the lower branch introduces), and see if
> +that huge change is consistent with what have been done between
> +Z and A.
> +
> +On the other hand, if you did not merge at C but rebased the
> +history between Z to B on top of A, you would have get this
> +linear history:
> +
> +................................................................
> + ---Z---o---X--...---o---A---o---o---Y*--...---o---B*--D*
> +................................................................
> +
> +Bisecting between Z and D* would hit a single culprit commit Y*
> +instead. This tends to be easier to understand why it is broken.
> +
> +For this reason, many experienced git users, even when they are
> +working on an otherwise merge-heavy project, keep the histories
> +linear by rebasing their work on top of public upstreams before
> +publishing.
I'd say "partly for this reason", as I don't think this is the only
reason people do that.
I've done the above revisions and a few others and pushed them to
git://linux-nfs.org/~bfields/git.git maint
I'll take another look in the morning.
--b.
^ permalink raw reply
* Re: [PATCH 3/3] send-pack: assign remote errors to each ref
From: Junio C Hamano @ 2007-11-18 4:47 UTC (permalink / raw)
To: Jeff King; +Cc: git, Alex Riesen, Pierre Habouzit, Daniel Barkalow
In-Reply-To: <20071118023942.GA4560@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Sat, Nov 17, 2007 at 05:03:57PM -0800, Junio C Hamano wrote:
>
>> > + for (ref = refs; ref; ref = ref->next) {
>> > + const char *msg;
>> > + if (prefixcmp(line, ref->name))
>> > + continue;
>>
>> It probably would not matter for sane repositories, but with
>> thousands of refs, strlen() and prefixcmp() may start to hurt:
>
> It is actually _just_ prefixcmp. Or do you mean the strlen we call in
> prefixcmp? If so, I think the right solution is to make prefixcmp
> faster. :)
I was referring to strlen(ref->name) taken for all refs during
the loop. Micro-optimized one finds the end of refname on the
"ng" line once before entering the loop.
> OK. Since it is already in next, do you want a style fixup patch?
I do not think it is particularly a big deal -- perhaps clean it
before we touch the vicinity of the code next time. The same
goes for the "} else {" stuff.
>> Before receive_status() is called, can the refs already have the
>> error status and string set?
>
> Nothing else sets the string, so the latter is not possible (perhaps it
> should be "remote_error" for clarity). It is less clear that we are not
> ... if you look at do_send_pack, we
> only actually send the remote refs that are getting REF_STATUS_OK.
Ah, Ok.
^ permalink raw reply
* Re: [PATCH 3/3] send-pack: assign remote errors to each ref
From: Jeff King @ 2007-11-18 5:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Alex Riesen, Pierre Habouzit, Daniel Barkalow
In-Reply-To: <7vwssgxir4.fsf@gitster.siamese.dyndns.org>
On Sat, Nov 17, 2007 at 08:47:11PM -0800, Junio C Hamano wrote:
>>> + for (ref = refs; ref; ref = ref->next) {
>>> + const char *msg;
>>> + if (prefixcmp(line, ref->name))
>>> + continue;
>>> + msg = line + strlen(ref->name);
>>> + if (*msg++ != ' ')
>>> + continue;
>>> + ref->status = REF_STATUS_REMOTE_REJECT;
>>> + ref->error = xstrdup(msg);
>>> + ref->error[strlen(ref->error)-1] = '\0';
>>> + return ref;
>>> + }
>>> + return NULL;
>>> +}
> > It is actually _just_ prefixcmp. Or do you mean the strlen we call in
> > prefixcmp? If so, I think the right solution is to make prefixcmp
> > faster. :)
> I was referring to strlen(ref->name) taken for all refs during
> the loop. Micro-optimized one finds the end of refname on the
> "ng" line once before entering the loop.
I don't see such a strlen, except in the implementation of prefixcmp,
because we continue most of the time based on its result. If you have a
false match on the prefixcmp (i.e., a prefix of another ref), you do an
extra strlen.
But I don't think this is worth micro-optimizing.
-Peff
^ permalink raw reply
* [PATCH] grep -An -Bm: fix invocation of external grep command
From: Junio C Hamano @ 2007-11-18 5:18 UTC (permalink / raw)
To: git
When building command line to invoke external grep, the
arguments to -A/-B/-C options were placd in randarg[] buffer,
but the code forgot that snprintf() does not count terminating
NUL in its return value. This caused "git grep -A1 -B2" to
invoke external grep with "-B21 -A1".
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* This bug was present since mid May, 2006.
builtin-grep.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin-grep.c b/builtin-grep.c
index 185876b..bbf747f 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -294,7 +294,7 @@ static int external_grep(struct grep_opt *opt, const char
if (opt->pre_context) {
push_arg("-B");
len += snprintf(argptr, sizeof(randarg)-len,
- "%u", opt->pre_context);
+ "%u", opt->pre_context) + 1;
if (sizeof(randarg) <= len)
die("maximum length of args exceeded");
push_arg(argptr);
@@ -303,7 +303,7 @@ static int external_grep(struct grep_opt *opt, const char
if (opt->post_context) {
push_arg("-A");
len += snprintf(argptr, sizeof(randarg)-len,
- "%u", opt->post_context);
+ "%u", opt->post_context) + 1;
if (sizeof(randarg) <= len)
die("maximum length of args exceeded");
push_arg(argptr);
@@ -313,7 +313,7 @@ static int external_grep(struct grep_opt *opt, const char
else {
push_arg("-C");
len += snprintf(argptr, sizeof(randarg)-len,
- "%u", opt->post_context);
+ "%u", opt->post_context) + 1;
if (sizeof(randarg) <= len)
die("maximum length of args exceeded");
push_arg(argptr);
^ permalink raw reply related
* [PATCH] send-pack: improve error reporting for total remote unpack
From: Jeff King @ 2007-11-18 5:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Daniel Barkalow
If the remote doesn't give us per-ref status reports, then
we pessimistically assume that all refs failed. However,
instead of just exiting the function, we now mark them
individually as failed.
This lets us print the usual status table, but refs which
failed in this way are marked individually (alongside refs
which may have failed for other reasons, like being rejected
for non-ff status).
Signed-off-by: Jeff King <peff@peff.net>
---
On Sat, Nov 17, 2007 at 08:47:11PM -0800, Junio C Hamano wrote:
> Hmm. When we did not receive status, we cannot tell what
> succeeded or failed, but what we _can_ tell the user is which
> refs we attempted to push. I wonder if robbing that information
> from the user with this "return -1" is a good idea. Perhaps we
> would instead want to set the status of all the refs to error
> and call print_push_status() anyway? I dunno.
I think this is a bit nicer. However, I noticed that it's hard to follow
these error paths, anyway. packet_read_line likes to die on bad input
anyway. And I couldn't get receive_pack to give me a bad unpack status;
instead, it just died with a fatal error and reported nothing.
> > OK. Since it is already in next, do you want a style fixup patch [for
> > the comment]?
> I do not think it is particularly a big deal -- perhaps clean it
> before we touch the vicinity of the code next time. The same
> goes for the "} else {" stuff.
Well, it fixed itself here (and it looks like you tweaked the else
cuddling when you applied to next).
One other thing I noticed while doing this patch is that our remote ref
status reporting isn't foolproof. We set every ref we send to 'OK' with
the nice effect that if status reporting isn't enabled, we just assume
that it worked. However, if the status coming back is truncated (i.e.,
some refs are missing in receive_status), we will just fail to notice and
assume all is well. So to be perfect, we would need a
REF_STATUS_EXPECTING_REPORT.
I can implement this if desired. OTOH, this isn't a new bug at all, and it
would be pretty tricky to trigger it (you would need to die exactly on a
line boundary).
builtin-send-pack.c | 31 +++++++++++++++++++++----------
cache.h | 1 +
2 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 5fadd0b..602e196 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -165,9 +165,14 @@ static struct ref *set_ref_error(struct ref *refs, const char *line)
return NULL;
}
-/* a return value of -1 indicates that an error occurred,
- * but we were able to set individual ref errors. A return
- * value of -2 means we couldn't even get that far. */
+static void set_ref_error_all(struct ref *refs)
+{
+ struct ref *ref;
+ for (ref = refs; ref; ref = ref->next)
+ if (ref->status == REF_STATUS_OK)
+ ref->status = REF_STATUS_REMOTE_FAILURE;
+}
+
static int receive_status(int in, struct ref *refs)
{
struct ref *hint;
@@ -175,11 +180,15 @@ static int receive_status(int in, struct ref *refs)
int ret = 0;
int len = packet_read_line(in, line, sizeof(line));
if (len < 10 || memcmp(line, "unpack ", 7)) {
- fprintf(stderr, "did not receive status back\n");
- return -2;
+ error("did not receive remote status");
+ set_ref_error_all(refs);
+ return -1;
}
if (memcmp(line, "unpack ok\n", 10)) {
- fputs(line, stderr);
+ char *p = line + strlen(line) - 1;
+ if (*p == '\n')
+ *p = '\0';
+ error("unpack failed: %s", line + 7);
ret = -1;
}
hint = NULL;
@@ -329,6 +338,11 @@ static void print_push_status(const char *dest, struct ref *refs)
else
print_ref_status('!', "[remote rejected]", ref, ref->peer_ref, ref->error);
break;
+ case REF_STATUS_REMOTE_FAILURE:
+ print_ref_status('!', "[remote failure]", ref,
+ ref->deletion ? ref->peer_ref : NULL,
+ NULL);
+ break;
case REF_STATUS_OK:
print_ok_ref_status(ref);
break;
@@ -462,11 +476,8 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
}
close(out);
- if (expect_status_report) {
+ if (expect_status_report)
ret = receive_status(in, remote_refs);
- if (ret == -2)
- return -1;
- }
else
ret = 0;
diff --git a/cache.h b/cache.h
index ba9178f..de011bf 100644
--- a/cache.h
+++ b/cache.h
@@ -511,6 +511,7 @@ struct ref {
REF_STATUS_REJECT_NODELETE,
REF_STATUS_UPTODATE,
REF_STATUS_REMOTE_REJECT,
+ REF_STATUS_REMOTE_FAILURE,
} status;
char *error;
struct ref *peer_ref; /* when renaming */
--
1.5.3.5.1815.g9445b-dirty
^ permalink raw reply related
* Re: [PATCH] send-pack: improve error reporting for total remote unpack
From: Jeff King @ 2007-11-18 5:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Daniel Barkalow
In-Reply-To: <20071118055804.GA19313@sigill.intra.peff.net>
> Subject: [PATCH] send-pack: improve error reporting for total remote unpack
Oops, the subject was supposed to be "...total remote _failure_".
-Peff
^ permalink raw reply
* Re: [PATCH] send-pack: improve error reporting for total remote unpack
From: Jeff King @ 2007-11-18 7:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Daniel Barkalow
In-Reply-To: <20071118055804.GA19313@sigill.intra.peff.net>
On Sun, Nov 18, 2007 at 12:58:06AM -0500, Jeff King wrote:
> One other thing I noticed while doing this patch is that our remote ref
> status reporting isn't foolproof. We set every ref we send to 'OK' with
> the nice effect that if status reporting isn't enabled, we just assume
> that it worked. However, if the status coming back is truncated (i.e.,
> some refs are missing in receive_status), we will just fail to notice and
> assume all is well. So to be perfect, we would need a
> REF_STATUS_EXPECTING_REPORT.
This turned out to be pretty easy, given the other recent changes, and I
think it actually makes the code a bit clearer. Please scrap the patch
to which I am replying, and I will post a replacement series in a
second.
-Peff
^ permalink raw reply
* [PATCH 1/2] make "find_ref_by_name" a public function
From: Jeff King @ 2007-11-18 7:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Daniel Barkalow
In-Reply-To: <20071118055804.GA19313@sigill.intra.peff.net>
This was a static in remote.c, but is generally useful.
Signed-off-by: Jeff King <peff@peff.net>
---
cache.h | 2 ++
refs.c | 8 ++++++++
remote.c | 8 --------
3 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/cache.h b/cache.h
index ba9178f..1f3f113 100644
--- a/cache.h
+++ b/cache.h
@@ -521,6 +521,8 @@ struct ref {
#define REF_HEADS (1u << 1)
#define REF_TAGS (1u << 2)
+extern struct ref *find_ref_by_name(struct ref *list, const char *name);
+
#define CONNECT_VERBOSE (1u << 0)
extern struct child_process *git_connect(int fd[2], const char *url, const char *prog, int flags);
extern int finish_connect(struct child_process *conn);
diff --git a/refs.c b/refs.c
index ae53254..54ec98d 100644
--- a/refs.c
+++ b/refs.c
@@ -1433,3 +1433,11 @@ int update_ref(const char *action, const char *refname,
}
return 0;
}
+
+struct ref *find_ref_by_name(struct ref *list, const char *name)
+{
+ for ( ; list; list = list->next)
+ if (!strcmp(list->name, name))
+ return list;
+ return NULL;
+}
diff --git a/remote.c b/remote.c
index 09b7aad..bb01059 100644
--- a/remote.c
+++ b/remote.c
@@ -696,14 +696,6 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,
return -errs;
}
-static struct ref *find_ref_by_name(struct ref *list, const char *name)
-{
- for ( ; list; list = list->next)
- if (!strcmp(list->name, name))
- return list;
- return NULL;
-}
-
static const struct refspec *check_pattern_match(const struct refspec *rs,
int rs_nr,
const struct ref *src)
--
1.5.3.5.1817.g37c1a-dirty
^ permalink raw reply related
* [PATCH 2/2] send-pack: tighten remote error reporting
From: Jeff King @ 2007-11-18 7:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Daniel Barkalow
In-Reply-To: <20071118055804.GA19313@sigill.intra.peff.net>
Previously, we set all ref pushes to 'OK', and then marked
them as errors if the remote reported so. This has the
problem that if the remote dies or fails to report a ref, we
just assume it was OK.
Instead, we use a new non-OK state to indicate that we are
expecting status (if the remote doesn't support the
report-status feature, we fall back on the old behavior).
Thus we can flag refs for which we expected a status, but
got none (conversely, we now also print a warning for refs
for which we get a status, but weren't expecting one).
This also allows us to simplify the receive_status exit
code, since each ref is individually marked with failure
until we get a success response. We can just print the usual
status table, so the user still gets a sense of what we were
trying to do when the failure happened.
Signed-off-by: Jeff King <peff@peff.net>
---
This is a lot more robust, and I think the code is easier to follow. The
ref->error member is now ref->remote_status, which is hopefully a bit
more obvious as to when it is set.
The ref matching is also slightly more micro-optimized, Junio. :)
builtin-send-pack.c | 94 ++++++++++++++++++++++++++++-----------------------
cache.h | 3 +-
2 files changed, 54 insertions(+), 43 deletions(-)
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 5fadd0b..349e02f 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -146,60 +146,67 @@ static void get_local_heads(void)
for_each_ref(one_local_ref, NULL);
}
-static struct ref *set_ref_error(struct ref *refs, const char *line)
-{
- struct ref *ref;
-
- for (ref = refs; ref; ref = ref->next) {
- const char *msg;
- if (prefixcmp(line, ref->name))
- continue;
- msg = line + strlen(ref->name);
- if (*msg++ != ' ')
- continue;
- ref->status = REF_STATUS_REMOTE_REJECT;
- ref->error = xstrdup(msg);
- ref->error[strlen(ref->error)-1] = '\0';
- return ref;
- }
- return NULL;
-}
-
-/* a return value of -1 indicates that an error occurred,
- * but we were able to set individual ref errors. A return
- * value of -2 means we couldn't even get that far. */
static int receive_status(int in, struct ref *refs)
{
struct ref *hint;
char line[1000];
int ret = 0;
int len = packet_read_line(in, line, sizeof(line));
- if (len < 10 || memcmp(line, "unpack ", 7)) {
- fprintf(stderr, "did not receive status back\n");
- return -2;
- }
+ if (len < 10 || memcmp(line, "unpack ", 7))
+ return error("did not receive remote status");
if (memcmp(line, "unpack ok\n", 10)) {
- fputs(line, stderr);
+ char *p = line + strlen(line) - 1;
+ if (*p == '\n')
+ *p = '\0';
+ error("unpack failed: %s", line + 7);
ret = -1;
}
hint = NULL;
while (1) {
+ char *refname;
+ char *msg;
len = packet_read_line(in, line, sizeof(line));
if (!len)
break;
if (len < 3 ||
- (memcmp(line, "ok", 2) && memcmp(line, "ng", 2))) {
+ (memcmp(line, "ok ", 3) && memcmp(line, "ng ", 3))) {
fprintf(stderr, "protocol error: %s\n", line);
ret = -1;
break;
}
- if (!memcmp(line, "ok", 2))
- continue;
+
+ line[strlen(line)-1] = '\0';
+ refname = line + 3;
+ msg = strchr(refname, ' ');
+ if (msg)
+ *msg++ = '\0';
+
+ /* first try searching at our hint, falling back to all refs */
if (hint)
- hint = set_ref_error(hint, line + 3);
+ hint = find_ref_by_name(hint, refname);
if (!hint)
- hint = set_ref_error(refs, line + 3);
- ret = -1;
+ hint = find_ref_by_name(refs, refname);
+ if (!hint) {
+ warning("remote reported status on unknown ref: %s",
+ refname);
+ continue;
+ }
+ if (hint->status != REF_STATUS_EXPECTING_REPORT) {
+ warning("remote reported status on unexpected ref: %s",
+ refname);
+ continue;
+ }
+
+ if (line[0] == 'o' && line[1] == 'k')
+ hint->status = REF_STATUS_OK;
+ else {
+ hint->status = REF_STATUS_REMOTE_REJECT;
+ ret = -1;
+ }
+ if (msg)
+ hint->remote_status = xstrdup(msg);
+ /* start our next search from the next ref */
+ hint = hint->next;
}
return ret;
}
@@ -324,10 +331,14 @@ static void print_push_status(const char *dest, struct ref *refs)
"non-fast forward");
break;
case REF_STATUS_REMOTE_REJECT:
- if (ref->deletion)
- print_ref_status('!', "[remote rejected]", ref, NULL, ref->error);
- else
- print_ref_status('!', "[remote rejected]", ref, ref->peer_ref, ref->error);
+ print_ref_status('!', "[remote rejected]", ref,
+ ref->deletion ? ref->peer_ref : NULL,
+ ref->remote_status);
+ break;
+ case REF_STATUS_EXPECTING_REPORT:
+ print_ref_status('!', "[remote failure]", ref,
+ ref->deletion ? ref->peer_ref : NULL,
+ "remote failed to report status");
break;
case REF_STATUS_OK:
print_ok_ref_status(ref);
@@ -434,7 +445,6 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
hashcpy(ref->new_sha1, new_sha1);
if (!ref->deletion)
new_refs++;
- ref->status = REF_STATUS_OK;
if (!args.dry_run) {
char *old_hex = sha1_to_hex(ref->old_sha1);
@@ -451,6 +461,9 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
packet_write(out, "%s %s %s",
old_hex, new_hex, ref->name);
}
+ ref->status = expect_status_report ?
+ REF_STATUS_EXPECTING_REPORT :
+ REF_STATUS_OK;
}
packet_flush(out);
@@ -462,11 +475,8 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
}
close(out);
- if (expect_status_report) {
+ if (expect_status_report)
ret = receive_status(in, remote_refs);
- if (ret == -2)
- return -1;
- }
else
ret = 0;
diff --git a/cache.h b/cache.h
index 1f3f113..0dff61a 100644
--- a/cache.h
+++ b/cache.h
@@ -511,8 +511,9 @@ struct ref {
REF_STATUS_REJECT_NODELETE,
REF_STATUS_UPTODATE,
REF_STATUS_REMOTE_REJECT,
+ REF_STATUS_EXPECTING_REPORT,
} status;
- char *error;
+ char *remote_status;
struct ref *peer_ref; /* when renaming */
char name[FLEX_ARRAY]; /* more */
};
--
1.5.3.5.1817.g37c1a-dirty
^ permalink raw reply related
* Re: [PATCH 2/2] send-pack: tighten remote error reporting
From: Jeff King @ 2007-11-18 7:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Daniel Barkalow
In-Reply-To: <20071118071651.GB18467@sigill.intra.peff.net>
On Sun, Nov 18, 2007 at 02:16:52AM -0500, Jeff King wrote:
> + print_ref_status('!', "[remote rejected]", ref,
> + ref->deletion ? ref->peer_ref : NULL,
> + ref->remote_status);
Gah, that should of course be:
ref->deletion ? NULL : ref->peer_ref
> + case REF_STATUS_EXPECTING_REPORT:
> + print_ref_status('!', "[remote failure]", ref,
> + ref->deletion ? ref->peer_ref : NULL,
> + "remote failed to report status");
And the same here.
I had resisted making a test that checked the exact output format,
because such things are often a pain to keep up to date. But that's two
regressions in patches I've submitted that would have been caught. Maybe
I should just pay more attention.
-Peff
^ permalink raw reply
* [PATCH 3/2] send-pack: fix "everything up-to-date" message
From: Jeff King @ 2007-11-18 8:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Daniel Barkalow
In-Reply-To: <20071118055804.GA19313@sigill.intra.peff.net>
This has always been slightly inaccurate, since it used the
new_refs counter, which really meant "did we send any
objects," so deletions were not counted.
It has gotten even worse with recent patches, since we no
longer look at the 'ret' value, meaning we would say "up to
date" if non-ff pushes were rejected.
Instead, we now claim up to date iff every ref is either
unmatched or up to date. Any other case should already have
generated a status line.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin-send-pack.c | 16 +++++++++++++++-
1 files changed, 15 insertions(+), 1 deletions(-)
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 14447bb..3aab89c 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -347,6 +347,20 @@ static void print_push_status(const char *dest, struct ref *refs)
}
}
+static int refs_pushed(struct ref *ref)
+{
+ for (; ref; ref = ref->next) {
+ switch(ref->status) {
+ case REF_STATUS_NONE:
+ case REF_STATUS_UPTODATE:
+ break;
+ default:
+ return 1;
+ }
+ }
+ return 0;
+}
+
static int do_send_pack(int in, int out, struct remote *remote, const char *dest, int nr_refspec, const char **refspec)
{
struct ref *ref;
@@ -487,7 +501,7 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
update_tracking_ref(remote, ref);
}
- if (!new_refs)
+ if (!refs_pushed(remote_refs))
fprintf(stderr, "Everything up-to-date\n");
if (ret < 0)
return ret;
--
1.5.3.5.1817.gd2b4b-dirty
^ permalink raw reply related
* [PATCH] avoid "defined but not used" warning for fetch_objs_via_walker
From: Jeff King @ 2007-11-18 8:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Daniel Barkalow
Because this function is static and used only by the
http-walker, when NO_CURL is defined, gcc emits a "defined
but not used" warning.
Signed-off-by: Jeff King <peff@peff.net>
---
On master. I like to compile with -Werror to make sure I don't miss
warnings as the compile scrolls by.
This fix feels a little wrong, since the function isn't specific to http
support, but hopefully the comment should be obvious if we ever add
another similar commit walker that needs it.
transport.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/transport.c b/transport.c
index e8a2608..43b9e7c 100644
--- a/transport.c
+++ b/transport.c
@@ -344,6 +344,7 @@ static int rsync_transport_push(struct transport *transport,
/* Generic functions for using commit walkers */
+#ifndef NO_CURL /* http fetch is the only user */
static int fetch_objs_via_walker(struct transport *transport,
int nr_objs, struct ref **to_fetch)
{
@@ -370,6 +371,7 @@ static int fetch_objs_via_walker(struct transport *transport,
free(dest);
return 0;
}
+#endif /* NO_CURL */
static int disconnect_walker(struct transport *transport)
{
--
1.5.3.5.1817.gd2b4b-dirty
^ permalink raw reply related
* Re: preserving mtime
From: Mike Hommey @ 2007-11-18 8:45 UTC (permalink / raw)
To: Wayne Davison; +Cc: Andreas Ericsson, git
In-Reply-To: <20071117182236.GD23659@blorf.net>
On Sat, Nov 17, 2007 at 10:22:36AM -0800, Wayne Davison wrote:
> On Fri, Nov 16, 2007 at 11:15:34AM +0100, Andreas Ericsson wrote:
> >> is it possible to tell git to preserve the file modification time in
> >> a checked out copy?
>
> > Fabrizio Pollastri wrote:
> > No. Doing so would seriously break build-systems.
>
> I wish that the initial clone would set the modification time to the
> commit time. It would make the intial checkout have a more accurate
> representation of when a file was last changed instead of all files
> being set to the clone date. Then, files that are being updated would
> get their time set as they do now. I supposed I'll just use the handy
> git-set-file-times script (mentioned in another reply) every time I do
> a clone.
For completeness, it would make sense to do so every time you git
checkout (like, when switching branches).
Mike
^ permalink raw reply
* Re: [PATCH] builtin-commit: fix "git add x y && git commit y" committing x, too
From: Junio C Hamano @ 2007-11-18 9:18 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Kristian Høgsberg, git
In-Reply-To: <7vk5ohuunv.fsf@gitster.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> I noticed that the implementation left next-index crufts almost
> every time it was run, and started to clean it up. Here is
> still a WIP and it does not optimize the read_tree(HEAD) part,
> but you should be able to replace that part with your one-way
> merge easily. As I haven't done that ls-files --error-unmatch
> equivalent, this does not pass tests that involve partial
> commits with added or removed paths.
I was working on this tonight. Will send out a proposed fix
based on this WIP shortly. The result seems to pass all the
tests.
^ permalink raw reply
* [PATCH] Fix warning about bitfield in struct ref
From: Shawn O. Pearce @ 2007-11-18 9:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
cache.h:503: warning: type of bit-field 'force' is a GCC extension
cache.h:504: warning: type of bit-field 'merge' is a GCC extension
cache.h:505: warning: type of bit-field 'nonfastforward' is a GCC extension
cache.h:506: warning: type of bit-field 'deletion' is a GCC extension
So we change it to an 'unsigned int' which is not a GCC extension.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
cache.h | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/cache.h b/cache.h
index ba9178f..65e019e 100644
--- a/cache.h
+++ b/cache.h
@@ -500,10 +500,10 @@ struct ref {
struct ref *next;
unsigned char old_sha1[20];
unsigned char new_sha1[20];
- unsigned char force : 1;
- unsigned char merge : 1;
- unsigned char nonfastforward : 1;
- unsigned char deletion : 1;
+ unsigned int force:1,
+ merge:1,
+ nonfastforward:1,
+ deletion:1;
enum {
REF_STATUS_NONE = 0,
REF_STATUS_OK,
--
1.5.3.5.1794.g083e
^ permalink raw reply related
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