* [PATCH] diffcore: Allow users to decide what funcname to use
From: Andreas Ericsson @ 2007-11-13 9:15 UTC (permalink / raw)
To: Miles Bader; +Cc: Junio C Hamano, Johannes Schindelin, Git Mailing List
In-Reply-To: <473954F8.8070908@op5.se>
Andreas Ericsson wrote:
> Miles Bader wrote:
>> Andreas Ericsson <ae@op5.se> writes:
>>> I notice it, and I don't like it. I guess I'm just used to git being
>>> smarter than their GNU tool equivalents, especially since it only ever
>>> applies patches in full.
>>
>> It's not at all obvious that this behavior is actually wrong -- it seems
>> perfectly reasonable to use either old or new text for the hunk headers.
>>
>
> Right, which is why I've made it configurable.
>
My git hacking has been stalled at the office for now, and I'm swanked at
home since my girlfriend just moved in and brought temporary pandemonium
with her. Here's what I've got now. It passes all tests, but there are no
new ones added. Documentation also needs updating.
Extract with
sed -n -e /^#CUTSTART/,/^#CUTEND/p -e /^#/d
#CUTSTART---%<---%<---%<---
From: Andreas Ericsson <ae@op5.se>
Date: Tue, 13 Nov 2007 09:47:43 +0100
Subject: [PATCH] diffcore: Allow users to decide what funcname to use
The function name being printed with the header of each
hunk is fetched from the "old" file today. Since git by
default applies patches either in full or not at all it's
arguably more correct to use the function from the "new"
file, at least when manually reviewing commits.
I stumbled upon this hunk when reviewing a series of
commits which caused the resulting code to segfault
under certain circumstances. Several hunks before, the
function declaration was changed and "status" was now
declared as an auto variable of type "int". The hunk
looks obviously bogus, and since I wasn't properly
awake, I reported this hunk to be the bogus one.
@@ -583,75 +346,100 @@ double jitter_request(int *status){
context
context
if(!syncsource_found){
- *status = STATE_UNKNOWN;
+ status = STATE_WARNING;
if(verbose) printf("warning: no sync source found\n")
}
This is what GNU "diff -p" would have reported under the
same circumstances, but GNU diff has no notion of version
control, and as such will not know if it's being used on
content where the patch by definition will apply in full.
Git can be smarter than that, and imo it should. This
patch lets the diffcore grok a new configuration variable,
"diff.funcnames", which can be set to "new", "old", or a
boolean value, which will cause it to be "old" (for 'true')
and 'none' (for 'false').
Signed-off-by: Andreas Ericsson <ae@op5.se>
---
diff.c | 20 ++++++++++++++++++--
diffcore-break.c | 4 ++--
xdiff/xdiff.h | 3 ++-
xdiff/xemit.c | 7 ++++++-
4 files changed, 28 insertions(+), 6 deletions(-)
diff --git a/diff.c b/diff.c
index 6bb902f..057bba8 100644
--- a/diff.c
+++ b/diff.c
@@ -20,6 +20,7 @@
static int diff_detect_rename_default;
static int diff_rename_limit_default = 100;
static int diff_use_color_default;
+static unsigned long xdl_emit_flags = XDL_EMIT_FUNCNAMES;
int diff_auto_refresh_index = 1;
static char diff_colors[][COLOR_MAXLEN] = {
@@ -178,6 +179,20 @@ int git_diff_ui_config(const char *var, const char *value)
color_parse(value, var, diff_colors[slot]);
return 0;
}
+ if (!prefixcmp(var, "diff.funcnames")) {
+ if (!value)
+ xdl_emit_flags = XDL_EMIT_COMMON;
+ else if (!strcasecmp(value, "new"))
+ xdl_emit_flags = XDL_EMIT_FUNCNAMES_NEW;
+ else if (!strcasecmp(value, "old") || !strcasecmp(value, "default"))
+ xdl_emit_flags = XDL_EMIT_FUNCNAMES;
+ else {
+ if (git_config_bool(var, value))
+ xdl_emit_flags = XDL_EMIT_FUNCNAMES;
+ else
+ xdl_emit_flags = XDL_EMIT_COMMON;
+ }
+ }
return git_default_config(var, value);
}
@@ -1332,7 +1347,8 @@ static void builtin_diff(const char *name_a,
ecbdata.found_changesp = &o->found_changes;
xpp.flags = XDF_NEED_MINIMAL | o->xdl_opts;
xecfg.ctxlen = o->context;
- xecfg.flags = XDL_EMIT_FUNCNAMES;
+ xecfg.flags = xdl_emit_flags;
+
if (funcname_pattern)
xdiff_set_find_func(&xecfg, funcname_pattern);
if (!diffopts)
@@ -2844,7 +2860,7 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)
xpp.flags = XDF_NEED_MINIMAL;
xecfg.ctxlen = 3;
- xecfg.flags = XDL_EMIT_FUNCNAMES;
+ xecfg.flags = xdl_emit_flags;
ecb.outf = xdiff_outf;
ecb.priv = &data;
xdl_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);
diff --git a/diffcore-break.c b/diffcore-break.c
index c71a226..048ec25 100644
--- a/diffcore-break.c
+++ b/diffcore-break.c
@@ -257,8 +257,8 @@ void diffcore_merge_broken(void)
if (!p)
/* we already merged this with its peer */
continue;
- else if (p->broken_pair &&
- !strcmp(p->one->path, p->two->path)) {
+
+ if (p->broken_pair && !strcmp(p->one->path, p->two->path)) {
/* If the peer also survived rename/copy, then
* we merge them back together.
*/
diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index c00ddaa..326e1df 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -41,7 +41,8 @@ extern "C" {
#define XDL_EMIT_FUNCNAMES (1 << 0)
#define XDL_EMIT_COMMON (1 << 1)
-
+#define XDL_EMIT_FUNCNAMES_NEW (1 << 2)
+
#define XDL_MMB_READONLY (1 << 0)
#define XDL_MMF_ATOMIC (1 << 0)
diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index d3d9c84..51dd085 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -149,7 +149,12 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
* Emit current hunk header.
*/
- if (xecfg->flags & XDL_EMIT_FUNCNAMES) {
+ if (xecfg->flags & XDL_EMIT_FUNCNAMES_NEW) {
+ xdl_find_func(&xe->xdf2, s2, funcbuf,
+ sizeof(funcbuf), &funclen,
+ ff, xecfg->find_func_priv);
+ }
+ else if (xecfg->flags & XDL_EMIT_FUNCNAMES) {
xdl_find_func(&xe->xdf1, s1, funcbuf,
sizeof(funcbuf), &funclen,
ff, xecfg->find_func_priv);
--
1.5.3.5.1527.g6161
#CUTEND---%<---%<---%<---
--
Andreas Ericsson andreas.ericsson@op5.se
OP5 AB www.op5.se
Tel: +46 8-230225 Fax: +46 8-230231
^ permalink raw reply related
* Re: [BUG] fast-import producing very deep tree deltas
From: Shawn O. Pearce @ 2007-11-13 9:27 UTC (permalink / raw)
To: Brian Downing; +Cc: git
In-Reply-To: <20071113085307.GC14735@spearce.org>
"Shawn O. Pearce" <spearce@spearce.org> wrote:
> Brian Downing <bdowning@lavos.net> wrote:
> > I've happened upon a case where fast-import produces deep tree deltas.
> > How deep? Really deep. 6035 entries deep to be precise for this case:
> >
> > depths: count 135970 total 120567366 min 0 max 6035 mean 886.72 median 3 std_dev 1653.48
> >
> > 27b8a20bdf39fecd917e8401d3499013e49449d0 tree 32 99609547 6035 0000000000000000000000000000000000000000
> >
> > This was with git-fast-import from 'next' as of a couple days ago,
> > run with the default options (no --depth passed in).
> >
> > Needless to say the pack that resulted was just about useless. Trying to
> > repack it resulted in the "counting objects" phase running at about five
> > objects per second.
Brian, does this fix it?
--8>--
From ff39dd457564b9198344e0cc785afa8cac05b486 Mon Sep 17 00:00:00 2001
From: Shawn O. Pearce <spearce@spearce.org>
Date: Tue, 13 Nov 2007 04:26:24 -0500
Subject: [PATCH] Don't allow fast-import tree delta chains to exceed maximum depth
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org
Brian Downing noticed fast-import can produce tree depths of up
to 6,035 objects and even deeper. Long delta chains can create
very small packfiles but cause problems during repacking as git
needs to unpack each tree to count the reachable blobs.
What's happening here is the active branch cache isn't big enough.
We're swapping out the branch and thus recycling the tree information
(struct tree_content) back into the free pool. When we later reload
the tree we set the delta_depth to 0 but we kept the tree we just
reloaded as a delta base.
So if the tree we reloaded was already at the maximum depth we
wouldn't know it and make the new tree a delta. Multiply the
number of times the branch cache has to swap out the tree times
max_depth (10) and you get the maximum delta depth of a tree created
by fast-import. In Brian's case above the active branch cache had
to swap the branch out 603/604 times during this import to produce
a tree with a delta depth of 6035.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
fast-import.c | 16 ++++++++++++----
1 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index f93d7d6..215f1e7 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -153,13 +153,16 @@ Format of STDIN stream:
#define PACK_ID_BITS 16
#define MAX_PACK_ID ((1<<PACK_ID_BITS)-1)
+#define DEPTH_BITS 13
+#define MAX_DEPTH ((1<<DEPTH_BITS)-1)
struct object_entry
{
struct object_entry *next;
uint32_t offset;
- unsigned type : TYPE_BITS;
- unsigned pack_id : PACK_ID_BITS;
+ uint32_t type : TYPE_BITS,
+ pack_id : PACK_ID_BITS,
+ depth : DEPTH_BITS;
unsigned char sha1[20];
};
@@ -1084,6 +1087,7 @@ static int store_object(
delta_count_by_type[type]++;
last->depth++;
+ e->depth = last->depth;
hdrlen = encode_header(OBJ_OFS_DELTA, deltalen, hdr);
write_or_die(pack_data->pack_fd, hdr, hdrlen);
@@ -1097,6 +1101,7 @@ static int store_object(
} else {
if (last)
last->depth = 0;
+ e->depth = 0;
hdrlen = encode_header(type, dat->len, hdr);
write_or_die(pack_data->pack_fd, hdr, hdrlen);
pack_size += hdrlen;
@@ -1160,7 +1165,7 @@ static void load_tree(struct tree_entry *root)
if (myoe && myoe->pack_id != MAX_PACK_ID) {
if (myoe->type != OBJ_TREE)
die("Not a tree: %s", sha1_to_hex(sha1));
- t->delta_depth = 0;
+ t->delta_depth = myoe->depth;
buf = gfi_unpack_entry(myoe, &size);
} else {
enum object_type type;
@@ -2289,8 +2294,11 @@ int main(int argc, const char **argv)
}
else if (!prefixcmp(a, "--max-pack-size="))
max_packsize = strtoumax(a + 16, NULL, 0) * 1024 * 1024;
- else if (!prefixcmp(a, "--depth="))
+ else if (!prefixcmp(a, "--depth=")) {
max_depth = strtoul(a + 8, NULL, 0);
+ if (max_depth > MAX_DEPTH)
+ die("--depth cannot exceed %u", MAX_DEPTH);
+ }
else if (!prefixcmp(a, "--active-branches="))
max_active_branches = strtoul(a + 18, NULL, 0);
else if (!prefixcmp(a, "--import-marks="))
--
1.5.3.5.1661.gcbf0
--
Shawn.
^ permalink raw reply related
* Re: wishlist: git info
From: Jakub Narebski @ 2007-11-13 9:44 UTC (permalink / raw)
To: git
In-Reply-To: <fhbn50$uqu$1@ger.gmane.org>
Thomas Neumann wrote:
>> Perhaps also project description (if it exists?)
> one can specify a project description? I did not even know this. But
> yes, this would be useful, too.
It is project description for gitweb (git web interface), and I think also
for other web interfaces (cgit, wit, git-php). Or rather *repository*
description. It is in .git/description (in 'description' file in git
repository).
Unfortunately by default it contains (see: ${tempate_dir}/description)
Unnamed repository; edit this file to name it for gitweb.
> In general I think git info should show everything to quickly understand
> what is currently checked out. The name of the current branch should
> probably be included, too.
Name of current branch, current directory, name of topdir (with '.git'
stripped if it is bare repository), perhaps oneline description of top
commit. And if branch is under StGit or Guilt (patch management interfaces)
control.
See also helper functions to set shell prompt in
contrib/completion/git-completion.bash.
I for example have "1760:[gitweb/web!git]$ " as bash prompt:
- 1760 is number of command in history,
- 'gitweb/web' is name of branch I am on,
- 'git' is the name of repository
This migitates need got 'git-info' command.
> I use an alias with the commands proposed by Alex Riessen for now, but a
> more general command would be nice.
You can always write[*1*] git-info.sh or git-info.perl[*2*], and install it
as git-info in your git installation. And send patches here, to git mailing
list, for comments.
Footnotes:
----------
[*1*] Even if one of the most common complaints is "too many user-visible
commands".
[*2*] Scripts are better for ptototyping. Choose your own poison: POSIX
shell or Perl...
--
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
^ permalink raw reply
* Re: Cloning empty repositories, was Re: What is the idea for bare repositories?
From: Matthieu Moy @ 2007-11-13 9:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, Bill Lear, Jan Wielemaker, git
In-Reply-To: <7v4pfr2kmh.fsf@gitster.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> But both of Johannes's points apply equally well to an empty
> bare repository and to an empty non bare repository. IOW,
> bareness does not matter to the suggestion Johannes gave.
He was suggesting to create the initial commit before cloning:
>> So you need to populate the repository before starting _anyway_.
To create an initial commit in a non-bare repository, I put files in
it, git add, and git commit.
To create an initial commit in a bare repository, the most natural way
for me is to clone it, create the commit in the clone, and then push.
Bare-ness _does_ matter for that.
I repeat the use-case I mentionned above :
,----
| a typical use-case is when I want to create a new project. I'd
| like to initialize an empty bare repo on my backed up disk, and then
| clone it to my local-fast-unreliable disk to get a working copy and do
| the first commit there.
`----
I find this quite natural, and up to now, no one gave me either a
rationale not to do that, or a _simple_ way to achieve this. As I
said, it's currently not _very_ hard to do, but I have to edit
.git/config by hand, while git clone knows how to do this much faster
than I for non-empty repositories.
--
Matthieu
^ permalink raw reply
* Re: Subject: [PATCH 2/3] Let git-add--interactive read colors from .gitconfig
From: Jakub Narebski @ 2007-11-13 9:46 UTC (permalink / raw)
To: git
In-Reply-To: <47395F63.8040306@zwell.net>
Dan Zwell wrote:
> Junio C Hamano wrote:
>> But the point is, that you are not ignoring invalid color names
>> but instead giving back a random match aren't you?
>
> No, if there's no match, the token is ignored. False matches are
> possible in some cases (the bogus config option "colored" would match
> "red", for example),
Match /\b(?:red|...)\b/ then
--
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
^ permalink raw reply
* Re: Cloning from kernel.org, then switching to another repo
From: Matthieu Moy @ 2007-11-13 9:52 UTC (permalink / raw)
To: Jon Smirl; +Cc: Jeff King, Johannes Schindelin, Git Mailing List
In-Reply-To: <9e4733910711122030q7bbf6057ubb6b5b27e1885500@mail.gmail.com>
"Jon Smirl" <jonsmirl@gmail.com> writes:
> Execute bit was not set. I just set it for all the scripts. +x is not
> getting turned on with a default git init-db. I just made a new repo
> to check, no +x on the scripts.
That's by design: "git init" gives you _example_ hooks, but they won't
run until you activate them explicitely with the appropriate chmod.
That said, I'm not sure there's a really good reason not to run
update-server-info by default on push. It doesn't cost much and saves
a lot of troubles for beginners. Perhaps there are cases where the
performance cost is non-negligible.
--
Matthieu
^ permalink raw reply
* Re: [PATCH 4/2] Fix parent rewriting in --early-output
From: Paul Mackerras @ 2007-11-13 9:59 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Marco Costalba, Junio C Hamano, Git Mailing List
In-Reply-To: <alpine.LFD.0.9999.0711122046570.2786@woody.linux-foundation.org>
Linus Torvalds writes:
> However, while the parent is now correctly rewritten, it looks like gitk
> is confused by this. Gitk will remember the original parent information,
> even if a replay has given new parenthood information. Since the partial
> early-output information is triggered by timing, this means that gitk will
> show some totally random parent that quite possibly won't even be part of
> the final commit set at all!
Yep. It will be a little complex to deal with that because there are
bits of state that I set up for the parents, and if they're the wrong
parents, I'll have to go back and undo that.
In fact it would be easier for me if, instead of getting the id of
some random ancestor commit, I got an explicit indication to say
"unknown parent", such as just a "-" in place of the id of the
unknown parent(s). Would that be doable? I could then just not do
the processing for any unknown parent, and make sure to do it when I
see the final version of the commit.
Also, I have just about worked out an efficient way to do the commit
reordering incrementally, which would let me not use --topo-order or
--date-order, and display commits as they come in. I'll have to see
whether that turns out to be better overall than using --early-output.
Paul.
^ permalink raw reply
* Re: Cloning empty repositories, was Re: What is the idea for bare repositories?
From: Shawn O. Pearce @ 2007-11-13 10:02 UTC (permalink / raw)
To: Matthieu Moy
Cc: Junio C Hamano, Johannes Schindelin, Bill Lear, Jan Wielemaker,
git
In-Reply-To: <vpqzlxiiii6.fsf@bauges.imag.fr>
Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
> I repeat the use-case I mentionned above :
>
> ,----
> | a typical use-case is when I want to create a new project. I'd
> | like to initialize an empty bare repo on my backed up disk, and then
> | clone it to my local-fast-unreliable disk to get a working copy and do
> | the first commit there.
> `----
>
> I find this quite natural, and up to now, no one gave me either a
> rationale not to do that, or a _simple_ way to achieve this. As I
> said, it's currently not _very_ hard to do, but I have to edit
> .git/config by hand, while git clone knows how to do this much faster
> than I for non-empty repositories.
Its a goal to redefine git-clone as the following, as that is
really all it does:
mkdir foo && cd foo && git init &&
git remote add -f origin $url &&
git checkout -b master origin/master
So setting up an empty tree is basically that:
mkdir foo && cd foo && git init &&
git remote add origin $url
Is that really so difficult? git-clone is a handy crutch for when
we didn't have things like git-remote. Or remote tracking branches.
IMHO the above may seem a little low level but it may make it easier
to teach to newbies. They are more likely to grasp the concept of
their repository being just like someone else's, and that they can
track other repositories beyond just their origin.
--
Shawn.
^ permalink raw reply
* Re: [PATCH] diffcore: Allow users to decide what funcname to use
From: Jakub Narebski @ 2007-11-13 10:03 UTC (permalink / raw)
To: git
In-Reply-To: <47396B4C.6070406@op5.se>
Andreas Ericsson wrote:
> Git can be smarter than that, and imo it should. This
> patch lets the diffcore grok a new configuration variable,
> "diff.funcnames", which can be set to "new", "old", or a
> boolean value, which will cause it to be "old" (for 'true')
> and 'none' (for 'false').
Wouldn't it be better to use existing 'diff driver' infrastructure
for this? See "Defining a custom hunk-header" section in
gitattributes(5).
On the other hand... no.
--
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
^ permalink raw reply
* Re: [PATCH] diffcore: Allow users to decide what funcname to use
From: Andreas Ericsson @ 2007-11-13 10:07 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <fhbsor$ebf$1@ger.gmane.org>
Jakub Narebski wrote:
> Andreas Ericsson wrote:
>
>> Git can be smarter than that, and imo it should. This
>> patch lets the diffcore grok a new configuration variable,
>> "diff.funcnames", which can be set to "new", "old", or a
>> boolean value, which will cause it to be "old" (for 'true')
>> and 'none' (for 'false').
>
> Wouldn't it be better to use existing 'diff driver' infrastructure
> for this? See "Defining a custom hunk-header" section in
> gitattributes(5).
>
> On the other hand... no.
>
It's impossible to do that, since that driver will only ever be
fed the "old" file with the old code. I'm guessing you noticed
that yourself, so just explaining in case anyone else wonders.
--
Andreas Ericsson andreas.ericsson@op5.se
OP5 AB www.op5.se
Tel: +46 8-230225 Fax: +46 8-230231
^ permalink raw reply
* Re: Cloning empty repositories, was Re: What is the idea for bare repositories?
From: Jakub Narebski @ 2007-11-13 10:08 UTC (permalink / raw)
To: git
In-Reply-To: <vpqzlxiiii6.fsf@bauges.imag.fr>
Matthieu Moy wrote:
> I repeat the use-case I mentionned above :
>
> ,----
> | a typical use-case is when I want to create a new project. I'd
> | like to initialize an empty bare repo on my backed up disk, and then
> | clone it to my local-fast-unreliable disk to get a working copy and do
> | the first commit there.
> `----
>
> I find this quite natural, and up to now, no one gave me either a
> rationale not to do that,
The rationale is that current git just simply cannot do this.
You are welcome to add support for this corner case in git-clone,
or add git protocol extension for symref transfer.
--
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
^ permalink raw reply
* [RFC/PATCH 0/3] tracking per-ref errors on push
From: Jeff King @ 2007-11-13 10:25 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Pierre Habouzit, Daniel Barkalow, Alex Riesen
Following is a series to track push errors for individual refs. This
should have three immediate advantages (the first two of which I
implement in the series):
- updating tracking refs only for non-error refs
- incorporating remote rejection into the printed status
- printing errors in a different order than we processed
(e.g., consolidating non-ff errors near the end with
a special message)
The patches are:
1. send-pack: track errors for each ref
This is the groundwork for the other 2. I also think it makes the
code a bit more readable by splitting out the print formatting from
the push logic, and by explicitly naming the different states that
we were previously deducing from the states of other variables.
2. send-pack: check ref->status before updating tracking refs
This should fix the bugs that people are seeing from 334f483
3. send-pack: assign remote errors to each ref
This may have some problems, as I will explain below.
I have a few concerns which make this an RFC and not a real patch
submission:
- I have done pretty minimal testing, and there are no automated tests
(at least 2, which fixes a real bug, should get a test).
Unfortunately, I am out of time to work on this and am leaving the
country for a week. Between that and Thanksgiving travel, I'm not
sure when I'll have time to finish up, so I thought it best to have
comments waiting (and others should feel free to pick up the work if
they want -- Alex, I think your error consolidation should go nicely
on top of this).
- It looks like the push mirror code just made it into next, which
is going to require a conflict-heavy rebase on my part.
- There is a potential performance bottleneck in patch 3. See the
commit message.
- In patch 3, the 'ng' message sent by the remote are not
unambiguously parseable. See the commit message.
-Peff
^ permalink raw reply
* [PATCH/RFC 1/3] send-pack: track errors for each ref
From: Jeff King @ 2007-11-13 10:27 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Pierre Habouzit, Daniel Barkalow, Alex Riesen
In-Reply-To: <20071113102500.GA2767@sigill.intra.peff.net>
Instead of keeping the 'ret' variable, we instead have a
status flag for each ref that tracks what happened to it.
We then print the ref status after all of the refs have
been examined.
This paves the way for three improvements:
- updating tracking refs only for non-error refs
- incorporating remote rejection into the printed status
- printing errors in a different order than we processed
(e.g., consolidating non-ff errors near the end with
a special message)
---
builtin-send-pack.c | 201 ++++++++++++++++++++++++++++-----------------------
cache.h | 13 +++-
2 files changed, 121 insertions(+), 93 deletions(-)
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 5a0f5c6..3ac2615 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -207,8 +207,9 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref)
}
}
-static const char *prettify_ref(const char *name)
+static const char *prettify_ref(const struct ref *ref)
{
+ const char *name = ref->name;
return name + (
!prefixcmp(name, "refs/heads/") ? 11 :
!prefixcmp(name, "refs/tags/") ? 10 :
@@ -218,15 +219,90 @@ static const char *prettify_ref(const char *name)
#define SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
+static void print_ref_status(char flag, const char *summary, struct ref *to, struct ref *from, const char *msg)
+{
+ fprintf(stderr, " %c %-*s ", flag, SUMMARY_WIDTH, summary);
+ if (from)
+ fprintf(stderr, "%s -> %s", prettify_ref(from), prettify_ref(to));
+ else
+ fputs(prettify_ref(to), stderr);
+ if (msg) {
+ fputs(" (", stderr);
+ fputs(msg, stderr);
+ fputc(')', stderr);
+ }
+ fputc('\n', stderr);
+}
+
+
+static void print_push_status(const char *dest, struct ref *refs)
+{
+ struct ref *ref;
+ int shown_dest = 0;
+
+ for (ref = refs; ref; ref = ref->next) {
+ if (!ref->status)
+ continue;
+ if (ref->status == REF_STATUS_UPTODATE && !args.verbose)
+ continue;
+
+ if (!shown_dest) {
+ fprintf(stderr, "To %s\n", dest);
+ shown_dest = 1;
+ }
+
+ switch(ref->status) {
+ case REF_STATUS_NONE:
+ print_ref_status('X', "[no match]", ref, NULL, NULL);
+ break;
+ case REF_STATUS_NODELETE:
+ print_ref_status('!', "[rejected]", ref, NULL,
+ "remote does not support deleting refs");
+ break;
+ case REF_STATUS_UPTODATE:
+ print_ref_status('=', "[up to date]", ref, ref->peer_ref, NULL);
+ break;
+ case REF_STATUS_NONFF:
+ print_ref_status('!', "[rejected]", ref, ref->peer_ref, "non-fast forward");
+ break;
+ case REF_STATUS_OK:
+ if (ref->deletion)
+ print_ref_status('-', "[deleted]", ref, NULL, NULL);
+ else if (is_null_sha1(ref->old_sha1))
+ print_ref_status('*',
+ (prefixcmp(ref->name, "refs/tags/") ?
+ "[new branch]" : "[new tag]"),
+ ref, ref->peer_ref, NULL);
+ else {
+ char quickref[83];
+ char type = ' ';
+ const char *msg = NULL;
+ const char *old_abb;
+
+ old_abb = find_unique_abbrev(ref->old_sha1, DEFAULT_ABBREV);
+ strcpy(quickref, old_abb ? old_abb : sha1_to_hex(ref->old_sha1));
+ if (ref->nonff) {
+ strcat(quickref, "...");
+ type = '+';
+ msg = " (forced update)";
+ }
+ else
+ strcat(quickref, "..");
+ strcat(quickref, find_unique_abbrev(ref->new_sha1, DEFAULT_ABBREV));
+
+ print_ref_status(type, quickref, ref, ref->peer_ref, msg);
+ }
+ }
+ }
+}
+
static int do_send_pack(int in, int out, struct remote *remote, const char *dest, int nr_refspec, const char **refspec)
{
struct ref *ref;
int new_refs;
- int ret = 0;
int ask_for_status_report = 0;
int allow_deleting_refs = 0;
int expect_status_report = 0;
- int shown_dest = 0;
/* No funny business with the matcher */
remote_tail = get_remote_heads(in, &remote_refs, 0, NULL, REF_NORMAL);
@@ -256,35 +332,17 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
*/
new_refs = 0;
for (ref = remote_refs; ref; ref = ref->next) {
- char old_hex[60], *new_hex;
- int will_delete_ref;
- const char *pretty_ref;
- const char *pretty_peer;
-
if (!ref->peer_ref)
continue;
- if (!shown_dest) {
- fprintf(stderr, "To %s\n", dest);
- shown_dest = 1;
- }
-
- pretty_ref = prettify_ref(ref->name);
- pretty_peer = prettify_ref(ref->peer_ref->name);
-
- will_delete_ref = is_null_sha1(ref->peer_ref->new_sha1);
- if (will_delete_ref && !allow_deleting_refs) {
- fprintf(stderr, " ! %-*s %s (remote does not support deleting refs)\n",
- SUMMARY_WIDTH, "[rejected]", pretty_ref);
- ret = -2;
+ ref->deletion = is_null_sha1(ref->peer_ref->new_sha1);
+ if (ref->deletion && !allow_deleting_refs) {
+ ref->status = REF_STATUS_NODELETE;
continue;
}
- if (!will_delete_ref &&
+ if (!ref->deletion &&
!hashcmp(ref->old_sha1, ref->peer_ref->new_sha1)) {
- if (args.verbose)
- fprintf(stderr, " = %-*s %s -> %s\n",
- SUMMARY_WIDTH, "[up to date]",
- pretty_peer, pretty_ref);
+ ref->status = REF_STATUS_UPTODATE;
continue;
}
@@ -307,34 +365,26 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
* always allowed.
*/
- if (!args.force_update &&
- !will_delete_ref &&
+ ref->nonff =
+ !ref->deletion &&
!is_null_sha1(ref->old_sha1) &&
- !ref->force) {
- if (!has_sha1_file(ref->old_sha1) ||
- !ref_newer(ref->peer_ref->new_sha1,
- ref->old_sha1)) {
- /* We do not have the remote ref, or
- * we know that the remote ref is not
- * an ancestor of what we are trying to
- * push. Either way this can be losing
- * commits at the remote end and likely
- * we were not up to date to begin with.
- */
- fprintf(stderr, " ! %-*s %s -> %s (non-fast forward)\n",
- SUMMARY_WIDTH, "[rejected]",
- pretty_peer, pretty_ref);
- ret = -2;
- continue;
- }
+ (!has_sha1_file(ref->old_sha1)
+ || !ref_newer(ref->peer_ref->new_sha1, ref->old_sha1));
+
+ if (ref->nonff && !ref->force && !args.force_update) {
+ ref->status = REF_STATUS_NONFF;
+ continue;
}
+
hashcpy(ref->new_sha1, ref->peer_ref->new_sha1);
- if (!will_delete_ref)
+ if (!ref->deletion)
new_refs++;
- strcpy(old_hex, sha1_to_hex(ref->old_sha1));
- new_hex = sha1_to_hex(ref->new_sha1);
+ ref->status = REF_STATUS_OK;
if (!args.dry_run) {
+ char *old_hex = sha1_to_hex(ref->old_sha1);
+ char *new_hex = sha1_to_hex(ref->new_sha1);
+
if (ask_for_status_report) {
packet_write(out, "%s %s %s%c%s",
old_hex, new_hex, ref->name, 0,
@@ -346,63 +396,32 @@ 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);
}
- if (will_delete_ref)
- fprintf(stderr, " - %-*s %s\n",
- SUMMARY_WIDTH, "[deleting]",
- pretty_ref);
- else if (is_null_sha1(ref->old_sha1)) {
- const char *msg;
-
- if (!prefixcmp(ref->name, "refs/tags/"))
- msg = "[new tag]";
- else
- msg = "[new branch]";
- fprintf(stderr, " * %-*s %s -> %s\n",
- SUMMARY_WIDTH, msg,
- pretty_peer, pretty_ref);
- }
- else {
- char quickref[83];
- char type = ' ';
- const char *msg = "";
- const char *old_abb;
- old_abb = find_unique_abbrev(ref->old_sha1, DEFAULT_ABBREV);
- strcpy(quickref, old_abb ? old_abb : old_hex);
- if (ref_newer(ref->peer_ref->new_sha1, ref->old_sha1))
- strcat(quickref, "..");
- else {
- strcat(quickref, "...");
- type = '+';
- msg = " (forced update)";
- }
- strcat(quickref, find_unique_abbrev(ref->new_sha1, DEFAULT_ABBREV));
-
- fprintf(stderr, " %c %-*s %s -> %s%s\n",
- type,
- SUMMARY_WIDTH, quickref,
- pretty_peer, pretty_ref,
- msg);
- }
}
packet_flush(out);
- if (new_refs && !args.dry_run)
- ret = pack_objects(out, remote_refs);
+ if (new_refs && !args.dry_run) {
+ if (pack_objects(out, remote_refs) < 0) {
+ close(out);
+ return -1;
+ }
+ }
close(out);
+ print_push_status(dest, remote_refs);
+
if (expect_status_report) {
if (receive_status(in))
- ret = -4;
+ return -1;
}
- if (!args.dry_run && remote && ret == 0) {
+ if (!args.dry_run && remote) {
for (ref = remote_refs; ref; ref = ref->next)
update_tracking_ref(remote, ref);
}
- if (!new_refs && ret == 0)
+ if (!new_refs)
fprintf(stderr, "Everything up-to-date\n");
- return ret;
+ return 0;
}
static void verify_remote_names(int nr_heads, const char **heads)
diff --git a/cache.h b/cache.h
index 5f40e12..ca5d96d 100644
--- a/cache.h
+++ b/cache.h
@@ -499,8 +499,17 @@ struct ref {
struct ref *next;
unsigned char old_sha1[20];
unsigned char new_sha1[20];
- unsigned char force;
- unsigned char merge;
+ unsigned char force : 1;
+ unsigned char merge : 1;
+ unsigned char nonff : 1;
+ unsigned char deletion : 1;
+ enum {
+ REF_STATUS_NONE = 0,
+ REF_STATUS_OK,
+ REF_STATUS_NONFF,
+ REF_STATUS_NODELETE,
+ REF_STATUS_UPTODATE,
+ } status;
struct ref *peer_ref; /* when renaming */
char name[FLEX_ARRAY]; /* more */
};
--
1.5.3.5.1704.g24d42-dirty
^ permalink raw reply related
* [PATCH/RFC 2/3] send-pack: check ref->status before updating tracking refs
From: Jeff King @ 2007-11-13 10:27 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Pierre Habouzit, Daniel Barkalow, Alex Riesen
In-Reply-To: <20071113102500.GA2767@sigill.intra.peff.net>
Previously, we manually checked the 'NONE' and 'UPTODATE'
conditions. Now that we have ref->status, we can easily
say "only update if we pushed successfully".
---
builtin-send-pack.c | 15 ++++-----------
1 files changed, 4 insertions(+), 11 deletions(-)
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 3ac2615..2805c92 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -180,24 +180,17 @@ static int receive_status(int in)
static void update_tracking_ref(struct remote *remote, struct ref *ref)
{
struct refspec rs;
- int will_delete_ref;
- rs.src = ref->name;
- rs.dst = NULL;
-
- if (!ref->peer_ref)
+ if (ref->status != REF_STATUS_OK)
return;
- will_delete_ref = is_null_sha1(ref->peer_ref->new_sha1);
-
- if (!will_delete_ref &&
- !hashcmp(ref->old_sha1, ref->peer_ref->new_sha1))
- return;
+ rs.src = ref->name;
+ rs.dst = NULL;
if (!remote_find_tracking(remote, &rs)) {
if (args.verbose)
fprintf(stderr, "updating local tracking ref '%s'\n", rs.dst);
- if (is_null_sha1(ref->peer_ref->new_sha1)) {
+ if (ref->deletion) {
if (delete_ref(rs.dst, NULL))
error("Failed to delete");
} else
--
1.5.3.5.1704.g24d42-dirty
^ permalink raw reply related
* [PATCH/RFC 3/3] send-pack: assign remote errors to each ref
From: Jeff King @ 2007-11-13 10:29 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Pierre Habouzit, Daniel Barkalow, Alex Riesen
In-Reply-To: <20071113102500.GA2767@sigill.intra.peff.net>
This lets us show remote errors (e.g., a denied hook) along
with the usual push output. There are two drawbacks to this
change:
1. cross-referencing the incoming status with the ref list
is worst case O(n^2) (where n = number of refs); this
can be fixed with a smarter implementation
2. the status parsing is not foolproof. We get a line like
ng refs/heads/master arbitrary msg
which cannot be parsed unambiguously in the face of
refnames with spaces. We do a prefix-match so that
you will only run into problems if you have two refs,
one of which is a prefix match of the other, and the
longer having a space right after the prefix.
---
builtin-send-pack.c | 44 +++++++++++++++++++++++++++++++++++++-------
cache.h | 2 ++
2 files changed, 39 insertions(+), 7 deletions(-)
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 2805c92..a2307fa 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -146,14 +146,34 @@ static void get_local_heads(void)
for_each_ref(one_local_ref, NULL);
}
-static int receive_status(int in)
+static void 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;
+ }
+}
+
+/* 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)
{
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 -1;
+ return -2;
}
if (memcmp(line, "unpack ok\n", 10)) {
fputs(line, stderr);
@@ -171,7 +191,7 @@ static int receive_status(int in)
}
if (!memcmp(line, "ok", 2))
continue;
- fputs(line, stderr);
+ set_ref_error(refs, line + 3);
ret = -1;
}
return ret;
@@ -258,6 +278,12 @@ static void print_push_status(const char *dest, struct ref *refs)
case REF_STATUS_NONFF:
print_ref_status('!', "[rejected]", ref, ref->peer_ref, "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);
+ break;
case REF_STATUS_OK:
if (ref->deletion)
print_ref_status('-', "[deleted]", ref, NULL, NULL);
@@ -296,6 +322,7 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
int ask_for_status_report = 0;
int allow_deleting_refs = 0;
int expect_status_report = 0;
+ int ret;
/* No funny business with the matcher */
remote_tail = get_remote_heads(in, &remote_refs, 0, NULL, REF_NORMAL);
@@ -400,12 +427,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;
}
+ else
+ ret = 0;
+
+ print_push_status(dest, remote_refs);
if (!args.dry_run && remote) {
for (ref = remote_refs; ref; ref = ref->next)
@@ -414,7 +444,7 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
if (!new_refs)
fprintf(stderr, "Everything up-to-date\n");
- return 0;
+ return ret;
}
static void verify_remote_names(int nr_heads, const char **heads)
diff --git a/cache.h b/cache.h
index ca5d96d..082e03b 100644
--- a/cache.h
+++ b/cache.h
@@ -509,7 +509,9 @@ struct ref {
REF_STATUS_NONFF,
REF_STATUS_NODELETE,
REF_STATUS_UPTODATE,
+ REF_STATUS_REMOTE_REJECT,
} status;
+ char *error;
struct ref *peer_ref; /* when renaming */
char name[FLEX_ARRAY]; /* more */
};
--
1.5.3.5.1704.g24d42-dirty
^ permalink raw reply related
* Re: [PATCH/RFC 3/3] send-pack: assign remote errors to each ref
From: Jeff King @ 2007-11-13 10:29 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Pierre Habouzit, Daniel Barkalow, Alex Riesen
In-Reply-To: <20071113102900.GC2905@sigill.intra.peff.net>
On Tue, Nov 13, 2007 at 05:29:00AM -0500, Jeff King wrote:
> + ref->error = xstrdup(msg);
Another problem with this patch: this is a leak.
-Peff
^ permalink raw reply
* Re: Cloning empty repositories, was Re: What is the idea for bare repositories?
From: Matthieu Moy @ 2007-11-13 10:50 UTC (permalink / raw)
To: Shawn O. Pearce
Cc: Junio C Hamano, Johannes Schindelin, Bill Lear, Jan Wielemaker,
git
In-Reply-To: <20071113100209.GE14735@spearce.org>
"Shawn O. Pearce" <spearce@spearce.org> writes:
> So setting up an empty tree is basically that:
>
> mkdir foo && cd foo && git init &&
> git remote add origin $url
It is not.
The "git remote add" thing adds this to my .git/config:
[remote "origin"]
url = /tmp/git1
fetch = +refs/heads/*:refs/remotes/origin/*
While clone normally does a bit more:
[remote "origin"]
url = /tmp/git1/.git
fetch = +refs/heads/*:refs/remotes/origin/*
[branch "master"]
remote = origin
merge = refs/heads/master
So, it's really
$ git remote add origin url
$ $EDITOR .git/config # or perhaps I missed the way to set the two
# options easily.
I find it so conveinient to have it for non-empty clones, it's
frustrating to have to do it by hand for empty clones.
--
Matthieu
^ permalink raw reply
* Re: wishlist: git info
From: Johannes Schindelin @ 2007-11-13 11:13 UTC (permalink / raw)
To: Thomas Neumann; +Cc: git
In-Reply-To: <fhbn50$uqu$1@ger.gmane.org>
Hi,
On Tue, 13 Nov 2007, Thomas Neumann wrote:
> Perhaps also project description (if it exists?) one can specify a
> project description? I did not even know this. But yes, this would be
> useful, too. In general I think git info should show everything to
> quickly understand what is currently checked out.
Is slightly troubles me that you put so much emphasis on what I would call
"remote information". I understand that in svn, your working directory
without the server is not very useful. But we do not have that problem.
> The name of the current branch should probably be included, too.
FWIW I think a much better idea is to have that bash prompt that was
posted some months ago; there's not even a need to run a program manually
then!
> I use an alias with the commands proposed by Alex Riessen for now, but a
> more general command would be nice.
His name is "Riesen", just like in the German translation of the famous
Newton statement.
Ciao,
Dscho
^ permalink raw reply
* Re: Cloning empty repositories, was Re: What is the idea for bare repositories?
From: Johannes Schindelin @ 2007-11-13 11:19 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Junio C Hamano, Bill Lear, Jan Wielemaker, git
In-Reply-To: <vpqzlxiiii6.fsf@bauges.imag.fr>
Hi,
On Tue, 13 Nov 2007, Matthieu Moy wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > But both of Johannes's points apply equally well to an empty bare
> > repository and to an empty non bare repository. IOW, bareness does
> > not matter to the suggestion Johannes gave.
>
> He was suggesting to create the initial commit before cloning:
>
> >> So you need to populate the repository before starting _anyway_.
Of course I was suggesting to push into the still empty, bare repository,
before anybody is cloning.
Ciao,
Dscho
^ permalink raw reply
* Re: wishlist: git info
From: Thomas Neumann @ 2007-11-13 11:32 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <Pine.LNX.4.64.0711131111220.4362@racer.site>
> Is slightly troubles me that you put so much emphasis on what I would call
> "remote information". I understand that in svn, your working directory
> without the server is not very useful. But we do not have that problem.
that is true. My usage pattern probably stems from the fact that I am a
long term svn user :) And I use git for work now, where there is indeed
some kind of central repository just as in a Subversion setting.
In a fully decentralized setting the remote information is probably not
as important, although you might still want to know what happens if you
issue "git pull".
> FWIW I think a much better idea is to have that bash prompt that was
> posted some months ago; there's not even a need to run a program manually
> then!
a bash prompt is nice too, of course. But there is only so much
information you can reasonably encode in the prompt.
When you know the remote url (ok, this assumes a "centralized" model),
branch, head commit and date of the head commit (this is just for
humans), you know very precisely what you are looking at. For the more
decentralized users some other information might be relevant, I don't know.
While the head commit hash is enough to identify a point in the revision
history, the other information allows a human to identify the point in
the revision history easily. So I can see what is checked out, how old
the checkout is etc.
> His name is "Riesen", just like in the German translation of the famous
> Newton statement.
sorry for the typo, I noticed it just the moment I had pressed send...
Sometimes I really wish I could edit mails after sending them.
Thomas
^ permalink raw reply
* Re: [PATCH/RFC 1/3] send-pack: track errors for each ref
From: Jeff King @ 2007-11-13 11:32 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Pierre Habouzit, Daniel Barkalow, Alex Riesen
In-Reply-To: <20071113102709.GA2905@sigill.intra.peff.net>
On Tue, Nov 13, 2007 at 05:27:09AM -0500, Jeff King wrote:
> Instead of keeping the 'ret' variable, we instead have a
> status flag for each ref that tracks what happened to it.
> We then print the ref status after all of the refs have
> been examined.
Argh, this fails t5400 quite badly without the following patch:
---
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 3ac2615..eff84e0 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -421,6 +421,16 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
if (!new_refs)
fprintf(stderr, "Everything up-to-date\n");
+ for (ref = remote_refs; ref; ref = ref->next) {
+ switch (ref->status) {
+ case REF_STATUS_NONE:
+ case REF_STATUS_UPTODATE:
+ case REF_STATUS_OK:
+ break;
+ default:
+ return -1;
+ }
+ }
return 0;
}
^ permalink raw reply related
* Re: [RFC/PATCH 0/3] tracking per-ref errors on push
From: Jeff King @ 2007-11-13 11:34 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Pierre Habouzit, Daniel Barkalow, Alex Riesen
In-Reply-To: <20071113102500.GA2767@sigill.intra.peff.net>
On Tue, Nov 13, 2007 at 05:25:01AM -0500, Jeff King wrote:
> - It looks like the push mirror code just made it into next, which
> is going to require a conflict-heavy rebase on my part.
This turned out not to be too bad. A rebased series (with the extra
patch I just posted) follows, and it passes the test suite (though I
still think more tests are in order).
-Peff
^ permalink raw reply
* [PATCH v2 1/3] send-pack: track errors for each ref
From: Jeff King @ 2007-11-13 11:36 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Pierre Habouzit, Daniel Barkalow, Alex Riesen
In-Reply-To: <20071113102500.GA2767@sigill.intra.peff.net>
Instead of keeping the 'ret' variable, we instead have a
status flag for each ref that tracks what happened to it.
We then print the ref status after all of the refs have
been examined.
This paves the way for three improvements:
- updating tracking refs only for non-error refs
- incorporating remote rejection into the printed status
- printing errors in a different order than we processed
(e.g., consolidating non-ff errors near the end with
a special message)
Signed-off-by: Jeff King <peff@peff.net>
---
builtin-send-pack.c | 213 +++++++++++++++++++++++++++++----------------------
cache.h | 13 +++-
2 files changed, 132 insertions(+), 94 deletions(-)
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 418925e..c01a9c4 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -207,8 +207,9 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref)
}
}
-static const char *prettify_ref(const char *name)
+static const char *prettify_ref(const struct ref *ref)
{
+ const char *name = ref->name;
return name + (
!prefixcmp(name, "refs/heads/") ? 11 :
!prefixcmp(name, "refs/tags/") ? 10 :
@@ -218,15 +219,90 @@ static const char *prettify_ref(const char *name)
#define SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
+static void print_ref_status(char flag, const char *summary, struct ref *to, struct ref *from, const char *msg)
+{
+ fprintf(stderr, " %c %-*s ", flag, SUMMARY_WIDTH, summary);
+ if (from)
+ fprintf(stderr, "%s -> %s", prettify_ref(from), prettify_ref(to));
+ else
+ fputs(prettify_ref(to), stderr);
+ if (msg) {
+ fputs(" (", stderr);
+ fputs(msg, stderr);
+ fputc(')', stderr);
+ }
+ fputc('\n', stderr);
+}
+
+
+static void print_push_status(const char *dest, struct ref *refs)
+{
+ struct ref *ref;
+ int shown_dest = 0;
+
+ for (ref = refs; ref; ref = ref->next) {
+ if (!ref->status)
+ continue;
+ if (ref->status == REF_STATUS_UPTODATE && !args.verbose)
+ continue;
+
+ if (!shown_dest) {
+ fprintf(stderr, "To %s\n", dest);
+ shown_dest = 1;
+ }
+
+ switch(ref->status) {
+ case REF_STATUS_NONE:
+ print_ref_status('X', "[no match]", ref, NULL, NULL);
+ break;
+ case REF_STATUS_NODELETE:
+ print_ref_status('!', "[rejected]", ref, NULL,
+ "remote does not support deleting refs");
+ break;
+ case REF_STATUS_UPTODATE:
+ print_ref_status('=', "[up to date]", ref, ref->peer_ref, NULL);
+ break;
+ case REF_STATUS_NONFF:
+ print_ref_status('!', "[rejected]", ref, ref->peer_ref, "non-fast forward");
+ break;
+ case REF_STATUS_OK:
+ if (ref->deletion)
+ print_ref_status('-', "[deleted]", ref, NULL, NULL);
+ else if (is_null_sha1(ref->old_sha1))
+ print_ref_status('*',
+ (prefixcmp(ref->name, "refs/tags/") ?
+ "[new branch]" : "[new tag]"),
+ ref, ref->peer_ref, NULL);
+ else {
+ char quickref[83];
+ char type = ' ';
+ const char *msg = NULL;
+ const char *old_abb;
+
+ old_abb = find_unique_abbrev(ref->old_sha1, DEFAULT_ABBREV);
+ strcpy(quickref, old_abb ? old_abb : sha1_to_hex(ref->old_sha1));
+ if (ref->nonff) {
+ strcat(quickref, "...");
+ type = '+';
+ msg = " (forced update)";
+ }
+ else
+ strcat(quickref, "..");
+ strcat(quickref, find_unique_abbrev(ref->new_sha1, DEFAULT_ABBREV));
+
+ print_ref_status(type, quickref, ref, ref->peer_ref, msg);
+ }
+ }
+ }
+}
+
static int do_send_pack(int in, int out, struct remote *remote, const char *dest, int nr_refspec, const char **refspec)
{
struct ref *ref;
int new_refs;
- int ret = 0;
int ask_for_status_report = 0;
int allow_deleting_refs = 0;
int expect_status_report = 0;
- int shown_dest = 0;
int flags = MATCH_REFS_NONE;
if (args.send_all)
@@ -262,10 +338,6 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
*/
new_refs = 0;
for (ref = remote_refs; ref; ref = ref->next) {
- char old_hex[60], *new_hex;
- int will_delete_ref;
- const char *pretty_ref;
- const char *pretty_peer = NULL; /* only used when not deleting */
const unsigned char *new_sha1;
if (!ref->peer_ref) {
@@ -276,29 +348,15 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
else
new_sha1 = ref->peer_ref->new_sha1;
- if (!shown_dest) {
- fprintf(stderr, "To %s\n", dest);
- shown_dest = 1;
- }
-
- will_delete_ref = is_null_sha1(new_sha1);
-
- pretty_ref = prettify_ref(ref->name);
- if (!will_delete_ref)
- pretty_peer = prettify_ref(ref->peer_ref->name);
- if (will_delete_ref && !allow_deleting_refs) {
- fprintf(stderr, " ! %-*s %s (remote does not support deleting refs)\n",
- SUMMARY_WIDTH, "[rejected]", pretty_ref);
- ret = -2;
+ ref->deletion = is_null_sha1(new_sha1);
+ if (ref->deletion && !allow_deleting_refs) {
+ ref->status = REF_STATUS_NODELETE;
continue;
}
- if (!will_delete_ref &&
+ if (!ref->deletion &&
!hashcmp(ref->old_sha1, new_sha1)) {
- if (args.verbose)
- fprintf(stderr, " = %-*s %s -> %s\n",
- SUMMARY_WIDTH, "[up to date]",
- pretty_peer, pretty_ref);
+ ref->status = REF_STATUS_UPTODATE;
continue;
}
@@ -321,33 +379,26 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
* always allowed.
*/
- if (!args.force_update &&
- !will_delete_ref &&
+ ref->nonff =
+ !ref->deletion &&
!is_null_sha1(ref->old_sha1) &&
- !ref->force) {
- if (!has_sha1_file(ref->old_sha1) ||
- !ref_newer(new_sha1, ref->old_sha1)) {
- /* We do not have the remote ref, or
- * we know that the remote ref is not
- * an ancestor of what we are trying to
- * push. Either way this can be losing
- * commits at the remote end and likely
- * we were not up to date to begin with.
- */
- fprintf(stderr, " ! %-*s %s -> %s (non-fast forward)\n",
- SUMMARY_WIDTH, "[rejected]",
- pretty_peer, pretty_ref);
- ret = -2;
- continue;
- }
+ (!has_sha1_file(ref->old_sha1)
+ || !ref_newer(new_sha1, ref->old_sha1));
+
+ if (ref->nonff && !ref->force && !args.force_update) {
+ ref->status = REF_STATUS_NONFF;
+ continue;
}
+
hashcpy(ref->new_sha1, new_sha1);
- if (!will_delete_ref)
+ if (!ref->deletion)
new_refs++;
- strcpy(old_hex, sha1_to_hex(ref->old_sha1));
- new_hex = sha1_to_hex(ref->new_sha1);
+ ref->status = REF_STATUS_OK;
if (!args.dry_run) {
+ char *old_hex = sha1_to_hex(ref->old_sha1);
+ char *new_hex = sha1_to_hex(ref->new_sha1);
+
if (ask_for_status_report) {
packet_write(out, "%s %s %s%c%s",
old_hex, new_hex, ref->name, 0,
@@ -359,64 +410,42 @@ 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);
}
- if (will_delete_ref)
- fprintf(stderr, " - %-*s %s\n",
- SUMMARY_WIDTH, "[deleting]",
- pretty_ref);
- else if (is_null_sha1(ref->old_sha1)) {
- const char *msg;
-
- if (!prefixcmp(ref->name, "refs/tags/"))
- msg = "[new tag]";
- else
- msg = "[new branch]";
- fprintf(stderr, " * %-*s %s -> %s\n",
- SUMMARY_WIDTH, msg,
- pretty_peer, pretty_ref);
- }
- else {
- char quickref[83];
- char type = ' ';
- const char *msg = "";
- const char *old_abb;
- old_abb = find_unique_abbrev(ref->old_sha1, DEFAULT_ABBREV);
- strcpy(quickref, old_abb ? old_abb : old_hex);
- if (ref_newer(ref->peer_ref->new_sha1, ref->old_sha1))
- strcat(quickref, "..");
- else {
- strcat(quickref, "...");
- type = '+';
- msg = " (forced update)";
- }
- strcat(quickref, find_unique_abbrev(ref->new_sha1, DEFAULT_ABBREV));
-
- fprintf(stderr, " %c %-*s %s -> %s%s\n",
- type,
- SUMMARY_WIDTH, quickref,
- pretty_peer, pretty_ref,
- msg);
- }
}
packet_flush(out);
- if (new_refs && !args.dry_run)
- ret = pack_objects(out, remote_refs);
+ if (new_refs && !args.dry_run) {
+ if (pack_objects(out, remote_refs) < 0) {
+ close(out);
+ return -1;
+ }
+ }
close(out);
+ print_push_status(dest, remote_refs);
+
if (expect_status_report) {
if (receive_status(in))
- ret = -4;
+ return -1;
}
- if (!args.dry_run && remote && ret == 0) {
+ if (!args.dry_run && remote) {
for (ref = remote_refs; ref; ref = ref->next)
- if (!is_null_sha1(ref->new_sha1))
- update_tracking_ref(remote, ref);
+ update_tracking_ref(remote, ref);
}
- if (!new_refs && ret == 0)
+ if (!new_refs)
fprintf(stderr, "Everything up-to-date\n");
- return ret;
+ for (ref = remote_refs; ref; ref = ref->next) {
+ switch (ref->status) {
+ case REF_STATUS_NONE:
+ case REF_STATUS_UPTODATE:
+ case REF_STATUS_OK:
+ break;
+ default:
+ return -1;
+ }
+ }
+ return 0;
}
static void verify_remote_names(int nr_heads, const char **heads)
diff --git a/cache.h b/cache.h
index 5f40e12..ca5d96d 100644
--- a/cache.h
+++ b/cache.h
@@ -499,8 +499,17 @@ struct ref {
struct ref *next;
unsigned char old_sha1[20];
unsigned char new_sha1[20];
- unsigned char force;
- unsigned char merge;
+ unsigned char force : 1;
+ unsigned char merge : 1;
+ unsigned char nonff : 1;
+ unsigned char deletion : 1;
+ enum {
+ REF_STATUS_NONE = 0,
+ REF_STATUS_OK,
+ REF_STATUS_NONFF,
+ REF_STATUS_NODELETE,
+ REF_STATUS_UPTODATE,
+ } status;
struct ref *peer_ref; /* when renaming */
char name[FLEX_ARRAY]; /* more */
};
--
1.5.3.5.1731.g0763
^ permalink raw reply related
* [PATCH v2 2/3] send-pack: check ref->status before updating tracking refs
From: Jeff King @ 2007-11-13 11:36 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Pierre Habouzit, Daniel Barkalow, Alex Riesen
In-Reply-To: <20071113102500.GA2767@sigill.intra.peff.net>
Previously, we manually checked the 'NONE' and 'UPTODATE'
conditions. Now that we have ref->status, we can easily
say "only update if we pushed successfully".
Signed-off-by: Jeff King <peff@peff.net>
---
builtin-send-pack.c | 15 ++++-----------
1 files changed, 4 insertions(+), 11 deletions(-)
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index c01a9c4..5c84766 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -180,24 +180,17 @@ static int receive_status(int in)
static void update_tracking_ref(struct remote *remote, struct ref *ref)
{
struct refspec rs;
- int will_delete_ref;
- rs.src = ref->name;
- rs.dst = NULL;
-
- if (!ref->peer_ref)
+ if (ref->status != REF_STATUS_OK)
return;
- will_delete_ref = is_null_sha1(ref->peer_ref->new_sha1);
-
- if (!will_delete_ref &&
- !hashcmp(ref->old_sha1, ref->peer_ref->new_sha1))
- return;
+ rs.src = ref->name;
+ rs.dst = NULL;
if (!remote_find_tracking(remote, &rs)) {
if (args.verbose)
fprintf(stderr, "updating local tracking ref '%s'\n", rs.dst);
- if (is_null_sha1(ref->peer_ref->new_sha1)) {
+ if (ref->deletion) {
if (delete_ref(rs.dst, NULL))
error("Failed to delete");
} else
--
1.5.3.5.1731.g0763
^ permalink raw reply related
* [PATCH v2 3/3] send-pack: assign remote errors to each ref
From: Jeff King @ 2007-11-13 11:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Pierre Habouzit, Daniel Barkalow, Alex Riesen
In-Reply-To: <20071113102500.GA2767@sigill.intra.peff.net>
This lets us show remote errors (e.g., a denied hook) along
with the usual push output. There are two drawbacks to this
change:
1. cross-referencing the incoming status with the ref list
is worst case O(n^2) (where n = number of refs); this
can be fixed with a smarter implementation
2. the status parsing is not foolproof. We get a line like
ng refs/heads/master arbitrary msg
which cannot be parsed unambiguously in the face of
refnames with spaces. We do a prefix-match so that
you will only run into problems if you have two refs,
one of which is a prefix match of the other, and the
longer having a space right after the prefix.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin-send-pack.c | 44 ++++++++++++++++++++++++++++++++++++++------
cache.h | 2 ++
2 files changed, 40 insertions(+), 6 deletions(-)
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 5c84766..7d466d9 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -146,14 +146,34 @@ static void get_local_heads(void)
for_each_ref(one_local_ref, NULL);
}
-static int receive_status(int in)
+static void 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;
+ }
+}
+
+/* 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)
{
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 -1;
+ return -2;
}
if (memcmp(line, "unpack ok\n", 10)) {
fputs(line, stderr);
@@ -171,7 +191,7 @@ static int receive_status(int in)
}
if (!memcmp(line, "ok", 2))
continue;
- fputs(line, stderr);
+ set_ref_error(refs, line + 3);
ret = -1;
}
return ret;
@@ -258,6 +278,12 @@ static void print_push_status(const char *dest, struct ref *refs)
case REF_STATUS_NONFF:
print_ref_status('!', "[rejected]", ref, ref->peer_ref, "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);
+ break;
case REF_STATUS_OK:
if (ref->deletion)
print_ref_status('-', "[deleted]", ref, NULL, NULL);
@@ -297,6 +323,7 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
int allow_deleting_refs = 0;
int expect_status_report = 0;
int flags = MATCH_REFS_NONE;
+ int ret;
if (args.send_all)
flags |= MATCH_REFS_ALL;
@@ -414,12 +441,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;
}
+ else
+ ret = 0;
+
+ print_push_status(dest, remote_refs);
if (!args.dry_run && remote) {
for (ref = remote_refs; ref; ref = ref->next)
@@ -428,6 +458,8 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
if (!new_refs)
fprintf(stderr, "Everything up-to-date\n");
+ if (ret < 0)
+ return ret;
for (ref = remote_refs; ref; ref = ref->next) {
switch (ref->status) {
case REF_STATUS_NONE:
diff --git a/cache.h b/cache.h
index ca5d96d..082e03b 100644
--- a/cache.h
+++ b/cache.h
@@ -509,7 +509,9 @@ struct ref {
REF_STATUS_NONFF,
REF_STATUS_NODELETE,
REF_STATUS_UPTODATE,
+ REF_STATUS_REMOTE_REJECT,
} status;
+ char *error;
struct ref *peer_ref; /* when renaming */
char name[FLEX_ARRAY]; /* more */
};
--
1.5.3.5.1731.g0763
^ 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