Generalize reference locking in tests #1634
Conversation
Welcome to GitGitGadgetHi @jltobler, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that your Pull Request has a good description, as it will be used as cover letter. You can CC potential reviewers by adding a footer to the PR description with the following syntax: Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment If you want to see what email(s) would be sent for a After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail). If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the curl -g --user "<EMailAddress>:<Password>" \
--url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txtTo iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description): To send a new iteration, just add another PR comment with the contents: Need help?New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join. You may also be able to find help in real time in the developer IRC channel, |
jltobler
force-pushed
the
jt/backend-generic-ref-lock
branch
from
2e18d07
to
a4a2f36
Compare
|
/allow |
|
User jltobler is now allowed to use GitGitGadget. WARNING: jltobler has no public email address set on GitHub; |
jltobler
force-pushed
the
jt/backend-generic-ref-lock
branch
from
a4a2f36
to
11fd509
Compare
|
/submit |
|
Submitted as pull.1634.git.1704912750.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Justin Tobler via GitGitGadget" <gitgitgadget@gmail.com> writes:
> This approach is more verbose and may warrant consideration of implementing
> an abstraction to further simplify this operation. There appears to be one
> other existing test in t1400 that also follows this pattern. Being that
> there is only a handful of affected tests the implemented approach may be
> sufficient as is.
The use of two fifos and avoiding deadlocking parent and child is
tricky enough that it does feel that it warrants a helper function
to do the common part of what these two patches add.
I think I read t1401 patch carefully enough to understand what is
going on, but I cannot yet claim the same for the other one.
Thanks.
> Justin Tobler (2):
> t1401: generalize reference locking
> t5541: generalize reference locking
>
> t/t1401-symbolic-ref.sh | 28 ++++++++++++++++++++++++----
> t/t5541-http-push-smart.sh | 25 +++++++++++++++++++++++--
> 2 files changed, 47 insertions(+), 6 deletions(-)
>
>
> base-commit: 624eb90fa8f65a79396615f3c2842ac5a3743350
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1634%2Fjltobler%2Fjt%2Fbackend-generic-ref-lock-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1634/jltobler/jt/backend-generic-ref-lock-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1634 |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
t/t1401-symbolic-ref.sh
Outdated
| @@ -105,10 +105,30 @@ test_expect_success LONG_REF 'we can parse long symbolic ref' ' | |||
| test_cmp expect actual | |||
There was a problem hiding this comment.
On the Git mailing list, Jeff King wrote (reply to this):
On Wed, Jan 10, 2024 at 06:52:29PM +0000, Justin Tobler via GitGitGadget wrote:
> From: Justin Tobler <jltobler@gmail.com>
>
> Some tests set up reference locks by directly creating the lockfile.
> While this works for the files reference backend, reftable reference
> locks operate differently and are incompatible with this approach.
> Refactor the test to use git-update-ref(1) to lock refs instead so that
> the test does not need to be aware of how the ref backend locks refs.
It looks like you re-create this situation in a backend-agnostic way by
having two simultaneous updates that conflict on the lock (but don't
care how that lock is implemented).
That works, but I think we could keep it simple. This test doesn't care
about the exact error condition we create. The point was just to die in
create_symref() and make sure the exit code was propagated. So something
like this would work:
$ git symbolic-ref refs/heads refs/heads/foo
error: unable to write symref for refs/heads: Is a directory
(note that you get a different error message if the refs are packed,
since there we can notice the d/f conflict manually).
There may be other ways to stimulate a failure. I thought "symbolic-ref
HEAD refs/heads/.invalid" might work, but sadly the refname format check
happens earlier.
I think it is worth avoiding the fifo magic if we can. It's complicated,
and it means that not all platforms run the test.
-PeffThere was a problem hiding this comment.
On the Git mailing list, Patrick Steinhardt wrote (reply to this):
On Thu, Jan 11, 2024 at 02:13:29AM -0500, Jeff King wrote:
> On Wed, Jan 10, 2024 at 06:52:29PM +0000, Justin Tobler via GitGitGadget wrote:
>
> > From: Justin Tobler <jltobler@gmail.com>
> >
> > Some tests set up reference locks by directly creating the lockfile.
> > While this works for the files reference backend, reftable reference
> > locks operate differently and are incompatible with this approach.
> > Refactor the test to use git-update-ref(1) to lock refs instead so that
> > the test does not need to be aware of how the ref backend locks refs.
>
> It looks like you re-create this situation in a backend-agnostic way by
> having two simultaneous updates that conflict on the lock (but don't
> care how that lock is implemented).
>
> That works, but I think we could keep it simple. This test doesn't care
> about the exact error condition we create. The point was just to die in
> create_symref() and make sure the exit code was propagated. So something
> like this would work:
>
> $ git symbolic-ref refs/heads refs/heads/foo
> error: unable to write symref for refs/heads: Is a directory
>
> (note that you get a different error message if the refs are packed,
> since there we can notice the d/f conflict manually).
If all we care for is the exit code then this would work for the
reftable backend, too:
```
$ git init --ref-format=reftable repo
Initialized empty Git repository in /tmp/repo/.git/
$ cd repo/
$ git commit --allow-empty --message message
[main (root-commit) c2512d3] x
$ git symbolic-ref refs/heads refs/heads/foo
$ echo $?
1
```
A bit unfortunate that there is no proper error message in that case,
but that is a different topic.
Patrick
> There may be other ways to stimulate a failure. I thought "symbolic-ref
> HEAD refs/heads/.invalid" might work, but sadly the refname format check
> happens earlier.
>
> I think it is worth avoiding the fifo magic if we can. It's complicated,
> and it means that not all platforms run the test.
>
> -Peff
> There was a problem hiding this comment.
On the Git mailing list, Justin Tobler wrote (reply to this):
Jeff King <peff@peff.net> writes:
> It looks like you re-create this situation in a backend-agnostic way by
> having two simultaneous updates that conflict on the lock (but don't
> care how that lock is implemented).
>
> That works, but I think we could keep it simple. This test doesn't care
> about the exact error condition we create. The point was just to die in
> create_symref() and make sure the exit code was propagated.
As you mentioned, the original intent was to recreate the same error
condition in a reference backend agnostic manner. Since the test doesn't
care about the exact error condition being used, I agree that creating a
d/f conflict instead is a much simpler and better approach. In the next
patch version I've updated the test in t1401 to use git-symbolic-ref(1)
to produce a d/f error.
Thanks,
Justin
On Thu, Jan 11, 2024 at 1:13 AM Jeff King <peff@peff.net> wrote:
>
> On Wed, Jan 10, 2024 at 06:52:29PM +0000, Justin Tobler via GitGitGadget wrote:
>
> > From: Justin Tobler <jltobler@gmail.com>
> >
> > Some tests set up reference locks by directly creating the lockfile.
> > While this works for the files reference backend, reftable reference
> > locks operate differently and are incompatible with this approach.
> > Refactor the test to use git-update-ref(1) to lock refs instead so that
> > the test does not need to be aware of how the ref backend locks refs.
>
> It looks like you re-create this situation in a backend-agnostic way by
> having two simultaneous updates that conflict on the lock (but don't
> care how that lock is implemented).
>
> That works, but I think we could keep it simple. This test doesn't care
> about the exact error condition we create. The point was just to die in
> create_symref() and make sure the exit code was propagated. So something
> like this would work:
>
> $ git symbolic-ref refs/heads refs/heads/foo
> error: unable to write symref for refs/heads: Is a directory
>
> (note that you get a different error message if the refs are packed,
> since there we can notice the d/f conflict manually).
>
> There may be other ways to stimulate a failure. I thought "symbolic-ref
> HEAD refs/heads/.invalid" might work, but sadly the refname format check
> happens earlier.
>
> I think it is worth avoiding the fifo magic if we can. It's complicated,
> and it means that not all platforms run the test.
>
> -PeffThere was a problem hiding this comment.
On the Git mailing list, Jeff King wrote (reply to this):
On Thu, Jan 11, 2024 at 12:08:44PM +0100, Patrick Steinhardt wrote:
> > (note that you get a different error message if the refs are packed,
> > since there we can notice the d/f conflict manually).
>
> If all we care for is the exit code then this would work for the
> reftable backend, too:
>
> ```
> $ git init --ref-format=reftable repo
> Initialized empty Git repository in /tmp/repo/.git/
> $ cd repo/
> $ git commit --allow-empty --message message
> [main (root-commit) c2512d3] x
> $ git symbolic-ref refs/heads refs/heads/foo
> $ echo $?
> 1
> ```
Yep, exactly. That should work for both and cover what the test was
originally trying to do.
> A bit unfortunate that there is no proper error message in that case,
> but that is a different topic.
Yeah, I would call that an outright bug. It does not have to be part of
this patch, but is worth fixing (and testing). I suspect it's not going
to be the only place with this problem. Most of the files-backend ref
code is very happy to spew to stdout using error(), but the reftable
code, having been written from a more lib-conscious perspective,
probably doesn't.
The obvious quick fix is to sprinkle more error() into the reftable
code. But in the longer term, I think the right direction is that the
ref code should accept an error strbuf or similar mechanism to propagate
human-readable error test to the caller.
-PeffThere was a problem hiding this comment.
On the Git mailing list, Patrick Steinhardt wrote (reply to this):
On Fri, Jan 12, 2024 at 02:01:42AM -0500, Jeff King wrote:
> On Thu, Jan 11, 2024 at 12:08:44PM +0100, Patrick Steinhardt wrote:
>
> > > (note that you get a different error message if the refs are packed,
> > > since there we can notice the d/f conflict manually).
> >
> > If all we care for is the exit code then this would work for the
> > reftable backend, too:
> >
> > ```
> > $ git init --ref-format=reftable repo
> > Initialized empty Git repository in /tmp/repo/.git/
> > $ cd repo/
> > $ git commit --allow-empty --message message
> > [main (root-commit) c2512d3] x
> > $ git symbolic-ref refs/heads refs/heads/foo
> > $ echo $?
> > 1
> > ```
>
> Yep, exactly. That should work for both and cover what the test was
> originally trying to do.
>
> > A bit unfortunate that there is no proper error message in that case,
> > but that is a different topic.
>
> Yeah, I would call that an outright bug. It does not have to be part of
> this patch, but is worth fixing (and testing). I suspect it's not going
> to be the only place with this problem. Most of the files-backend ref
> code is very happy to spew to stdout using error(), but the reftable
> code, having been written from a more lib-conscious perspective,
> probably doesn't.
Yup, I've already fixed this bug in the reftable backend.
> The obvious quick fix is to sprinkle more error() into the reftable
> code. But in the longer term, I think the right direction is that the
> ref code should accept an error strbuf or similar mechanism to propagate
> human-readable error test to the caller.
Agreed, I think it's good that the reftable library itself does not
print error messages. In this particular case we are already able to
provide a proper error message due to the error code that the library
returns. But there are certainly going to be other cases where it might
make sense to pass in an error strbuf.
PatrickThere was a problem hiding this comment.
On the Git mailing list, Jeff King wrote (reply to this):
On Fri, Jan 12, 2024 at 08:45:22AM +0100, Patrick Steinhardt wrote:
> > The obvious quick fix is to sprinkle more error() into the reftable
> > code. But in the longer term, I think the right direction is that the
> > ref code should accept an error strbuf or similar mechanism to propagate
> > human-readable error test to the caller.
>
> Agreed, I think it's good that the reftable library itself does not
> print error messages. In this particular case we are already able to
> provide a proper error message due to the error code that the library
> returns. But there are certainly going to be other cases where it might
> make sense to pass in an error strbuf.
Oh, if there is an error code you can use already, that is even better. :)
Thanks for taking care of this case.
-Peff
|
User |
| @@ -232,8 +232,29 @@ test_expect_success 'push --atomic fails on server-side errors' ' | |||
| test_config -C "$d" http.receivepack true && | |||
There was a problem hiding this comment.
On the Git mailing list, Jeff King wrote (reply to this):
On Wed, Jan 10, 2024 at 06:52:30PM +0000, Justin Tobler via GitGitGadget wrote:
> From: Justin Tobler <jltobler@gmail.com>
>
> Some tests set up reference locks by directly creating the lockfile.
> While this works for the files reference backend, reftable reference
> locks operate differently and are incompatible with this approach.
> Generalize reference locking by preparing a reference transaction.
As with the first patch, I think we could use d/f conflicts to get the
same effect. Perhaps something like this:
diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index df758e187d..7eb0e887e1 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -233,7 +233,8 @@ test_expect_success 'push --atomic fails on server-side errors' '
up="$HTTPD_URL"/smart/atomic-branches.git &&
# break ref updates for other on the remote site
- mkdir "$d/refs/heads/other.lock" &&
+ git -C "$d" update-ref -d refs/heads/other &&
+ git -C "$d" update-ref refs/heads/other/block-me HEAD &&
# add the new commit to other
git branch -f other collateral &&
@@ -244,12 +245,8 @@ test_expect_success 'push --atomic fails on server-side errors' '
# the new branch should not have been created upstream
test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
- # upstream should still reflect atomic2, the last thing we pushed
- # successfully
- git rev-parse atomic2 >expected &&
- # ...to other.
- git -C "$d" rev-parse refs/heads/other >actual &&
- test_cmp expected actual &&
+ # upstream should not have updated, as it cannot be written at all.
+ test_must_fail git -C "$d" rev-parse --verify refs/heads/other &&
# the new branch should not have been created upstream
test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
I do think that the original was slightly more interesting (since we
could check that "other" still existed but was not updated), but I think
the main point of the test is that "atomic" was not pushed at all.
-PeffThere was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Jeff King <peff@peff.net> writes:
> On Wed, Jan 10, 2024 at 06:52:30PM +0000, Justin Tobler via GitGitGadget wrote:
>
>> From: Justin Tobler <jltobler@gmail.com>
>>
>> Some tests set up reference locks by directly creating the lockfile.
>> While this works for the files reference backend, reftable reference
>> locks operate differently and are incompatible with this approach.
>> Generalize reference locking by preparing a reference transaction.
>
> As with the first patch, I think we could use d/f conflicts to get the
> same effect. Perhaps something like this:
Thanks for a great alternative. I agree that avoiding fifo indeed
is a better way to go.There was a problem hiding this comment.
On the Git mailing list, Justin Tobler wrote (reply to this):
Junio C Hamano <gitster@pobox.com> writes:
> Jeff King <peff@peff.net> writes:
>
>> On Wed, Jan 10, 2024 at 06:52:30PM +0000, Justin Tobler via GitGitGadget wrote:
>>
>>> From: Justin Tobler <jltobler@gmail.com>
>>>
>>> Some tests set up reference locks by directly creating the lockfile.
>>> While this works for the files reference backend, reftable reference
>>> locks operate differently and are incompatible with this approach.
>>> Generalize reference locking by preparing a reference transaction.
>>
>> As with the first patch, I think we could use d/f conflicts to get the
>> same effect. Perhaps something like this:
>
> Thanks for a great alternative. I agree that avoiding fifo indeed
> is a better way to go.
For this patch, in the next version, I have also followed Peff's
suggestion to create d/f conflicts to trigger an error condition instead
of using fifos.
Thanks to everyone for the feedback!
Justin
On Thu, Jan 11, 2024 at 12:48 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Jeff King <peff@peff.net> writes:
>
> > On Wed, Jan 10, 2024 at 06:52:30PM +0000, Justin Tobler via GitGitGadget wrote:
> >
> >> From: Justin Tobler <jltobler@gmail.com>
> >>
> >> Some tests set up reference locks by directly creating the lockfile.
> >> While this works for the files reference backend, reftable reference
> >> locks operate differently and are incompatible with this approach.
> >> Generalize reference locking by preparing a reference transaction.
> >
> > As with the first patch, I think we could use d/f conflicts to get the
> > same effect. Perhaps something like this:
>
> Thanks for a great alternative. I agree that avoiding fifo indeed
> is a better way to go.
|
User |
jltobler
force-pushed
the
jt/backend-generic-ref-lock
branch
2 times, most recently
from
1b7ed51
to
acde71a
Compare
To create error conditions, some tests set up reference locks by directly creating its lockfile. While this works for the files reference backend, this approach is incompatible with the reftable backend. Refactor the test to create a d/f conflict via git-symbolic-ref(1) instead so that the test is reference backend agnostic. Signed-off-by: Justin Tobler <jltobler@gmail.com>
To create error conditions, some tests set up reference locks by directly creating its lockfile. While this works for the files reference backend, this approach is incompatible with the reftable backend. Refactor the test to create a d/f conflict via git-update-ref(1) instead so that the test is reference backend agnostic. Signed-off-by: Justin Tobler <jltobler@gmail.com>
jltobler
force-pushed
the
jt/backend-generic-ref-lock
branch
from
acde71a
to
f953a66
Compare
|
/submit |
|
Submitted as pull.1634.v2.git.1705004670.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Justin Tobler wrote (reply to this): Junio C Hamano <gitster@pobox.com> writes:
> The use of two fifos and avoiding deadlocking parent and child is
> tricky enough that it does feel that it warrants a helper function
> to do the common part of what these two patches add.
To avoid the trickiness that comes with introducing fifos to these
tests, in the next patch version I've followed the suggestion of Peff to
instead create d/f conflicts to create an error condition. It appears
that these tests are not particular to the exact error condition being
triggered and therefore d/f conflicts are much simpler to produce.
Thanks,
Justin
On Wed, Jan 10, 2024 at 6:36 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Justin Tobler via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > This approach is more verbose and may warrant consideration of implementing
> > an abstraction to further simplify this operation. There appears to be one
> > other existing test in t1400 that also follows this pattern. Being that
> > there is only a handful of affected tests the implemented approach may be
> > sufficient as is.
>
> The use of two fifos and avoiding deadlocking parent and child is
> tricky enough that it does feel that it warrants a helper function
> to do the common part of what these two patches add.
>
> I think I read t1401 patch carefully enough to understand what is
> going on, but I cannot yet claim the same for the other one.
>
> Thanks.
>
> > Justin Tobler (2):
> > t1401: generalize reference locking
> > t5541: generalize reference locking
> >
> > t/t1401-symbolic-ref.sh | 28 ++++++++++++++++++++++++----
> > t/t5541-http-push-smart.sh | 25 +++++++++++++++++++++++--
> > 2 files changed, 47 insertions(+), 6 deletions(-)
> >
> >
> > base-commit: 624eb90fa8f65a79396615f3c2842ac5a3743350
> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1634%2Fjltobler%2Fjt%2Fbackend-generic-ref-lock-v1
> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1634/jltobler/jt/backend-generic-ref-lock-v1
> > Pull-Request: https://github.com/gitgitgadget/git/pull/1634 |
| @@ -232,27 +232,19 @@ test_expect_success 'push --atomic fails on server-side errors' ' | |||
| test_config -C "$d" http.receivepack true && | |||
There was a problem hiding this comment.
On the Git mailing list, Jeff King wrote (reply to this):
On Thu, Jan 11, 2024 at 08:24:30PM +0000, Justin Tobler via GitGitGadget wrote:
> - # the new branch should not have been created upstream
> - test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
> -
> - # upstream should still reflect atomic2, the last thing we pushed
> - # successfully
> - git rev-parse atomic2 >expected &&
> - # ...to other.
> - git -C "$d" rev-parse refs/heads/other >actual &&
> - test_cmp expected actual &&
> -
> - # the new branch should not have been created upstream
> + # The atomic and other branches should be created upstream.
> test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
> + test_must_fail git -C "$d" show-ref --verify refs/heads/other &&
This last comment should say "should not be created", I think?
Other than that, both patches look good to me.
-PeffThere was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Jeff King <peff@peff.net> writes:
> On Thu, Jan 11, 2024 at 08:24:30PM +0000, Justin Tobler via GitGitGadget wrote:
>
>> - # the new branch should not have been created upstream
>> - test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
>> -
>> - # upstream should still reflect atomic2, the last thing we pushed
>> - # successfully
>> - git rev-parse atomic2 >expected &&
>> - # ...to other.
>> - git -C "$d" rev-parse refs/heads/other >actual &&
>> - test_cmp expected actual &&
>> -
>> - # the new branch should not have been created upstream
>> + # The atomic and other branches should be created upstream.
>> test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
>> + test_must_fail git -C "$d" show-ref --verify refs/heads/other &&
>
> This last comment should say "should not be created", I think?
>
> Other than that, both patches look good to me.
Thanks. Will queue with the following and "Acked-by: peff".
diff --git c/t/t5541-http-push-smart.sh w/t/t5541-http-push-smart.sh
index 9a8bed6c32..71428f3d5c 100755
--- c/t/t5541-http-push-smart.sh
+++ w/t/t5541-http-push-smart.sh
@@ -242,7 +242,7 @@ test_expect_success 'push --atomic fails on server-side errors' '
# --atomic should cause entire push to be rejected
test_must_fail git push --atomic "$up" atomic other 2>output &&
- # The atomic and other branches should be created upstream.
+ # The atomic and other branches should not be created upstream.
test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
test_must_fail git -C "$d" show-ref --verify refs/heads/other &&
There was a problem hiding this comment.
On the Git mailing list, Justin Tobler wrote (reply to this):
Great! Thanks everyone for the help!
-Justin
On Fri, Jan 12, 2024 at 11:58 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Jeff King <peff@peff.net> writes:
>
> > On Thu, Jan 11, 2024 at 08:24:30PM +0000, Justin Tobler via GitGitGadget wrote:
> >
> >> - # the new branch should not have been created upstream
> >> - test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
> >> -
> >> - # upstream should still reflect atomic2, the last thing we pushed
> >> - # successfully
> >> - git rev-parse atomic2 >expected &&
> >> - # ...to other.
> >> - git -C "$d" rev-parse refs/heads/other >actual &&
> >> - test_cmp expected actual &&
> >> -
> >> - # the new branch should not have been created upstream
> >> + # The atomic and other branches should be created upstream.
> >> test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
> >> + test_must_fail git -C "$d" show-ref --verify refs/heads/other &&
> >
> > This last comment should say "should not be created", I think?
> >
> > Other than that, both patches look good to me.
>
> Thanks. Will queue with the following and "Acked-by: peff".
>
> diff --git c/t/t5541-http-push-smart.sh w/t/t5541-http-push-smart.sh
> index 9a8bed6c32..71428f3d5c 100755
> --- c/t/t5541-http-push-smart.sh
> +++ w/t/t5541-http-push-smart.sh
> @@ -242,7 +242,7 @@ test_expect_success 'push --atomic fails on server-side errors' '
> # --atomic should cause entire push to be rejected
> test_must_fail git push --atomic "$up" atomic other 2>output &&
>
> - # The atomic and other branches should be created upstream.
> + # The atomic and other branches should not be created upstream.
> test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
> test_must_fail git -C "$d" show-ref --verify refs/heads/other &&
>
>
|
This branch is now known as |
|
This patch series was integrated into seen via git@a03f0ae. |
|
This patch series was integrated into seen via git@c5d458a. |
|
There was a status update in the "New Topics" section about the branch Tweak a few tests not to manually modify the reference database (hence easier to work with other backends like reftable). Will merge to 'next'. source: <pull.1634.v2.git.1705004670.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@8c1a94c. |
|
This patch series was integrated into seen via git@3ca3bfd. |
|
This patch series was integrated into seen via git@485b381. |
|
This patch series was integrated into seen via git@838b13c. |
|
This patch series was integrated into next via git@498d203. |
|
There was a status update in the "Cooking" section about the branch Tweak a few tests not to manually modify the reference database (hence easier to work with other backends like reftable). Will merge to 'master'. source: <pull.1634.v2.git.1705004670.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@ca7b0a4. |
|
This patch series was integrated into seen via git@7aa290a. |
|
This patch series was integrated into seen via git@e5588f4. |
|
There was a status update in the "Cooking" section about the branch Tweak a few tests not to manually modify the reference database (hence easier to work with other backends like reftable). Will merge to 'master'. source: <pull.1634.v2.git.1705004670.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@d8af71b. |
|
This patch series was integrated into seen via git@1e74d42. |
|
This patch series was integrated into seen via git@bb98703. |
|
This patch series was integrated into master via git@bb98703. |
|
This patch series was integrated into next via git@bb98703. |
|
Closed via bb98703. |
Hello again,
This is the second version my patch series that refactors two tests to no longer be dependent on the files reference backend. Changes compared to v1:
Thanks for taking a look!
Justin
cc: Jeff King peff@peff.net
cc: Patrick Steinhardt ps@pks.im