* Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
From: Jonathan Nieder @ 2012-10-31 1:51 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Jeff King, Junio C Hamano, Sverre Rabbelier,
Johannes Schindelin, Elijah Newren
In-Reply-To: <CAMP44s0RcbAiUmvGACxO+H-b-anQSPXxUqUuZwYRKWfrpXYeew@mail.gmail.com>
Felipe Contreras wrote:
> It's not my job to
> explain to you that 'git fast-export' doesn't work this way, you have
> a command line to type those commands and see for yourself if they do
> what you think they do with a vanilla version of git. That's exactly
> what I did, to make sure I'm not using assumptions as basis for
> arguing, it took me a few minutes.
Well no, when I run "git blame" 10 years down the line and do not
understand what your code is doing, it is not at all reasonable to
expect me to checkout the parent commit, get it to compile with a
modern toolchain, and type those commands for myself.
Instead, the commit message should be self-contained and explain what
the patch does.
That has multiple parts:
- first, what the current behavior is
- second, what the intent behind the current behavior is. This is
crucial information because presumably we want the change not to
break that.
- third, what change the patch makes
- fourth, what the consequences of that are, in terms of new use
cases that become possible and old use cases that become less
convenient
- fifth, optionally, how the need for this change was discovered
(real-life usage, code inspection, or something else)
- sixth, optionally, implementation considerations and alternate
approaches that were discarded
If you run "git log", you'll see many good and bad examples to think
over and compare to this goal. It's hard work to describe one's work
well in terms that other people can understand, but I think it can be
satisfying, and in any event, it's just as necessary as including
comments near confusing code.
Sincerely,
Jonathan
^ permalink raw reply
* Re: [PATCH] test-lib: avoid full path to store test results
From: Felipe Contreras @ 2012-10-31 1:59 UTC (permalink / raw)
To: Jonathan Nieder
Cc: git, Junio C Hamano, Jeff King, Ævar Arnfjörð,
Johannes Sixt
In-Reply-To: <20121031012730.GY15167@elie.Belkin>
On Wed, Oct 31, 2012 at 2:27 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>> On Tue, Oct 30, 2012 at 5:46 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> > Felipe Contreras wrote:
>
>>>> No reason to use the full path in case this is used externally.
>>>>
>>>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>>>
>>> "No reason not to" is not a reason to do anything. What symptoms does
>>> this prevent? Could you describe the benefit of this patch in a
>>> paragraph starting "Now you can ..."?
>>
>> ./test-lib.sh: line 394:
>> /home/felipec/dev/git/t/test-results//home/felipec/dev/git/contrib/remote-hg/test-21865.counts:
>> No such file or directory
>
> Ok, so a description for this patch is
>
> test: use test's basename to name results file
Is this solving an actual problem or is it just something nice to do?
Like in all good novels, one has to read more find out...
> Running a test using its full path produces an error:
I'm not sure what that even means. Do you mean this produces an error?
% make -C t $PWD/t9902-completion.sh
Well, sure it does, but this patch doesn't fix that.
If you want a precise explanation of what kind of usages are enabled
by this patch, that would require some work, and no I haven't done it,
and no, I'm not sure.
> $ ~/dev/git/contrib/remote-hg/test-21865.sh
> [...]
> ./test-lib.sh: line 394: /home/felipec/dev/t/\
> test-results//home/felipec/dev/git/contrib/remote-hg/\
> test-21865.counts: No such file or directory
Except that I didn't do this. So the fact that this happens is an
assumption, and I'm not willing to make that.
Most likely if somebody does that they are doing something wrong; they
didn't define the TESTDIR variable (or something like that).
It's all fun and games to write explanations for things, but it's not
that easy when you want those explanations to be actually true, and
corrent--you have to spend time to make sure of that.
> In --tee and --valgrind modes we already use the basename
> to name the .out and .exit files; this patch teaches the test-lib
> to name the .counts file the same way.
I don't see the point of listing each and every place where this
already happens. As a matter of fact, the base-name is used in other
places as well, and just saying "This is already done in other
places", is more than enough. But who says they are not the ones doing
it wrong? Maybe this part of the code is right, and it's the others
that need fixing. I don't see how saying "Others are doing it" makes
the patch better or worse in any way. There might also be different
reasons for why they do it that doesn't apply here.
> That is still not enough to tell if it is a good change, though.
> Should the test results for contrib/remote-hg/test-* be included with
> the results for t/t*.sh when I run "make aggregate-results"?
>
> Before 60d02ccc, git-svn had its own testsuite under contrib/, with
> glue in contrib/git-svn/t/lib-git-svn.sh to use test-lib --- maybe
> that code could provide some inspiration for questions like these.
Or maybe they are the ones that should look for inspiration in
contrib/remote-hg.
The patch is obviously correct; it's generally good not to name files
with slashes in them, and $0 is not guaranteed not to have slashes.
Even if you run all the tests inside the 't' directory, this script is
not only used by git, and others might want sub-directories, and not
thousands of tests on the same directory like git.
Either way, if obvious fixes that are one-liners require an essay for
you, I give up.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly
From: Felipe Contreras @ 2012-10-31 2:08 UTC (permalink / raw)
To: Jonathan Nieder
Cc: git, Jeff King, Junio C Hamano, Sverre Rabbelier,
Johannes Schindelin, Elijah Newren
In-Reply-To: <20121031001117.GA29486@elie.Belkin>
On Wed, Oct 31, 2012 at 1:11 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> (cc-ing the git list)
> Felipe Contreras wrote:
>
>> When an object has already been exported (and thus is in the marks) it
>> is flagged as SHOWN, so it will not be exported again, even if this time
>> it's exported through a different ref.
>>
>> We don't need the object to be exported again, but we want the ref
>> updated
>
> Yes, makes perfect sense.
>
> For what it's worth,
> Acked-by: Jonathan Nieder <jrnieder@gmail.com>
Yay!
> [...]
>> --- a/t/t5800-remote-helpers.sh
>> +++ b/t/t5800-remote-helpers.sh
>> @@ -145,4 +145,15 @@ test_expect_failure 'push new branch with old:new refspec' '
>> compare_refs clone HEAD server refs/heads/new-refspec
>> '
>>
>> +test_expect_success 'push ref with existing object' '
>> + (cd localclone &&
>> + git branch point-to-master master &&
>> + git push origin point-to-master
>> + ) &&
>> +
>> + (cd server &&
>> + git show-ref refs/heads/point-to-master
>> + )
>
> Style: if you indent like this, the test becomes clearer:
And then it would become inconsistent with the rest of the file.
>> + git fast-export --import-marks=tmp-marks \
>> + --export-marks=tmp-marks master > actual &&
>> + test_cmp expected actual
>
> Redirections in git shell scripts are generally spelled as
> "do_something >actual", without a space between the operator and
> filename.
I generally am OK with adapting to whatever code-style is used
(sometimes under protest), but this is a place where I draw the
line. Sorry, '>actual' is more annoying to me than a knife screeching
glass. Fortunately, '> actual' is used in many other places in 't/',
so I'm going to use the other people jumping over the bridge argument.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly
From: Felipe Contreras @ 2012-10-31 2:13 UTC (permalink / raw)
To: Jonathan Nieder
Cc: git, Jeff King, Junio C Hamano, Sverre Rabbelier,
Johannes Schindelin, Elijah Newren
In-Reply-To: <20121031003721.GV15167@elie.Belkin>
On Wed, Oct 31, 2012 at 1:37 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> --- a/builtin/fast-export.c
>> +++ b/builtin/fast-export.c
>> @@ -523,11 +523,16 @@ static void get_tags_and_duplicates(struct object_array *pending,
>> typename(e->item->type));
>> continue;
>> }
>> - if (commit->util) {
>> - /* more than one name for the same object */
>> +
>> + /*
>> + * This ref will not be updated through a commit, lets make
>> + * sure it gets properly upddated eventually.
>> + */
>> + if (commit->util || commit->object.flags & SHOWN) {
>> if (!(commit->object.flags & UNINTERESTING))
>> string_list_append(extra_refs, full_name)->util = commit;
>> - } else
>> + }
>> + if (!commit->util)
>> commit->util = full_name;
>
> Here's an explanation of why the above makes sense to me.
>
> get_tags_and_duplicates() gets called after the marks import and
> before the revision walk. It walks through the revs from the
> commandline and for each one:
>
> - peels it to a refname, and then to a commit
> - stores the refname so fast-export knows what arg to pass to
> the "commit" command during the revision walk
> - if it already had a refname stored, instead adds the
> (refname, commit) pair to the extra_refs list, so fast-export
> knows to add a "reset" command later.
>
> If the commit already has the SHOWN flag set because it was pointed to
> by a mark, it is not going to come up in the revision walk, so it will
> not be mentioned in the output stream unless it is added to
> extra_refs. That's what this patch does.
That is correct.
> Incidentally, the change from "else" to "if (!commit->util)" is
> unnecessary because if a commit is already SHOWN then it will not be
> encountered in the revision walk so commit->util does not need to be
> set.
Maybe, but that's yet another change, and with more changes come more
possibilities of regressions. I haven't verified this is the case.
If this makes sense, I would do it in another, separate patch.
> If the commit does not have the SHOWN or UNINTERESTING flag set but it
> is going to get the UNINTERESTING flag set during the walk because of
> a negative commit listed on the command line, this patch won't help.
I don't know what that means in practice.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH] test-lib: avoid full path to store test results
From: Jonathan Nieder @ 2012-10-31 2:13 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Junio C Hamano, Jeff King, Ævar Arnfjörð,
Johannes Sixt
In-Reply-To: <CAMP44s1xAeW2QZsNwRVRx+DEhYVVdiKbw-y-aNuo6unuv7pYZQ@mail.gmail.com>
Felipe Contreras wrote:
> It's all fun and games to write explanations for things, but it's not
> that easy when you want those explanations to be actually true, and
> corrent--you have to spend time to make sure of that.
That's why it's useful for the patch submitter to write them, asking
for help when necessary.
As a bonus, it helps reviewers understand the effect of the patch.
Bugs averted!
[...]
> Either way, if obvious fixes that are one-liners require an essay for
> you, I give up.
I guess it is fair to call a reasonable subject line plus a couple of
sentences an essay. Yes, obvious fixes especially require that, since
the bug caused by an obvious fix is one of the worst kinds.
^ permalink raw reply
* Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
From: Felipe Contreras @ 2012-10-31 2:22 UTC (permalink / raw)
To: Jonathan Nieder
Cc: git, Jeff King, Junio C Hamano, Sverre Rabbelier,
Johannes Schindelin, Elijah Newren
In-Reply-To: <20121031015103.GA15167@elie.Belkin>
On Wed, Oct 31, 2012 at 2:51 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> It's not my job to
>> explain to you that 'git fast-export' doesn't work this way, you have
>> a command line to type those commands and see for yourself if they do
>> what you think they do with a vanilla version of git. That's exactly
>> what I did, to make sure I'm not using assumptions as basis for
>> arguing, it took me a few minutes.
>
> Well no, when I run "git blame" 10 years down the line and do not
> understand what your code is doing, it is not at all reasonable to
> expect me to checkout the parent commit, get it to compile with a
> modern toolchain, and type those commands for myself.
>
> Instead, the commit message should be self-contained and explain what
> the patch does.
>
> That has multiple parts:
>
> - first, what the current behavior is
>
> - second, what the intent behind the current behavior is. This is
> crucial information because presumably we want the change not to
> break that.
>
> - third, what change the patch makes
>
> - fourth, what the consequences of that are, in terms of new use
> cases that become possible and old use cases that become less
> convenient
>
> - fifth, optionally, how the need for this change was discovered
> (real-life usage, code inspection, or something else)
>
> - sixth, optionally, implementation considerations and alternate
> approaches that were discarded
I don't see any "Explain in detail what different commands do, even if
they are irrelevant to the patch in question because someone might
think they would get broken by this patch when in fact they wouldn't",
that might belong in the discussion, but not in the commit message,
and certainly not in the form of any entitlement.
Again, it's _your_ responsibility to make sure the commands you say
might get broken do actually work with your current git, it's not mine
to run them for you, even though that's exactly what I did, because
I'm interested in getting things correctly on record.
And FTR, since you removed it, here is what I proposed to add to the
commit message:
---
The reason this happens is that before traversing the commits,
fast-export checks if any of the refs point to the same object, and
any duplicated ref gets added to a list in order to issue 'reset'
commands after the traversing. Unfortunately, it's not even checking
if the commit is flagged as UNINTERESTING. The fix of course, is to do
precisely that.
---
With that, all the points above are tackled, except fourth, because
there aren't any.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH] test-lib: avoid full path to store test results
From: Felipe Contreras @ 2012-10-31 2:28 UTC (permalink / raw)
To: Jonathan Nieder
Cc: git, Junio C Hamano, Jeff King, Ævar Arnfjörð,
Johannes Sixt
In-Reply-To: <20121031021318.GB15167@elie.Belkin>
On Wed, Oct 31, 2012 at 3:13 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> It's all fun and games to write explanations for things, but it's not
>> that easy when you want those explanations to be actually true, and
>> corrent--you have to spend time to make sure of that.
>
> That's why it's useful for the patch submitter to write them, asking
> for help when necessary.
>
> As a bonus, it helps reviewers understand the effect of the patch.
> Bugs averted!
Yeah, that would be nice. Too bad I don't have that information, and
have _zero_ motivation to go and get it for you.
> [...]
>> Either way, if obvious fixes that are one-liners require an essay for
>> you, I give up.
>
> I guess it is fair to call a reasonable subject line plus a couple of
> sentences an essay. Yes, obvious fixes especially require that, since
> the bug caused by an obvious fix is one of the worst kinds.
Yes, I've written essays for one-line fixes, in the Linux kernel,
where details matter, and things are not so obvious.
This is not the case here. This is as obvious and simple as things
get. If there was a problem with it, somebody would have found it by
now.
Let not the perfect be the enemy of the good. Or do. Up to you.
--
Felipe Contreras
^ permalink raw reply
* [PATCH] git-push: update remote tags only with force
From: Chris Rorvick @ 2012-10-31 5:37 UTC (permalink / raw)
To: git
References are allowed to update from one commit-ish to another if the
former is a ancestor of the latter. This behavior is oriented to
branches which are expected to move with commits. Tag references are
expected to be static in a repository, though, thus an update to a
tag (lightweight and annotated) should be rejected unless the update is
forced.
To enable this functionality, the following checks have been added to
set_ref_status_for_push() for updating refs (i.e, not new or deletion)
to restrict fast-forwarding in pushes:
1) The old and new references must be commits. If this fails,
it is not a valid update for a branch.
2) The reference name cannot start with "refs/tags/". This
catches lightweight tags which (usually) point to commits
and therefore would not be caught by (1).
If either of these checks fails, then it is flagged (by default) with a
status indicating the update is being rejected due to the reference
already existing in the remote. This can be overridden by passing
--force to git push.
The new status has the added benefit of being able to provide accurate
feedback as to why the ref update failed and what can be done.
Currently all ref update rejections are assumed to be for branches.
Signed-off-by: Chris Rorvick <chris@rorvick.com>
---
Documentation/git-push.txt | 10 +++++-----
builtin/push.c | 12 ++++++++++++
builtin/send-pack.c | 6 ++++++
cache.h | 3 +++
remote.c | 34 +++++++++++++++++++++++++++-------
t/t5516-fetch-push.sh | 30 +++++++++++++++++++++++++++++-
transport-helper.c | 6 ++++++
transport.c | 25 ++++++++++++++++---------
transport.h | 3 ++-
9 files changed, 106 insertions(+), 23 deletions(-)
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 22d2580..7107d76 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -51,11 +51,11 @@ be named. If `:`<dst> is omitted, the same ref as <src> will be
updated.
+
The object referenced by <src> is used to update the <dst> reference
-on the remote side, but by default this is only allowed if the
-update can fast-forward <dst>. By having the optional leading `+`,
-you can tell git to update the <dst> ref even when the update is not a
-fast-forward. This does *not* attempt to merge <src> into <dst>. See
-EXAMPLES below for details.
+on the remote side. By default this is only allowed if the update is
+a branch, and then only if it can fast-forward <dst>. By having the
+optional leading `+`, you can tell git to update the <dst> ref even when
+the update is not a branch or it is not a fast-forward. This does *not*
+attempt to merge <src> into <dst>. See EXAMPLES below for details.
+
`tag <tag>` means the same as `refs/tags/<tag>:refs/tags/<tag>`.
+
diff --git a/builtin/push.c b/builtin/push.c
index db9ba30..fabcea0 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -220,6 +220,10 @@ static const char message_advice_checkout_pull_push[] =
"(e.g. 'git pull') before pushing again.\n"
"See the 'Note about fast-forwards' in 'git push --help' for details.");
+static const char message_advice_ref_already_exists[] =
+ N_("Updates were rejected because a matching reference already exists in\n"
+ "the remote. Use git push -f if you really want to make this update.");
+
static void advise_pull_before_push(void)
{
if (!advice_push_non_ff_current || !advice_push_nonfastforward)
@@ -241,6 +245,11 @@ static void advise_checkout_pull_push(void)
advise(_(message_advice_checkout_pull_push));
}
+static void advise_ref_already_exists(void)
+{
+ advise(_(message_advice_ref_already_exists));
+}
+
static int push_with_options(struct transport *transport, int flags)
{
int err;
@@ -277,6 +286,9 @@ static int push_with_options(struct transport *transport, int flags)
else
advise_checkout_pull_push();
break;
+ case ALREADY_EXISTS:
+ advise_ref_already_exists();
+ break;
}
return 1;
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 7d05064..f159ec3 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -202,6 +202,11 @@ static void print_helper_status(struct ref *ref)
msg = "non-fast forward";
break;
+ case REF_STATUS_REJECT_ALREADY_EXISTS:
+ res = "error";
+ msg = "already exists";
+ break;
+
case REF_STATUS_REJECT_NODELETE:
case REF_STATUS_REMOTE_REJECT:
res = "error";
@@ -288,6 +293,7 @@ int send_pack(struct send_pack_args *args,
/* Check for statuses set by set_ref_status_for_push() */
switch (ref->status) {
case REF_STATUS_REJECT_NONFASTFORWARD:
+ case REF_STATUS_REJECT_ALREADY_EXISTS:
case REF_STATUS_UPTODATE:
continue;
default:
diff --git a/cache.h b/cache.h
index a58df84..2d160a9 100644
--- a/cache.h
+++ b/cache.h
@@ -1002,11 +1002,14 @@ struct ref {
unsigned int force:1,
merge:1,
nonfastforward:1,
+ forwardable:1,
+ update:1,
deletion:1;
enum {
REF_STATUS_NONE = 0,
REF_STATUS_OK,
REF_STATUS_REJECT_NONFASTFORWARD,
+ REF_STATUS_REJECT_ALREADY_EXISTS,
REF_STATUS_REJECT_NODELETE,
REF_STATUS_UPTODATE,
REF_STATUS_REMOTE_REJECT,
diff --git a/remote.c b/remote.c
index 04fd9ea..0d94888 100644
--- a/remote.c
+++ b/remote.c
@@ -1309,22 +1309,42 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
* to overwrite it; you would not know what you are losing
* otherwise.
*
- * (3) if both new and old are commit-ish, and new is a
- * descendant of old, it is OK.
+ * (3) if both new and old are commits, the reference is not
+ * a tag, and new is a descendant of old, it is OK.
*
* (4) regardless of all of the above, removing :B is
* always allowed.
*/
- ref->nonfastforward =
+ ref->update =
!ref->deletion &&
- !is_null_sha1(ref->old_sha1) &&
+ !is_null_sha1(ref->old_sha1);
+
+ ref->nonfastforward =
+ ref->update &&
(!has_sha1_file(ref->old_sha1)
|| !ref_newer(ref->new_sha1, ref->old_sha1));
- if (ref->nonfastforward && !ref->force && !force_update) {
- ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
- continue;
+ if (prefixcmp(ref->name, "refs/tags/")) {
+ struct object *old = parse_object(ref->old_sha1);
+ struct object *new = parse_object(ref->new_sha1);
+ ref->forwardable = (old && new &&
+ old->type == OBJ_COMMIT && new->type == OBJ_COMMIT);
+ } else
+ ref->forwardable = 0;
+
+ if (!ref->force && !force_update) {
+ if (ref->forwardable) {
+ if (ref->nonfastforward) {
+ ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
+ continue;
+ }
+ } else {
+ if (ref->update) {
+ ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
+ continue;
+ }
+ }
}
}
}
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index b5417cc..cff559f 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -368,7 +368,7 @@ test_expect_success 'push with colon-less refspec (2)' '
git branch -D frotz
fi &&
git tag -f frotz &&
- git push testrepo frotz &&
+ git push -f testrepo frotz &&
check_push_result $the_commit tags/frotz &&
check_push_result $the_first_commit heads/frotz
@@ -929,6 +929,34 @@ test_expect_success 'push into aliased refs (inconsistent)' '
)
'
+test_expect_success 'push tag requires --force to update remote tag' '
+ mk_test heads/master &&
+ mk_child child1 &&
+ mk_child child2 &&
+ (
+ cd child1 &&
+ git tag lw_tag &&
+ git tag -a -m "message 1" ann_tag &&
+ git push ../child2 lw_tag &&
+ git push ../child2 ann_tag &&
+ >file1 &&
+ git add file1 &&
+ git commit -m "file1" &&
+ git tag -f lw_tag &&
+ git tag -f -a -m "message 2" ann_tag &&
+ ! git push ../child2 lw_tag &&
+ ! git push ../child2 ann_tag &&
+ git push --force ../child2 lw_tag &&
+ git push --force ../child2 ann_tag &&
+ git tag -f lw_tag HEAD~ &&
+ git tag -f -a -m "message 3" ann_tag &&
+ ! git push ../child2 lw_tag &&
+ ! git push ../child2 ann_tag &&
+ git push --force ../child2 lw_tag &&
+ git push --force ../child2 ann_tag
+ )
+'
+
test_expect_success 'push --porcelain' '
mk_empty &&
echo >.git/foo "To testrepo" &&
diff --git a/transport-helper.c b/transport-helper.c
index cfe0988..ef9a6f8 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -643,6 +643,11 @@ static void push_update_ref_status(struct strbuf *buf,
free(msg);
msg = NULL;
}
+ else if (!strcmp(msg, "already exists")) {
+ status = REF_STATUS_REJECT_ALREADY_EXISTS;
+ free(msg);
+ msg = NULL;
+ }
}
if (*ref)
@@ -702,6 +707,7 @@ static int push_refs_with_push(struct transport *transport,
/* Check for statuses set by set_ref_status_for_push() */
switch (ref->status) {
case REF_STATUS_REJECT_NONFASTFORWARD:
+ case REF_STATUS_REJECT_ALREADY_EXISTS:
case REF_STATUS_UPTODATE:
continue;
default:
diff --git a/transport.c b/transport.c
index 9932f40..d218884 100644
--- a/transport.c
+++ b/transport.c
@@ -659,7 +659,7 @@ static void print_ok_ref_status(struct ref *ref, int porcelain)
const char *msg;
strcpy(quickref, status_abbrev(ref->old_sha1));
- if (ref->nonfastforward) {
+ if (ref->nonfastforward || (!ref->forwardable && ref->update)) {
strcat(quickref, "...");
type = '+';
msg = "forced update";
@@ -695,6 +695,10 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
print_ref_status('!', "[rejected]", ref, ref->peer_ref,
"non-fast-forward", porcelain);
break;
+ case REF_STATUS_REJECT_ALREADY_EXISTS:
+ print_ref_status('!', "[rejected]", ref, ref->peer_ref,
+ "already exists", porcelain);
+ break;
case REF_STATUS_REMOTE_REJECT:
print_ref_status('!', "[remote rejected]", ref,
ref->deletion ? NULL : ref->peer_ref,
@@ -714,7 +718,7 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
}
void transport_print_push_status(const char *dest, struct ref *refs,
- int verbose, int porcelain, int *nonfastforward)
+ int verbose, int porcelain, int *willnotupdate)
{
struct ref *ref;
int n = 0;
@@ -733,18 +737,21 @@ void transport_print_push_status(const char *dest, struct ref *refs,
if (ref->status == REF_STATUS_OK)
n += print_one_push_status(ref, dest, n, porcelain);
- *nonfastforward = 0;
+ *willnotupdate = 0;
for (ref = refs; ref; ref = ref->next) {
if (ref->status != REF_STATUS_NONE &&
ref->status != REF_STATUS_UPTODATE &&
ref->status != REF_STATUS_OK)
n += print_one_push_status(ref, dest, n, porcelain);
if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD &&
- *nonfastforward != NON_FF_HEAD) {
+ *willnotupdate != NON_FF_HEAD) {
if (!strcmp(head, ref->name))
- *nonfastforward = NON_FF_HEAD;
+ *willnotupdate = NON_FF_HEAD;
else
- *nonfastforward = NON_FF_OTHER;
+ *willnotupdate = NON_FF_OTHER;
+ } else if (ref->status == REF_STATUS_REJECT_ALREADY_EXISTS &&
+ *willnotupdate == 0) {
+ *willnotupdate = ALREADY_EXISTS;
}
}
}
@@ -1031,9 +1038,9 @@ static void die_with_unpushed_submodules(struct string_list *needs_pushing)
int transport_push(struct transport *transport,
int refspec_nr, const char **refspec, int flags,
- int *nonfastforward)
+ int *willnotupdate)
{
- *nonfastforward = 0;
+ *willnotupdate = 0;
transport_verify_remote_names(refspec_nr, refspec);
if (transport->push) {
@@ -1099,7 +1106,7 @@ int transport_push(struct transport *transport,
if (!quiet || err)
transport_print_push_status(transport->url, remote_refs,
verbose | porcelain, porcelain,
- nonfastforward);
+ willnotupdate);
if (flags & TRANSPORT_PUSH_SET_UPSTREAM)
set_upstreams(transport, remote_refs, pretend);
diff --git a/transport.h b/transport.h
index 3b21c4a..326271e 100644
--- a/transport.h
+++ b/transport.h
@@ -142,6 +142,7 @@ void transport_set_verbosity(struct transport *transport, int verbosity,
#define NON_FF_HEAD 1
#define NON_FF_OTHER 2
+#define ALREADY_EXISTS 3
int transport_push(struct transport *connection,
int refspec_nr, const char **refspec, int flags,
int * nonfastforward);
@@ -170,7 +171,7 @@ void transport_update_tracking_ref(struct remote *remote, struct ref *ref, int v
int transport_refs_pushed(struct ref *ref);
void transport_print_push_status(const char *dest, struct ref *refs,
- int verbose, int porcelain, int *nonfastforward);
+ int verbose, int porcelain, int *willnotupdate);
typedef void alternate_ref_fn(const struct ref *, void *);
extern void for_each_alternate_ref(alternate_ref_fn, void *);
--
1.8.0.1.ga7c0bae
^ permalink raw reply related
* Re: [PATCH] git-push: update remote tags only with force
From: Felipe Contreras @ 2012-10-31 5:55 UTC (permalink / raw)
To: Chris Rorvick; +Cc: git
In-Reply-To: <1351661875-4307-1-git-send-email-chris@rorvick.com>
Hi,
(again because the mailing list rejected it) (Gmal switched interface
and HTML is the default)
On Wed, Oct 31, 2012 at 6:37 AM, Chris Rorvick <chris@rorvick.com> wrote:
>
> References are allowed to update from one commit-ish to another if the
> former is a ancestor of the latter. This behavior is oriented to
> branches which are expected to move with commits. Tag references are
> expected to be static in a repository, though, thus an update to a
> tag (lightweight and annotated) should be rejected unless the update is
> forced.
>
> To enable this functionality, the following checks have been added to
> set_ref_status_for_push() for updating refs (i.e, not new or deletion)
> to restrict fast-forwarding in pushes:
>
> 1) The old and new references must be commits. If this fails,
> it is not a valid update for a branch.
>
> 2) The reference name cannot start with "refs/tags/". This
> catches lightweight tags which (usually) point to commits
> and therefore would not be caught by (1).
>
> If either of these checks fails, then it is flagged (by default) with a
> status indicating the update is being rejected due to the reference
> already existing in the remote. This can be overridden by passing
> --force to git push.
>
> The new status has the added benefit of being able to provide accurate
> feedback as to why the ref update failed and what can be done.
> Currently all ref update rejections are assumed to be for branches.
Makes sense to me. I've believe I've been hit by this a couple of
times when tags were updated, and a colleague did 'git push' and they
went all back, or something like that. To handle that case properly
probably more changes are needed, but this is a change in the right
direction.
> +test_expect_success 'push tag requires --force to update remote tag' '
> + mk_test heads/master &&
> + mk_child child1 &&
> + mk_child child2 &&
> + (
> + cd child1 &&
> + git tag lw_tag &&
> + git tag -a -m "message 1" ann_tag &&
> + git push ../child2 lw_tag &&
> + git push ../child2 ann_tag &&
> + >file1 &&
> + git add file1 &&
> + git commit -m "file1" &&
> + git tag -f lw_tag &&
> + git tag -f -a -m "message 2" ann_tag &&
> + ! git push ../child2 lw_tag &&
You probably should use test_must_fail.
I don't see anything wrong with the patch, but I wonder if it might be
possible to split it to ease the review.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly
From: Jonathan Nieder @ 2012-10-31 6:05 UTC (permalink / raw)
To: Sverre Rabbelier
Cc: Felipe Contreras, git, Jeff King, Junio C Hamano,
Johannes Schindelin, Elijah Newren
In-Reply-To: <CAGdFq_jNM_48muXJ0BX2ehC=k8T9GLui_QtRO8D8C7h6b5jyHg@mail.gmail.com>
Sverre Rabbelier wrote:
> Thanks for the thorough explanation. Perhaps some of that could make
> it's way into the commit message?
It's fine with me if it doesn't, since the original commit message
covers the basics (current behavior and intent of the change) in its
first two paragraphs and anyone wanting more detail can use
GIT_NOTES_REF=refs/remotes/charon/notes/full \
git show --show-notes <commit>
to find more details.
Thanks,
Jonathan
^ permalink raw reply
* Re: [PATCH] Teach rm to remove submodules when given with a trailing '/'
From: Johannes Sixt @ 2012-10-31 6:29 UTC (permalink / raw)
To: Jens Lehmann; +Cc: Git Mailing List, Jeff King
In-Reply-To: <50904677.2020308@web.de>
Am 10/30/2012 22:28, schrieb Jens Lehmann:
> Am 29.10.2012 08:11, schrieb Johannes Sixt:
>> Am 10/29/2012 0:28, schrieb Jens Lehmann:
>>> + /* Remove trailing '/' from directories to find submodules in the index */
>>> + for (i = 0; i < argc; i++) {
>>> + size_t pathlen = strlen(argv[i]);
>>> + if (pathlen && is_directory(argv[i]) && (argv[i][pathlen - 1] == '/'))
>>> + argv[i] = xmemdupz(argv[i], pathlen - 1);
>>> + }
>>> +
>>> pathspec = get_pathspec(prefix, argv);
>>> refresh_index(&the_index, REFRESH_QUIET, pathspec, NULL, NULL);
>>
>> That's wrong: Either move the check below get_pathspec() (which normalizes
>> backslashes to forward-slashes on Windows) or use is_dir_sep().
>
> Thanks for bringing this up.
>
>> But isn't it somewhat dangerous to check pathspec for existance in the
>> worktree without interpreting them? Think of magic pathspec syntax (that
>> we do not have yet, but which may materialize sometime in the future).
>
> I have to admit I'm not aware of magic pathspec syntax. Do you happen to
> have any pointers where I could look at code doing similar things right?
cmd_mv() in builtin/mv.c looks like a good candidate. It has to check
whether the destination (the last argument) is a directory.
-- Hannes
^ permalink raw reply
* Re: Can't understand the behaviour of git-diff --submodule
From: Francis Moreau @ 2012-10-31 7:36 UTC (permalink / raw)
To: Jens Lehmann; +Cc: git
In-Reply-To: <509043DA.6040606@web.de>
Hi,
On Tue, Oct 30, 2012 at 10:17 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
> Am 28.10.2012 01:02, schrieb Jens Lehmann:
>> Am 26.10.2012 22:43, schrieb Francis Moreau:
>>> On Fri, Oct 26, 2012 at 10:05 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>>> [...]
>>>>
>>>> That is weird, "git diff --submodule" should show that too. Is there
>>>> anything unusual about your setup? (The only explanation I can come
>>>> up with after checking the code is that your submodule has neither a
>>>> .git directory nor a gitfile or the objects directory in there doesn't
>>>> contain these commits)
>>>
>>> Oh now you're asking, I think the submodule has been added by using
>>> the --reference option of git-submodule-add.
>>>
>>> $ cd configs
>>> $ cat .git
>>> gitdir: ../.git/modules/configs
>>
>> Thanks, I suspect the --reference option makes the difference here,
>> I'll check that as soon as I find some time.
>
> Since 1.7.11 and 1.7.10.3 git does handle submodules with alternates
> (which is what --reference uses) correctly. What version are you
> seeing this problem with?
git version 1.7.10.4
Thanks.
--
Francis
^ permalink raw reply
* Re: [PATCH] test-lib: avoid full path to store test results
From: Stefano Lattarini @ 2012-10-31 9:05 UTC (permalink / raw)
To: Elia Pinto
Cc: Jonathan Nieder, Felipe Contreras, git, Junio C Hamano, Jeff King,
Ævar Arnfjörð, Johannes Sixt
In-Reply-To: <CA+EOSBmTjwmf+dO-dgU+rGQaVEKDZw7u9ujrh5jYZkPM2zisOA@mail.gmail.com>
On 10/30/2012 11:17 PM, Elia Pinto wrote:
> Thanks. I know that posix support these usages, but exists some
> traditional shell that not support it.
>
True, but those shells are not POSIX shells -- the major example that
comes to mind is the accursed Solaris /bin/sh.
Since Git assumes a POSIX shell in its scripts and testsuite, use of
any POSIX feature should be fine -- until someone can show a real-world
POSIX shell that (likely due to a bug) fails to grasp such feature, in
which case a "pragmatic" workaround is needed.
Oh, and BTW, there are talks (and mostly consensus) among the Autotools
developers to start requiring a POSIX shell in the configure scripts
and Makefile recipes in the near future:
<http://lists.gnu.org/archive/html/bug-autoconf/2012-06/msg00009.html>
And also, related:
<http://lists.gnu.org/archive/html/automake/2012-08/msg00046.html>
<http://lists.gnu.org/archive/html/coreutils/2012-10/msg00127.html>
>These are described in the
> autoconf manual, last time i have checked. As the construct ; export
> var = x should be portable, but it is not.
>
I don't think POSIX requires that to be portable.
> If this is important these days i don't know.
>
I hope the above helps to clarify the matter a little.
Regards,
Stefano
^ permalink raw reply
* Re: [PATCH v4 00/13] New remote-hg helper
From: Michael J Gruber @ 2012-10-31 9:30 UTC (permalink / raw)
To: Felipe Contreras
Cc: Johannes Schindelin, Jeff King, git, Junio C Hamano,
Sverre Rabbelier, Ilari Liusvaara, Daniel Barkalow
In-Reply-To: <CAMP44s0akZ7_Nd1Q1AaZJuXnyTJv2MzNqDus76Y82y4LbWVO+Q@mail.gmail.com>
[quotes heavily cut down by me]
Felipe Contreras venit, vidit, dixit 30.10.2012 21:15:
> Hi,
>
> On Tue, Oct 30, 2012 at 8:33 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>> On Tue, 30 Oct 2012, Felipe Contreras wrote:
>>
>>> But you mentioned something about cooperation, and I've yet to see how
>>> is it that you are planning to cooperate. If you say you don't have time
>>> to spend on this, I don't see why I should worry about testing this
>>> series of patches.
>>
>> It has been mentioned before that the communication style including all
>> these snarky and nasty comments is not helpful.
>
For the record, Johannes is not the only one being kept from looking at
this series (further) by the tone of this discussion. Per hominem
attacks are neither professional nor helpful. We prefer to discuss code
here, just code. From my comments on an earlier version of your series
you can see I've tried. The way other comment threads on this series
unfolded made me choose to be a mere by-stander again.
>> and I've yet to see how is it that you are planning to cooperate.
>
> This is also a fact. You haven't provided a branch, you haven't reviewed
> my implementation, you haven't tried it. You mentioned something about
This does not become true through iteration. Max' recent post 'On
git-remote-hg (the "native" one)' [1] points at the msysgit wiki on
remote-hg [2] and his remote-hg branch [3], which is based on and points
at Sverre's original branch [4] and mine [5] which is [4] being
regularly rebased on origin/next. The msysgit devel branch is in heavy
use; I don't use mine often but run the test suite on every rebase
before pushing out.
If the issues that Sverre and Dscho tried to address with their git.git
core (non-helper) patches turn out to be non-issues then I assume
everyone will be happy, including them. You and they have thought a lot
about these things and the way hg-git sync can work. There seems to be
diagreement about the way fast-export/the remote helpers communicate
which revs and refs that are to be synced and updated. This is not
hg-specific, and I suggest to try and clarify that issue as thoroughly
and calmly as possible. Everyone will benefit, and it will make clearer
which tests are appropriate, and accordingly which fixes fix real problems.
Orthogonal to this, it seems that all hg-git interfaces could take
advantage of a "git heads" feature if we resurrect the old ideas (can't
find the thread right now).
Hoping for the best,
Michael
[1] http://permalink.gmane.org/gmane.comp.version-control.git/201083
[2] https://github.com/msysgit/msysgit/wiki/Guide-to-git-remote-hg
[3] https://github.com/fingolfin/git/tree/remote-hg
[4] https://github.com/SRabbelier/git/tree/remote-hg
[5] https://github.com/mjg/git/tree/remote-hg
^ permalink raw reply
* [OT] How to get the discussion details via notes
From: Peter Baumann @ 2012-10-31 9:53 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git
In-Reply-To: <20121031060529.GA30432@elie.Belkin>
Dropping the Cc list, as this is off topic
On Tue, Oct 30, 2012 at 11:05:29PM -0700, Jonathan Nieder wrote:
> Sverre Rabbelier wrote:
>
> > Thanks for the thorough explanation. Perhaps some of that could make
> > it's way into the commit message?
>
> It's fine with me if it doesn't, since the original commit message
> covers the basics (current behavior and intent of the change) in its
> first two paragraphs and anyone wanting more detail can use
>
> GIT_NOTES_REF=refs/remotes/charon/notes/full \
> git show --show-notes <commit>
>
> to find more details.
I seem to miss something here, but I don't get it how the notes ref
becomes magically filled with the details of this discussion.
Care to explain?
-Peter
^ permalink raw reply
* Re: [PATCH v4 00/13] New remote-hg helper
From: Jeff King @ 2012-10-31 10:27 UTC (permalink / raw)
To: git
Cc: Michael J Gruber, Felipe Contreras, Johannes Schindelin,
Junio C Hamano, Sverre Rabbelier, Ilari Liusvaara,
Daniel Barkalow
In-Reply-To: <5090EFCA.7070606@drmicha.warpmail.net>
On Wed, Oct 31, 2012 at 10:30:50AM +0100, Michael J Gruber wrote:
> For the record, Johannes is not the only one being kept from looking at
> this series (further) by the tone of this discussion. Per hominem
> attacks are neither professional nor helpful. We prefer to discuss code
> here, just code. From my comments on an earlier version of your series
> you can see I've tried. The way other comment threads on this series
> unfolded made me choose to be a mere by-stander again.
Me too. I really like some of the directions the series is taking, and
as the maintainer, I'd like to pick it up. But there is a big question
mark for me still about how it relates to the work in msysgit,
especially:
- What advantages does this implementation have over the one in
msysgit (i.e., new features that the other one does not have)?
- What disadvantages? If this implementation goes into git.git,
the msysgit one is likely to wane in popularity. What will we be
losing by doing so? If the answer is not "nothing", how hard would
it be to port over the missing bits?
- The msysgit one got held up by fixes needed for fast-export. Why
aren't those a problem for this implementation? If we are using a
different strategy that avoids the issue, what are the limitations
(if any) of that strategy?
I have a feeling that some of those answers are buried deep within the
discussion, but I have had a hard time following all of the back and
forth due to the volume and tone of the discussion. Are we at a point
now where some of the participants can try to summarize the situation?
I am not saying that this implementation must be 100% better than the
msysgit one. I do not want perfect to to be the enemy of good and end up
with nothing. But at the same time, there really are two competing
implementations, one of which has received substantially more field use.
Even though the msysgit one is not in git.git, it seems like the path
for making it happen exists (even if it has not been followed yet).
Before merging an alternative implementation, I would want to know what
we are potentially throwing away from the msysgit side, and make sure
that we are not following a wrong path that msysgit has already tried
and found to be lacking.
-Peff
^ permalink raw reply
* Re: Workflow for templates?
From: Josef Wolf @ 2012-10-31 10:44 UTC (permalink / raw)
To: git
In-Reply-To: <3190de06-2eaf-4a39-91aa-9cc34c20fc8e@zcs>
On Sat, Oct 27, 2012 at 08:45:45PM +0200, Enrico Weigelt wrote:
> I'd suggest a 3 level branch hierachy (IOW: the lower level
> is rebased ontop of the next higher level):
>
> * #0: upstream branch
> * #1: generic local maintenance branch
> * #2: per-instance cutomization branches
>
> Normal additions go to the lowest level #2. When you've got
> some generic commit, you propagate it to the next level
> (cherry-pick) and rebase layer #2 ontop of it.
> Now you can send your layer #1 to upstream for integration.
>
> When upstream updated his branch, you simply rebase #1
> ontop of it, do your checks etc, then proceed to rebasing #3.
>
> You could also introduce more intermediate layers (eg when you've
> got different groups of similar instance that share certain changes)
Thanks for the suggestion, Enrico!
I am somewhat unsure whether it would work this way. After all, there seems to
be an unbreakable rule with git: never rebase published branches.
Thus, once I have published my work to other people who also need to work on
the same localizations as I do, I have no longer the option of rebasing to get
rid of the localizations and put the generic template stuff for upstream.
I guess, my concern is because I have not yet fully understood the problems of
rebasing, and how to recover from them.
Maybe I should try to explain the problem in terms of repository
hierarchy. Let's assume, there is this hierarchy of repositories:
upstream: central repository, containing the generic template
foo-site: repository for site foo. Here we have localizations for a specific
administrative entity named foo (say, google).
This is where clones for production are made from, and production
boxes pull from here to be kept up-to-date.
foo-prodA: A clone of foo-site, put in production and pulling from a specific
branch on foo-site to receive released, blessed updates.
foo-prodB: Similar to foo-prodA, but on another box.
foo-devA: A clone of foo-site to make development, releases, and whatever for
foo.
foo-devB: One more clone of foo-site, Developer B is working here.
Then, we might have more administrative entities: bar-site, bar-prodA,
bar-prodB, bar-devA, bar-devB, for example. This might be Microsoft, for
example.
Further, foo-devA might be the same person as bar-devA.
So when foo-devA pulls from foo-devB, then foo-devB will create problems when
he rebases after that pull.
I think I have some kind of misunderstanding here, but I just can't figure
what it is.
Maybe I should try to explain the problem in yet other words:
What I am trying to achieve, is to extend the workflow from development to
deployment across multiple administrative entities. As a picture:
upstream (templates only).
^
|
v
development (configured, might contain experimental changes)
^
|
v
deployment (configured)
This workflow should not stop at administrative borders. Just replace foo by
google and bar by Microsoft to get an idea of what I am trying to achieve.
^ permalink raw reply
* Re: Links broken in ref docs.
From: Kevin @ 2012-10-31 11:17 UTC (permalink / raw)
To: Mike Norman; +Cc: Holger Hellmuth (IKS), Scott Chacon, Andrew Ardill, git
In-Reply-To: <CAJr+XPH4MTg_F_YuT=R1mLVwOUYPB0US3w2mZ1+DDvrQV7vVfQ@mail.gmail.com>
Looks fixed here too
On Wed, Oct 31, 2012 at 12:30 AM, Mike Norman <mknorman@gmail.com> wrote:
> I just checked and the issue seems to be fixed! Clicked around on a
> bunch of previously broken links and they work!
>
> On Tue, Oct 30, 2012 at 3:38 AM, Holger Hellmuth (IKS)
> <hellmuth@ira.uka.de> wrote:
>> Am 30.10.2012 09:07, schrieb Mike Norman:
>>
>>> Not seen any recently. I'm guessing the dev is in the path of
>>> hurricane Sandy? (Not sarcasm, btw.)
>>
>>
>> Do you still see failures? I checked out the website just now and it seemed
>> to work flawlessly (at least the links I tried, could not find any Sharing
>> or Updating section). New design since I last visited, more end-user polish.
>>
>>
^ permalink raw reply
* Re: relative objects/info/alternates doesn't work on remote SMB repo
From: Nguyen Thai Ngoc Duy @ 2012-10-31 11:41 UTC (permalink / raw)
To: Orgad Shaneh; +Cc: git, msysGit
In-Reply-To: <CAGHpTB+o4gHfgFgLqie_hbjzWjxL94xRQi11GwS9F-Qhik0qVA@mail.gmail.com>
On Tue, Oct 30, 2012 at 4:28 PM, Orgad Shaneh
> Any news? This still doesn't work with 1.8.0.
Nope, sorry. It's still in my todo list.
--
Duy
^ permalink raw reply
* Re: git smart-http do not authent to allow git ls-remote to be called anonymously
From: Jeff King @ 2012-10-31 11:53 UTC (permalink / raw)
To: 乙酸鋰; +Cc: git
In-Reply-To: <CAHtLG6SsMgwquaD_Rd0YvR9Ues-u1ktEdOXC2MAfEM9Naac=5g@mail.gmail.com>
[+cc git@vger; please keep discussion on the list]
On Sun, Oct 28, 2012 at 01:26:51PM +0800, 乙酸鋰 wrote:
> > POST /git/Cat1/SubCat1/xsp.git/git-upload-pack HTTP/1.1
> User-Agent: git/1.8.0
> Host: localhost
> Accept-Encoding: gzip
> Content-Type: application/x-git-upload-pack-request
> Accept: application/x-git-upload-pack-result
> Content-Length: 190
>
> * The requested URL returned error: 401
> * Closing connection #0
> Username for 'http://localhost': user
> Password for 'http://user@localhost':
> fatal: The remote end hung up unexpectedly
OK, I see what is going on. The code in b81401c to retry POST requests
does not handle gzipped contents, and upload-pack tends to gzip what it
sends.
Your apache configuration is not really something that we ever intended
to support, and I am a little dubious of the security tradeoff being
made. But it is actually pretty easy for us to support, and it
eliminates a special case from the code, so I am tempted to do so.
The following patch series (on top of the current master, as they
require some cleanup that did not make it into 1.8.0) seems to fix it
for me.
[1/2]: remote-curl: hoist gzip buffer size to top of post_rpc
[2/2]: remote-curl: retry failed requests for auth even with gzip
-Peff
^ permalink raw reply
* [PATCH 1/2] remote-curl: hoist gzip buffer size to top of post_rpc
From: Jeff King @ 2012-10-31 11:55 UTC (permalink / raw)
To: 乙酸鋰; +Cc: git
In-Reply-To: <20121031115346.GC30879@sigill.intra.peff.net>
When we gzip the post data for a smart-http rpc request, we
compute the gzip body and its size inside the "use_gzip"
conditional. We keep track of the body after the conditional
ends, but not the size. Let's remember both, which will
enable us to retry failed gzip requests in a future patch.
Signed-off-by: Jeff King <peff@peff.net>
---
This is a tiny change conceptually, but the name change makes the diff
quite noisy. Thus I pulled it into a separate patch from 2/2.
remote-curl.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index aefafd3..10cd47d 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -413,6 +413,7 @@ static int post_rpc(struct rpc_state *rpc)
struct curl_slist *headers = NULL;
int use_gzip = rpc->gzip_request;
char *gzip_body = NULL;
+ size_t gzip_size;
int err, large_request = 0;
/* Try to load the entire request, if we can fit it into the
@@ -478,19 +479,18 @@ retry:
* we can try to deflate it ourselves, this may save on.
* the transfer time.
*/
- size_t size;
git_zstream stream;
int ret;
memset(&stream, 0, sizeof(stream));
git_deflate_init_gzip(&stream, Z_BEST_COMPRESSION);
- size = git_deflate_bound(&stream, rpc->len);
- gzip_body = xmalloc(size);
+ gzip_size = git_deflate_bound(&stream, rpc->len);
+ gzip_body = xmalloc(gzip_size);
stream.next_in = (unsigned char *)rpc->buf;
stream.avail_in = rpc->len;
stream.next_out = (unsigned char *)gzip_body;
- stream.avail_out = size;
+ stream.avail_out = gzip_size;
ret = git_deflate(&stream, Z_FINISH);
if (ret != Z_STREAM_END)
@@ -500,16 +500,16 @@ retry:
if (ret != Z_OK)
die("cannot deflate request; zlib end error %d", ret);
- size = stream.total_out;
+ gzip_size = stream.total_out;
headers = curl_slist_append(headers, "Content-Encoding: gzip");
curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, gzip_body);
- curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, size);
+ curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, gzip_size);
if (options.verbosity > 1) {
fprintf(stderr, "POST %s (gzip %lu to %lu bytes)\n",
rpc->service_name,
- (unsigned long)rpc->len, (unsigned long)size);
+ (unsigned long)rpc->len, (unsigned long)gzip_size);
fflush(stderr);
}
} else {
--
1.8.0.207.gdf2154c
^ permalink raw reply related
* [PATCH 2/2] remote-curl: retry failed requests for auth even with gzip
From: Jeff King @ 2012-10-31 12:01 UTC (permalink / raw)
To: 乙酸鋰; +Cc: git
In-Reply-To: <20121031115346.GC30879@sigill.intra.peff.net>
Commit b81401c taught the post_rpc function to retry the
http request after prompting for credentials. However, it
did not handle two cases:
1. If we have a large request, we do not retry. That's OK,
since we would have sent a probe (with retry) already.
2. If we are gzipping the request, we do not retry. That
was considered OK, because the intended use was for
push (e.g., listing refs is OK, but actually pushing
objects is not), and we never gzip on push.
This patch teaches post_rpc to retry even a gzipped request.
This has two advantages:
1. It is possible to configure a "half-auth" state for
fetching, where the set of refs and their sha1s are
advertised, but one cannot actually fetch objects.
This is not a recommended configuration, as it leaks
some information about what is in the repository (e.g.,
an attacker can try brute-forcing possible content in
your repository and checking whether it matches your
branch sha1). However, it can be slightly more
convenient, since a no-op fetch will not require a
password at all.
2. It future-proofs us should we decide to ever gzip more
requests.
Signed-off-by: Jeff King <peff@peff.net>
---
I doubt we would ever want to gzip push requests. The bulk of the data
is objects, which are already compressed. In theory we could gzip the
non-pack parts, but that would first mean splitting them into a separate
request. The tiny bit of savings are almost certainly not worth the
complexity.
But the future-proofing might still be valuable if we ever modify the
protocol to have a new phase.
remote-curl.c | 11 ++++++++++-
t/lib-httpd/apache.conf | 7 +++++++
t/t5551-http-fetch.sh | 15 +++++++++++++++
3 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/remote-curl.c b/remote-curl.c
index 10cd47d..fac2bef 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -474,6 +474,15 @@ retry:
fflush(stderr);
}
+ } else if (gzip_body) {
+ /*
+ * If we are looping to retry authentication, then the previous
+ * run will have set up the headers and gzip buffer already,
+ * and we just need to send it.
+ */
+ curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, gzip_body);
+ curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, gzip_size);
+
} else if (use_gzip && 1024 < rpc->len) {
/* The client backend isn't giving us compressed data so
* we can try to deflate it ourselves, this may save on.
@@ -530,7 +539,7 @@ retry:
curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
err = run_slot(slot);
- if (err == HTTP_REAUTH && !large_request && !use_gzip)
+ if (err == HTTP_REAUTH && !large_request)
goto retry;
if (err != HTTP_OK)
err = -1;
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index ec8618d..15a3c71 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -96,6 +96,13 @@ SSLEngine On
Require valid-user
</LocationMatch>
+<LocationMatch "^/auth-fetch/.*/git-upload-pack$">
+ AuthType Basic
+ AuthName "git-auth"
+ AuthUserFile passwd
+ Require valid-user
+</LocationMatch>
+
<IfDefine DAV>
LoadModule dav_module modules/mod_dav.so
LoadModule dav_fs_module modules/mod_dav_fs.so
diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
index 7380f2a..5f174da 100755
--- a/t/t5551-http-fetch.sh
+++ b/t/t5551-http-fetch.sh
@@ -129,6 +129,21 @@ test_expect_success 'clone from auth-only-for-push repository' '
test_cmp expect actual
'
+test_expect_success 'clone from auth-only-for-objects repository' '
+ echo two >expect &&
+ set_askpass user@host &&
+ git clone --bare "$HTTPD_URL/auth-fetch/smart/repo.git" half-auth &&
+ expect_askpass both user@host &&
+ git --git-dir=half-auth log -1 --format=%s >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'no-op half-auth fetch does not require a password' '
+ set_askpass wrong &&
+ git --git-dir=half-auth fetch &&
+ expect_askpass none
+'
+
test -n "$GIT_TEST_LONG" && test_set_prereq EXPENSIVE
test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '
--
1.8.0.207.gdf2154c
^ permalink raw reply related
* Re: change symlink
From: Jeff King @ 2012-10-31 12:05 UTC (permalink / raw)
To: shawn wilson; +Cc: Andreas Schwab, git
In-Reply-To: <CAH_OBidmrJsmw1QQ2WONieA1EQmS_Y4WJ8Mu2Mh90tEPU0uWgw@mail.gmail.com>
On Tue, Oct 30, 2012 at 10:22:04PM +0000, shawn wilson wrote:
> On Tue, Oct 30, 2012 at 10:09 PM, Andreas Schwab <schwab@linux-m68k.org> wrote:
> > shawn wilson <ag4ve.us@gmail.com> writes:
> >
> >> and once it's added, status says:
> >> # renamed: t2 -> t2/one/test
> >>
> >> that's not exactly true, but...
> >
> > What's wrong with it? Both files have the same contents, which is the
> > link target for symlinks.
> >
>
> ok, you're right about that (another test where i changed where the
> symlink pointed):
> # deleted: test
> # new file: test/one/t3
>
>
> however (what got me started wondering about this and a point i forgot
> about) - t2/one/test doesn't show up under 'untracked files' in in
> status that scenario. shouldn't it?
Yes, I think that is a bug.
My guess is that the presence of "test" in the index fools us from
descending into the directory. And indeed, if you try "git status
-uall", you will see the untracked file. So it is something with the
directory traversal cutoff in the regular "-unormal" mode.
-Peff
^ permalink raw reply
* Re: [PATCH] git-push: update remote tags only with force
From: Drew Northup @ 2012-10-31 12:26 UTC (permalink / raw)
To: Chris Rorvick
Cc: git, Felipe Contreras, Jeff King, Michael Haggerty,
Angelo Borsotti, Philip Oakley, Johannes Sixt, Kacper Kornet
In-Reply-To: <CAMP44s2T9Rmfjd8r+2+eYh8JBPXEofm3cHuEkkY+R3cW6R6HxA@mail.gmail.com>
On Wed, Oct 31, 2012 at 1:55 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> but I wonder if it might be
> possible to split it to ease the review.
Agreed. Also, do please CC ALL interested parties from the pre-patch
discussion thread as well as those who previously maintained that
chunk of code.
[Attempted to reconstruct CC list of discussion]
--
-Drew Northup
--------------------------------------------------------------
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59
^ permalink raw reply
* Re: [OT] How to get the discussion details via notes
From: Drew Northup @ 2012-10-31 12:29 UTC (permalink / raw)
To: Peter Baumann; +Cc: Jonathan Nieder, git
In-Reply-To: <20121031095327.GB18557@m62s10.vlinux.de>
On Wed, Oct 31, 2012 at 5:53 AM, Peter Baumann <waste.manager@gmx.de> wrote:
> Dropping the Cc list, as this is off topic
>
> On Tue, Oct 30, 2012 at 11:05:29PM -0700, Jonathan Nieder wrote:
>> Sverre Rabbelier wrote:
>>
>> > Thanks for the thorough explanation. Perhaps some of that could make
>> > it's way into the commit message?
>>
>> It's fine with me if it doesn't, since the original commit message
>> covers the basics (current behavior and intent of the change) in its
>> first two paragraphs and anyone wanting more detail can use
>>
>> GIT_NOTES_REF=refs/remotes/charon/notes/full \
>> git show --show-notes <commit>
>>
>> to find more details.
>
> I seem to miss something here, but I don't get it how the notes ref
> becomes magically filled with the details of this discussion.
>
> Care to explain?
If I have an email thread I'd like to store alongside a commit I'll
put that into a note, but I usually don't push that kind of thing out
to a remote repo.
Does that help?
--
-Drew Northup
--------------------------------------------------------------
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox