[PATCH 0/2] transport-helper: general fixes

[PATCH 0/2] transport-helper: general fixes

[Felipe Contreras]

Hi,

Here are a couple of patches that fix a few problems with remote-helpers.

Felipe Contreras (2):
  transport-helper: improve push messages
  transport-helper: update remote helper namespace

 t/t5801-remote-helpers.sh | 26 ++++++++++++++++++++++++++
 transport-helper.c        | 24 ++++++++++++++++++++----
 2 files changed, 46 insertions(+), 4 deletions(-)

-- 
1.8.2.1

[PATCH 1/2] transport-helper: improve push messages

[Felipe Contreras]

If there's already a remote-helper tracking ref, we can fetch the SHA-1
to report proper push messages (as opposed to always reporting
[new branch]).

The remote-helper currently can specify the old SHA-1 to avoid this
problem, but there's no point in forcing all remote-helpers to be aware
of git commit ids; they should be able to be agnostic of them.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t5801-remote-helpers.sh | 14 ++++++++++++++
 transport-helper.c        |  1 +
 2 files changed, 15 insertions(+)

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index f387027..214aa40 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -166,4 +166,18 @@ test_expect_success 'push ref with existing object' '
 	compare_refs local dup server dup
 '
 
+test_expect_success 'push messages' '
+	(cd local &&
+	git checkout -b new_branch master &&
+	echo new >>file &&
+	git commit -a -m new &&
+	git push origin new_branch &&
+	git fetch origin &&
+	echo new >>file &&
+	git commit -a -m new &&
+	git push origin new_branch 2> msg &&
+	! grep "\[new branch\]" msg
+	)
+'
+
 test_done
diff --git a/transport-helper.c b/transport-helper.c
index cb3ef7d..2257025 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -801,6 +801,7 @@ static int push_refs_with_export(struct transport *transport,
 		if (private && !get_sha1(private, sha1)) {
 			strbuf_addf(&buf, "^%s", private);
 			string_list_append(&revlist_args, strbuf_detach(&buf, NULL));
+			hashcpy(ref->old_sha1, sha1);
 		}
 		free(private);
 
-- 
1.8.2.1

Re: [PATCH 1/2] transport-helper: improve push messages

[Jeff King]

On Wed, Apr 10, 2013 at 07:07:11PM -0500, Felipe Contreras wrote:

> If there's already a remote-helper tracking ref, we can fetch the SHA-1
> to report proper push messages (as opposed to always reporting
> [new branch]).
> 
> The remote-helper currently can specify the old SHA-1 to avoid this
> problem, but there's no point in forcing all remote-helpers to be aware
> of git commit ids; they should be able to be agnostic of them.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

Yeah, this looks correct to me. Thanks.

-Peff

[PATCH 2/2] transport-helper: update remote helper namespace

[Felipe Contreras]

When pushing, the remote namespace is updated correctly
(e.g. refs/origin/master), but not the remote helper's
(e.g. refs/testgit/origin/master).

Let's update it correctly.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t5801-remote-helpers.sh | 12 ++++++++++++
 transport-helper.c        | 23 +++++++++++++++++++----
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index 214aa40..07f2862 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -180,4 +180,16 @@ test_expect_success 'push messages' '
 	)
 '
 
+test_expect_success 'push update refs' '
+	(cd local &&
+	git checkout -b update master &&
+	echo update >>file &&
+	git commit -a -m update &&
+	git push origin update
+	git rev-parse --verify testgit/origin/heads/update >expect &&
+	git rev-parse --verify remotes/origin/update >actual
+	test_cmp expect actual
+	)
+'
+
 test_done
diff --git a/transport-helper.c b/transport-helper.c
index 2257025..7dff0c2 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -11,6 +11,7 @@
 #include "thread-utils.h"
 #include "sigchain.h"
 #include "argv-array.h"
+#include "refs.h"
 
 static int debug;
 
@@ -618,7 +619,7 @@ static int fetch(struct transport *transport,
 	return -1;
 }
 
-static void push_update_ref_status(struct strbuf *buf,
+static int push_update_ref_status(struct strbuf *buf,
 				   struct ref **ref,
 				   struct ref *remote_refs)
 {
@@ -684,7 +685,7 @@ static void push_update_ref_status(struct strbuf *buf,
 		*ref = find_ref_by_name(remote_refs, refname);
 	if (!*ref) {
 		warning("helper reported unexpected status of %s", refname);
-		return;
+		return 1;
 	}
 
 	if ((*ref)->status != REF_STATUS_NONE) {
@@ -693,11 +694,12 @@ static void push_update_ref_status(struct strbuf *buf,
 		 * status reported by the remote helper if the latter is 'no match'.
 		 */
 		if (status == REF_STATUS_NONE)
-			return;
+			return 1;
 	}
 
 	(*ref)->status = status;
 	(*ref)->remote_status = msg;
+	return 0;
 }
 
 static void push_update_refs_status(struct helper_data *data,
@@ -706,11 +708,24 @@ static void push_update_refs_status(struct helper_data *data,
 	struct strbuf buf = STRBUF_INIT;
 	struct ref *ref = remote_refs;
 	for (;;) {
+		char *private;
+
 		recvline(data, &buf);
 		if (!buf.len)
 			break;
 
-		push_update_ref_status(&buf, &ref, remote_refs);
+		if (push_update_ref_status(&buf, &ref, remote_refs))
+			continue;
+
+		if (!data->refspecs)
+			continue;
+
+		/* propagate back the update to the remote namespace */
+		private = apply_refspecs(data->refspecs, data->refspec_nr, ref->name);
+		if (!private)
+			continue;
+		update_ref("update by helper", private, ref->new_sha1, NULL, 0, 0);
+		free(private);
 	}
 	strbuf_release(&buf);
 }
-- 
1.8.2.1

Re: [PATCH 2/2] transport-helper: update remote helper namespace

[Jeff King]

On Wed, Apr 10, 2013 at 07:07:12PM -0500, Felipe Contreras wrote:

> When pushing, the remote namespace is updated correctly
> (e.g. refs/origin/master), but not the remote helper's
> (e.g. refs/testgit/origin/master).
> 
> Let's update it correctly.

I would have thought it was the helper's responsibility to update these.
Obviously remote-testgit can handle this fine, but will any other
helpers be using these refs as a marker to know the last point they
imported, and get confused if we update the refs behind their back?

For example, during the import, a helper might know that it has imported
up to X on a foreign vcs, and that resulted in commit Y in git, which it
stored in refs/$helper/heads/master during the last import. When we
fetch from it again, it picks up from X to the tip of the foreign vcs,
and then imports that history on top of commit Y.

But if we push some commits to the helper, moving Y up to Z, then it
would build the new commit (which contains the foreign-vcs's equivalent of
Y..Z) on top of Z, not Y.

I do not offhand know of any helpers that are implemented this way,
though. vcs-svn does not seem to use the refspec feature at all, and I
assume that your hg/bzr helpers do not have this problem. So perhaps it
is not worth worrying about.

-Peff

Re: [PATCH 2/2] transport-helper: update remote helper namespace

[Felipe Contreras]

On Wed, Apr 10, 2013 at 11:33 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Apr 10, 2013 at 07:07:12PM -0500, Felipe Contreras wrote:
>
>> When pushing, the remote namespace is updated correctly
>> (e.g. refs/origin/master), but not the remote helper's
>> (e.g. refs/testgit/origin/master).
>>
>> Let's update it correctly.
>
> I would have thought it was the helper's responsibility to update these.
> Obviously remote-testgit can handle this fine, but will any other
> helpers be using these refs as a marker to know the last point they
> imported, and get confused if we update the refs behind their back?
>
> For example, during the import, a helper might know that it has imported
> up to X on a foreign vcs, and that resulted in commit Y in git, which it
> stored in refs/$helper/heads/master during the last import. When we
> fetch from it again, it picks up from X to the tip of the foreign vcs,
> and then imports that history on top of commit Y.
>
> But if we push some commits to the helper, moving Y up to Z, then it
> would build the new commit (which contains the foreign-vcs's equivalent of
> Y..Z) on top of Z, not Y.

Why would it do that? If X points to say revision 100, presumably it
was stored somewhere while doing a fetch. Similarly, if foreign
version of Z is 150, it can update that number while doing a push. The
next fetch it would start from 151.

All this is hypothetical of course, because...

> I do not offhand know of any helpers that are implemented this way,
> though. vcs-svn does not seem to use the refspec feature at all, and I
> assume that your hg/bzr helpers do not have this problem. So perhaps it
> is not worth worrying about.

They cannot be implemented this way, because as I have already argued
and shown[1], remote helpers must be using marks, they don't work
otherwise (transport helper is broken for those cases).

Cheers.

[1] http://article.gmane.org/gmane.comp.version-control.git/210306

--
Felipe Contreras

Re: [PATCH 2/2] transport-helper: update remote helper namespace

[Jeff King]

On Wed, Apr 10, 2013 at 11:53:38PM -0500, Felipe Contreras wrote:

> > But if we push some commits to the helper, moving Y up to Z, then it
> > would build the new commit (which contains the foreign-vcs's equivalent of
> > Y..Z) on top of Z, not Y.
> 
> Why would it do that? If X points to say revision 100, presumably it
> was stored somewhere while doing a fetch. Similarly, if foreign
> version of Z is 150, it can update that number while doing a push. The
> next fetch it would start from 151.

I think the only reason not to bump the marker forward during the push
would be if the helper wants for some reason to "re-import" from the
foreign source rather than accepting the git versions of the commits.
Something like git-svn's markup of the commit messages with revision ids
comes to mind. But if it matters, then by definition that would mean
that the import/export is not bidirectionally clean. git-svn is
definitely not that, but I hope that vcs-svn will be (I have not kept up
on its status).

So I can buy the argument that bumping it forward ourselves will not
matter for any well-implemented helper.

That is the sort of thing that might be helpful to include in the commit
message; if somebody does run across such a helper and bisects to your
commit, then they can understand the rationale for the decision.

-Peff

Re: [PATCH 2/2] transport-helper: update remote helper namespace

[Felipe Contreras]

On Thu, Apr 11, 2013 at 12:05 AM, Jeff King <peff@peff.net> wrote:
> On Wed, Apr 10, 2013 at 11:53:38PM -0500, Felipe Contreras wrote:
>
>> > But if we push some commits to the helper, moving Y up to Z, then it
>> > would build the new commit (which contains the foreign-vcs's equivalent of
>> > Y..Z) on top of Z, not Y.
>>
>> Why would it do that? If X points to say revision 100, presumably it
>> was stored somewhere while doing a fetch. Similarly, if foreign
>> version of Z is 150, it can update that number while doing a push. The
>> next fetch it would start from 151.
>
> I think the only reason not to bump the marker forward during the push
> would be if the helper wants for some reason to "re-import" from the
> foreign source rather than accepting the git versions of the commits.
> Something like git-svn's markup of the commit messages with revision ids
> comes to mind.

Yeah, but that's already a second level hypothesis. First,
remote-helpers would need to be able to work without marks, and they
can't.

> But if it matters, then by definition that would mean
> that the import/export is not bidirectionally clean.

I don't see how would that matter.

> So I can buy the argument that bumping it forward ourselves will not
> matter for any well-implemented helper.

Or any helper.

> That is the sort of thing that might be helpful to include in the commit
> message; if somebody does run across such a helper and bisects to your
> commit, then they can understand the rationale for the decision.

If it did matter, it would be mentioned. I will updated it later if
there's no further comments.

--
Felipe Contreras

Re: [PATCH 2/2] transport-helper: update remote helper namespace

[Felipe Contreras]

Hi,

Why wasn't this patch merged to 'pu'? To my knowledge nobody raised
any real concerns.

Should I explain in every commit that touches transport-helper how
remote-helpers without marks are impossible? I know I said I was going
to update the commit message, but I don't think that reason to not put
it in 'pu'. Also, the only reason I said so was to make Jeff happy,
but now that I think again, it doesn't really belong there; remote
helpers cannot be using these refs, they just can't. They cannot work
without marks, it's not possible. To think otherwise is simply a
mistake.

On Thu, Apr 11, 2013 at 12:18 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Thu, Apr 11, 2013 at 12:05 AM, Jeff King <peff@peff.net> wrote:
>> On Wed, Apr 10, 2013 at 11:53:38PM -0500, Felipe Contreras wrote:
>>
>>> > But if we push some commits to the helper, moving Y up to Z, then it
>>> > would build the new commit (which contains the foreign-vcs's equivalent of
>>> > Y..Z) on top of Z, not Y.
>>>
>>> Why would it do that? If X points to say revision 100, presumably it
>>> was stored somewhere while doing a fetch. Similarly, if foreign
>>> version of Z is 150, it can update that number while doing a push. The
>>> next fetch it would start from 151.
>>
>> I think the only reason not to bump the marker forward during the push
>> would be if the helper wants for some reason to "re-import" from the
>> foreign source rather than accepting the git versions of the commits.
>> Something like git-svn's markup of the commit messages with revision ids
>> comes to mind.
>
> Yeah, but that's already a second level hypothesis. First,
> remote-helpers would need to be able to work without marks, and they
> can't.
>
>> But if it matters, then by definition that would mean
>> that the import/export is not bidirectionally clean.
>
> I don't see how would that matter.
>
>> So I can buy the argument that bumping it forward ourselves will not
>> matter for any well-implemented helper.
>
> Or any helper.
>
>> That is the sort of thing that might be helpful to include in the commit
>> message; if somebody does run across such a helper and bisects to your
>> commit, then they can understand the rationale for the decision.
>
> If it did matter, it would be mentioned. I will updated it later if
> there's no further comments.

-- 
Felipe Contreras

Re: [PATCH 2/2] transport-helper: update remote helper namespace

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Why wasn't this patch merged to 'pu'? To my knowledge nobody raised
> any real concerns.

There are many reasons not to queue _everything_ ever posted to the
list on 'pu', and they are almost always not a deliberate rejection.

The maintainer may have thought he is not the best person to judge
changes to the area the patch touches, and may be expecting further
comments from others, but haven't said "Comments?" and waiting for
them to say something without being asked. Or the maintainer may
have judged that it is likely to result in wasted work if he queues
that version of the patch, fixing trivial nits himself, only to see
a reroll arrive before the day's integration cycle finishes (which
makes him run the cycle again). Or the maintainer may have been busy
tending to other topics. Or the maintainer may have pushed the patch
down the queue for any of the above reasons to deal with it later,
and after having tended to others' topics, may have forgotten about
that patch.

Do I need to go on?

> I know I said I was going
> to update the commit message, but I don't think that reason to not put
> it in 'pu'.

For this particular case, I think that was exactly the reason why it
is not in 'pu' today. I actually did not remember the reason when
you asked above, until I read the above "I said I'll update".

One major reason why I queue a patch that is clearly not ready for
'next' is because I do not want to forget about the topic and not
because I want to keep the particular version of the patch. I do so
especially when contributor is unlikely to come back soon. If you
said you would come back soon, I do not even have to judge if it is
"clearly not ready" or "it is good enough" for next, and I have to
remember one less thing. The more I can put in "this will come back
so I do not have to do anything" bin, not in the "queue it as-is for
now, because it is likely that it won't be rerolled soon and I'll
forget about it" bin, the easier for me and we as the development
process as a whole can scale better.

Re: [PATCH 2/2] transport-helper: update remote helper namespace

[Felipe Contreras]

On Sun, Apr 14, 2013 at 12:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Why wasn't this patch merged to 'pu'? To my knowledge nobody raised
>> any real concerns.
>
> There are many reasons not to queue _everything_ ever posted to the
> list on 'pu', and they are almost always not a deliberate rejection.
>
> The maintainer may have thought he is not the best person to judge
> changes to the area the patch touches, and may be expecting further
> comments from others, but haven't said "Comments?" and waiting for
> them to say something without being asked. Or the maintainer may
> have judged that it is likely to result in wasted work if he queues
> that version of the patch, fixing trivial nits himself, only to see
> a reroll arrive before the day's integration cycle finishes (which
> makes him run the cycle again). Or the maintainer may have been busy
> tending to other topics. Or the maintainer may have pushed the patch
> down the queue for any of the above reasons to deal with it later,
> and after having tended to others' topics, may have forgotten about
> that patch.

The world is full of possibilities, but most of them are irrelevant,
specially since 'the maintainer' is right here and can mention the
reason himself. Is there anything wrong in asking?

>> I know I said I was going
>> to update the commit message, but I don't think that reason to not put
>> it in 'pu'.
>
> For this particular case, I think that was exactly the reason why it
> is not in 'pu' today. I actually did not remember the reason when
> you asked above, until I read the above "I said I'll update".
>
> One major reason why I queue a patch that is clearly not ready for
> 'next' is because I do not want to forget about the topic and not
> because I want to keep the particular version of the patch. I do so
> especially when contributor is unlikely to come back soon. If you
> said you would come back soon, I do not even have to judge if it is
> "clearly not ready" or "it is good enough" for next, and I have to
> remember one less thing. The more I can put in "this will come back
> so I do not have to do anything" bin, not in the "queue it as-is for
> now, because it is likely that it won't be rerolled soon and I'll
> forget about it" bin, the easier for me and we as the development
> process as a whole can scale better.

Well, I'm telling you I think it's good as it is. Other people might
disagree, but if they do so, it's possibly on the basis that other
transport-helpers might not be using marks, which is something I have
said time and again is not possible, and to think it is, is a mistake.

I am willing to demonstrate the above claim beyond any reasonable
doubt to any sensible person (haven't I done so already?), so that we
can avoid these discussions again in the future, and include in the
documentation that marks are necessary, or throw a warning for the
people developing a remote helper, or something along those lines.

But if that's not what you want me to do, then what is needed to get
this patch into 'pu'? That's what I'm wondering.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 2/2] transport-helper: update remote helper namespace

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Sun, Apr 14, 2013 at 12:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> Why wasn't this patch merged to 'pu'? To my knowledge nobody raised
>>> any real concerns.
>>
>> There are many reasons not to queue _everything_ ever posted to the
>> list on 'pu', and they are almost always not a deliberate rejection.
>>
>> The maintainer may have thought he is not the best person to judge
>> changes to the area the patch touches, and may be expecting further
>> comments from others, but haven't said "Comments?" and waiting for
>> them to say something without being asked. Or the maintainer may
>> have judged that it is likely to result in wasted work if he queues
>> that version of the patch, fixing trivial nits himself, only to see
>> a reroll arrive before the day's integration cycle finishes (which
>> makes him run the cycle again). Or the maintainer may have been busy
>> tending to other topics. Or the maintainer may have pushed the patch
>> down the queue for any of the above reasons to deal with it later,
>> and after having tended to others' topics, may have forgotten about
>> that patch.
>
> The world is full of possibilities, but most of them are irrelevant,
> specially since 'the maintainer' is right here and can mention the
> reason himself. Is there anything wrong in asking?

An earlier draft of my message starte with "Do you have to be
combative to ask a simple 'did you forget this?' question?", but
later I removed it. That was what made it irrelevant ;-)

Just rerolling with what _you_ think is an appropriate level of
explanation (either or both in log and in-code) and see what happens
would probably be the best way to proceed, I think, at this
point. Either you hear "It still is wrong and too sketchy", "Yeah,
thinking about it again, this is sufficient" from others.  Or a
silent, which I am inclined to take as much closer to the latter
after all the discussion.

Re: [PATCH 2/2] transport-helper: update remote helper namespace

[Jeff King]

On Sun, Apr 14, 2013 at 11:45:10AM -0700, Junio C Hamano wrote:

> Just rerolling with what _you_ think is an appropriate level of
> explanation (either or both in log and in-code) and see what happens
> would probably be the best way to proceed, I think, at this
> point. Either you hear "It still is wrong and too sketchy", "Yeah,
> thinking about it again, this is sufficient" from others.  Or a
> silent, which I am inclined to take as much closer to the latter
> after all the discussion.

FWIW, the last email I wrote on this patch said:

  So I can buy the argument that bumping it forward ourselves will not
  matter for any well-implemented helper.

and I was the only reviewer, so I think the code is probably OK. I also
said:

  That is the sort of thing that might be helpful to include in the
  commit message[...]

Felipe of course did not agree, but I have no interest in trying to
persuade him on that front, as it seems to just waste everyone's time.

-Peff

Re: [PATCH 2/2] transport-helper: update remote helper namespace

[Jonathan Nieder]

Junio C Hamano wrote:

> Just rerolling with what _you_ think is an appropriate level of
> explanation (either or both in log and in-code) and see what happens
> would probably be the best way to proceed, I think, at this
> point. Either you hear "It still is wrong and too sketchy", "Yeah,
> thinking about it again, this is sufficient" from others.  Or a
> silent, which I am inclined to take as much closer to the latter
> after all the discussion.

For future reference, sometimes when I am silent it does not mean
agreement but means "I am fed up and don't consider it to be worth the
drain on my energy to deal with this mess."  Which is pretty close to
"let's move on; this is sufficient", yes.

Thanks,
Jonathan