Re: [PATCH v2 4/6] transport-helper: warn when refspec is not used

2013-04-18 Thread Junio C Hamano 
Felipe Contreras felipe.contre...@gmail.com writes:

 For the modes that need it. In the future we should probably error out,
 instead of providing half-assed support.

 The reason we want to do this is because if it's not present, the remote
 helper might be updating refs/heads/*, or refs/remotes/origin/*,
 directly, and in the process fetch will get confused trying to update
 refs that are already updated, or older than what they should be. We
 shouldn't be messing with the rest of git.

So that answers my question in the response to an earlier one in
this series.  We expect the ref updates to be done by the fetch or
push that drives the helper, and do not want the helper to interfere
with its ref updates.

So it is not just 'refspec' _allows_ the refs to be constrained to a
private namespace, like the earlier updates made the documentation
say; it _is_ mandatory to use refspecs to constrain them to avoid
touching refs/heads and refs/remotes namespace.

Am I reading you correctly?

Assuming I am, the patch in this message looks reasonable.

It makes the documentation updates a few patches ago look a bit
wanting, though.

Thanks.

 Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
 ---
  t/t5801-remote-helpers.sh | 6 --
  transport-helper.c| 2 ++
  2 files changed, 6 insertions(+), 2 deletions(-)

 diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
 index 3eeb309..1bb7529 100755
 --- a/t/t5801-remote-helpers.sh
 +++ b/t/t5801-remote-helpers.sh
 @@ -100,14 +100,16 @@ test_expect_failure 'push new branch with old:new 
 refspec' '
  
  test_expect_success 'cloning without refspec' '
   GIT_REMOTE_TESTGIT_REFSPEC= \
 - git clone testgit::${PWD}/server local2 
 + git clone testgit::${PWD}/server local2 2 error 
 + grep This remote helper should implement refspec capability error 
   compare_refs local2 HEAD server HEAD
  '
  
  test_expect_success 'pulling without refspecs' '
   (cd local2 
   git reset --hard 
 - GIT_REMOTE_TESTGIT_REFSPEC= git pull) 
 + GIT_REMOTE_TESTGIT_REFSPEC= git pull 2 ../error) 
 + grep This remote helper should implement refspec capability error 
   compare_refs local2 HEAD server HEAD
  '
  
 diff --git a/transport-helper.c b/transport-helper.c
 index 4d98567..573eaf7 100644
 --- a/transport-helper.c
 +++ b/transport-helper.c
 @@ -215,6 +215,8 @@ static struct child_process *get_helper(struct transport 
 *transport)
   free((char *)refspecs[i]);
   }
   free(refspecs);
 + } else if (data-import || data-bidi_import || data-export) {
 + warning(This remote helper should implement refspec 
 capability.);
   }
   strbuf_release(buf);
   if (debug)
--
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 v2 4/6] transport-helper: warn when refspec is not used

2013-04-18 Thread Felipe Contreras 
On Thu, Apr 18, 2013 at 12:30 PM, Junio C Hamano gits...@pobox.com wrote:
 Felipe Contreras felipe.contre...@gmail.com writes:

 For the modes that need it. In the future we should probably error out,
 instead of providing half-assed support.

 The reason we want to do this is because if it's not present, the remote
 helper might be updating refs/heads/*, or refs/remotes/origin/*,
 directly, and in the process fetch will get confused trying to update
 refs that are already updated, or older than what they should be. We
 shouldn't be messing with the rest of git.

 So that answers my question in the response to an earlier one in
 this series.  We expect the ref updates to be done by the fetch or
 push that drives the helper, and do not want the helper to interfere
 with its ref updates.

 So it is not just 'refspec' _allows_ the refs to be constrained to a
 private namespace, like the earlier updates made the documentation
 say; it _is_ mandatory to use refspecs to constrain them to avoid
 touching refs/heads and refs/remotes namespace.

Yeah, it was not stated that it was mandatory, but I think it's in
everybody's best interest that it is.

-- 
Felipe Contreras
--
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

[PATCH v2 4/6] transport-helper: warn when refspec is not used

2013-04-17 Thread Felipe Contreras 
For the modes that need it. In the future we should probably error out,
instead of providing half-assed support.

The reason we want to do this is because if it's not present, the remote
helper might be updating refs/heads/*, or refs/remotes/origin/*,
directly, and in the process fetch will get confused trying to update
refs that are already updated, or older than what they should be. We
shouldn't be messing with the rest of git.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 t/t5801-remote-helpers.sh | 6 --
 transport-helper.c| 2 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index 3eeb309..1bb7529 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -100,14 +100,16 @@ test_expect_failure 'push new branch with old:new 
refspec' '
 
 test_expect_success 'cloning without refspec' '
GIT_REMOTE_TESTGIT_REFSPEC= \
-   git clone testgit::${PWD}/server local2 
+   git clone testgit::${PWD}/server local2 2 error 
+   grep This remote helper should implement refspec capability error 
compare_refs local2 HEAD server HEAD
 '
 
 test_expect_success 'pulling without refspecs' '
(cd local2 
git reset --hard 
-   GIT_REMOTE_TESTGIT_REFSPEC= git pull) 
+   GIT_REMOTE_TESTGIT_REFSPEC= git pull 2 ../error) 
+   grep This remote helper should implement refspec capability error 
compare_refs local2 HEAD server HEAD
 '
 
diff --git a/transport-helper.c b/transport-helper.c
index 4d98567..573eaf7 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -215,6 +215,8 @@ static struct child_process *get_helper(struct transport 
*transport)
free((char *)refspecs[i]);
}
free(refspecs);
+   } else if (data-import || data-bidi_import || data-export) {
+   warning(This remote helper should implement refspec 
capability.);
}
strbuf_release(buf);
if (debug)
-- 
1.8.2.1.679.g509521a

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