From: Patrick Steinhardt <hidden> Date: 2020-11-04 14:57:24
Hi,
A few months back, I've implemented a more interactive protocol for
`git-update-ref --stdin`, which converted it to read commands linewise
and allow for more control via a set of new controlling commands. One
thing that was noticeably missing was the ability to create multiple
transactions in a single git-update-ref invocation.
As Peff rightly pointed out a few months back, we're actually almost
there to have that. So I took a look today and finally decided to
implement it while I was busy implementing another series. And in fact,
it really didn't require a lof of changes.
So with this small series, it's now possible to start new transaction
after the current transaction has either been committed or aborted. E.g.
sessions like the following are now possible:
> start
start: ok
> create refs/heads/test $A
> commit
commit: ok
> start
start: ok
> update refs/heads/test $B $A
> commit
commit: ok
> start
start: ok
> update refs/heads/test $A $B
> abort
abort: ok
> start
start: ok
> delete refs/heads/test $B
> commit
commit: ok
I've also updated the performance test in p1400 to use this new
mechanism, which is a much more direct way to test what we want to test
in there compared to using not-atomic pushes.
Patrick
Patrick Steinhardt (2):
update-ref: Allow creation of multiple transactions
p1400: Use `git-update-ref --stdin` to test multiple transactions
Documentation/git-update-ref.txt | 3 +-
builtin/update-ref.c | 13 ++++++++-
t/perf/p1400-update-ref.sh | 20 +++++--------
t/t1400-update-ref.sh | 50 ++++++++++++++++++++++++++++++++
4 files changed, 71 insertions(+), 15 deletions(-)
--
2.29.2
From: Patrick Steinhardt <hidden> Date: 2020-11-04 14:57:24
While git-update-ref has recently grown commands which allow interactive
control of transactions in e48cf33b61 (update-ref: implement interactive
transaction handling, 2020-04-02), it is not yet possible to create
multiple transactions in a single session. To do so, one currently still
needs to invoke the executable multiple times.
This commit addresses this shortcoming by allowing the "start" command
to create a new transaction if the current transaction has already been
either committed or aborted.
Signed-off-by: Patrick Steinhardt <redacted>
---
Documentation/git-update-ref.txt | 3 +-
builtin/update-ref.c | 13 ++++++++-
t/t1400-update-ref.sh | 50 ++++++++++++++++++++++++++++++++
3 files changed, 64 insertions(+), 2 deletions(-)
@@ -125,7 +125,8 @@ option:: start:: Start a transaction. In contrast to a non-transactional session, a transaction will automatically abort if the session ends without an- explicit commit.+ explicit commit. This command may create a new empty transaction when+ the current one has been committed or aborted already. prepare:: Prepare to commit the transaction. This will create lock files for all
@@ -1526,4 +1526,54 @@ test_expect_success 'transaction with prepare aborts by default' 'test_path_is_missing.git/$b'+test_expect_success'transaction can commit multiple times''+cat>stdin<<-EOF&&+start+createrefs/heads/branch-1$A+commit+start+createrefs/heads/branch-2$B+commit+EOF+gitupdate-ref--stdin<stdin>actual&&+printf"%s: ok\n"startcommitstartcommit>expect&&+test_cmpexpectactual&&+echo"$A">expect&&+gitrev-parserefs/heads/branch-1>actual&&+test_cmpexpectactual&&+echo"$B">expect&&+gitrev-parserefs/heads/branch-2>actual&&+test_cmpexpectactual+'++test_expect_success'transaction can create and delete''+cat>stdin<<-EOF&&+start+createrefs/heads/create-and-delete$A+commit+start+deleterefs/heads/create-and-delete$A+commit+EOF+gitupdate-ref--stdin<stdin>actual&&+printf"%s: ok\n"startcommitstartcommit>expect&&+test_path_is_missing.git/refs/heads/create-and-delete+'++test_expect_success'transaction can commit after abort''+cat>stdin<<-EOF&&+start+createrefs/heads/abort$A+abort+start+createrefs/heads/abort$A+commit+EOF+gitupdate-ref--stdin<stdin>actual&&+printf"%s: ok\n"startabortstartcommit>expect&&+echo"$A">expect&&+gitrev-parserefs/heads/abort>actual&&+test_cmpexpectactual+'+ test_done
From: Patrick Steinhardt <hidden> Date: 2020-11-04 14:57:33
In commit 0a0fbbe3ff (refs: remove lookup cache for
reference-transaction hook, 2020-08-25), a new benchmark was added to
p1400 which has the intention to exercise creation of multiple
transactions in a single process. As git-update-ref wasn't yet able to
create multiple transactions with a single run we instead used git-push.
As its non-atomic version creates a transaction per reference update,
this was the best approximation we could make at that point in time.
Now that `git-update-ref --stdin` supports creation of multiple
transactions, let's convert the benchmark to use that instead. It has
less overhead and it's also a lot clearer what the actual intention is.
Signed-off-by: Patrick Steinhardt <redacted>
---
t/perf/p1400-update-ref.sh | 20 +++++++-------------
1 file changed, 7 insertions(+), 13 deletions(-)
From: Jeff King <hidden> Date: 2020-11-05 19:29:03
On Wed, Nov 04, 2020 at 03:57:17PM +0100, Patrick Steinhardt wrote:
While git-update-ref has recently grown commands which allow interactive
control of transactions in e48cf33b61 (update-ref: implement interactive
transaction handling, 2020-04-02), it is not yet possible to create
multiple transactions in a single session. To do so, one currently still
needs to invoke the executable multiple times.
This commit addresses this shortcoming by allowing the "start" command
to create a new transaction if the current transaction has already been
either committed or aborted.
Thanks for working on this. The amount of change needed is indeed quite
pleasant.
@@ -125,7 +125,8 @@ option:: start:: Start a transaction. In contrast to a non-transactional session, a transaction will automatically abort if the session ends without an- explicit commit.+ explicit commit. This command may create a new empty transaction when+ the current one has been committed or aborted already.
Reading this made me wonder what would happen if we send a "start" when
the current one _hasn't_ been committed or aborted. I.e., what does:
git update-ref --stdin <<EOF
start
create refs/heads/foo ...
start
commit
EOF
do? It turns out that the second start is ignored totally (and the
commit does indeed update foo). I wonder if we ought to complain about
it. But that is completely orthogonal to your patch. The behavior is the
same before and after.
@@ -385,14 +385,10 @@ static const struct parse_cmd {staticvoidupdate_refs_stdin(void){structstrbufinput=STRBUF_INIT,err=STRBUF_INIT;-enumupdate_refs_statestate=UPDATE_REFS_OPEN;+enumupdate_refs_statestate=UPDATE_REFS_CLOSED;structref_transaction*transaction;inti,j;-transaction=ref_transaction_begin(&err);-if(!transaction)-die("%s",err.buf);-/* Read each line dispatch its command */while(!strbuf_getwholeline(&input,stdin,line_termination)){conststructparse_cmd*cmd=NULL;
and just have it auto-open. But of course that doesn't work because we
might not see an "open" command at all. Traditional callers will start
with create/update/etc, and our "auto-open" would complain.
The tests all look quite reasonable to me. Touching .git/refs like this
is a bit gross (and something we may have to deal with if we introduce
reftables, etc). But it's pretty pervasive in this file, so matching
the existing style is the best option for now.
-Peff
From: Jeff King <hidden> Date: 2020-11-05 19:34:58
On Wed, Nov 04, 2020 at 03:57:22PM +0100, Patrick Steinhardt wrote:
In commit 0a0fbbe3ff (refs: remove lookup cache for
reference-transaction hook, 2020-08-25), a new benchmark was added to
p1400 which has the intention to exercise creation of multiple
transactions in a single process. As git-update-ref wasn't yet able to
create multiple transactions with a single run we instead used git-push.
As its non-atomic version creates a transaction per reference update,
this was the best approximation we could make at that point in time.
Now that `git-update-ref --stdin` supports creation of multiple
transactions, let's convert the benchmark to use that instead. It has
less overhead and it's also a lot clearer what the actual intention is.
Good direction. The diff confused me for a moment...
...because we're dropping _two_ tests here. But I think they were
testing the same thing, just with varying degrees of quality.
It could possibly be useful to have perf results broken down by
operation type (create vs delete vs update), but the original certainly
didn't do that. And it's not clear to me it would actually produce
interesting results; certainly not related to the hook, but possibly
related to benchmarking ref updates in general. So I don't think it's
worth worrying about.
-Peff
From: Patrick Steinhardt <hidden> Date: 2020-11-06 06:37:03
On Thu, Nov 05, 2020 at 02:29:01PM -0500, Jeff King wrote:
On Wed, Nov 04, 2020 at 03:57:17PM +0100, Patrick Steinhardt wrote:
quoted
While git-update-ref has recently grown commands which allow interactive
control of transactions in e48cf33b61 (update-ref: implement interactive
transaction handling, 2020-04-02), it is not yet possible to create
multiple transactions in a single session. To do so, one currently still
needs to invoke the executable multiple times.
This commit addresses this shortcoming by allowing the "start" command
to create a new transaction if the current transaction has already been
either committed or aborted.
Thanks for working on this. The amount of change needed is indeed quite
pleasant.
@@ -125,7 +125,8 @@ option:: start:: Start a transaction. In contrast to a non-transactional session, a transaction will automatically abort if the session ends without an- explicit commit.+ explicit commit. This command may create a new empty transaction when+ the current one has been committed or aborted already.
Reading this made me wonder what would happen if we send a "start" when
the current one _hasn't_ been committed or aborted. I.e., what does:
git update-ref --stdin <<EOF
start
create refs/heads/foo ...
start
commit
EOF
do? It turns out that the second start is ignored totally (and the
commit does indeed update foo). I wonder if we ought to complain about
it. But that is completely orthogonal to your patch. The behavior is the
same before and after.
Agreed, that's a case where we should raise an error. Doing nothing
without any indication is a bad way of handling it.
Patrick
From: Patrick Steinhardt <hidden> Date: 2020-11-09 10:06:52
Hi,
this is the second version of this patch series implementing support for
creation of multiple reference transactions in a single git-update-ref
process.
There's two changes compared to v1:
- A small refactoring of t1400, which refactors many tests to not
touch references via the filesystem but instead to use
git-update-ref and git-show-ref. There's still tests in there
which do, but converting them is harder as they create and read
broken references.
- I've added another commit on top which disallows restarting of
transactions. E.g. writing "start\nstart\n" to git-update-ref is
now going to fail.
Patrick
Patrick Steinhardt (4):
t1400: Avoid touching refs on filesystem
update-ref: Allow creation of multiple transactions
p1400: Use `git-update-ref --stdin` to test multiple transactions
update-ref: Disallow restart of ongoing transactions
Documentation/git-update-ref.txt | 3 +-
builtin/update-ref.c | 15 +++-
t/perf/p1400-update-ref.sh | 20 ++---
t/t1400-update-ref.sh | 124 ++++++++++++++++++++++++-------
4 files changed, 119 insertions(+), 43 deletions(-)
--
2.29.2
From: Patrick Steinhardt <hidden> Date: 2020-11-09 10:06:54
The testcase t1400 exercises the git-update-ref(1) utility. To do so,
many tests directly read and write references via the filesystem,
assuming that we always use loose and/or packed references. While this
is true now, it'll change with the introduction of the reftable backend.
Convert those tests to use git-update-ref(1) and git-show-ref(1) where
possible. As some tests exercise behaviour with broken references and
neither of those tools actually allows writing or reading broken
references, this commit doesn't adjust all tests.
Signed-off-by: Patrick Steinhardt <redacted>
---
t/t1400-update-ref.sh | 63 ++++++++++++++++++++++++-------------------
1 file changed, 35 insertions(+), 28 deletions(-)
@@ -14,6 +14,12 @@ n=$n_dir/fixesoutside=refs/foobare=bare-repo+# Some of the tests delete HEAD, which causes us to not treat the current+# working directory as a Git repository anymore. To avoid using any potential+# parent repository to be discovered, we need to set up the ceiling directories.+GIT_CEILING_DIRECTORIES="$PWD/.."+exportGIT_CEILING_DIRECTORIES+ create_test_commits(){prfx="$1"
@@ -48,17 +54,17 @@ test_expect_success "fail to delete $m with stale ref" 'test$B="$(gitshow-ref-s--verify$m)"' test_expect_success"delete $m"'-test_when_finished"rm -f .git/$m"&&+test_when_finished"git update-ref -d $m"&&gitupdate-ref-d$m$B&&-test_path_is_missing.git/$m+test_must_failgitshow-ref--verify-q$m' test_expect_success"delete $m without oldvalue verification"'-test_when_finished"rm -f .git/$m"&&+test_when_finished"git update-ref -d $m"&&gitupdate-ref$m$A&&test$A=$(gitshow-ref-s--verify$m)&&gitupdate-ref-d$m&&-test_path_is_missing.git/$m+test_must_failgitshow-ref--verify-q$m' test_expect_success"fail to create $n"'
@@ -80,26 +86,26 @@ test_expect_success "fail to delete $m (by HEAD) with stale ref" 'test$B=$(gitshow-ref-s--verify$m)' test_expect_success"delete $m (by HEAD)"'-test_when_finished"rm -f .git/$m"&&+test_when_finished"git update-ref -d $m"&&gitupdate-ref-dHEAD$B&&-test_path_is_missing.git/$m+test_must_failgitshow-ref--verify-q$m' test_expect_success"deleting current branch adds message to HEAD's log"'-test_when_finished"rm -f .git/$m"&&+test_when_finished"git update-ref -d $m"&&gitupdate-ref$m$A&&gitsymbolic-refHEAD$m&&gitupdate-ref-mdelete-$m-d$m&&-test_path_is_missing.git/$m&&+test_must_failgitshow-ref--verify-q$m&&grep"delete-$m$".git/logs/HEAD' test_expect_success"deleting by HEAD adds message to HEAD's log"'-test_when_finished"rm -f .git/$m"&&+test_when_finished"git update-ref -d $m"&&gitupdate-ref$m$A&&gitsymbolic-refHEAD$m&&gitupdate-ref-mdelete-by-head-dHEAD&&-test_path_is_missing.git/$m&&+test_must_failgitshow-ref--verify-q$m&&grep"delete-by-head$".git/logs/HEAD'
@@ -188,17 +194,17 @@ test_expect_success "move $m (by HEAD)" 'test$B=$(gitshow-ref-s--verify$m)' test_expect_success"delete $m (by HEAD) should remove both packed and loose $m"'-test_when_finished"rm -f .git/$m"&&+test_when_finished"rm -f git update-ref -d $m"&&gitupdate-ref-dHEAD$B&&!grep"$m".git/packed-refs&&-test_path_is_missing.git/$m+test_must_failgitshow-ref--verify-q$m' cp-f.git/HEAD.git/HEAD.orig test_expect_success'delete symref without dereference''test_when_finished"cp -f .git/HEAD.orig .git/HEAD"&&gitupdate-ref--no-deref-dHEAD&&-test_path_is_missing.git/HEAD+test_must_failgitshow-ref--verify-qHEAD' test_expect_success'delete symref without dereference when the referred ref is packed''
@@ -208,7 +214,7 @@ test_expect_success 'delete symref without dereference when the referred ref isgitcommit-mfoo&&gitpack-refs--all&&gitupdate-ref--no-deref-dHEAD&&-test_path_is_missing.git/HEAD+test_must_failgitshow-ref--verify-qHEAD' gitupdate-ref-d$m
@@ -226,25 +232,25 @@ test_expect_success 'update-ref --no-deref -d can delete self-reference' 'test_when_finished"rm -f .git/refs/heads/self"&&test_path_is_file.git/refs/heads/self&&gitupdate-ref--no-deref-drefs/heads/self&&-test_path_is_missing.git/refs/heads/self+test_must_failgitshow-ref--verify-qrefs/heads/self' test_expect_success'update-ref --no-deref -d can delete reference to bad ref''>.git/refs/heads/bad&&test_when_finished"rm -f .git/refs/heads/bad"&&gitsymbolic-refrefs/heads/ref-to-badrefs/heads/bad&&-test_when_finished"rm -f .git/refs/heads/ref-to-bad"&&+test_when_finished"git update-ref -d refs/heads/ref-to-bad"&&test_path_is_file.git/refs/heads/ref-to-bad&&gitupdate-ref--no-deref-drefs/heads/ref-to-bad&&-test_path_is_missing.git/refs/heads/ref-to-bad+test_must_failgitshow-ref--verify-qrefs/heads/ref-to-bad' test_expect_success'(not) create HEAD with old sha1''test_must_failgitupdate-refHEAD$A$B' test_expect_success"(not) prior created .git/$m"'-test_when_finished"rm -f .git/$m"&&-test_path_is_missing.git/$m+test_when_finished"git update-ref -d $m"&&+test_must_failgitshow-ref--verify-q$m' test_expect_success'create HEAD''
@@ -254,7 +260,7 @@ test_expect_success '(not) change HEAD with wrong SHA1' 'test_must_failgitupdate-refHEAD$B$Z' test_expect_success"(not) changed .git/$m"'-test_when_finished"rm -f .git/$m"&&+test_when_finished"git update-ref -d $m"&&!test$B=$(gitshow-ref-s--verify$m)'
From: Patrick Steinhardt <hidden> Date: 2020-11-09 10:06:58
While git-update-ref has recently grown commands which allow interactive
control of transactions in e48cf33b61 (update-ref: implement interactive
transaction handling, 2020-04-02), it is not yet possible to create
multiple transactions in a single session. To do so, one currently still
needs to invoke the executable multiple times.
This commit addresses this shortcoming by allowing the "start" command
to create a new transaction if the current transaction has already been
either committed or aborted.
Signed-off-by: Patrick Steinhardt <redacted>
---
Documentation/git-update-ref.txt | 3 +-
builtin/update-ref.c | 13 ++++++++-
t/t1400-update-ref.sh | 50 ++++++++++++++++++++++++++++++++
3 files changed, 64 insertions(+), 2 deletions(-)
@@ -125,7 +125,8 @@ option:: start:: Start a transaction. In contrast to a non-transactional session, a transaction will automatically abort if the session ends without an- explicit commit.+ explicit commit. This command may create a new empty transaction when+ the current one has been committed or aborted already. prepare:: Prepare to commit the transaction. This will create lock files for all
@@ -1533,4 +1533,54 @@ test_expect_success 'transaction with prepare aborts by default' 'test_must_failgitshow-ref--verify-q$b'+test_expect_success'transaction can commit multiple times''+cat>stdin<<-EOF&&+start+createrefs/heads/branch-1$A+commit+start+createrefs/heads/branch-2$B+commit+EOF+gitupdate-ref--stdin<stdin>actual&&+printf"%s: ok\n"startcommitstartcommit>expect&&+test_cmpexpectactual&&+echo"$A">expect&&+gitrev-parserefs/heads/branch-1>actual&&+test_cmpexpectactual&&+echo"$B">expect&&+gitrev-parserefs/heads/branch-2>actual&&+test_cmpexpectactual+'++test_expect_success'transaction can create and delete''+cat>stdin<<-EOF&&+start+createrefs/heads/create-and-delete$A+commit+start+deleterefs/heads/create-and-delete$A+commit+EOF+gitupdate-ref--stdin<stdin>actual&&+printf"%s: ok\n"startcommitstartcommit>expect&&+test_must_failgitshow-ref--verifyrefs/heads/create-and-delete+'++test_expect_success'transaction can commit after abort''+cat>stdin<<-EOF&&+start+createrefs/heads/abort$A+abort+start+createrefs/heads/abort$A+commit+EOF+gitupdate-ref--stdin<stdin>actual&&+printf"%s: ok\n"startabortstartcommit>expect&&+echo"$A">expect&&+gitrev-parserefs/heads/abort>actual&&+test_cmpexpectactual+'+ test_done
From: Patrick Steinhardt <hidden> Date: 2020-11-09 10:07:02
In commit 0a0fbbe3ff (refs: remove lookup cache for
reference-transaction hook, 2020-08-25), a new benchmark was added to
p1400 which has the intention to exercise creation of multiple
transactions in a single process. As git-update-ref wasn't yet able to
create multiple transactions with a single run we instead used git-push.
As its non-atomic version creates a transaction per reference update,
this was the best approximation we could make at that point in time.
Now that `git-update-ref --stdin` supports creation of multiple
transactions, let's convert the benchmark to use that instead. It has
less overhead and it's also a lot clearer what the actual intention is.
Signed-off-by: Patrick Steinhardt <redacted>
---
t/perf/p1400-update-ref.sh | 20 +++++++-------------
1 file changed, 7 insertions(+), 13 deletions(-)
From: Patrick Steinhardt <hidden> Date: 2020-11-09 10:07:06
It is currently possible to write multiple "start" commands into
git-update-ref(1) for a single session, which doesn't make a lot of
sense to do in the first place. It's also not quite obvious what should
actually happen. Would this just go on with the current transaction as
if nothing was written or would it recreate a new session which doesn't
yet have any references? Silently ignoring this usage isn't helping
either as it may indicate erroneous use of the interface.
This commit catches this use and instead raises an error if the user is
trying to restart an ongoing transaction.
Signed-off-by: Patrick Steinhardt <redacted>
---
builtin/update-ref.c | 2 ++
t/t1400-update-ref.sh | 11 +++++++++++
2 files changed, 13 insertions(+)
@@ -436,6 +436,8 @@ static void update_refs_stdin(void)switch(state){caseUPDATE_REFS_OPEN:caseUPDATE_REFS_STARTED:+if(state==UPDATE_REFS_STARTED&&cmd->state==UPDATE_REFS_STARTED)+die("cannot restart ongoing transaction");/* Do not downgrade a transaction to a non-transaction. */if(cmd->state>=state)state=cmd->state;
From: Jeff King <hidden> Date: 2020-11-09 22:33:32
On Mon, Nov 09, 2020 at 11:06:43AM +0100, Patrick Steinhardt wrote:
this is the second version of this patch series implementing support for
creation of multiple reference transactions in a single git-update-ref
process.
There's two changes compared to v1:
- A small refactoring of t1400, which refactors many tests to not
touch references via the filesystem but instead to use
git-update-ref and git-show-ref. There's still tests in there
which do, but converting them is harder as they create and read
broken references.
- I've added another commit on top which disallows restarting of
transactions. E.g. writing "start\nstart\n" to git-update-ref is
now going to fail.
Thanks. Aside from the issues raised by Junio, this all looks good to me
(and I agree on the fourth one it is just a matter of the commit
message; what the code is doing is a definite improvement).
-Peff
From: Patrick Steinhardt <hidden> Date: 2020-11-11 07:05:24
Hi,
this is the third version of this patch series implementing support for
creation of multiple reference transactions in a single git-update-ref
process.
There's three changes compared to v2:
- Commit subjects have been changed to start with a lowercase.
- The commit message in patch 4 has been improved.
- t1400 has been changed to not delete HEAD anymore.
Patrick
Patrick Steinhardt (4):
t1400: avoid touching refs on filesystem
update-ref: allow creation of multiple transactions
p1400: use `git-update-ref --stdin` to test multiple transactions
update-ref: disallow "start" for ongoing transactions
Documentation/git-update-ref.txt | 3 +-
builtin/update-ref.c | 15 +++-
t/perf/p1400-update-ref.sh | 20 ++---
t/t1400-update-ref.sh | 133 +++++++++++++++++++++++--------
4 files changed, 123 insertions(+), 48 deletions(-)
--
2.29.2
From: Patrick Steinhardt <hidden> Date: 2020-11-11 07:05:26
The testcase t1400 exercises the git-update-ref(1) utility. To do so,
many tests directly read and write references via the filesystem,
assuming that we always use loose and/or packed references. While this
is true now, it'll change with the introduction of the reftable backend.
Convert those tests to use git-update-ref(1) and git-show-ref(1) where
possible. As some tests exercise behaviour with broken references and
neither of those tools actually allows writing or reading broken
references, this commit doesn't adjust all tests.
Signed-off-by: Patrick Steinhardt <redacted>
---
t/t1400-update-ref.sh | 72 +++++++++++++++++++++++--------------------
1 file changed, 39 insertions(+), 33 deletions(-)
@@ -14,6 +14,12 @@ n=$n_dir/fixesoutside=refs/foobare=bare-repo+# Some of the tests delete HEAD, which causes us to not treat the current+# working directory as a Git repository anymore. To avoid using any potential+# parent repository to be discovered, we need to set up the ceiling directories.+GIT_CEILING_DIRECTORIES="$PWD/.."+exportGIT_CEILING_DIRECTORIES+ create_test_commits(){prfx="$1"
@@ -48,17 +54,17 @@ test_expect_success "fail to delete $m with stale ref" 'test$B="$(gitshow-ref-s--verify$m)"' test_expect_success"delete $m"'-test_when_finished"rm -f .git/$m"&&+test_when_finished"git update-ref -d $m"&&gitupdate-ref-d$m$B&&-test_path_is_missing.git/$m+test_must_failgitshow-ref--verify-q$m' test_expect_success"delete $m without oldvalue verification"'-test_when_finished"rm -f .git/$m"&&+test_when_finished"git update-ref -d $m"&&gitupdate-ref$m$A&&test$A=$(gitshow-ref-s--verify$m)&&gitupdate-ref-d$m&&-test_path_is_missing.git/$m+test_must_failgitshow-ref--verify-q$m' test_expect_success"fail to create $n"'
@@ -80,26 +86,26 @@ test_expect_success "fail to delete $m (by HEAD) with stale ref" 'test$B=$(gitshow-ref-s--verify$m)' test_expect_success"delete $m (by HEAD)"'-test_when_finished"rm -f .git/$m"&&+test_when_finished"git update-ref -d $m"&&gitupdate-ref-dHEAD$B&&-test_path_is_missing.git/$m+test_must_failgitshow-ref--verify-q$m' test_expect_success"deleting current branch adds message to HEAD's log"'-test_when_finished"rm -f .git/$m"&&+test_when_finished"git update-ref -d $m"&&gitupdate-ref$m$A&&gitsymbolic-refHEAD$m&&gitupdate-ref-mdelete-$m-d$m&&-test_path_is_missing.git/$m&&+test_must_failgitshow-ref--verify-q$m&&grep"delete-$m$".git/logs/HEAD' test_expect_success"deleting by HEAD adds message to HEAD's log"'-test_when_finished"rm -f .git/$m"&&+test_when_finished"git update-ref -d $m"&&gitupdate-ref$m$A&&gitsymbolic-refHEAD$m&&gitupdate-ref-mdelete-by-head-dHEAD&&-test_path_is_missing.git/$m&&+test_must_failgitshow-ref--verify-q$m&&grep"delete-by-head$".git/logs/HEAD'
@@ -188,27 +194,26 @@ test_expect_success "move $m (by HEAD)" 'test$B=$(gitshow-ref-s--verify$m)' test_expect_success"delete $m (by HEAD) should remove both packed and loose $m"'-test_when_finished"rm -f .git/$m"&&+test_when_finished"rm -f git update-ref -d $m"&&gitupdate-ref-dHEAD$B&&!grep"$m".git/packed-refs&&-test_path_is_missing.git/$m+test_must_failgitshow-ref--verify-q$m'-cp-f.git/HEAD.git/HEAD.orig test_expect_success'delete symref without dereference''-test_when_finished"cp -f .git/HEAD.orig .git/HEAD"&&-gitupdate-ref--no-deref-dHEAD&&-test_path_is_missing.git/HEAD+gitsymbolic-refSYMREF$m&&+gitupdate-ref--no-deref-dSYMREF&&+test_must_failgitshow-ref--verify-qSYMREF' test_expect_success'delete symref without dereference when the referred ref is packed''-test_when_finished"cp -f .git/HEAD.orig .git/HEAD"&&echofoo>foo.c&&gitaddfoo.c&&gitcommit-mfoo&&+gitsymbolic-refSYMREF$m&&gitpack-refs--all&&-gitupdate-ref--no-deref-dHEAD&&-test_path_is_missing.git/HEAD+gitupdate-ref--no-deref-dSYMREF&&+test_must_failgitshow-ref--verify-qSYMREF' gitupdate-ref-d$m
@@ -226,25 +231,25 @@ test_expect_success 'update-ref --no-deref -d can delete self-reference' 'test_when_finished"rm -f .git/refs/heads/self"&&test_path_is_file.git/refs/heads/self&&gitupdate-ref--no-deref-drefs/heads/self&&-test_path_is_missing.git/refs/heads/self+test_must_failgitshow-ref--verify-qrefs/heads/self' test_expect_success'update-ref --no-deref -d can delete reference to bad ref''>.git/refs/heads/bad&&test_when_finished"rm -f .git/refs/heads/bad"&&gitsymbolic-refrefs/heads/ref-to-badrefs/heads/bad&&-test_when_finished"rm -f .git/refs/heads/ref-to-bad"&&+test_when_finished"git update-ref -d refs/heads/ref-to-bad"&&test_path_is_file.git/refs/heads/ref-to-bad&&gitupdate-ref--no-deref-drefs/heads/ref-to-bad&&-test_path_is_missing.git/refs/heads/ref-to-bad+test_must_failgitshow-ref--verify-qrefs/heads/ref-to-bad' test_expect_success'(not) create HEAD with old sha1''test_must_failgitupdate-refHEAD$A$B' test_expect_success"(not) prior created .git/$m"'-test_when_finished"rm -f .git/$m"&&-test_path_is_missing.git/$m+test_when_finished"git update-ref -d $m"&&+test_must_failgitshow-ref--verify-q$m' test_expect_success'create HEAD''
@@ -254,7 +259,7 @@ test_expect_success '(not) change HEAD with wrong SHA1' 'test_must_failgitupdate-refHEAD$B$Z' test_expect_success"(not) changed .git/$m"'-test_when_finished"rm -f .git/$m"&&+test_when_finished"git update-ref -d $m"&&!test$B=$(gitshow-ref-s--verify$m)'
From: Patrick Steinhardt <hidden> Date: 2020-11-11 07:05:29
While git-update-ref has recently grown commands which allow interactive
control of transactions in e48cf33b61 (update-ref: implement interactive
transaction handling, 2020-04-02), it is not yet possible to create
multiple transactions in a single session. To do so, one currently still
needs to invoke the executable multiple times.
This commit addresses this shortcoming by allowing the "start" command
to create a new transaction if the current transaction has already been
either committed or aborted.
Signed-off-by: Patrick Steinhardt <redacted>
---
Documentation/git-update-ref.txt | 3 +-
builtin/update-ref.c | 13 ++++++++-
t/t1400-update-ref.sh | 50 ++++++++++++++++++++++++++++++++
3 files changed, 64 insertions(+), 2 deletions(-)
@@ -125,7 +125,8 @@ option:: start:: Start a transaction. In contrast to a non-transactional session, a transaction will automatically abort if the session ends without an- explicit commit.+ explicit commit. This command may create a new empty transaction when+ the current one has been committed or aborted already. prepare:: Prepare to commit the transaction. This will create lock files for all
@@ -1532,4 +1532,54 @@ test_expect_success 'transaction with prepare aborts by default' 'test_must_failgitshow-ref--verify-q$b'+test_expect_success'transaction can commit multiple times''+cat>stdin<<-EOF&&+start+createrefs/heads/branch-1$A+commit+start+createrefs/heads/branch-2$B+commit+EOF+gitupdate-ref--stdin<stdin>actual&&+printf"%s: ok\n"startcommitstartcommit>expect&&+test_cmpexpectactual&&+echo"$A">expect&&+gitrev-parserefs/heads/branch-1>actual&&+test_cmpexpectactual&&+echo"$B">expect&&+gitrev-parserefs/heads/branch-2>actual&&+test_cmpexpectactual+'++test_expect_success'transaction can create and delete''+cat>stdin<<-EOF&&+start+createrefs/heads/create-and-delete$A+commit+start+deleterefs/heads/create-and-delete$A+commit+EOF+gitupdate-ref--stdin<stdin>actual&&+printf"%s: ok\n"startcommitstartcommit>expect&&+test_must_failgitshow-ref--verifyrefs/heads/create-and-delete+'++test_expect_success'transaction can commit after abort''+cat>stdin<<-EOF&&+start+createrefs/heads/abort$A+abort+start+createrefs/heads/abort$A+commit+EOF+gitupdate-ref--stdin<stdin>actual&&+printf"%s: ok\n"startabortstartcommit>expect&&+echo"$A">expect&&+gitrev-parserefs/heads/abort>actual&&+test_cmpexpectactual+'+ test_done
From: Patrick Steinhardt <hidden> Date: 2020-11-11 07:05:32
In commit 0a0fbbe3ff (refs: remove lookup cache for
reference-transaction hook, 2020-08-25), a new benchmark was added to
p1400 which has the intention to exercise creation of multiple
transactions in a single process. As git-update-ref wasn't yet able to
create multiple transactions with a single run we instead used git-push.
As its non-atomic version creates a transaction per reference update,
this was the best approximation we could make at that point in time.
Now that `git-update-ref --stdin` supports creation of multiple
transactions, let's convert the benchmark to use that instead. It has
less overhead and it's also a lot clearer what the actual intention is.
Signed-off-by: Patrick Steinhardt <redacted>
---
t/perf/p1400-update-ref.sh | 20 +++++++-------------
1 file changed, 7 insertions(+), 13 deletions(-)
From: Patrick Steinhardt <hidden> Date: 2020-11-11 07:05:40
It is currently possible to write multiple "start" commands into
git-update-ref(1) for a single session, but none of them except for the
first one actually have any effect.
Using such nested "start"s may eventually have a sensible effect. One
may imagine that it restarts the current transaction, effectively
emptying it and creating a new one. It may also allow for creation of
nested transactions. But currently, none of these are implemented.
Silently ignoring this misuse is making it hard to iterate in the future
if "start" is ever going to have meaningful semantics in such a context.
This commit thus makes sure to error out in case we see such use.
Signed-off-by: Patrick Steinhardt <redacted>
---
builtin/update-ref.c | 2 ++
t/t1400-update-ref.sh | 11 +++++++++++
2 files changed, 13 insertions(+)
@@ -436,6 +436,8 @@ static void update_refs_stdin(void)switch(state){caseUPDATE_REFS_OPEN:caseUPDATE_REFS_STARTED:+if(state==UPDATE_REFS_STARTED&&cmd->state==UPDATE_REFS_STARTED)+die("cannot restart ongoing transaction");/* Do not downgrade a transaction to a non-transaction. */if(cmd->state>=state)state=cmd->state;
From: SZEDER Gábor <hidden> Date: 2020-11-11 09:05:09
On Wed, Nov 11, 2020 at 07:58:38AM +0100, Patrick Steinhardt wrote:
quoted hunk
The testcase t1400 exercises the git-update-ref(1) utility. To do so,
many tests directly read and write references via the filesystem,
assuming that we always use loose and/or packed references. While this
is true now, it'll change with the introduction of the reftable backend.
Convert those tests to use git-update-ref(1) and git-show-ref(1) where
possible. As some tests exercise behaviour with broken references and
neither of those tools actually allows writing or reading broken
references, this commit doesn't adjust all tests.
Signed-off-by: Patrick Steinhardt <redacted>
---
t/t1400-update-ref.sh | 72 +++++++++++++++++++++++--------------------
1 file changed, 39 insertions(+), 33 deletions(-)
From: Patrick Steinhardt <hidden> Date: 2020-11-11 10:00:25
On Wed, Nov 11, 2020 at 10:04:54AM +0100, SZEDER Gábor wrote:
On Wed, Nov 11, 2020 at 07:58:38AM +0100, Patrick Steinhardt wrote:
quoted
The testcase t1400 exercises the git-update-ref(1) utility. To do so,
many tests directly read and write references via the filesystem,
assuming that we always use loose and/or packed references. While this
is true now, it'll change with the introduction of the reftable backend.
Convert those tests to use git-update-ref(1) and git-show-ref(1) where
possible. As some tests exercise behaviour with broken references and
neither of those tools actually allows writing or reading broken
references, this commit doesn't adjust all tests.
Signed-off-by: Patrick Steinhardt <redacted>
---
t/t1400-update-ref.sh | 72 +++++++++++++++++++++++--------------------
1 file changed, 39 insertions(+), 33 deletions(-)
From: SZEDER Gábor <hidden> Date: 2020-11-11 10:24:18
On Wed, Nov 11, 2020 at 11:00:15AM +0100, Patrick Steinhardt wrote:
On Wed, Nov 11, 2020 at 10:04:54AM +0100, SZEDER Gábor wrote:
quoted
On Wed, Nov 11, 2020 at 07:58:38AM +0100, Patrick Steinhardt wrote:
quoted
The testcase t1400 exercises the git-update-ref(1) utility. To do so,
many tests directly read and write references via the filesystem,
assuming that we always use loose and/or packed references. While this
is true now, it'll change with the introduction of the reftable backend.
Convert those tests to use git-update-ref(1) and git-show-ref(1) where
possible. As some tests exercise behaviour with broken references and
neither of those tools actually allows writing or reading broken
references, this commit doesn't adjust all tests.
Signed-off-by: Patrick Steinhardt <redacted>
---
t/t1400-update-ref.sh | 72 +++++++++++++++++++++++--------------------
1 file changed, 39 insertions(+), 33 deletions(-)
Oops, thanks a lot for catching. Funny that it still passed.
The '-f' option makes it ignore missing files like 'git', 'update-ref'
and whatever $m happens to expand to in this test, and '/bin/rm' from
coreutils (and apparently on macOS) understands the '-d' option
("remove empty directories"), so there was no error to fail this test
on common setups. (I didn't investigate whether and how the not
deleted $m ref affects subsequent tests...) But nowadays we have a CI
job running the tests with 'busybox sh' whose builtin 'rm' doesn't
have a '-d' option, and it errors out with:
+ rm -f git update-ref -d refs/heads/master
rm: unrecognized option: d
BusyBox v1.31.1 () multi-call binary.
Usage: ......
https://travis-ci.org/github/git/git/jobs/742890453#L2853
From: Jeff King <hidden> Date: 2020-11-12 01:32:24
On Wed, Nov 11, 2020 at 07:58:38AM +0100, Patrick Steinhardt wrote:
The testcase t1400 exercises the git-update-ref(1) utility. To do so,
many tests directly read and write references via the filesystem,
assuming that we always use loose and/or packed references. While this
is true now, it'll change with the introduction of the reftable backend.
Convert those tests to use git-update-ref(1) and git-show-ref(1) where
possible. As some tests exercise behaviour with broken references and
neither of those tools actually allows writing or reading broken
references, this commit doesn't adjust all tests.
Do you want to mention the switch away from using HEAD in some tests
here?
+# Some of the tests delete HEAD, which causes us to not treat the current
+# working directory as a Git repository anymore. To avoid using any potential
+# parent repository to be discovered, we need to set up the ceiling directories.
+GIT_CEILING_DIRECTORIES="$PWD/.."
+export GIT_CEILING_DIRECTORIES
Do we still need this, now that we're not deleting HEAD? I think we do
still delete a branch via HEAD, but that should leave an unborn branch,
which is still a valid repo.
And now this one is safe. Good.
I wonder, though...is it still testing the same thing as the original?
This is not related to the use of SYMREF vs HEAD, but wouldn't show-ref
similarly fail if we had deleted $m, but left SYMREF in place (i.e., if
--no-deref didn't actually do anything)?
Perhaps this would be better:
# confirm that the pointed-to ref is still there
git show-ref --verify $m &&
# but our symref is not
test_must_fail git show-ref --verify SYMREF &&
test_must_fail git symbolic-ref SYMREF
From: Patrick Steinhardt <hidden> Date: 2020-11-13 08:08:56
On Wed, Nov 11, 2020 at 06:06:59PM -0500, Jeff King wrote:
On Wed, Nov 11, 2020 at 07:58:38AM +0100, Patrick Steinhardt wrote:
quoted
+# Some of the tests delete HEAD, which causes us to not treat the current
+# working directory as a Git repository anymore. To avoid using any potential
+# parent repository to be discovered, we need to set up the ceiling directories.
+GIT_CEILING_DIRECTORIES="$PWD/.."
+export GIT_CEILING_DIRECTORIES
Do we still need this, now that we're not deleting HEAD? I think we do
still delete a branch via HEAD, but that should leave an unborn branch,
which is still a valid repo.
And now this one is safe. Good.
I wonder, though...is it still testing the same thing as the original?
This is not related to the use of SYMREF vs HEAD, but wouldn't show-ref
similarly fail if we had deleted $m, but left SYMREF in place (i.e., if
--no-deref didn't actually do anything)?
Perhaps this would be better:
# confirm that the pointed-to ref is still there
git show-ref --verify $m &&
# but our symref is not
test_must_fail git show-ref --verify SYMREF &&
test_must_fail git symbolic-ref SYMREF
It would be, but I bailed at this point because we don't actually have
"$m" at this point. But agreed, i'll also include this into both tests.
Patrick
From: Patrick Steinhardt <hidden> Date: 2020-11-13 08:12:33
Hi,
this is the fourth version of this patch series implementing support for
creation of multiple reference transactions in a single git-update-ref
process.
All changes are only in the first patch updating t1400:
- A stale 'rm -f' was removed.
- The workaround around broken repos was removed as we no longer
delete HEAD.
- Tests have been improved to verify that deleting a symref won't
delete their target.
Patrick
Patrick Steinhardt (4):
t1400: avoid touching refs on filesystem
update-ref: allow creation of multiple transactions
p1400: use `git-update-ref --stdin` to test multiple transactions
update-ref: disallow "start" for ongoing transactions
Documentation/git-update-ref.txt | 3 +-
builtin/update-ref.c | 15 +++-
t/perf/p1400-update-ref.sh | 20 ++---
t/t1400-update-ref.sh | 138 +++++++++++++++++++++++--------
4 files changed, 126 insertions(+), 50 deletions(-)
--
2.29.2
From: Patrick Steinhardt <hidden> Date: 2020-11-13 08:12:38
The testcase t1400 exercises the git-update-ref(1) utility. To do so,
many tests directly read and write references via the filesystem,
assuming that we always use loose and/or packed references. While this
is true now, it'll change with the introduction of the reftable backend.
Convert those tests to use git-update-ref(1) and git-show-ref(1) where
possible. Furthermore, two tests are converted to not delete HEAD
anymore, as this results in a broken repository. They've instead been
updated to create a non-mandatory symybolic reference and delete that
one instead.
Some tests remain which exercise behaviour with broken references, which
cannot currently be converted to use regular git tooling.
Signed-off-by: Patrick Steinhardt <redacted>
---
t/t1400-update-ref.sh | 77 +++++++++++++++++++++++--------------------
1 file changed, 42 insertions(+), 35 deletions(-)
From: Patrick Steinhardt <hidden> Date: 2020-11-13 08:12:41
While git-update-ref has recently grown commands which allow interactive
control of transactions in e48cf33b61 (update-ref: implement interactive
transaction handling, 2020-04-02), it is not yet possible to create
multiple transactions in a single session. To do so, one currently still
needs to invoke the executable multiple times.
This commit addresses this shortcoming by allowing the "start" command
to create a new transaction if the current transaction has already been
either committed or aborted.
Signed-off-by: Patrick Steinhardt <redacted>
---
Documentation/git-update-ref.txt | 3 +-
builtin/update-ref.c | 13 ++++++++-
t/t1400-update-ref.sh | 50 ++++++++++++++++++++++++++++++++
3 files changed, 64 insertions(+), 2 deletions(-)
@@ -125,7 +125,8 @@ option:: start:: Start a transaction. In contrast to a non-transactional session, a transaction will automatically abort if the session ends without an- explicit commit.+ explicit commit. This command may create a new empty transaction when+ the current one has been committed or aborted already. prepare:: Prepare to commit the transaction. This will create lock files for all
@@ -1533,4 +1533,54 @@ test_expect_success 'transaction with prepare aborts by default' 'test_must_failgitshow-ref--verify-q$b'+test_expect_success'transaction can commit multiple times''+cat>stdin<<-EOF&&+start+createrefs/heads/branch-1$A+commit+start+createrefs/heads/branch-2$B+commit+EOF+gitupdate-ref--stdin<stdin>actual&&+printf"%s: ok\n"startcommitstartcommit>expect&&+test_cmpexpectactual&&+echo"$A">expect&&+gitrev-parserefs/heads/branch-1>actual&&+test_cmpexpectactual&&+echo"$B">expect&&+gitrev-parserefs/heads/branch-2>actual&&+test_cmpexpectactual+'++test_expect_success'transaction can create and delete''+cat>stdin<<-EOF&&+start+createrefs/heads/create-and-delete$A+commit+start+deleterefs/heads/create-and-delete$A+commit+EOF+gitupdate-ref--stdin<stdin>actual&&+printf"%s: ok\n"startcommitstartcommit>expect&&+test_must_failgitshow-ref--verifyrefs/heads/create-and-delete+'++test_expect_success'transaction can commit after abort''+cat>stdin<<-EOF&&+start+createrefs/heads/abort$A+abort+start+createrefs/heads/abort$A+commit+EOF+gitupdate-ref--stdin<stdin>actual&&+printf"%s: ok\n"startabortstartcommit>expect&&+echo"$A">expect&&+gitrev-parserefs/heads/abort>actual&&+test_cmpexpectactual+'+ test_done
From: Patrick Steinhardt <hidden> Date: 2020-11-13 08:12:45
In commit 0a0fbbe3ff (refs: remove lookup cache for
reference-transaction hook, 2020-08-25), a new benchmark was added to
p1400 which has the intention to exercise creation of multiple
transactions in a single process. As git-update-ref wasn't yet able to
create multiple transactions with a single run we instead used git-push.
As its non-atomic version creates a transaction per reference update,
this was the best approximation we could make at that point in time.
Now that `git-update-ref --stdin` supports creation of multiple
transactions, let's convert the benchmark to use that instead. It has
less overhead and it's also a lot clearer what the actual intention is.
Signed-off-by: Patrick Steinhardt <redacted>
---
t/perf/p1400-update-ref.sh | 20 +++++++-------------
1 file changed, 7 insertions(+), 13 deletions(-)
From: Patrick Steinhardt <hidden> Date: 2020-11-13 08:12:49
It is currently possible to write multiple "start" commands into
git-update-ref(1) for a single session, but none of them except for the
first one actually have any effect.
Using such nested "start"s may eventually have a sensible effect. One
may imagine that it restarts the current transaction, effectively
emptying it and creating a new one. It may also allow for creation of
nested transactions. But currently, none of these are implemented.
Silently ignoring this misuse is making it hard to iterate in the future
if "start" is ever going to have meaningful semantics in such a context.
This commit thus makes sure to error out in case we see such use.
Signed-off-by: Patrick Steinhardt <redacted>
---
builtin/update-ref.c | 2 ++
t/t1400-update-ref.sh | 11 +++++++++++
2 files changed, 13 insertions(+)
@@ -436,6 +436,8 @@ static void update_refs_stdin(void)switch(state){caseUPDATE_REFS_OPEN:caseUPDATE_REFS_STARTED:+if(state==UPDATE_REFS_STARTED&&cmd->state==UPDATE_REFS_STARTED)+die("cannot restart ongoing transaction");/* Do not downgrade a transaction to a non-transaction. */if(cmd->state>=state)state=cmd->state;
From: Jeff King <hidden> Date: 2020-11-13 20:40:33
On Fri, Nov 13, 2020 at 09:12:31AM +0100, Patrick Steinhardt wrote:
The testcase t1400 exercises the git-update-ref(1) utility. To do so,
many tests directly read and write references via the filesystem,
assuming that we always use loose and/or packed references. While this
is true now, it'll change with the introduction of the reftable backend.
Convert those tests to use git-update-ref(1) and git-show-ref(1) where
possible. Furthermore, two tests are converted to not delete HEAD
anymore, as this results in a broken repository. They've instead been
updated to create a non-mandatory symybolic reference and delete that
one instead.
s/symybolic/symbolic/
Other than, this whole series looks good to me. Thanks for taking the
time to do the extra cleanup (which ended up being way more complicated
than the original goal :) ).
-Peff
From: Patrick Steinhardt <hidden> Date: 2020-11-18 06:49:06
On Fri, Nov 13, 2020 at 03:40:31PM -0500, Jeff King wrote:
On Fri, Nov 13, 2020 at 09:12:31AM +0100, Patrick Steinhardt wrote:
quoted
The testcase t1400 exercises the git-update-ref(1) utility. To do so,
many tests directly read and write references via the filesystem,
assuming that we always use loose and/or packed references. While this
is true now, it'll change with the introduction of the reftable backend.
Convert those tests to use git-update-ref(1) and git-show-ref(1) where
possible. Furthermore, two tests are converted to not delete HEAD
anymore, as this results in a broken repository. They've instead been
updated to create a non-mandatory symybolic reference and delete that
one instead.
s/symybolic/symbolic/
Other than, this whole series looks good to me. Thanks for taking the
time to do the extra cleanup (which ended up being way more complicated
than the original goal :) ).
Thanks!
Junio, shall I fix this typo with another version or will you fix this
up locally?
Patrick