[PATCH v6 0/8] push: update remote tags only with force

2012-11-29 Thread Chris Rorvick 
This patch series originated in response to the following thread:

  http://thread.gmane.org/gmane.comp.version-control.git/208354

I made some adjustments based on Junio's last round of feedback
including a new patch reworking the "push rules" comment in remote.c.
Also refined some of the log messages--nothing major.  Finally, took a
stab at putting something together for the release notes, see below.

Chris

Release notes:

"git push" no longer updates tags (lightweight or annotated) by default.
Specifically, if the destination reference already exists and is under
refs/tags/ or it points to a tag object, it is not allowed to fast-
forward (unless forced using +A:B notation or by passing --force.)  This
is consistent with how a tag is normally thought of: a reference that
does not move once defined.  It also ensures a push will not
inadvertently clobber an already existing tag--something that can go
unnoticed if fast-forwarding is allowed.

Chris Rorvick (8):
  push: return reject reasons as a bitset
  push: add advice for rejected tag reference
  push: flag updates
  push: flag updates that require force
  push: require force for refs under refs/tags/
  push: require force for annotated tags
  push: clarify rejection of update to non-commit-ish
  push: cleanup push rules comment

 Documentation/git-push.txt |  9 ++---
 builtin/push.c | 24 +-
 builtin/send-pack.c|  9 +++--
 cache.h|  7 +++-
 remote.c   | 83 +++---
 send-pack.c|  1 +
 t/t5516-fetch-push.sh  | 44 +++-
 transport-helper.c |  6 
 transport.c| 25 --
 transport.h| 10 +++---
 10 files changed, 167 insertions(+), 51 deletions(-)

-- 
1.8.0.158.g0c4328c

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/8] push: update remote tags only with force

2012-12-03 Thread Junio C Hamano 
Thanks; will queue.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/8] push: update remote tags only with force

2013-01-16 Thread Max Horn 
Hi there,

I was just working on improving git-remote-helper.txt by documenting how remote 
helper can signal error conditions to git. This lead me to notice a (to me) 
surprising change in behavior between master and next that I traced back to 
this patch series.

Specifically:

On 30.11.2012, at 02:41, Chris Rorvick wrote:

> This patch series originated in response to the following thread:
> 
>  http://thread.gmane.org/gmane.comp.version-control.git/208354
> 
> I made some adjustments based on Junio's last round of feedback
> including a new patch reworking the "push rules" comment in remote.c.
> Also refined some of the log messages--nothing major.  Finally, took a
> stab at putting something together for the release notes, see below.

>From the discussion in that gmane thread and from the commits in this series, 
>I had the impression that it should mostly affect pushing tags. However, this 
>is not the case: It also changes messages upon regular push "conflicts. 
>Consider this test script:


#!/bin/sh -ex
git init repo_orig
cd repo_orig
echo a > a
git add a
git commit -m a
cd ..

git clone repo_orig repo_clone

cd repo_orig
echo b > b
git add b
git commit -m b
cd ..

cd repo_clone
echo B > b
git add b
git commit -m B
git push


With git 1.8.1, I get this message:

 ! [rejected]master -> master (non-fast-forward)
error: failed to push some refs to 
'/Users/mhorn/Projekte/foreign/gitifyhg/bugs/git-push-conflict/repo_orig'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Merge the remote changes (e.g. 'git pull')
hint: before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.



But with next, I get this:


 ! [rejected]master -> master (already exists)
error: failed to push some refs to 
'/Users/mhorn/Projekte/foreign/gitifyhg/bugs/git-push-conflict/repo_orig'
hint: Updates were rejected because the destination reference already exists
hint: in the remote.


This looks like a regression to me. No tags were involve, and the new message 
is very confusing if not outright wrong -- at least in my mind, but perhaps I 
am missing a way to interpret it "correctly" ? What am I missing?


Cheers,
Max

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/8] push: update remote tags only with force

2013-01-16 Thread Junio C Hamano 
Max Horn  writes:

> But with next, I get this:
>
>
>  ! [rejected]master -> master (already exists)
> error: failed to push some refs to '/Users/mhorn/Proje...o_orig'
> hint: Updates were rejected because the destination reference already exists
> hint: in the remote.
>
> This looks like a regression to me.

It is an outright bug.  The new helper function is_forwrdable() is
bogus to assume that both original and updated objects can be
locally inspected, but you do not necessarily have the original
locally.

 remote.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/remote.c b/remote.c
index aa6b719..4a253ef 100644
--- a/remote.c
+++ b/remote.c
@@ -1286,9 +1286,12 @@ static inline int is_forwardable(struct ref* ref)
if (!prefixcmp(ref->name, "refs/tags/"))
return 0;
 
-   /* old object must be a commit */
+   /*
+* old object must be a commit, but we may be forcing
+* without having it in the first place!
+*/
o = parse_object(ref->old_sha1);
-   if (!o || o->type != OBJ_COMMIT)
+   if (o && o->type != OBJ_COMMIT)
return 0;
 
/* new object must be commit-ish */
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/8] push: update remote tags only with force

2013-01-16 Thread Junio C Hamano 
Junio C Hamano  writes:

> Max Horn  writes:
>
>> But with next, I get this:
>>
>>  ! [rejected]master -> master (already exists)
>> error: failed to push some refs to '/Users/mhorn/Proje...o_orig'
>> hint: Updates were rejected because the destination reference already exists
>> hint: in the remote.
>>
>> This looks like a regression to me.
>
> It is an outright bug.  The new helper function is_forwrdable() is
> bogus to assume that both original and updated objects can be
> locally inspected, but you do not necessarily have the original
> locally.

The way the caller uses the result of this function is equally
questionable.  If this function says "we do not want to let this
push go through", it translates that unconditionally into "we
blocked it because the destination already exists".

It is fine when pushing into "refs/tags/" hierarchy.  It is *NOT*
OK if the type check does not satisfy this function.  In that case,
we do not actually see the existence of the destination as a
problem, but it is reported as such.  We are blocking because we do
not like the type of the new object or the type of the old object.
If the destination points at a commit, the push can succeed if the
user changes what object to push, so saying "you cannot push because
the destination already exists" is just wrong in such a case.



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/8] push: update remote tags only with force

2013-01-16 Thread Jeff King 
On Wed, Jan 16, 2013 at 02:32:03PM +0100, Max Horn wrote:

> With git 1.8.1, I get this message:
> 
>  ! [rejected]master -> master (non-fast-forward)
> [...]
> But with next, I get this:
> 
>  ! [rejected]master -> master (already exists)

Thanks for the detailed report. I was able to reproduce easily here.

The problem is the logic in is_forwardable:

static inline int is_forwardable(struct ref* ref)
{
struct object *o;

if (!prefixcmp(ref->name, "refs/tags/"))
return 0;

/* old object must be a commit */
o = parse_object(ref->old_sha1);
if (!o || o->type != OBJ_COMMIT)
return 0;

/* new object must be commit-ish */
o = deref_tag(parse_object(ref->new_sha1), NULL, 0);
if (!o || o->type != OBJ_COMMIT)
return 0;

return 1;
}

The intent is to allow fast-forward only between objects that both point
to commits eventually. But we are doing this check on the client, which
does not necessarily have the object for ref->old_sha1 at all. So it
cannot know the type, and cannot enforce this condition accurately.

I.e., we trigger the "!o" branch after the parse_object in your example.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/8] push: update remote tags only with force

2013-01-16 Thread Junio C Hamano 
Max Horn  writes:

> But with next, I get this:
>
>  ! [rejected]master -> master (already exists)
> error: failed to push some refs to 
> '/Users/mhorn/Projekte/foreign/gitifyhg/bugs/git-push-conflict/repo_orig'
> hint: Updates were rejected because the destination reference already exists
> hint: in the remote.
>
> This looks like a regression to me.

It is in master now X-<, and this looks like a bug to me.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/8] push: update remote tags only with force

2013-01-16 Thread Junio C Hamano 
Jeff King  writes:

> I.e., we trigger the "!o" branch after the parse_object in your example.

Heh, I didn't see this message until now (gmane seems to be lagging
a bit).

I am very tempted to do this.

 * Remove unnecessary not_forwardable from "struct ref"; it is only
   used inside set_ref_status_for_push();

 * "refs/tags/" is the only hierarchy that cannot be replaced
   without --force;

 * Remove the misguided attempt to force that everything that
   updates an existing ref has to be a commit outside "refs/tags/"
   hierarchy.  This code does not know what kind of objects the user
   wants to place in "refs/frotz/" hierarchy it knows nothing about.

I feel moderately strongly about the last point.  Defining special
semantics for one hierarchy (e.g. "refs/tags/") and implementing a
policy for enforcement is one thing, but a random policy that
depends on object type that applies globally is simply insane.  The
user may want to do "refs/tested/" hierarchy that is meant to hold
references to commit, with one annotated tag "refs/tested/latest"
that points at the "latest tested version" with some commentary, and
maintain the latter by keep pushing to it.  If that is the semantics
the user wanted to ahve in the "refs/tested/" hierarchy, it is not
reasonable to require --force for such a workflow.  The user knows
better than Git in such a case.


 cache.h   |  1 -
 remote.c  | 24 +---
 t/t5516-fetch-push.sh | 21 -
 3 files changed, 1 insertion(+), 45 deletions(-)

diff --git a/cache.h b/cache.h
index a32a0ea..a942bbd 100644
--- a/cache.h
+++ b/cache.h
@@ -1004,7 +1004,6 @@ struct ref {
requires_force:1,
merge:1,
nonfastforward:1,
-   not_forwardable:1,
update:1,
deletion:1;
enum {
diff --git a/remote.c b/remote.c
index aa6b719..2c747c4 100644
--- a/remote.c
+++ b/remote.c
@@ -1279,26 +1279,6 @@ int match_push_refs(struct ref *src, struct ref **dst,
return 0;
 }
 
-static inline int is_forwardable(struct ref* ref)
-{
-   struct object *o;
-
-   if (!prefixcmp(ref->name, "refs/tags/"))
-   return 0;
-
-   /* old object must be a commit */
-   o = parse_object(ref->old_sha1);
-   if (!o || o->type != OBJ_COMMIT)
-   return 0;
-
-   /* new object must be commit-ish */
-   o = deref_tag(parse_object(ref->new_sha1), NULL, 0);
-   if (!o || o->type != OBJ_COMMIT)
-   return 0;
-
-   return 1;
-}
-
 void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
int force_update)
 {
@@ -1344,8 +1324,6 @@ void set_ref_status_for_push(struct ref *remote_refs, int 
send_mirror,
 * passing the --force argument
 */
 
-   ref->not_forwardable = !is_forwardable(ref);
-
ref->update =
!ref->deletion &&
!is_null_sha1(ref->old_sha1);
@@ -1355,7 +1333,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int 
send_mirror,
!has_sha1_file(ref->old_sha1)
  || !ref_newer(ref->new_sha1, ref->old_sha1);
 
-   if (ref->not_forwardable) {
+   if (!prefixcmp(ref->name, "refs/tags/")) {
ref->requires_force = 1;
if (!force_ref_update) {
ref->status = 
REF_STATUS_REJECT_ALREADY_EXISTS;
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 6009372..8f024a0 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -950,27 +950,6 @@ test_expect_success 'push requires --force to update 
lightweight tag' '
)
 '
 
-test_expect_success 'push requires --force to update annotated tag' '
-   mk_test heads/master &&
-   mk_child child1 &&
-   mk_child child2 &&
-   (
-   cd child1 &&
-   git tag -a -m "message 1" Tag &&
-   git push ../child2 Tag:refs/tmp/Tag &&
-   git push ../child2 Tag:refs/tmp/Tag &&
-   >file1 &&
-   git add file1 &&
-   git commit -m "file1" &&
-   git tag -f -a -m "message 2" Tag &&
-   test_must_fail git push ../child2 Tag:refs/tmp/Tag &&
-   git push --force ../child2 Tag:refs/tmp/Tag &&
-   git tag -f -a -m "message 3" Tag HEAD~ &&
-   test_must_fail git push ../child2 Tag:refs/tmp/Tag &&
-   git push --force ../child2 Tag:refs/tmp/Tag
-   )
-'
-
 test_expect_success 'push --porcelain' '
mk_empty &&
echo >.git/foo  "To testrepo" &&
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/8] push: update remote tags only with force

2013-01-16 Thread Jeff King 
On Wed, Jan 16, 2013 at 09:10:10AM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > I.e., we trigger the "!o" branch after the parse_object in your example.
> 
> Heh, I didn't see this message until now (gmane seems to be lagging
> a bit).

I think it is vger lagging, actually.

> I am very tempted to do this.
> 
>  * Remove unnecessary not_forwardable from "struct ref"; it is only
>used inside set_ref_status_for_push();
> 
>  * "refs/tags/" is the only hierarchy that cannot be replaced
>without --force;

Agreed.

>  * Remove the misguided attempt to force that everything that
>updates an existing ref has to be a commit outside "refs/tags/"
>hierarchy.  This code does not know what kind of objects the user
>wants to place in "refs/frotz/" hierarchy it knows nothing about.

I agree with what your patch does, but my thinking is a bit different.

My original suggestion with respect to object types was that the rule
for --force should be "do not ever lose any objects without --force". So
a fast-forward is OK, as the new objects reference the old. A non-fast
forward is not, because objects become unreferenced. Replacing a tag
object is not OK, even if it points to the same commit, as you are
losing the old tag object (replacing an object with a tag that points to
the original object or its descendent is OK in theory, though I doubt it
is common enough to worry about).

I think that is a reasonable rule that could be applied across all parts
of the namespace hierarchy. And it could be applied by the client,
because all you need to know is whether ref->old_sha1 is reachable from
ref->new_sha1.

But it is somewhat orthogonal to the "already exists" idea, and checking
refs/tags/. Those ideas are about enforcing sane rules on the tag
hierarchy. My rule is a safety valve that is meant to extend the idea of
"is fast-forwardable" to non-commit object types. If we do it at all, it
should be part of the fast-forward check (e.g., as part of ref_newer).

The current code conflates the two under the "already exists" condition,
which is just wrong.  I think the best thing at this point is to split
the two ideas apart, keep the refs/tags check (and translate it to
"already exists" in the UI, as we do), and table the safety valve. I am
not even sure if it is something that is useful, and it can come later
if we decide it is.

> I feel moderately strongly about the last point.  Defining special
> semantics for one hierarchy (e.g. "refs/tags/") and implementing a
> policy for enforcement is one thing, but a random policy that
> depends on object type that applies globally is simply insane.  The
> user may want to do "refs/tested/" hierarchy that is meant to hold
> references to commit, with one annotated tag "refs/tested/latest"
> that points at the "latest tested version" with some commentary, and
> maintain the latter by keep pushing to it.  If that is the semantics
> the user wanted to ahve in the "refs/tested/" hierarchy, it is not
> reasonable to require --force for such a workflow.  The user knows
> better than Git in such a case.

I see what you are saying, but I think the ship has already sailed to
some degree. We already implement the non-fast-forward check everywhere,
and I cannot have a "refs/tested" hierarchy that pushes arbitrary
commits without regard to their history. If I have such a hierarchy, I
have to use "--force" (or more likely, mark the refspec with "+").

In my mind, the object-type checking is just making that fast-forward
check more thorough (i.e., extending it to non-commit objects).

>  cache.h   |  1 -
>  remote.c  | 24 +---
>  t/t5516-fetch-push.sh | 21 -
>  3 files changed, 1 insertion(+), 45 deletions(-)

The patch itself looks fine to me. Whether we agree on the fast-forward
object-type checking or not, it is the correct first step to take in
either case.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/8] push: update remote tags only with force

2013-01-16 Thread Junio C Hamano 
Jeff King  writes:

> I see what you are saying, but I think the ship has already sailed to
> some degree. We already implement the non-fast-forward check everywhere,
> and I cannot have a "refs/tested" hierarchy that pushes arbitrary
> commits without regard to their history. If I have such a hierarchy, I
> have to use "--force" (or more likely, mark the refspec with "+").

Yeah, actually in that example, I meant refs/tested/ would have
pointers to bare tree objects. I often rebuild 'pu' and another
private integration branch for testing, reordering the series that
are still not in 'next' and also after rewriting log messages of
some commits. It is not unusual to end up the updated 'pu' having
the identical tree as 'pu' before update, and I want to skip testing
the result (tree equality matters while commit equality does not in
such a use case).

> In my mind, the object-type checking is just making that fast-forward
> check more thorough (i.e., extending it to non-commit objects).

Yes, I agree with that point of view.

Thanks.

Here is what I am planning to queue (the patch is the same, but the
message is different on the third point). 

-- >8 --
Subject: [PATCH] push: fix "refs/tags/ hierarchy cannot be updated without 
--force"

When pushing to update a branch with a commit that is not a
descendant of the commit at the tip, a wrong message "already
exists" was given, instead of the correct "non-fast-forward", if we
do not have the object sitting in the destination repository at the
tip of the ref we are updating.

The primary cause of the bug is that the check in a new helper
function is_forwardable() assumed both old and new objects are
available and can be checked, which is not always the case.

The way the caller uses the result of this function is also wrong.
If the helper says "we do not want to let this push go through", the
caller unconditionally translates it into "we blocked it because the
destination already exists", which is not true at all in this case.

Fix this by doing these three things:

 * Remove unnecessary not_forwardable from "struct ref"; it is only
   used inside set_ref_status_for_push();

 * Make "refs/tags/" the only hierarchy that cannot be replaced
   without --force;

 * Remove the misguided attempt to force that everything that
   updates an existing ref has to be a commit outside "refs/tags/"
   hierarchy.

The policy last one tried to implement may later be resurrected and
extended to ensure fast-forwardness (defined as "not losing
objects", extending from the traditional "not losing commits from
the resulting history") when objects that are not commit are
involved (e.g. an annotated tag in hierarchies outside refs/tags),
but such a logic belongs to "is this a fast-forward?" check that is
done by ref_newer(); is_forwardable(), which is now removed, was not
the right place to do so.

Signed-off-by: Junio C Hamano 
---
 cache.h   |  1 -
 remote.c  | 43 +++
 t/t5516-fetch-push.sh | 21 -
 3 files changed, 7 insertions(+), 58 deletions(-)

diff --git a/cache.h b/cache.h
index a32a0ea..a942bbd 100644
--- a/cache.h
+++ b/cache.h
@@ -1004,7 +1004,6 @@ struct ref {
requires_force:1,
merge:1,
nonfastforward:1,
-   not_forwardable:1,
update:1,
deletion:1;
enum {
diff --git a/remote.c b/remote.c
index aa6b719..d3a1ca2 100644
--- a/remote.c
+++ b/remote.c
@@ -1279,26 +1279,6 @@ int match_push_refs(struct ref *src, struct ref **dst,
return 0;
 }
 
-static inline int is_forwardable(struct ref* ref)
-{
-   struct object *o;
-
-   if (!prefixcmp(ref->name, "refs/tags/"))
-   return 0;
-
-   /* old object must be a commit */
-   o = parse_object(ref->old_sha1);
-   if (!o || o->type != OBJ_COMMIT)
-   return 0;
-
-   /* new object must be commit-ish */
-   o = deref_tag(parse_object(ref->new_sha1), NULL, 0);
-   if (!o || o->type != OBJ_COMMIT)
-   return 0;
-
-   return 1;
-}
-
 void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
int force_update)
 {
@@ -1320,32 +1300,23 @@ void set_ref_status_for_push(struct ref *remote_refs, 
int send_mirror,
}
 
/*
-* The below logic determines whether an individual
-* refspec A:B can be pushed.  The push will succeed
-* if any of the following are true:
+* Decide whether an individual refspec A:B can be
+* pushed.  The push will succeed if any of the
+* following are true:
 *
 * (1) the remote reference B does not exist
 *
 * (2) the remote reference B is being removed (i.e.,
 * pushing :B where no source is specified)
 *
-*

Re: [PATCH v6 0/8] push: update remote tags only with force

2013-01-16 Thread Chris Rorvick 
On Wed, Jan 16, 2013 at 11:43 AM, Jeff King  wrote:
> I think that is a reasonable rule that could be applied across all parts
> of the namespace hierarchy. And it could be applied by the client,
> because all you need to know is whether ref->old_sha1 is reachable from
> ref->new_sha1.

is_forwardable() did solve a UI issue.  Previously all instances where
old is not reachable by new were assumed to be addressable with a
merge.  is_forwardable() attempted to determine if the concept of
forwarding made sense given the inputs.  For example, if old is a blob
it is useless to suggest merging it.

Chris
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/8] push: update remote tags only with force

2013-01-16 Thread Jeff King 
On Wed, Jan 16, 2013 at 08:19:28PM -0600, Chris Rorvick wrote:

> On Wed, Jan 16, 2013 at 11:43 AM, Jeff King  wrote:
> > I think that is a reasonable rule that could be applied across all parts
> > of the namespace hierarchy. And it could be applied by the client,
> > because all you need to know is whether ref->old_sha1 is reachable from
> > ref->new_sha1.
> 
> is_forwardable() did solve a UI issue.  Previously all instances where
> old is not reachable by new were assumed to be addressable with a
> merge.  is_forwardable() attempted to determine if the concept of
> forwarding made sense given the inputs.  For example, if old is a blob
> it is useless to suggest merging it.

I think it makes sense to mark such a case as different from a regular
non-fast-forward (because "git pull" is not the right advice), but:

  1. is_forwardable should assume a missing object is a commit not to
 regress the common case; otherwise we do not show the pull advice
 when we probably should, and most of the time it is going to be a
 commit

  2. When we know that we are not working with commits, I am not sure
 that "already exists" is the right advice to give for such a case.
 It is neither "this tag already exists, so we do not update it",
 nor is it strictly "cannot fast forward this commit", but rather
 something else.

 The expanded definition of "what is a fast forward" that I
 suggested would let this fall naturally between the two.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/8] push: update remote tags only with force

2013-01-16 Thread Chris Rorvick 
On Wed, Jan 16, 2013 at 9:11 PM, Jeff King  wrote:
>> is_forwardable() did solve a UI issue.  Previously all instances where
>> old is not reachable by new were assumed to be addressable with a
>> merge.  is_forwardable() attempted to determine if the concept of
>> forwarding made sense given the inputs.  For example, if old is a blob
>> it is useless to suggest merging it.
>
> I think it makes sense to mark such a case as different from a regular
> non-fast-forward (because "git pull" is not the right advice), but:
>
>   1. is_forwardable should assume a missing object is a commit not to
>  regress the common case; otherwise we do not show the pull advice
>  when we probably should, and most of the time it is going to be a
>  commit

Yes, obviously this was a bug, thus the use of "attempted" above.  It
would have been better to assume a missing 'old' was potentially
forwardable to present the user with the most helpful advice.

>   2. When we know that we are not working with commits, I am not sure
>  that "already exists" is the right advice to give for such a case.
>  It is neither "this tag already exists, so we do not update it",
>  nor is it strictly "cannot fast forward this commit", but rather
>  something else.

But the reference already existing in the remote is a substantial
reason for not allowing the push in all of these cases.  You can break
this out further if you like to explain why the specific reference
shouldn't be moved on the remote, but this is even more complicated a
simple "is old reachable from new?" test.

Chris
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/8] push: update remote tags only with force

2013-01-16 Thread Chris Rorvick 
On Wed, Jan 16, 2013 at 10:48 AM, Junio C Hamano  wrote:
> It is fine when pushing into "refs/tags/" hierarchy.  It is *NOT*
> OK if the type check does not satisfy this function.  In that case,
> we do not actually see the existence of the destination as a
> problem, but it is reported as such.  We are blocking because we do
> not like the type of the new object or the type of the old object.
> If the destination points at a commit, the push can succeed if the
> user changes what object to push, so saying "you cannot push because
> the destination already exists" is just wrong in such a case.

So the solution is to revert back to recommending a merge?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/8] push: update remote tags only with force

2013-01-16 Thread Junio C Hamano 
Chris Rorvick  writes:

> On Wed, Jan 16, 2013 at 10:48 AM, Junio C Hamano  wrote:
>> It is fine when pushing into "refs/tags/" hierarchy.  It is *NOT*
>> OK if the type check does not satisfy this function.  In that case,
>> we do not actually see the existence of the destination as a
>> problem, but it is reported as such.  We are blocking because we do
>> not like the type of the new object or the type of the old object.
>> If the destination points at a commit, the push can succeed if the
>> user changes what object to push, so saying "you cannot push because
>> the destination already exists" is just wrong in such a case.
>
> So the solution is to revert back to recommending a merge?

Of course not, because at that point you may not even have what you
were attempting to overwrite.  Nobody says it is even something you
could merge.

The recommended solution certainly will involve a "fetch" (not
"pull" or "pull --rebase").  You fetch from over there to check what
you were about to overwrite, examine the situation to decide what
the appropriate action is.

The point is that Git in general, and the codepath that was touched
by the patch in particular, does not have enough information to
decide what the appropriate action is for the user, especially when
the ref is outside the ones we know what the conventional uses of
them are.  We can make policy decisions like "tags are meant to be
unmoving anchor points, so it is unusual to overwrite any old with
any new", "heads are meant to be branch tips, and because rewinding
them while more than one repositories are working with them will
cause issues to other repositories, it is unusual to push a
non-fast-forward" and enforcement mechanism for such policy
decisions will help users, but that is only because we know what
their uses are.

The immediate action we should take is to get closer to the original
behaviour of not complaining with "ref already exists", which is
nonsensical.  That does not mean that we will forbid improving the
codepath by giving different advices depending on the case.

One of the new advices could tell them to "fetch it and inspect the
situation", if old is not something we do not even have (hence we
cannot check its type, let alone the ancestry relationship of it
with new), for example.


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/8] push: update remote tags only with force

2013-01-17 Thread Chris Rorvick 
On Thu, Jan 17, 2013 at 12:59 AM, Junio C Hamano  wrote:
> Chris Rorvick  writes:
>
>> On Wed, Jan 16, 2013 at 10:48 AM, Junio C Hamano  wrote:
>>> It is fine when pushing into "refs/tags/" hierarchy.  It is *NOT*
>>> OK if the type check does not satisfy this function.  In that case,
>>> we do not actually see the existence of the destination as a
>>> problem, but it is reported as such.  We are blocking because we do
>>> not like the type of the new object or the type of the old object.
>>> If the destination points at a commit, the push can succeed if the
>>> user changes what object to push, so saying "you cannot push because
>>> the destination already exists" is just wrong in such a case.
>>
>> So the solution is to revert back to recommending a merge?
>
> Of course not, because at that point you may not even have what you
> were attempting to overwrite.  Nobody says it is even something you
> could merge.

I was referring to your concern about rejecting based on type.  A push
causing a reference to move (for example) from a commit to a blob is
rejected as "already exists" with this patch.  You emphatically state
this is not OK and your solution is to revert back to behavior that
advises a merge.

Clearly the bug regarding an 'old' unknown to the client should be
fixed.  This is a obvious test case I should have covered and it's
unfortunate it made it into master.  But I don't understand why
is_forwardable() was misguided (maybe poorly named) nor why
ref_newer() is a better place to solve the issues it was addressing.

Chris
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/8] push: update remote tags only with force

2013-01-17 Thread Jeff King 
On Thu, Jan 17, 2013 at 07:09:16AM -0600, Chris Rorvick wrote:

> I was referring to your concern about rejecting based on type.  A push
> causing a reference to move (for example) from a commit to a blob is
> rejected as "already exists" with this patch.  You emphatically state
> this is not OK and your solution is to revert back to behavior that
> advises a merge.
> 
> Clearly the bug regarding an 'old' unknown to the client should be
> fixed.  This is a obvious test case I should have covered and it's
> unfortunate it made it into master.  But I don't understand why
> is_forwardable() was misguided (maybe poorly named) nor why
> ref_newer() is a better place to solve the issues it was addressing.

I think that a type-based rule that relies on knowing the type of the
other side will always have to guess in some cases, because we do not
necessarily have that information. 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. 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).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/8] push: update remote tags only with force

2013-01-17 Thread Chris Rorvick 
On Thu, Jan 17, 2013 at 7:06 PM, Jeff King  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.

> 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.)

Chris
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/8] push: update remote tags only with force

2013-01-17 Thread Junio C Hamano 
Jeff King  writes:

> 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.  And that
> is logically an extension of the fast-forward rule,...

It may be an extension of the fast-forward, but only in the graph
reachability sense.  I can buy that it is mathmatically consistent
with the mode that has proven to be useful for commits at the branch
tips, which we know why "fast-forward" rule is an appropriate
default for.  You haven't shown if that mathmatical consistency is
useful for non-commit case.


The primary reason "fast-forward" is a good default for branches is
not that "we do not want to lose objects to gc" (you have reflog for
that).  The reason is non fast-forward is a sign of unintended
rewind, and later will cause duplicated history with merge
conflicts.

That comes from the way objects pointed by refs/heads aka branches
are used.  It is not just "commit" (as object type), but how these
objects are used.  Think why we decided it was a good idea to do one
thing in the topic that introduced the regression under discussion:
"Even if the new commit is a descendant of the old commit, we do not
want to fast-forward a ref if it is under refs/tags/".  Type of object
may be one factor, but how it is used is more important factor in
deciding what kind of policy is appropriate.

If users have workflows that want to have a ref hierarchy that point
at a blob, there will not be any update to such a ref that will
satisfy your definition of "extended" fast-forward requirement, and
that requirement came solely from mathematical purity (i.e. graph
reachability), not from any workflow consideration.  That is very
disturbing to me.

A workflow that employes such a "blob at a ref" may perfectly be
happy with replacing the blob as last-one-wins basis. I do not think
the client side should enforce a policy to forbid such a push.

I personally think the current client side that insists that updates
to any ref has to have the current object and must fast-forward and
requires --force otherwise was a mistake (this predates the change
by Chris).  The receiving end does not implement such an arbitrary
restriction outside refs/heads/, and does so only for refs/heads/
only when deny-non-fast-forwards is set.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/8] push: update remote tags only with force

2013-01-21 Thread Jeff King 
On Thu, Jan 17, 2013 at 09:18:50PM -0600, Chris Rorvick wrote:

> On Thu, Jan 17, 2013 at 7:06 PM, Jeff King  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".

> > 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.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/8] push: update remote tags only with force

2013-01-21 Thread Junio C Hamano 
Jeff King  writes:

> ... 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.

Very well said.

Please make it so ;-) or should I?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/8] push: update remote tags only with force

2013-01-21 Thread Chris Rorvick 
On Mon, Jan 21, 2013 at 5:40 PM, Jeff King  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  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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/8] push: update remote tags only with force

2013-01-21 Thread Junio C Hamano 
Chris Rorvick  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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html