Re: [PATCH 1/6] transport-helper: clarify *:* refspec
On Thu, Apr 18, 2013 at 7:39 AM, Thomas Rast wrote: > Felipe Contreras writes: > >> On Thu, Apr 18, 2013 at 2:28 AM, Thomas Rast 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). >>> [...] -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 -' >>> >>> So what's wrong with the tests? Do they fail to test what they claim >>> (how?), test something that wasn't reasonable to begin with, or >>> something entirely different? >> >> Look at the code comment, and look at the now updated documentation >> that assumes that *:* was reasonable. Given the available information, >> it would be reasonable to assume that *:* did work, but it didn't >> work, and it's not really possible to fix it, even if we wanted to, it >> would be a hack. It's better to accept that fact and stop worrying too >> much about what would be the best way to do the wrong thing. > > Ok, you say that the *failing* test set an expectation that is > unrealistic, so let's drop it. > > But then what about the successful test? Does it actually work (and by > removing the test, you are saying that we don't care if we subsequently > break that (mis)feature)? Or did it test the wrong thing? Yeah, it works, in the sense that peeing in a bottle is a solution; it might work, but it's not recommendable. So, if suddenly working, frankly I don't care. I added those tests, and I don't think they are needed. In a not too distant future it should not be permitted to "work"; we don't want developers to shoot themselves in the foot, and heir users too. -- 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 1/6] transport-helper: clarify *:* refspec
Felipe Contreras writes:
> On Thu, Apr 18, 2013 at 2:28 AM, Thomas Rast 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).
>> [...]
>>> -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
>>> -'
>>
>> So what's wrong with the tests? Do they fail to test what they claim
>> (how?), test something that wasn't reasonable to begin with, or
>> something entirely different?
>
> Look at the code comment, and look at the now updated documentation
> that assumes that *:* was reasonable. Given the available information,
> it would be reasonable to assume that *:* did work, but it didn't
> work, and it's not really possible to fix it, even if we wanted to, it
> would be a hack. It's better to accept that fact and stop worrying too
> much about what would be the best way to do the wrong thing.
Ok, you say that the *failing* test set an expectation that is
unrealistic, so let's drop it.
But then what about the successful test? Does it actually work (and by
removing the test, you are saying that we don't care if we subsequently
break that (mis)feature)? Or did it test the wrong thing?
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
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 1/6] transport-helper: clarify *:* refspec
On Thu, Apr 18, 2013 at 2:28 AM, Thomas Rast 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). > [...] >> -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 >> -' > > So what's wrong with the tests? Do they fail to test what they claim > (how?), test something that wasn't reasonable to begin with, or > something entirely different? Look at the code comment, and look at the now updated documentation that assumes that *:* was reasonable. Given the available information, it would be reasonable to assume that *:* did work, but it didn't work, and it's not really possible to fix it, even if we wanted to, it would be a hack. It's better to accept that fact and stop worrying too much about what would be the best way to do the wrong thing. -- 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 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).
[...]
> -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
> -'
So what's wrong with the tests? Do they fail to test what they claim
(how?), test something that wasn't reasonable to begin with, or
something entirely different?
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
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 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
