Re: [PATCH v2 1/6] transport-helper: clarify *:* refspec
On Thu, Apr 18, 2013 at 12:27 PM, Junio C Hamano wrote: > Felipe Contreras 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? I believe s/.$/, but you should specify an apropriate one as described above./ Would be better. >> 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? Not really. This particular fetch does work, and it's tricky to explain all the reasons why, even if you update the server ref before fetching. The reason it's written this way is that it comes from from a branch of mine that has a hack to make the push above works, and since the transport-helper's push doesn't update the ref in this version, the test did actually do something and was working. I am removing the test because we don't care if it works or not, remote-helpers should not be doing that. The fact that the test wasn't actually doing anything is icing on the cake. Cheers. -- 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
Felipe Contreras 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
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 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
On Thu, Apr 18, 2013 at 4:45 AM, John Keeping 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 04:27:57AM -0500, Felipe Contreras wrote: > On Thu, Apr 18, 2013 at 3:24 AM, John Keeping 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 3:24 AM, John Keeping 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 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 > --- > 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
[PATCH v2 1/6] transport-helper: clarify *:* refspec
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). Signed-off-by: Felipe Contreras --- 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 *:*`. '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
