Re: [PATCH v2 1/6] transport-helper: clarify *:* refspec
On Wed, Apr 17, 2013 at 11:14:28PM -0500, Felipe Contreras wrote: The *:* refspec doesn't work, and never has, clarify the code and documentation to reflect that. This in effect reverts commit 9e7673e (gitremote-helpers(1): clarify refspec behaviour). In what way doesn't it work? If I specify that refspec then I do get output that appears sensible. Signed-off-by: Felipe Contreras felipe.contre...@gmail.com --- Documentation/gitremote-helpers.txt | 4 ++-- t/t5801-remote-helpers.sh | 15 --- transport-helper.c | 2 +- 3 files changed, 3 insertions(+), 18 deletions(-) diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt index f506031..0c91aba 100644 --- a/Documentation/gitremote-helpers.txt +++ b/Documentation/gitremote-helpers.txt @@ -174,8 +174,8 @@ ref. This capability can be advertised multiple times. The first applicable refspec takes precedence. The left-hand of refspecs advertised with this capability must cover all refs reported by -the list command. If a helper does not need a specific 'refspec' -capability then it should advertise `refspec *:*`. +the list command. If no 'refspec' capability is advertised, +there is an implied `refspec *:*`. This is wrong. As your later patch makes clearer, there is no implied refspec for push - it only works for fetch. I found the wording you've reverted to extremely misleading. How about something like this: For historical reasons, 'import' treats the absence of a 'refspec' line as equivalent to `refspec *:*`; remote helpers should always specify an explicit refspec. ? 'bidi-import':: This modifies the 'import' capability. diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh index f387027..cd1873c 100755 --- a/t/t5801-remote-helpers.sh +++ b/t/t5801-remote-helpers.sh @@ -120,21 +120,6 @@ test_expect_failure 'pushing without refspecs' ' compare_refs local2 HEAD server HEAD ' -test_expect_success 'pulling with straight refspec' ' - (cd local2 - GIT_REMOTE_TESTGIT_REFSPEC=*:* git pull) - compare_refs local2 HEAD server HEAD -' - -test_expect_failure 'pushing with straight refspec' ' - test_when_finished (cd local2 git reset --hard origin) - (cd local2 - echo content file - git commit -a -m eleven - GIT_REMOTE_TESTGIT_REFSPEC=*:* git push) - compare_refs local2 HEAD server HEAD -' - test_expect_success 'pulling without marks' ' (cd local2 GIT_REMOTE_TESTGIT_NO_MARKS=1 git pull) diff --git a/transport-helper.c b/transport-helper.c index dcd8d97..cea787c 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -469,7 +469,7 @@ static int fetch_with_import(struct transport *transport, * were fetching. * * (If no refspec capability was specified, for historical - * reasons we default to *:*.) + * reasons we default to the equivalent of *:*.) * * Store the result in to_fetch[i].old_sha1. Callers such * as git fetch can use the value to write feedback to the -- 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
Re: [PATCH v2 1/6] transport-helper: clarify *:* refspec
On Thu, Apr 18, 2013 at 3:24 AM, John Keeping j...@keeping.me.uk wrote: On Wed, Apr 17, 2013 at 11:14:28PM -0500, Felipe Contreras wrote: The *:* refspec doesn't work, and never has, clarify the code and documentation to reflect that. This in effect reverts commit 9e7673e (gitremote-helpers(1): clarify refspec behaviour). In what way doesn't it work? If I specify that refspec then I do get output that appears sensible. % git checkout origin/master % make -C t t5801-remote-helpers.sh not ok 15 - pushing with straight refspec # TODO known breakage It fails for me here. Documentation/gitremote-helpers.txt | 4 ++-- t/t5801-remote-helpers.sh | 15 --- transport-helper.c | 2 +- 3 files changed, 3 insertions(+), 18 deletions(-) diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt index f506031..0c91aba 100644 --- a/Documentation/gitremote-helpers.txt +++ b/Documentation/gitremote-helpers.txt @@ -174,8 +174,8 @@ ref. This capability can be advertised multiple times. The first applicable refspec takes precedence. The left-hand of refspecs advertised with this capability must cover all refs reported by -the list command. If a helper does not need a specific 'refspec' -capability then it should advertise `refspec *:*`. +the list command. If no 'refspec' capability is advertised, +there is an implied `refspec *:*`. This is wrong. It has been like that since v1.7.0. As your later patch makes clearer, there is no implied refspec for push - it only works for fetch. I found the wording you've reverted to extremely misleading. How about something like this: For historical reasons, 'import' treats the absence of a 'refspec' line as equivalent to `refspec *:*`; remote helpers should always specify an explicit refspec. Maybe my version was misleading because it didn't mention it was for historical reasons, but yours is plain wrong; remote helpers might not be using 'import' or 'export' at all, so not all remote helpers should always specify an explicit refspec. And if the previous text It is recommended that all importers providing the 'import' capability use this. It's mandatory for 'export'. does not convey the idea these remote helpers should always specify an explicit refspec, I don't know what it does. -- 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
Re: [PATCH v2 1/6] transport-helper: clarify *:* refspec
On Thu, Apr 18, 2013 at 04:27:57AM -0500, Felipe Contreras wrote: On Thu, Apr 18, 2013 at 3:24 AM, John Keeping j...@keeping.me.uk wrote: On Wed, Apr 17, 2013 at 11:14:28PM -0500, Felipe Contreras wrote: diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt index f506031..0c91aba 100644 --- a/Documentation/gitremote-helpers.txt +++ b/Documentation/gitremote-helpers.txt @@ -174,8 +174,8 @@ ref. This capability can be advertised multiple times. The first applicable refspec takes precedence. The left-hand of refspecs advertised with this capability must cover all refs reported by -the list command. If a helper does not need a specific 'refspec' -capability then it should advertise `refspec *:*`. +the list command. If no 'refspec' capability is advertised, +there is an implied `refspec *:*`. This is wrong. It has been like that since v1.7.0. That doesn't mean it's right. I suspect that it was originally correct, but then 'export' was added at which point this became stale. As your later patch makes clearer, there is no implied refspec for push - it only works for fetch. I found the wording you've reverted to extremely misleading. How about something like this: For historical reasons, 'import' treats the absence of a 'refspec' line as equivalent to `refspec *:*`; remote helpers should always specify an explicit refspec. Maybe my version was misleading because it didn't mention it was for historical reasons, but yours is plain wrong; remote helpers might not be using 'import' or 'export' at all, so not all remote helpers should always specify an explicit refspec. And if the previous text It is recommended that all importers providing the 'import' capability use this. It's mandatory for 'export'. does not convey the idea these remote helpers should always specify an explicit refspec, I don't know what it does. I didn't say mine was correct, but there was a reason that I wanted to change the existing text. Just going back to what was there before is not a good way to make things better. In my copy of pu I don't see the text It's mandatory for 'export', it just stops after It is recommended that all importers providing the 'import' capability use this. That paragraph also starts with This modifies the 'import' capability making no mention of export. Perhaps we need a slightly more extensive re-write of the documentation for refspec to make it very clear where it applies and when it is needed. -- 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 1/6] transport-helper: clarify *:* refspec
On Thu, Apr 18, 2013 at 4:45 AM, John Keeping j...@keeping.me.uk wrote: On Thu, Apr 18, 2013 at 04:27:57AM -0500, Felipe Contreras wrote: Maybe my version was misleading because it didn't mention it was for historical reasons, but yours is plain wrong; remote helpers might not be using 'import' or 'export' at all, so not all remote helpers should always specify an explicit refspec. And if the previous text It is recommended that all importers providing the 'import' capability use this. It's mandatory for 'export'. does not convey the idea these remote helpers should always specify an explicit refspec, I don't know what it does. I didn't say mine was correct, but there was a reason that I wanted to change the existing text. Just going back to what was there before is not a good way to make things better. And just because it was that way before doesn't mean it was worst. Your patch essentially switched from it is implied, to it should be explicit, which is wrong. Going back to the previous text is the simplest change that restores a reasonable explanation. The next patches in this series deal with the rest of the issues in this text. In my copy of pu I don't see the text It's mandatory for 'export', it just stops after It is recommended that all importers providing the 'import' capability use this. That paragraph also starts with This modifies the 'import' capability making no mention of export. This is patch 1 of 6, keep going. -- 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
Re: [PATCH v2 1/6] transport-helper: clarify *:* refspec
On Thu, Apr 18, 2013 at 05:02:11AM -0500, Felipe Contreras wrote: On Thu, Apr 18, 2013 at 4:45 AM, John Keeping j...@keeping.me.uk wrote: In my copy of pu I don't see the text It's mandatory for 'export', it just stops after It is recommended that all importers providing the 'import' capability use this. That paragraph also starts with This modifies the 'import' capability making no mention of export. This is patch 1 of 6, keep going. Ah, I see now. I had read all of the patches initially, but then focused on this one for discussion. The end result after this series does look good, but I find it slightly strange to take a step backwards on the way. -- 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 1/6] transport-helper: clarify *:* refspec
Felipe Contreras felipe.contre...@gmail.com writes: The *:* refspec doesn't work, and never has, clarify the code and documentation to reflect that. This in effect reverts commit 9e7673e (gitremote-helpers(1): clarify refspec behaviour). ... applicable refspec takes precedence. The left-hand of refspecs advertised with this capability must cover all refs reported by -the list command. If a helper does not need a specific 'refspec' -capability then it should advertise `refspec *:*`. +the list command. If no 'refspec' capability is advertised, +there is an implied `refspec *:*`. I presume s/.$/, but `*:*` does not work so do not use it./ is implied? diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh index f387027..cd1873c 100755 --- a/t/t5801-remote-helpers.sh +++ b/t/t5801-remote-helpers.sh @@ -120,21 +120,6 @@ test_expect_failure 'pushing without refspecs' ' compare_refs local2 HEAD server HEAD ' -test_expect_success 'pulling with straight refspec' ' - (cd local2 - GIT_REMOTE_TESTGIT_REFSPEC=*:* git pull) - compare_refs local2 HEAD server HEAD -' This one made me raise my eyebrows, first. The only reason this test passes is because this particular pull, due to what local2 and server happens to have before this test runs, is an Already up-to-date and a no-op. You are removing this because it is not really testing anything useful, and if you change it to test something real, e.g. by rewinding local2, it will reveal the breakage. Am I reading you correctly? diff --git a/transport-helper.c b/transport-helper.c index dcd8d97..cea787c 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -469,7 +469,7 @@ static int fetch_with_import(struct transport *transport, * were fetching. * * (If no refspec capability was specified, for historical - * reasons we default to *:*.) + * reasons we default to the equivalent of *:*.) * * Store the result in to_fetch[i].old_sha1. Callers such * as git fetch can use the value to write feedback to the -- 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
