* Re: [RFC] Instruct git-completion.bash that we are in test mode
From: Junio C Hamano @ 2013-01-22 4:31 UTC (permalink / raw)
To: Jeff King; +Cc: Jean-Noël AVILA, git
In-Reply-To: <20130122003954.GA23297@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I really hate to suggest this, but should it be more like:
>
> if test -z "$FAKE_COMMAND_LIST"; then
> __git_cmdlist() {
> git help -a | egrep '^ [a-zA-Z0-9]'
> }
> else
> __git_cmdlist() {
> printf '%s' "$FAKE_COMMAND_LIST"
> }
> fi
>
> That gives us a nice predictable starting point for actually testing the
> completion code. The downside is that it doesn't let us test that we
> remain compatible with the output of "help -a".
Yeah, I think this is simpler and more to the point for the test in
t9902. If we really want to test something that is the same as, or
at least any closer than this approach (or my "help --standard"), to
what the real users use, the test has to become inherently flaky, so
I think we should go for the simplicity of this patch shows.
^ permalink raw reply
* Re: [PATCH v6 0/8] push: update remote tags only with force
From: Chris Rorvick @ 2013-01-22 4:59 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Max Horn, git, Angelo Borsotti, Drew Northup,
Michael Haggerty, Philip Oakley, Johannes Sixt, Kacper Kornet,
Felipe Contreras
In-Reply-To: <20130121234002.GE17156@sigill.intra.peff.net>
On Mon, Jan 21, 2013 at 5:40 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Jan 17, 2013 at 09:18:50PM -0600, Chris Rorvick wrote:
>
>> On Thu, Jan 17, 2013 at 7:06 PM, Jeff King <peff@peff.net> wrote:
>> > However, if instead of the rule being
>> > "blobs on the remote side cannot be replaced", if it becomes "the old
>> > value on the remote side must be referenced by what we replace it with",
>> > that _is_ something we can calculate reliably on the sending side.
>>
>> Interesting. I would have thought knowing reachability implied having
>> the old object in the sending repository.
>
> No, because if you do not have it, then you know it is not reachable
> from your refs (or your repository is corrupted). If you do have it, it
> _might_ be reachable. For commits, checking is cheap (merge-base) and we
> already do it. For trees and blobs, it is much more expensive, as you
> have to walk the whole object graph. While it might be "more correct"
> in some sense to say "it's OK to replace a tree with a commit that
> points to it", in practice I doubt anyone cares, so you can probably
> just punt on those ones and say "no, it's not a fast forward".
Thanks for explaining this further. I'm not exactly sure what I was
thinking when I wrote the above other than I didn't fully grasp you
point and responded in a confused state. Clear on all fronts now.
>> > And
>> > that is logically an extension of the fast-forward rule, which is why I
>> > suggested placing it with ref_newer (but the latter should probably be
>> > extended to not suggest merging if we _know_ it is a non-commit object).
>>
>> Sounds great, especially if it is not dependent on the sender actually
>> having the old object. Until this is implemented, though, I don't
>> understand what was wrong with doing the checks in the
>> is_forwardable() helper function (of course after fixing the
>> regression/bug.)
>
> I don't think it is wrong per se; I just think that the check would go
> more naturally where we are checking whether the object does indeed
> fast-forward. Because is_forwardable in some cases must say "I don't
> know; I don't have the object to check its type, so maybe it is
> forwardable, and maybe it is not". Whereas when we do the actual
> reachability check, we can say definitely "this is not reachable because
> I don't have it, or this is not reachable because it is a commit and I
> checked, or this might be reachable but I don't care to check because it
> has a funny type".
>
> I think looking at it as the latter makes it more obvious how to handle
> the "maybe" situation (e.g., the bug in is_forwardable was hard to see).
>
> Anyway, I do not care that much where it goes. To me, the important
> thing is the error message. I do think the error "already exists" is a
> reasonable one for refs/tags (we do not allow non-force pushes of
> existing tags), but not necessarily for other cases, like trying to push
> a blob over a blob. The problem there is not "already exists" but rather
> "a blob is not something that can fast-forward". Using the existing
> REJECT_NONFASTFORWARD is insufficient (because later code will recommend
> pull-then-push, which is wrong). So I'd be in favor of creating a new
> error status for it.
I agree with everything above. I just don't understand why reverting
the "already exists" behavior for non-commit-ish objects was a
prerequisite to fixing this. Despite the flaws (I am not referring to
the buggy behavior) you and Junio have pointed out, this still seems
like an improvement over the previous (and soon-to-be current)
behavior. Saying the remote reference already exists is true, and it
implies that removing it might solve the problem which is also true.
Adding another error status will allow the error message to be made
clearer in both cases (i.e., I avoided the word "tag" specifically so
that it would apply to other cases, or so I thought.)
Chris
^ permalink raw reply
* Build broken for contrib/remote-helpers...
From: John Szakmeister @ 2013-01-22 5:49 UTC (permalink / raw)
To: git
I tried running make in contrib/remote-helpers and it died with:
:: make
make -e -C ../../t test
rm -f -r test-results
duplicate test numbers: /Users/jszakmeister/sources/git
make[1]: *** [test-lint-duplicates] Error 1
make: *** [test] Error 2
The path shown is not quite correct. I have the sources extracted to
/Users/jszakmeister/sources/git-1.8.1.1. It appears that the Makefile
in contrib/remote-helpers is exporting T, which is causing the
duplicate test detection to fail.
Thought you'd like to know!
-John
^ permalink raw reply
* [PATCH 0/3] Finishing touches to "push" advises
From: Junio C Hamano @ 2013-01-22 5:53 UTC (permalink / raw)
To: git; +Cc: Jeff King, Chris Rorvick
In-Reply-To: <20130121234002.GE17156@sigill.intra.peff.net>
This builds on Chris Rorvick's earlier effort to forbid unforced
updates to refs/tags/ hierarchy and giving sensible error and advise
messages for that case (we are not rejecting such a push due to fast
forwardness, and suggesting to fetch and integrate before pushing
again does not make sense).
The main change is in the second patch. When we
* do not have the object at the tip of the remote;
* the object at the tip of the remote is not a commit; or
* the object we are pushing is not a commit,
there is no point suggesting to fetch, integrate and push again.
If we do not have the current object at the tip of the remote, we
should tell the user to fetch first and evaluate the situation
before deciding what to do next.
Otherwise, if the current object is not a commit, or if we are
trying to push an object that is not a commit, then the user does
not have to fetch first (we already have the object), but it still
does not make sense to suggest to integrate and re-push. Just tell
them that such a push requires a force in such a case.
Junio C Hamano (3):
push: further clean up fields of "struct ref"
push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE
push: further reduce "struct ref" and simplify the logic
advice.c | 4 ++++
advice.h | 2 ++
builtin/push.c | 25 +++++++++++++++++++++++++
builtin/send-pack.c | 10 ++++++++++
cache.h | 6 +++---
remote.c | 38 ++++++++++++++++----------------------
send-pack.c | 2 ++
transport-helper.c | 10 ++++++++++
transport.c | 14 +++++++++++++-
transport.h | 2 ++
10 files changed, 87 insertions(+), 26 deletions(-)
--
1.8.1.1.498.gfdee8be
^ permalink raw reply
* [PATCH 1/3] push: further clean up fields of "struct ref"
From: Junio C Hamano @ 2013-01-22 5:53 UTC (permalink / raw)
To: git; +Cc: Jeff King, Chris Rorvick
In-Reply-To: <1358834027-32039-1-git-send-email-gitster@pobox.com>
The "nonfastforward" field is only used to decide what value to
assign to the "status" locally in a single function. Remove it from
the "struct ref" and make it into a local variable.
The "requires_force" field is not used to decide if the proposed
update requires a --force option to succeed, or to record such a
decision made elsewhere. It is used by status reporting code that
the particular update was "forced". Rename it to "forced_udpate",
and move the code to assign to it around to further clarify how it
is used and what it is used for.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
cache.h | 3 +--
remote.c | 10 +++++-----
transport.c | 2 +-
3 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/cache.h b/cache.h
index a942bbd..baa47b4 100644
--- a/cache.h
+++ b/cache.h
@@ -1001,9 +1001,8 @@ struct ref {
char *symref;
unsigned int
force:1,
- requires_force:1,
+ forced_update:1,
merge:1,
- nonfastforward:1,
update:1,
deletion:1;
enum {
diff --git a/remote.c b/remote.c
index d3a1ca2..875296c 100644
--- a/remote.c
+++ b/remote.c
@@ -1322,22 +1322,22 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
!is_null_sha1(ref->old_sha1);
if (ref->update) {
- ref->nonfastforward =
+ int nonfastforward =
!has_sha1_file(ref->old_sha1)
- || !ref_newer(ref->new_sha1, ref->old_sha1);
+ || !ref_newer(ref->new_sha1, ref->old_sha1);
if (!prefixcmp(ref->name, "refs/tags/")) {
- ref->requires_force = 1;
if (!force_ref_update) {
ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
continue;
}
- } else if (ref->nonfastforward) {
- ref->requires_force = 1;
+ ref->forced_update = 1;
+ } else if (nonfastforward) {
if (!force_ref_update) {
ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
continue;
}
+ ref->forced_update = 1;
}
}
}
diff --git a/transport.c b/transport.c
index 2673d27..585ebcd 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->requires_force) {
+ if (ref->forced_update) {
strcat(quickref, "...");
type = '+';
msg = "forced update";
--
1.8.1.1.498.gfdee8be
^ permalink raw reply related
* [PATCH 3/3] push: further reduce "struct ref" and simplify the logic
From: Junio C Hamano @ 2013-01-22 5:53 UTC (permalink / raw)
To: git; +Cc: Jeff King, Chris Rorvick
In-Reply-To: <1358834027-32039-1-git-send-email-gitster@pobox.com>
The "update" field in "struct ref" is only used in a very narrow
scope in a single function. Remove it.
Also simplify the code that rejects an attempted push by first
checking if the proposed update is forced (in which case we do not
need any check on our end).
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
cache.h | 1 -
remote.c | 42 +++++++++++++-----------------------------
2 files changed, 13 insertions(+), 30 deletions(-)
diff --git a/cache.h b/cache.h
index 360bba5..377a3df 100644
--- a/cache.h
+++ b/cache.h
@@ -1003,7 +1003,6 @@ struct ref {
force:1,
forced_update:1,
merge:1,
- update:1,
deletion:1;
enum {
REF_STATUS_NONE = 0,
diff --git a/remote.c b/remote.c
index 689dcf7..248910f 100644
--- a/remote.c
+++ b/remote.c
@@ -1317,37 +1317,21 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
* passing the --force argument
*/
- ref->update =
- !ref->deletion &&
- !is_null_sha1(ref->old_sha1);
-
- if (ref->update) {
- if (!prefixcmp(ref->name, "refs/tags/")) {
- if (!force_ref_update) {
- ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
- continue;
- }
- ref->forced_update = 1;
- } else if (!has_sha1_file(ref->old_sha1) ||
- !lookup_commit_reference_gently(ref->old_sha1, 1)) {
- if (!force_ref_update) {
- ref->status = REF_STATUS_REJECT_FETCH_FIRST;
- continue;
- }
- ref->forced_update = 1;
- } else if (!lookup_commit_reference_gently(ref->new_sha1, 1)) {
- if (!force_ref_update) {
- ref->status = REF_STATUS_REJECT_NEEDS_FORCE;
- continue;
- }
- ref->forced_update = 1;
- } else if (!ref_newer(ref->new_sha1, ref->old_sha1)) {
- if (!force_ref_update) {
- ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
- continue;
- }
+ if (!ref->deletion && !is_null_sha1(ref->old_sha1)) {
+ if (force_ref_update) {
ref->forced_update = 1;
+ continue;
}
+
+ if (!prefixcmp(ref->name, "refs/tags/"))
+ ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
+ else if (!has_sha1_file(ref->old_sha1) ||
+ !lookup_commit_reference_gently(ref->old_sha1, 1))
+ ref->status = REF_STATUS_REJECT_FETCH_FIRST;
+ else if (!lookup_commit_reference_gently(ref->new_sha1, 1))
+ ref->status = REF_STATUS_REJECT_NEEDS_FORCE;
+ else if (!ref_newer(ref->new_sha1, ref->old_sha1))
+ ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
}
}
}
--
1.8.1.1.498.gfdee8be
^ permalink raw reply related
* [PATCH 2/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE
From: Junio C Hamano @ 2013-01-22 5:53 UTC (permalink / raw)
To: git; +Cc: Jeff King, Chris Rorvick
In-Reply-To: <1358834027-32039-1-git-send-email-gitster@pobox.com>
When pushing update an existing ref, we wouldn't even know if we are
fast-forwarding the ref on the other end if:
* we do not have the object currently at the tip of remote;
* the object currently at the tip of remote is not a committish; or
* the object we are pushing is not a committish.
In such a case, the push has been rejected on the client end, but we
used the same error and advice messages as the ones used when
rejecting a non-fast-forward push, i.e. pull from there and
integrate before pushing again. This did not make much sense.
Introduce two error classes and suggest fetching from the other side
first and evaluate the situation, if we do not have the current
object, or just tell the user the update needs --force when we do
have the current object and either it or the object we are trying to
push is not a committish, in which case it can never fast-forward
and we know there is no point suggesting to merge.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
advice.c | 4 ++++
advice.h | 2 ++
builtin/push.c | 25 +++++++++++++++++++++++++
builtin/send-pack.c | 10 ++++++++++
cache.h | 2 ++
remote.c | 22 ++++++++++++++++------
send-pack.c | 2 ++
transport-helper.c | 10 ++++++++++
transport.c | 12 ++++++++++++
transport.h | 2 ++
10 files changed, 85 insertions(+), 6 deletions(-)
diff --git a/advice.c b/advice.c
index d287927..780f58d 100644
--- a/advice.c
+++ b/advice.c
@@ -5,6 +5,8 @@ int advice_push_non_ff_current = 1;
int advice_push_non_ff_default = 1;
int advice_push_non_ff_matching = 1;
int advice_push_already_exists = 1;
+int advice_push_fetch_first = 1;
+int advice_push_needs_force = 1;
int advice_status_hints = 1;
int advice_commit_before_merge = 1;
int advice_resolve_conflict = 1;
@@ -20,6 +22,8 @@ static struct {
{ "pushnonffdefault", &advice_push_non_ff_default },
{ "pushnonffmatching", &advice_push_non_ff_matching },
{ "pushalreadyexists", &advice_push_already_exists },
+ { "pushfetchfirst", &advice_push_fetch_first },
+ { "pushneedsforce", &advice_push_needs_force },
{ "statushints", &advice_status_hints },
{ "commitbeforemerge", &advice_commit_before_merge },
{ "resolveconflict", &advice_resolve_conflict },
diff --git a/advice.h b/advice.h
index 8bf6356..fad36df 100644
--- a/advice.h
+++ b/advice.h
@@ -8,6 +8,8 @@ extern int advice_push_non_ff_current;
extern int advice_push_non_ff_default;
extern int advice_push_non_ff_matching;
extern int advice_push_already_exists;
+extern int advice_push_fetch_first;
+extern int advice_push_needs_force;
extern int advice_status_hints;
extern int advice_commit_before_merge;
extern int advice_resolve_conflict;
diff --git a/builtin/push.c b/builtin/push.c
index 8491e43..da928fa 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -224,6 +224,13 @@ static const char message_advice_ref_already_exists[] =
N_("Updates were rejected because the destination reference already exists\n"
"in the remote.");
+static const char message_advice_ref_fetch_first[] =
+ N_("Updates were rejected; you need to fetch the destination reference\n"
+ "to decide what to do.\n");
+
+static const char message_advice_ref_needs_force[] =
+ N_("Updates were rejected; you need to force update.\n");
+
static void advise_pull_before_push(void)
{
if (!advice_push_non_ff_current || !advice_push_update_rejected)
@@ -252,6 +259,20 @@ static void advise_ref_already_exists(void)
advise(_(message_advice_ref_already_exists));
}
+static void advise_ref_fetch_first(void)
+{
+ if (!advice_push_fetch_first || !advice_push_update_rejected)
+ return;
+ advise(_(message_advice_ref_fetch_first));
+}
+
+static void advise_ref_needs_force(void)
+{
+ if (!advice_push_needs_force || !advice_push_update_rejected)
+ return;
+ advise(_(message_advice_ref_needs_force));
+}
+
static int push_with_options(struct transport *transport, int flags)
{
int err;
@@ -285,6 +306,10 @@ static int push_with_options(struct transport *transport, int flags)
advise_checkout_pull_push();
} else if (reject_reasons & REJECT_ALREADY_EXISTS) {
advise_ref_already_exists();
+ } else if (reject_reasons & REJECT_FETCH_FIRST) {
+ advise_ref_fetch_first();
+ } else if (reject_reasons & REJECT_NEEDS_FORCE) {
+ advise_ref_needs_force();
}
return 1;
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index f849e0a..57a46b2 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -44,6 +44,16 @@ static void print_helper_status(struct ref *ref)
msg = "non-fast forward";
break;
+ case REF_STATUS_REJECT_FETCH_FIRST:
+ res = "error";
+ msg = "fetch first";
+ break;
+
+ case REF_STATUS_REJECT_NEEDS_FORCE:
+ res = "error";
+ msg = "needs force";
+ break;
+
case REF_STATUS_REJECT_ALREADY_EXISTS:
res = "error";
msg = "already exists";
diff --git a/cache.h b/cache.h
index baa47b4..360bba5 100644
--- a/cache.h
+++ b/cache.h
@@ -1011,6 +1011,8 @@ struct ref {
REF_STATUS_REJECT_NONFASTFORWARD,
REF_STATUS_REJECT_ALREADY_EXISTS,
REF_STATUS_REJECT_NODELETE,
+ REF_STATUS_REJECT_FETCH_FIRST,
+ REF_STATUS_REJECT_NEEDS_FORCE,
REF_STATUS_UPTODATE,
REF_STATUS_REMOTE_REJECT,
REF_STATUS_EXPECTING_REPORT
diff --git a/remote.c b/remote.c
index 875296c..689dcf7 100644
--- a/remote.c
+++ b/remote.c
@@ -1322,17 +1322,26 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
!is_null_sha1(ref->old_sha1);
if (ref->update) {
- int nonfastforward =
- !has_sha1_file(ref->old_sha1)
- || !ref_newer(ref->new_sha1, ref->old_sha1);
-
if (!prefixcmp(ref->name, "refs/tags/")) {
if (!force_ref_update) {
ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
continue;
}
ref->forced_update = 1;
- } else if (nonfastforward) {
+ } else if (!has_sha1_file(ref->old_sha1) ||
+ !lookup_commit_reference_gently(ref->old_sha1, 1)) {
+ if (!force_ref_update) {
+ ref->status = REF_STATUS_REJECT_FETCH_FIRST;
+ continue;
+ }
+ ref->forced_update = 1;
+ } else if (!lookup_commit_reference_gently(ref->new_sha1, 1)) {
+ if (!force_ref_update) {
+ ref->status = REF_STATUS_REJECT_NEEDS_FORCE;
+ continue;
+ }
+ ref->forced_update = 1;
+ } else if (!ref_newer(ref->new_sha1, ref->old_sha1)) {
if (!force_ref_update) {
ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
continue;
@@ -1521,7 +1530,8 @@ int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1)
struct commit_list *list, *used;
int found = 0;
- /* Both new and old must be commit-ish and new is descendant of
+ /*
+ * Both new and old must be commit-ish and new is descendant of
* old. Otherwise we require --force.
*/
o = deref_tag(parse_object(old_sha1), NULL, 0);
diff --git a/send-pack.c b/send-pack.c
index 1c375f0..97ab336 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -230,6 +230,8 @@ int send_pack(struct send_pack_args *args,
switch (ref->status) {
case REF_STATUS_REJECT_NONFASTFORWARD:
case REF_STATUS_REJECT_ALREADY_EXISTS:
+ case REF_STATUS_REJECT_FETCH_FIRST:
+ case REF_STATUS_REJECT_NEEDS_FORCE:
case REF_STATUS_UPTODATE:
continue;
default:
diff --git a/transport-helper.c b/transport-helper.c
index 965b778..cb3ef7d 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -666,6 +666,16 @@ static void push_update_ref_status(struct strbuf *buf,
free(msg);
msg = NULL;
}
+ else if (!strcmp(msg, "fetch first")) {
+ status = REF_STATUS_REJECT_FETCH_FIRST;
+ free(msg);
+ msg = NULL;
+ }
+ else if (!strcmp(msg, "needs force")) {
+ status = REF_STATUS_REJECT_NEEDS_FORCE;
+ free(msg);
+ msg = NULL;
+ }
}
if (*ref)
diff --git a/transport.c b/transport.c
index 585ebcd..5105562 100644
--- a/transport.c
+++ b/transport.c
@@ -699,6 +699,14 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
print_ref_status('!', "[rejected]", ref, ref->peer_ref,
"already exists", porcelain);
break;
+ case REF_STATUS_REJECT_FETCH_FIRST:
+ print_ref_status('!', "[rejected]", ref, ref->peer_ref,
+ "fetch first", porcelain);
+ break;
+ case REF_STATUS_REJECT_NEEDS_FORCE:
+ print_ref_status('!', "[rejected]", ref, ref->peer_ref,
+ "needs force", porcelain);
+ break;
case REF_STATUS_REMOTE_REJECT:
print_ref_status('!', "[remote rejected]", ref,
ref->deletion ? NULL : ref->peer_ref,
@@ -750,6 +758,10 @@ void transport_print_push_status(const char *dest, struct ref *refs,
*reject_reasons |= REJECT_NON_FF_OTHER;
} else if (ref->status == REF_STATUS_REJECT_ALREADY_EXISTS) {
*reject_reasons |= REJECT_ALREADY_EXISTS;
+ } else if (ref->status == REF_STATUS_REJECT_FETCH_FIRST) {
+ *reject_reasons |= REJECT_FETCH_FIRST;
+ } else if (ref->status == REF_STATUS_REJECT_NEEDS_FORCE) {
+ *reject_reasons |= REJECT_NEEDS_FORCE;
}
}
}
diff --git a/transport.h b/transport.h
index bfd2df5..c818763 100644
--- a/transport.h
+++ b/transport.h
@@ -143,6 +143,8 @@ void transport_set_verbosity(struct transport *transport, int verbosity,
#define REJECT_NON_FF_HEAD 0x01
#define REJECT_NON_FF_OTHER 0x02
#define REJECT_ALREADY_EXISTS 0x04
+#define REJECT_FETCH_FIRST 0x08
+#define REJECT_NEEDS_FORCE 0x10
int transport_push(struct transport *connection,
int refspec_nr, const char **refspec, int flags,
--
1.8.1.1.498.gfdee8be
^ permalink raw reply related
* Re: [PATCH 2/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE
From: Junio C Hamano @ 2013-01-22 6:04 UTC (permalink / raw)
To: git; +Cc: Jeff King, Chris Rorvick
In-Reply-To: <1358834027-32039-3-git-send-email-gitster@pobox.com>
This one has a logic flaw. The logic outlined in the cover letter
is correct, and the one described in the log message of this one is
not.
We should say "fetch first" only when we do not have old_sha1.
diff --git a/remote.c b/remote.c
index 248910f..8c39ea2 100644
--- a/remote.c
+++ b/remote.c
@@ -1325,10 +1325,10 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
if (!prefixcmp(ref->name, "refs/tags/"))
ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
- else if (!has_sha1_file(ref->old_sha1) ||
- !lookup_commit_reference_gently(ref->old_sha1, 1))
+ else if (!has_sha1_file(ref->old_sha1))
ref->status = REF_STATUS_REJECT_FETCH_FIRST;
- else if (!lookup_commit_reference_gently(ref->new_sha1, 1))
+ else if (!lookup_commit_reference_gently(ref->new_sha1, 1) ||
+ !lookup_commit_reference_gently(ref->old_sha1, 1))
ref->status = REF_STATUS_REJECT_NEEDS_FORCE;
else if (!ref_newer(ref->new_sha1, ref->old_sha1))
ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
^ permalink raw reply related
* Re: [PATCH 3/3] push: further reduce "struct ref" and simplify the logic
From: Junio C Hamano @ 2013-01-22 6:21 UTC (permalink / raw)
To: git; +Cc: Jeff King, Chris Rorvick
In-Reply-To: <1358834027-32039-4-git-send-email-gitster@pobox.com>
Junio C Hamano <gitster@pobox.com> writes:
> The "update" field in "struct ref" is only used in a very narrow
> scope in a single function. Remove it.
>
> Also simplify the code that rejects an attempted push by first
> checking if the proposed update is forced (in which case we do not
> need any check on our end).
Actually, the latter is a bad idea; it changes the semantics and
mark a push that was done with an unnecessary --force option.
I'm rerolling these three patches (the "update" removal will also be
moved to the preparatory clean-up patch).
^ permalink raw reply
* [PATCH 0/3] Finishing touches to "push" advises
From: Junio C Hamano @ 2013-01-22 6:30 UTC (permalink / raw)
To: git; +Cc: Jeff King, Chris Rorvick
In-Reply-To: <20130121234002.GE17156@sigill.intra.peff.net>
This builds on Chris Rorvick's earlier effort to forbid unforced
updates to refs/tags/ hierarchy and giving sensible error and advise
messages for that case (we are not rejecting such a push due to fast
forwardness, and suggesting to fetch and integrate before pushing
again does not make sense).
The series applies on top of 256b9d7 (push: fix "refs/tags/
hierarchy cannot be updated without --force", 2013-01-16).
The main change is in the second patch. When we
* do not have the object at the tip of the remote;
* the object at the tip of the remote is not a commit; or
* the object we are pushing is not a commit,
there is no point suggesting to fetch, integrate and push again.
If we do not have the current object at the tip of the remote, we
should tell the user to fetch first and evaluate the situation
before deciding what to do next.
Otherwise, if the current object is not a commit, or if we are
trying to push an object that is not a commit, then the user does
not have to fetch first (we already have the object), but it still
does not make sense to suggest to integrate and re-push. Just tell
them that such a push requires a force in such a case.
Junio C Hamano (3):
push: further clean up fields of "struct ref"
push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE
push: further simplify the logic to assign rejection status
advice.c | 4 ++++
advice.h | 2 ++
builtin/push.c | 25 +++++++++++++++++++++++++
builtin/send-pack.c | 10 ++++++++++
cache.h | 6 +++---
remote.c | 42 +++++++++++++++++++-----------------------
send-pack.c | 2 ++
transport-helper.c | 10 ++++++++++
transport.c | 14 +++++++++++++-
transport.h | 2 ++
10 files changed, 90 insertions(+), 27 deletions(-)
--
1.8.1.1.498.gfdee8be
^ permalink raw reply
* [PATCH v2 1/3] push: further clean up fields of "struct ref"
From: Junio C Hamano @ 2013-01-22 6:30 UTC (permalink / raw)
To: git; +Cc: Jeff King, Chris Rorvick
In-Reply-To: <1358836230-9197-1-git-send-email-gitster@pobox.com>
The "nonfastforward" and "update" fields are only used while
deciding what value to assign to the "status" locally in a single
function. Remove them from the "struct ref".
The "requires_force" field is not used to decide if the proposed
update requires a --force option to succeed, or to record such a
decision made elsewhere. It is used by status reporting code that
the particular update was "forced". Rename it to "forced_udpate",
and move the code to assign to it around to further clarify how it
is used and what it is used for.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* The "update" removal in v1 has been moved to this.
cache.h | 4 +---
remote.c | 16 ++++++----------
transport.c | 2 +-
3 files changed, 8 insertions(+), 14 deletions(-)
diff --git a/cache.h b/cache.h
index a942bbd..12631a1 100644
--- a/cache.h
+++ b/cache.h
@@ -1001,10 +1001,8 @@ struct ref {
char *symref;
unsigned int
force:1,
- requires_force:1,
+ forced_update:1,
merge:1,
- nonfastforward:1,
- update:1,
deletion:1;
enum {
REF_STATUS_NONE = 0,
diff --git a/remote.c b/remote.c
index d3a1ca2..3375914 100644
--- a/remote.c
+++ b/remote.c
@@ -1317,27 +1317,23 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
* passing the --force argument
*/
- ref->update =
- !ref->deletion &&
- !is_null_sha1(ref->old_sha1);
-
- if (ref->update) {
- ref->nonfastforward =
+ if (!ref->deletion && !is_null_sha1(ref->old_sha1)) {
+ int nonfastforward =
!has_sha1_file(ref->old_sha1)
- || !ref_newer(ref->new_sha1, ref->old_sha1);
+ || !ref_newer(ref->new_sha1, ref->old_sha1);
if (!prefixcmp(ref->name, "refs/tags/")) {
- ref->requires_force = 1;
if (!force_ref_update) {
ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
continue;
}
- } else if (ref->nonfastforward) {
- ref->requires_force = 1;
+ ref->forced_update = 1;
+ } else if (nonfastforward) {
if (!force_ref_update) {
ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
continue;
}
+ ref->forced_update = 1;
}
}
}
diff --git a/transport.c b/transport.c
index 2673d27..585ebcd 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->requires_force) {
+ if (ref->forced_update) {
strcat(quickref, "...");
type = '+';
msg = "forced update";
--
1.8.1.1.498.gfdee8be
^ permalink raw reply related
* [PATCH v2 2/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE
From: Junio C Hamano @ 2013-01-22 6:30 UTC (permalink / raw)
To: git; +Cc: Jeff King, Chris Rorvick
In-Reply-To: <1358836230-9197-1-git-send-email-gitster@pobox.com>
When we push to update an existing ref, if:
* we do not have the object at the tip of the remote; or
* the object at the tip of the remote is not a commit; or
* the object we are pushing is not a commit,
there is no point suggesting to fetch, integrate and push again.
If we do not have the current object at the tip of the remote, we
should tell the user to fetch first and evaluate the situation
before deciding what to do next.
Otherwise, if the current object is not a commit, or if we are
trying to push an object that is not a commit, then the user does
not have to fetch first (we already have the object), but it still
does not make sense to suggest to integrate and re-push. Just tell
them that such a push requires a force in such a case.
In these cases, the push was locally rejected on the client end, but
we used the same error and advice messages as the ones used when
rejecting a non-fast-forward push, i.e. pull from there and
integrate before pushing again. This did not make much sense.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* Updated log message and fixed the logic to decide "fetch first";
we should say "fetch first" only when we do not have the current
tip of the remote end.
send-pack.c has style violation that "else" is not on the same
line as closing brace of its corresponding "if", but I followed
the existing style of surrounding code. Cleaning them up is for
a separate topic.
advice.c | 4 ++++
advice.h | 2 ++
builtin/push.c | 25 +++++++++++++++++++++++++
builtin/send-pack.c | 10 ++++++++++
cache.h | 2 ++
remote.c | 22 ++++++++++++++++------
send-pack.c | 2 ++
transport-helper.c | 10 ++++++++++
transport.c | 12 ++++++++++++
transport.h | 2 ++
10 files changed, 85 insertions(+), 6 deletions(-)
diff --git a/advice.c b/advice.c
index d287927..780f58d 100644
--- a/advice.c
+++ b/advice.c
@@ -5,6 +5,8 @@ int advice_push_non_ff_current = 1;
int advice_push_non_ff_default = 1;
int advice_push_non_ff_matching = 1;
int advice_push_already_exists = 1;
+int advice_push_fetch_first = 1;
+int advice_push_needs_force = 1;
int advice_status_hints = 1;
int advice_commit_before_merge = 1;
int advice_resolve_conflict = 1;
@@ -20,6 +22,8 @@ static struct {
{ "pushnonffdefault", &advice_push_non_ff_default },
{ "pushnonffmatching", &advice_push_non_ff_matching },
{ "pushalreadyexists", &advice_push_already_exists },
+ { "pushfetchfirst", &advice_push_fetch_first },
+ { "pushneedsforce", &advice_push_needs_force },
{ "statushints", &advice_status_hints },
{ "commitbeforemerge", &advice_commit_before_merge },
{ "resolveconflict", &advice_resolve_conflict },
diff --git a/advice.h b/advice.h
index 8bf6356..fad36df 100644
--- a/advice.h
+++ b/advice.h
@@ -8,6 +8,8 @@ extern int advice_push_non_ff_current;
extern int advice_push_non_ff_default;
extern int advice_push_non_ff_matching;
extern int advice_push_already_exists;
+extern int advice_push_fetch_first;
+extern int advice_push_needs_force;
extern int advice_status_hints;
extern int advice_commit_before_merge;
extern int advice_resolve_conflict;
diff --git a/builtin/push.c b/builtin/push.c
index 8491e43..da928fa 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -224,6 +224,13 @@ static const char message_advice_ref_already_exists[] =
N_("Updates were rejected because the destination reference already exists\n"
"in the remote.");
+static const char message_advice_ref_fetch_first[] =
+ N_("Updates were rejected; you need to fetch the destination reference\n"
+ "to decide what to do.\n");
+
+static const char message_advice_ref_needs_force[] =
+ N_("Updates were rejected; you need to force update.\n");
+
static void advise_pull_before_push(void)
{
if (!advice_push_non_ff_current || !advice_push_update_rejected)
@@ -252,6 +259,20 @@ static void advise_ref_already_exists(void)
advise(_(message_advice_ref_already_exists));
}
+static void advise_ref_fetch_first(void)
+{
+ if (!advice_push_fetch_first || !advice_push_update_rejected)
+ return;
+ advise(_(message_advice_ref_fetch_first));
+}
+
+static void advise_ref_needs_force(void)
+{
+ if (!advice_push_needs_force || !advice_push_update_rejected)
+ return;
+ advise(_(message_advice_ref_needs_force));
+}
+
static int push_with_options(struct transport *transport, int flags)
{
int err;
@@ -285,6 +306,10 @@ static int push_with_options(struct transport *transport, int flags)
advise_checkout_pull_push();
} else if (reject_reasons & REJECT_ALREADY_EXISTS) {
advise_ref_already_exists();
+ } else if (reject_reasons & REJECT_FETCH_FIRST) {
+ advise_ref_fetch_first();
+ } else if (reject_reasons & REJECT_NEEDS_FORCE) {
+ advise_ref_needs_force();
}
return 1;
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index f849e0a..57a46b2 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -44,6 +44,16 @@ static void print_helper_status(struct ref *ref)
msg = "non-fast forward";
break;
+ case REF_STATUS_REJECT_FETCH_FIRST:
+ res = "error";
+ msg = "fetch first";
+ break;
+
+ case REF_STATUS_REJECT_NEEDS_FORCE:
+ res = "error";
+ msg = "needs force";
+ break;
+
case REF_STATUS_REJECT_ALREADY_EXISTS:
res = "error";
msg = "already exists";
diff --git a/cache.h b/cache.h
index 12631a1..377a3df 100644
--- a/cache.h
+++ b/cache.h
@@ -1010,6 +1010,8 @@ struct ref {
REF_STATUS_REJECT_NONFASTFORWARD,
REF_STATUS_REJECT_ALREADY_EXISTS,
REF_STATUS_REJECT_NODELETE,
+ REF_STATUS_REJECT_FETCH_FIRST,
+ REF_STATUS_REJECT_NEEDS_FORCE,
REF_STATUS_UPTODATE,
REF_STATUS_REMOTE_REJECT,
REF_STATUS_EXPECTING_REPORT
diff --git a/remote.c b/remote.c
index 3375914..c991915 100644
--- a/remote.c
+++ b/remote.c
@@ -1318,17 +1318,26 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
*/
if (!ref->deletion && !is_null_sha1(ref->old_sha1)) {
- int nonfastforward =
- !has_sha1_file(ref->old_sha1)
- || !ref_newer(ref->new_sha1, ref->old_sha1);
-
if (!prefixcmp(ref->name, "refs/tags/")) {
if (!force_ref_update) {
ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
continue;
}
ref->forced_update = 1;
- } else if (nonfastforward) {
+ } else if (!has_sha1_file(ref->old_sha1)) {
+ if (!force_ref_update) {
+ ref->status = REF_STATUS_REJECT_FETCH_FIRST;
+ continue;
+ }
+ ref->forced_update = 1;
+ } else if (!lookup_commit_reference_gently(ref->old_sha1, 1) ||
+ !lookup_commit_reference_gently(ref->new_sha1, 1)) {
+ if (!force_ref_update) {
+ ref->status = REF_STATUS_REJECT_NEEDS_FORCE;
+ continue;
+ }
+ ref->forced_update = 1;
+ } else if (!ref_newer(ref->new_sha1, ref->old_sha1)) {
if (!force_ref_update) {
ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
continue;
@@ -1517,7 +1526,8 @@ int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1)
struct commit_list *list, *used;
int found = 0;
- /* Both new and old must be commit-ish and new is descendant of
+ /*
+ * Both new and old must be commit-ish and new is descendant of
* old. Otherwise we require --force.
*/
o = deref_tag(parse_object(old_sha1), NULL, 0);
diff --git a/send-pack.c b/send-pack.c
index 1c375f0..97ab336 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -230,6 +230,8 @@ int send_pack(struct send_pack_args *args,
switch (ref->status) {
case REF_STATUS_REJECT_NONFASTFORWARD:
case REF_STATUS_REJECT_ALREADY_EXISTS:
+ case REF_STATUS_REJECT_FETCH_FIRST:
+ case REF_STATUS_REJECT_NEEDS_FORCE:
case REF_STATUS_UPTODATE:
continue;
default:
diff --git a/transport-helper.c b/transport-helper.c
index 965b778..cb3ef7d 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -666,6 +666,16 @@ static void push_update_ref_status(struct strbuf *buf,
free(msg);
msg = NULL;
}
+ else if (!strcmp(msg, "fetch first")) {
+ status = REF_STATUS_REJECT_FETCH_FIRST;
+ free(msg);
+ msg = NULL;
+ }
+ else if (!strcmp(msg, "needs force")) {
+ status = REF_STATUS_REJECT_NEEDS_FORCE;
+ free(msg);
+ msg = NULL;
+ }
}
if (*ref)
diff --git a/transport.c b/transport.c
index 585ebcd..5105562 100644
--- a/transport.c
+++ b/transport.c
@@ -699,6 +699,14 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
print_ref_status('!', "[rejected]", ref, ref->peer_ref,
"already exists", porcelain);
break;
+ case REF_STATUS_REJECT_FETCH_FIRST:
+ print_ref_status('!', "[rejected]", ref, ref->peer_ref,
+ "fetch first", porcelain);
+ break;
+ case REF_STATUS_REJECT_NEEDS_FORCE:
+ print_ref_status('!', "[rejected]", ref, ref->peer_ref,
+ "needs force", porcelain);
+ break;
case REF_STATUS_REMOTE_REJECT:
print_ref_status('!', "[remote rejected]", ref,
ref->deletion ? NULL : ref->peer_ref,
@@ -750,6 +758,10 @@ void transport_print_push_status(const char *dest, struct ref *refs,
*reject_reasons |= REJECT_NON_FF_OTHER;
} else if (ref->status == REF_STATUS_REJECT_ALREADY_EXISTS) {
*reject_reasons |= REJECT_ALREADY_EXISTS;
+ } else if (ref->status == REF_STATUS_REJECT_FETCH_FIRST) {
+ *reject_reasons |= REJECT_FETCH_FIRST;
+ } else if (ref->status == REF_STATUS_REJECT_NEEDS_FORCE) {
+ *reject_reasons |= REJECT_NEEDS_FORCE;
}
}
}
diff --git a/transport.h b/transport.h
index bfd2df5..c818763 100644
--- a/transport.h
+++ b/transport.h
@@ -143,6 +143,8 @@ void transport_set_verbosity(struct transport *transport, int verbosity,
#define REJECT_NON_FF_HEAD 0x01
#define REJECT_NON_FF_OTHER 0x02
#define REJECT_ALREADY_EXISTS 0x04
+#define REJECT_FETCH_FIRST 0x08
+#define REJECT_NEEDS_FORCE 0x10
int transport_push(struct transport *connection,
int refspec_nr, const char **refspec, int flags,
--
1.8.1.1.498.gfdee8be
^ permalink raw reply related
* [PATCH v2 3/3] push: further simplify the logic to assign rejection status
From: Junio C Hamano @ 2013-01-22 6:30 UTC (permalink / raw)
To: git; +Cc: Jeff King, Chris Rorvick
In-Reply-To: <1358836230-9197-1-git-send-email-gitster@pobox.com>
Instead of using deeply nested if/else statements, first decide what
rejection status we would get if this push weren't forced, and then
assign the rejection reason to the ref->status field and flip the
ref->forced_update field when we forced a push for a ref that indeed
required forcing.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* The first one mistakenly changed the semantics and reported a
forced push even when the push was done with useless and
unnecessary --force option (e.g. the update was properly
fast-forwarding but --force was given from the command line).
This fixes it.
remote.c | 40 +++++++++++++++-------------------------
1 file changed, 15 insertions(+), 25 deletions(-)
diff --git a/remote.c b/remote.c
index c991915..af2136d 100644
--- a/remote.c
+++ b/remote.c
@@ -1318,32 +1318,22 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
*/
if (!ref->deletion && !is_null_sha1(ref->old_sha1)) {
- if (!prefixcmp(ref->name, "refs/tags/")) {
- if (!force_ref_update) {
- ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
- continue;
- }
+ int status = 0;
+
+ if (!prefixcmp(ref->name, "refs/tags/"))
+ status = REF_STATUS_REJECT_ALREADY_EXISTS;
+ else if (!has_sha1_file(ref->old_sha1))
+ status = REF_STATUS_REJECT_FETCH_FIRST;
+ else if (!lookup_commit_reference_gently(ref->old_sha1, 1) ||
+ !lookup_commit_reference_gently(ref->new_sha1, 1))
+ status = REF_STATUS_REJECT_NEEDS_FORCE;
+ else if (!ref_newer(ref->new_sha1, ref->old_sha1))
+ status = REF_STATUS_REJECT_NONFASTFORWARD;
+
+ if (!force_ref_update)
+ ref->status = status;
+ else if (status)
ref->forced_update = 1;
- } else if (!has_sha1_file(ref->old_sha1)) {
- if (!force_ref_update) {
- ref->status = REF_STATUS_REJECT_FETCH_FIRST;
- continue;
- }
- ref->forced_update = 1;
- } else if (!lookup_commit_reference_gently(ref->old_sha1, 1) ||
- !lookup_commit_reference_gently(ref->new_sha1, 1)) {
- if (!force_ref_update) {
- ref->status = REF_STATUS_REJECT_NEEDS_FORCE;
- continue;
- }
- ref->forced_update = 1;
- } else if (!ref_newer(ref->new_sha1, ref->old_sha1)) {
- if (!force_ref_update) {
- ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
- continue;
- }
- ref->forced_update = 1;
- }
}
}
}
--
1.8.1.1.498.gfdee8be
^ permalink raw reply related
* Re: [PATCH v6 0/8] push: update remote tags only with force
From: Junio C Hamano @ 2013-01-22 6:44 UTC (permalink / raw)
To: Chris Rorvick
Cc: Jeff King, Max Horn, git, Angelo Borsotti, Drew Northup,
Michael Haggerty, Philip Oakley, Johannes Sixt, Kacper Kornet,
Felipe Contreras
In-Reply-To: <CAEUsAPYaK3PP67fc89-J3a83wzYcmu7HRyh7y1Kctg6d166LEQ@mail.gmail.com>
Chris Rorvick <chris@rorvick.com> writes:
> I agree with everything above. I just don't understand why reverting
> the "already exists" behavior for non-commit-ish objects was a
> prerequisite to fixing this.
Because it is a regression. People who did not force such a push
did not get "already exists", but with your patch they do.
By reverting the wrong message so that we get the old wrong message
instead, people will only have to deal with an already known
breakage; a known devil is better than an unknown new devil (or an
unknown angel).
When a change that brings in a regression and an improvement at the
same time, it does not matter what the improvement is; we fix the
regression first as soon as safely possible and we then attempt to
resurrect and polish the improvement.
^ permalink raw reply
* Re: [PATCH 0/3] Finishing touches to "push" advises
From: Junio C Hamano @ 2013-01-22 7:26 UTC (permalink / raw)
To: git; +Cc: Jeff King, Chris Rorvick
In-Reply-To: <1358836230-9197-1-git-send-email-gitster@pobox.com>
As far as I am concerned, I am pretty much done with this topic, at
least for now. Of course if there are bugreports I'll try to help
resolving them, but I do not expect myself adding new object-type
based policy decision to this codepath.
The call the updated call makes to ref_newer() no longer feeds
certain combinations to the function, because the NULL-ness of the
old and commit-ness of both are checked before making a call.
I notice that builtin/remote.c has another callsite for ref_newer().
Although I didn't look at the code, I think it is trying to see if
the branch can be pushed as a fast-forward to the remote (or the
remote tip moved since you started building on top of it).
It probably makes sense to refactor the logic that is run per-ref in
the loop in the set_ref_status_for_push() function into a new helper
function, inline ref_newer() there, and have the remaining callers
of ref_newer() to use that new helper function, which knows the new
rules such as "refs/tags/ cannot be replaced with anything without
force".
^ permalink raw reply
* Re: [RFC/PATCH] add: warn when -u or -A is used without filepattern
From: Matthieu Moy @ 2013-01-22 7:39 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, gitster, Eric James Michael Ritz, Tomas Carnecky
In-Reply-To: <20130121222248.GA3586@elie.Belkin>
Jonathan Nieder <jrnieder@gmail.com> writes:
> Would it be possible to make this conditional on cwd not being at the
> toplevel (the case where "git add -u :/" and "git add -u ." have
> different behavior)? E.g.,
>
> static const char *here[2] = { ".", NULL };
> if (prefix)
> warning(...);
I thought about this too, after writting the patch. Actually, I still I
it makes sense to warn even from the toplevel, since the point is to
teach people to stop using pathless "git add -u" for a while, so I'd say
it's easier to teach this in every condition. OTOH, the next step
(forbidding pathless "git add -u") can still allow it from the toplevel
to minimize the pain.
But I'm starting to be convinced ;-).
Any other thought on the question?
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* [PATCH v3] Enable minimal stat checking
From: Robin Rosenberg @ 2013-01-22 7:49 UTC (permalink / raw)
To: git, Junio C Hamano; +Cc: j sixt, Shawn Pearce, Robin Rosenberg
In-Reply-To: <7v4niblhr6.fsf@alter.siamese.dyndns.org>
Specifically the fields uid, gid, ctime, ino and dev are set to zero
by JGit. Other implementations, eg. Git in cygwin are allegedly also
somewhat incompatible with Git For Windows and on *nix platforms
the resolution of the timestamps may differ.
Any stat checking by git will then need to check content, which may
be very slow, particularly on Windows. Since mtime and size
is typically enough we should allow the user to tell git to avoid
checking these fields if they are set to zero in the index.
This change introduces a core.checkstat config option where the
the user can select to check all fields (default), or just size
and the whole second part of mtime (minimal).
Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
Documentation/config.txt | 6 ++++++
cache.h | 1 +
config.c | 8 ++++++++
environment.c | 1 +
read-cache.c | 28 ++++++++++++++++------------
5 files changed, 32 insertions(+), 12 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index d5809e0..47c213d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -235,6 +235,12 @@ core.trustctime::
crawlers and some backup systems).
See linkgit:git-update-index[1]. True by default.
+core.checkstat::
+ Determines which stat fields to match between the index
+ and work tree. The user can set this to 'default' or
+ 'minimal'. Default (or explicitly 'default'), is to check
+ all fields, including the sub-second part of mtime and ctime.
+
core.quotepath::
The commands that output paths (e.g. 'ls-files',
'diff'), when not given the `-z` option, will quote
diff --git a/cache.h b/cache.h
index c257953..ab20c4d 100644
--- a/cache.h
+++ b/cache.h
@@ -536,6 +536,7 @@ extern int delete_ref(const char *, const unsigned char *sha1, int delopt);
/* Environment bits from configuration mechanism */
extern int trust_executable_bit;
extern int trust_ctime;
+extern int check_stat;
extern int quote_path_fully;
extern int has_symlinks;
extern int minimum_abbrev, default_abbrev;
diff --git a/config.c b/config.c
index 7b444b6..2b58c75 100644
--- a/config.c
+++ b/config.c
@@ -566,6 +566,14 @@ static int git_default_core_config(const char *var, const char *value)
trust_ctime = git_config_bool(var, value);
return 0;
}
+ if (!strcmp(var, "core.statinfo")) {
+ if (!strcasecmp(value, "default"))
+ check_stat = 1;
+ else if (!strcasecmp(value, "minimal"))
+ check_stat = 0;
+ else
+ die_bad_config(var);
+ }
if (!strcmp(var, "core.quotepath")) {
quote_path_fully = git_config_bool(var, value);
diff --git a/environment.c b/environment.c
index 85edd7f..e828b37 100644
--- a/environment.c
+++ b/environment.c
@@ -13,6 +13,7 @@
int trust_executable_bit = 1;
int trust_ctime = 1;
+int check_stat = 1;
int has_symlinks = 1;
int minimum_abbrev = 4, default_abbrev = 7;
int ignore_case;
diff --git a/read-cache.c b/read-cache.c
index fda78bc..23db681 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -197,21 +197,25 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
}
if (ce->ce_mtime.sec != (unsigned int)st->st_mtime)
changed |= MTIME_CHANGED;
- if (trust_ctime && ce->ce_ctime.sec != (unsigned int)st->st_ctime)
- changed |= CTIME_CHANGED;
+ if (trust_ctime ? check_stat : trust_ctime/*false*/)
+ if (ce->ce_ctime.sec != (unsigned int)st->st_ctime)
+ changed |= CTIME_CHANGED;
#ifdef USE_NSEC
- if (ce->ce_mtime.nsec != ST_MTIME_NSEC(*st))
+ if (check_stat && ce->ce_mtime.nsec != ST_MTIME_NSEC(*st))
changed |= MTIME_CHANGED;
- if (trust_ctime && ce->ce_ctime.nsec != ST_CTIME_NSEC(*st))
- changed |= CTIME_CHANGED;
+ if (trust_ctime ? check_stat : trust_ctime/*false*/)
+ if (ce->ce_ctime.nsec != ST_CTIME_NSEC(*st))
+ changed |= CTIME_CHANGED;
#endif
- if (ce->ce_uid != (unsigned int) st->st_uid ||
- ce->ce_gid != (unsigned int) st->st_gid)
- changed |= OWNER_CHANGED;
- if (ce->ce_ino != (unsigned int) st->st_ino)
- changed |= INODE_CHANGED;
+ if (check_stat) {
+ if (ce->ce_uid != (unsigned int) st->st_uid ||
+ ce->ce_gid != (unsigned int) st->st_gid)
+ changed |= OWNER_CHANGED;
+ if (ce->ce_ino != (unsigned int) st->st_ino)
+ changed |= INODE_CHANGED;
+ }
#ifdef USE_STDEV
/*
@@ -219,8 +223,8 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
* clients will have different views of what "device"
* the filesystem is on
*/
- if (ce->ce_dev != (unsigned int) st->st_dev)
- changed |= INODE_CHANGED;
+ if (check_stat && ce->ce_dev != (unsigned int) st->st_dev)
+ changed |= INODE_CHANGED;
#endif
if (ce->ce_size != (unsigned int) st->st_size)
--
1.8.1.337.g6672977.dirty
^ permalink raw reply related
* Re: [PATCH v2 01/10] sequencer.c: remove broken support for rfc2822 continuation in footer
From: Jonathan Nieder @ 2013-01-22 7:54 UTC (permalink / raw)
To: Brandon Casey; +Cc: gitster, pclouds, git, Brandon Casey
In-Reply-To: <1358757627-16682-2-git-send-email-drafnel@gmail.com>
Hi,
Brandon Casey wrote:
> --- a/sequencer.c
> +++ b/sequencer.c
[...]
> @@ -1042,13 +1041,8 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
Git is checking if (sb->buf) ends with a "Signed-off-by:" style
line. If it doesn't, it will need to add an extra blank line
before adding a new sign-off.
First (snipped), it seeks back two newlines from the end and then
forward to the next non-newline character, so (buf + i) is at the
start of the last line of (the interesting part of) sb. Now:
> for (; i < len; i = k) {
> for (k = i; k < len && buf[k] != '\n'; k++)
> ; /* do nothing */
> k++;
(buf + k) points to the end of this line.
> - if ((buf[k] == ' ' || buf[k] == '\t') && !first)
> - continue;
This is always the first line examined, so this "continue" never
triggers.
> -
> - first = 0;
> -
> for (j = 0; i + j < len; j++) {
If the line matches /^[[:alnum:]-]*:/, it passes and git moves on to
the (nonexistent) next line. Otherwise, it fails.
Do I understand correctly? If so, this patch should be a no-op, which
is good, I guess.
But in that case, couldn't this function be made much simpler? As far
as I can tell, all the function needs to do is the following:
1. Find the last line.
2. Check if it is blank or matches /^[[:alnum:]-]*:/
3. There is no step 3. That's it.
In other words, something like:
const char *eol, *p;
/* End of line */
eol = memrchr(sb->buf, '\n', sb->len - ignore_footer);
if (!eol)
eol = sb->buf;
/* Start of line */
p = memrchr(sb->buf, '\n', eol - sb->buf);
if (p)
p++;
else
p = sb->buf;
if (p == eol) /* Blank line? */
return 1;
/* "Signed-off-by"-style field */
while ((isalnum(*p) || *p == '-') && p < eol)
p++;
return *p == ':';
where memrchr is defined roughly as follows[1]:
#ifdef __GLIBC_PREREQ
#if __GLIBC_PREREQ(2, 2)
#define HAVE_MEMRCHR
#endif
#endif
#ifndef HAVE_MEMRCHR
#define memrchr gitmemrchr
static inline void *gitmemrchr(const void *s, int c, size_t n)
{
const unsigned char *p = s;
p += n;
while (p != s)
if (*--p == (unsigned char) c)
return p;
return NULL;
}
#endif
Does that look right?
Jonathan
[1] http://thread.gmane.org/gmane.comp.version-control.git/159081/focus=159121
^ permalink raw reply
* Re: [PATCH v2 02/10] t/test-lib-functions.sh: allow to specify the tag name to test_commit
From: Jonathan Nieder @ 2013-01-22 8:02 UTC (permalink / raw)
To: Brandon Casey; +Cc: gitster, pclouds, git, Brandon Casey
In-Reply-To: <1358757627-16682-3-git-send-email-drafnel@gmail.com>
Brandon Casey wrote:
> The <message> part of test_commit() may not be appropriate for a tag name.
> So let's allow test_commit to accept a fourth argument to specify the tag
> name.
Yes!
[...]
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -135,12 +135,13 @@ test_pause () {
> fi
> }
>
> -# Call test_commit with the arguments "<message> [<file> [<contents>]]"
> +# Call test_commit with the arguments "<message> [<file> [<contents> [<tag>]]]"
> #
> # This will commit a file with the given contents and the given commit
> -# message. It will also add a tag with <message> as name.
> +# message. It will also add a tag with <message> as name unless <tag> is
> +# given.
> #
> -# Both <file> and <contents> default to <message>.
> +# <file>, <contents>, and <tag> all default to <message>.
Simpler:
# This will commit a file with the given contents and the given commit
# message and tag the resulting commit with the given tag name.
#
# <file>, <contents>, and <tag> all default to <message>.
With or without that change,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
^ permalink raw reply
* Re: [RFC] Instruct git-completion.bash that we are in test mode
From: Jean-Noël Avila @ 2013-01-22 8:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Jean-Noël AVILA, git
In-Reply-To: <7vham9dej2.fsf@alter.siamese.dyndns.org>
Le 22/01/2013 05:31, Junio C Hamano a écrit :
> Jeff King <peff@peff.net> writes:
>
>> I really hate to suggest this, but should it be more like:
>>
>> if test -z "$FAKE_COMMAND_LIST"; then __git_cmdlist() { git help -a
>> | egrep '^ [a-zA-Z0-9]' } else __git_cmdlist() { printf '%s'
>> "$FAKE_COMMAND_LIST" } fi
>>
>> That gives us a nice predictable starting point for actually
>> testing the completion code. The downside is that it doesn't let
>> us test that we remain compatible with the output of "help -a".
>
> Yeah, I think this is simpler and more to the point for the test in
> t9902. If we really want to test something that is the same as, or
> at least any closer than this approach (or my "help --standard"), to
> what the real users use, the test has to become inherently flaky, so
> I think we should go for the simplicity of this patch shows.
Instead of imposing the list of command, we could use the command
list argument to filter the ouput of git help -a. This would ensure that the
completions we want to test are still present in the installation while
still restricting them to the test case.
^ permalink raw reply
* Re: [PATCH v2 03/10] t/t3511: add some tests of 'cherry-pick -s' functionality
From: Jonathan Nieder @ 2013-01-22 8:17 UTC (permalink / raw)
To: Brandon Casey; +Cc: gitster, pclouds, git, Brandon Casey
In-Reply-To: <1358757627-16682-4-git-send-email-drafnel@gmail.com>
Brandon Casey wrote:
> --- /dev/null
> +++ b/t/t3511-cherry-pick-x.sh
> @@ -0,0 +1,111 @@
[...]
> + test_commit "$mesg_one_line" foo b mesg-one-line &&
> + git reset --hard initial &&
> + test_commit "$mesg_no_footer" foo b mesg-no-footer &&
> + git reset --hard initial &&
> + test_commit "$mesg_broken_footer" foo b mesg-broken-footer &&
> + git reset --hard initial &&
> + test_commit "$mesg_with_footer" foo b mesg-with-footer &&
> + git reset --hard initial &&
> + test_commit "$mesg_with_footer_sob" foo b mesg-with-footer-sob &&
Neat.
[...]
> +test_expect_success 'cherry-pick -s inserts blank line after one line subject' '
In particular, a blank line after a one-line subject that starts with
the usual "subsystem:" convention is not mistaken for an RFC2822-style
header. Good.
[...]
> +test_expect_failure 'cherry-pick -s inserts blank line after non-conforming footer' '
IIUC this is an illustration of false-positives from messages like
this one:
base: do something great without a sign-off
If he does that, it will be the best thing in the
world: or so I think.
A worthy cause. Could the example broken message be tweaked to
emphasize that use case? With the current example, I'd consider
either result (blank line or no blank line) to be ok behavior by git.
[...]
> +test_expect_success 'cherry-pick -s inserts blank line when conforming footer not found' '
This is a simpler version of the previous test, without the distracting
colon.
[...]
> +test_expect_success 'cherry-pick -s adds sob when last sob doesnt match committer' '
Nice test for basic "-s" functionality.
[...]
> +test_expect_success 'cherry-pick -s refrains from adding duplicate trailing sob' '
And the other side of basic "-s" functionality.
One more test would be interesting: what does "-s" do when asked to
produce a duplicate signoff with an interspersed signoff by someone else?
test: a patch with a more complicated life
This patch bounced from $GIT_COMMITTER_NAME to Ms. Thor for
tweaking, then back to $GIT_COMMITTER_NAME who will be
recording it in permanent history.
Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
Signed-off-by: A U Thor <author@example.com>
With the changes suggested above or similar ones,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
^ permalink raw reply
* Re: [PATCHv2] l10n: de.po: fix some minor issues
From: Joachim Schmitz @ 2013-01-22 8:18 UTC (permalink / raw)
To: git
In-Reply-To: <1358798856-5185-1-git-send-email-ralf.thielow@gmail.com>
Ralf Thielow wrote:
> This fixes some minor issues and improves the
> German translation a bit. The following things
> were changed:
> - use complete sentences in option related messages
> - translate "use" consistently as "verwendet"
> - don't translate "make sense" as "macht Sinn"
While your translations for these are OK, I wonder whether the original
english message needs to get fixed/changed, e.g. rather than "does't make
sense" use "are mutually exclusive" or "cannot be used with" or "cannot be
used together" like in other places?
Bye, Jojo
^ permalink raw reply
* Re: [PATCH v3] Enable minimal stat checking
From: Johannes Sixt @ 2013-01-22 8:25 UTC (permalink / raw)
To: Robin Rosenberg; +Cc: git, Junio C Hamano, Shawn Pearce
In-Reply-To: <1358840962-12316-1-git-send-email-robin.rosenberg@dewire.com>
Am 1/22/2013 8:49, schrieb Robin Rosenberg:
> Specifically the fields uid, gid, ctime, ino and dev are set to zero
> by JGit. Other implementations, eg. Git in cygwin are allegedly also
> somewhat incompatible with Git For Windows and on *nix platforms
> the resolution of the timestamps may differ.
>
> Any stat checking by git will then need to check content, which may
> be very slow, particularly on Windows. Since mtime and size
> is typically enough we should allow the user to tell git to avoid
> checking these fields if they are set to zero in the index.
Isn't this paragraph about slowness in the commit message misleading, as
what the patch does has no influence on the speed of stat checking? Am I
missing something?
> This change introduces a core.checkstat config option where the
> the user can select to check all fields (default), or just size
> and the whole second part of mtime (minimal).
> +core.checkstat::
> + Determines which stat fields to match between the index
> + and work tree. The user can set this to 'default' or
> + 'minimal'. Default (or explicitly 'default'), is to check
> + all fields, including the sub-second part of mtime and ctime.
I think this needs some more clarification, less 1337 speak, as well as a
hint when to set the option.
Determines which file attributes are checked to detect whether
a file has been modified. Set this option to 'minimal', when...,
which checks only the file size and whole-seconds of the last
modification time. Otherwise, leave unset or set to the value
'default'.
By starting with the hint when to set to 'minimal' in this way allows us
to omit a specification what the 'default' is.
> diff --git a/read-cache.c b/read-cache.c
> index fda78bc..23db681 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -197,21 +197,25 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
> }
> if (ce->ce_mtime.sec != (unsigned int)st->st_mtime)
> changed |= MTIME_CHANGED;
> - if (trust_ctime && ce->ce_ctime.sec != (unsigned int)st->st_ctime)
> - changed |= CTIME_CHANGED;
> + if (trust_ctime ? check_stat : trust_ctime/*false*/)
> + if (ce->ce_ctime.sec != (unsigned int)st->st_ctime)
> + changed |= CTIME_CHANGED;
It took me a while to understand why you write /*false*/ there. Isn't the
the condition merely this:
if (trust_ctime && check_stat &&
ce->ce_ctime.sec != (unsigned int)st->st_ctime)
changed |= CTIME_CHANGED;
>
> #ifdef USE_NSEC
> - if (ce->ce_mtime.nsec != ST_MTIME_NSEC(*st))
> + if (check_stat && ce->ce_mtime.nsec != ST_MTIME_NSEC(*st))
> changed |= MTIME_CHANGED;
> - if (trust_ctime && ce->ce_ctime.nsec != ST_CTIME_NSEC(*st))
> - changed |= CTIME_CHANGED;
> + if (trust_ctime ? check_stat : trust_ctime/*false*/)
> + if (ce->ce_ctime.nsec != ST_CTIME_NSEC(*st))
> + changed |= CTIME_CHANGED;
Same here.
> #endif
-- Hannes
^ permalink raw reply
* Re: [PATCH v2 04/10] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer
From: Jonathan Nieder @ 2013-01-22 8:27 UTC (permalink / raw)
To: Brandon Casey; +Cc: gitster, pclouds, git, Brandon Casey
In-Reply-To: <1358757627-16682-5-git-send-email-drafnel@gmail.com>
Brandon Casey wrote:
> Let's detect "(cherry picked from...)" as part of the footer so that we
> will produce this:
>
> Signed-off-by: A U Thor <author@example.com>
> (cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)
> Signed-off-by: C O Mmitter <committer@example.com>
>
> instead of this:
>
> Signed-off-by: A U Thor <author@example.com>
> (cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)
>
> Signed-off-by: C O Mmitter <committer@example.com>
Yes, looks sane.
A downside is that this produces an arguably worse result when using
"-x -s" for a commit that does not already have a sign-off. Before,
we had:
test: do something great
Do something fantastic in a clean and elegant way that
only takes two lines of explanation.
(cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)
Signed-off-by: C H Errypicker <cherry-picker@example.com>
Afterwards, we will have:
test: do something great
Do something fantastic in a clean and elegant way that
only takes two lines of explanation.
(cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)
Signed-off-by: C H Errypicker <cherry-picker@example.com>
An ideal result would be completely different:
test: do something great
commit da39a3ee5e6b4b0d3255bfef95601890afd80709 upstream.
Do something fantastic in a clean and elegant way that
only takes two lines of explanation.
Signed-off-by: C H Errypicker <cherry-picker@example.com>
In other words, the -x output format that puts the commit id at the
end with odd spacing seems to be of questionable taste anyway. But
given the constraint of leaving that alone, cramming together the
sign-off like this seems like the best we can do, so for what it's
worth,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
^ permalink raw reply
* Re: [PATCH v2 05/10] sequencer.c: always separate "(cherry picked from" from commit body
From: Jonathan Nieder @ 2013-01-22 8:32 UTC (permalink / raw)
To: Brandon Casey; +Cc: gitster, pclouds, git, Brandon Casey
In-Reply-To: <1358757627-16682-6-git-send-email-drafnel@gmail.com>
Brandon Casey wrote:
> Start treating the "(cherry picked from" line added by cherry-pick -x
> the same way that the s-o-b lines are treated. Namely, separate them
> from the main commit message body with an empty line.
Heh, you read my mind.
^ 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