| Message ID | eec7c2e8ec3e49b34066190d59fc45276bed637f.1604501265.git.ps@pks.im (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | update-ref: Allow creation of multiple transactions | expand |
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. > diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt > index d401234b03..48b6683071 100644 > --- a/Documentation/git-update-ref.txt > +++ b/Documentation/git-update-ref.txt > @@ -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. > --- a/builtin/update-ref.c > +++ b/builtin/update-ref.c > @@ -446,7 +446,18 @@ static void update_refs_stdin(void) > state = cmd->state; > break; > case UPDATE_REFS_CLOSED: > - die("transaction is closed"); > + if (cmd->state != UPDATE_REFS_STARTED) > + die("transaction is closed"); > + > + /* > + * Open a new transaction if we're currently closed and > + * get a "start". > + */ > + state = cmd->state; > + transaction = ref_transaction_begin(&err); > + if (!transaction) > + die("%s", err.buf); > + Very nice. This duplicates the state and transaction setup at the start of the function, which made me wonder if we could do this: diff --git a/builtin/update-ref.c b/builtin/update-ref.c index bb65129012..140f0d30e9 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -385,14 +385,10 @@ static const struct parse_cmd { static void update_refs_stdin(void) { struct strbuf input = STRBUF_INIT, err = STRBUF_INIT; - enum update_refs_state state = UPDATE_REFS_OPEN; + enum update_refs_state state = UPDATE_REFS_CLOSED; struct ref_transaction *transaction; int i, 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)) { const struct parse_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. > +test_expect_success 'transaction can create and delete' ' > + cat >stdin <<-EOF && > + start > + create refs/heads/create-and-delete $A > + commit > + start > + delete refs/heads/create-and-delete $A > + commit > + EOF > + git update-ref --stdin <stdin >actual && > + printf "%s: ok\n" start commit start commit >expect && > + test_path_is_missing .git/refs/heads/create-and-delete > +' 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
Jeff King <peff@peff.net> writes: >> +test_expect_success 'transaction can create and delete' ' >> + cat >stdin <<-EOF && >> + start >> + create refs/heads/create-and-delete $A >> + commit >> + start >> + delete refs/heads/create-and-delete $A >> + commit >> + EOF >> + git update-ref --stdin <stdin >actual && >> + printf "%s: ok\n" start commit start commit >expect && >> + test_path_is_missing .git/refs/heads/create-and-delete >> +' > > 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. Shouldn't "git show-ref --verify" be usable portably across ref backends to test if a well-formed ref was created (or was not created)? On the ref-creation side, there are cases where we need to directly futz with the filesystem entity. For example, "git update-ref" cannot be used to place a non-commit at "refs/heads/foo", so something like git rev-parse HEAD^{tree} >.git/refs/heads/bad-branch cannot be avoided (this is a tangent but we probably should add a way to force setting _any_ value to any ref, that may not even point at an existing object or an object of a wrong type, to help test scripts). But I do not think this is such a case.
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: > > > 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. > > > diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt > > index d401234b03..48b6683071 100644 > > --- a/Documentation/git-update-ref.txt > > +++ b/Documentation/git-update-ref.txt > > @@ -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
On Thu, Nov 05, 2020 at 01:34:20PM -0800, Junio C Hamano wrote: > > 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. > > > Shouldn't "git show-ref --verify" be usable portably across ref backends > to test if a well-formed ref was created (or was not created)? > > On the ref-creation side, there are cases where we need to directly > futz with the filesystem entity. For example, "git update-ref" > cannot be used to place a non-commit at "refs/heads/foo", so > something like > > git rev-parse HEAD^{tree} >.git/refs/heads/bad-branch > > cannot be avoided (this is a tangent but we probably should add a > way to force setting _any_ value to any ref, that may not even point > at an existing object or an object of a wrong type, to help test > scripts). > > But I do not think this is such a case. Yeah, I agree completely that we could be using rev-parse in this instance. But it's definitely not alone there: $ git grep -c test_path_is.*.git/refs t/t1400-update-ref.sh t/t1400-update-ref.sh:13 So it is a question of "do an ugly thing that fits in with neighbors" or "be inconsistent but set a good example". And I am OK with either. Of course, "improve the neighbors on top" would be better still. :) -Peff PS And yeah, I agree in the long run we may need some mechanism to override internal safeties in order to test broken cases with reftable. We have sometimes resorted to manually munging on-disk files in similar tests (e.g., for broken packs, etc), but it gets rather tricky.
Jeff King <peff@peff.net> writes: > On Thu, Nov 05, 2020 at 01:34:20PM -0800, Junio C Hamano wrote: > >> > 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. >> ... > Yeah, I agree completely that we could be using rev-parse in this > instance. But it's definitely not alone there: > ... Yup, this morning I was reviewing what we said in the previous day's exchanges and noticed that you weren't advocating but merely saying it is not making things worse, and I agree with the assessment. Perhaps two #leftoverbits are to (1) clean up this test to create refs using "update-ref", and verify refs using "show-ref --verify". (2) If (1) had to leave some direct filesystem access due to the built-in safety that cannot be circumvented, decide which is more appropirate between a test-update-ref test helper only to be used in tests, or a "--force" option usable to corrupt repositories with "update-ref", implement it, and use it to finish cleaning up tests. Thanks.
diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt index d401234b03..48b6683071 100644 --- a/Documentation/git-update-ref.txt +++ b/Documentation/git-update-ref.txt @@ -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 diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 8a2df4459c..bb65129012 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -446,7 +446,18 @@ static void update_refs_stdin(void) state = cmd->state; break; case UPDATE_REFS_CLOSED: - die("transaction is closed"); + if (cmd->state != UPDATE_REFS_STARTED) + die("transaction is closed"); + + /* + * Open a new transaction if we're currently closed and + * get a "start". + */ + state = cmd->state; + transaction = ref_transaction_begin(&err); + if (!transaction) + die("%s", err.buf); + break; } diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 4c01e08551..72d995aece 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -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 + create refs/heads/branch-1 $A + commit + start + create refs/heads/branch-2 $B + commit + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" start commit start commit >expect && + test_cmp expect actual && + echo "$A" >expect && + git rev-parse refs/heads/branch-1 >actual && + test_cmp expect actual && + echo "$B" >expect && + git rev-parse refs/heads/branch-2 >actual && + test_cmp expect actual +' + +test_expect_success 'transaction can create and delete' ' + cat >stdin <<-EOF && + start + create refs/heads/create-and-delete $A + commit + start + delete refs/heads/create-and-delete $A + commit + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" start commit start commit >expect && + test_path_is_missing .git/refs/heads/create-and-delete +' + +test_expect_success 'transaction can commit after abort' ' + cat >stdin <<-EOF && + start + create refs/heads/abort $A + abort + start + create refs/heads/abort $A + commit + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" start abort start commit >expect && + echo "$A" >expect && + git rev-parse refs/heads/abort >actual && + test_cmp expect actual +' + test_done
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 <ps@pks.im> --- Documentation/git-update-ref.txt | 3 +- builtin/update-ref.c | 13 ++++++++- t/t1400-update-ref.sh | 50 ++++++++++++++++++++++++++++++++ 3 files changed, 64 insertions(+), 2 deletions(-)