| Message ID | d9abffef71466a0752c210878bb6b8fca7a72287.1604908834.git.ps@pks.im (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | update-ref: Allow creation of multiple transactions | expand |
Patrick Steinhardt <ps@pks.im> writes: > Subject: Re: [PATCH v2 4/4] update-ref: Disallow restart of ongoing transactions Our convention (cf. "git shortlog --no-merges") is not to Capitalize the first word after the "<area>:". Shared across all four patches. > 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. Hmph, if you think of it as "restart", perhaps it would not make sense, but in other contexts where the concept of "transactions" are commonly used, it is not so unusual to support "nested" transactions where inside an ongoing transaction, a subtransaction whose effect can be rolled back as a whole, can be opened. So wouldn't it be more appropriate to explain this change like so? Since we do not support nested transactions, make sure a "start" command, seen after we saw "start" that is not closed by either "abort" or "commit", is flagged as an error. > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > builtin/update-ref.c | 2 ++ > t/t1400-update-ref.sh | 11 +++++++++++ > 2 files changed, 13 insertions(+) > > diff --git a/builtin/update-ref.c b/builtin/update-ref.c > index bb65129012..6029a80544 100644 > --- a/builtin/update-ref.c > +++ b/builtin/update-ref.c > @@ -436,6 +436,8 @@ static void update_refs_stdin(void) > switch (state) { > case UPDATE_REFS_OPEN: > case UPDATE_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; > diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh > index 7ed41bb328..e53d973d04 100755 > --- a/t/t1400-update-ref.sh > +++ b/t/t1400-update-ref.sh > @@ -1583,4 +1583,15 @@ test_expect_success 'transaction can commit after abort' ' > test_cmp expect actual > ' > > +test_expect_success 'transaction cannot restart ongoing transaction' ' > + cat >stdin <<-EOF && > + start > + create refs/heads/restart $A > + start > + commit > + EOF > + test_must_fail git update-ref --stdin <stdin >actual && > + test_must_fail git show-ref --verify refs/heads/restart > +' > + > test_done
diff --git a/builtin/update-ref.c b/builtin/update-ref.c index bb65129012..6029a80544 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -436,6 +436,8 @@ static void update_refs_stdin(void) switch (state) { case UPDATE_REFS_OPEN: case UPDATE_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; diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 7ed41bb328..e53d973d04 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -1583,4 +1583,15 @@ test_expect_success 'transaction can commit after abort' ' test_cmp expect actual ' +test_expect_success 'transaction cannot restart ongoing transaction' ' + cat >stdin <<-EOF && + start + create refs/heads/restart $A + start + commit + EOF + test_must_fail git update-ref --stdin <stdin >actual && + test_must_fail git show-ref --verify refs/heads/restart +' + test_done
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 <ps@pks.im> --- builtin/update-ref.c | 2 ++ t/t1400-update-ref.sh | 11 +++++++++++ 2 files changed, 13 insertions(+)