From: Kristoffer Haugsbakk <code@khaugsbakk.name>
To: git@vger.kernel.org
Cc: peff@peff.net, gitster@pobox.com,
Kristoffer Haugsbakk <code@khaugsbakk.name>
Subject: [PATCH v3 0/3] tag: keep the message file in case ref transaction fails
Date: Tue, 16 May 2023 19:55:43 +0200 [thread overview]
Message-ID: <cover.1684258780.git.code@khaugsbakk.name> (raw)
In-Reply-To: <cover.1684067644.git.code@khaugsbakk.name>
§ Introduction (v3)
The first part of this material is the same as v1 and v2; skip to “Based
on” and onwards for the new things.
Cheers.
―
The ref transaction can fail after the message has been written using the
editor. The ref transaction is attempted after the message file (`TAG_EDITMSG`)
has been unlinked, so there is no backup tag message file to retry the
command.[1]
This is unfortunate if someone has written more than e.g. “v1.99.4” in the
editor. (I don’t know if people write long tag messages in practice.)
Hold on to the tag message file until after the ref transaction in order
to preserve the backup.
† 1: On commit 91428f078b (The eighteenth batch, 2023-05-10)
§ Reproduction script
```
cd /tmp
dir=$(mktemp -d)
cd $dir
git init
git commit --allow-empty -mInit
git tag release/v1
# Fails
git tag -a release
```
Error message:
```
fatal: cannot lock ref 'refs/tags/release': 'refs/tags/release/v1' exists; cannot create 'refs/tags/release'
```
Better error message and behavior:
```
The tag message has been left in .git/TAG_EDITMSG
fatal: cannot lock ref 'refs/tags/release': 'refs/tags/release/v1' exists; cannot create 'refs/tags/release'
```
§ Alternatives considered
My first thought was to find a way to “dry run” the ref update before opening
the editor (the edge case of the ref update command succeeding the first time
but not the second *real* time seemed incredibly unlikely to happen by
happenstance, so I saw no reason to consider that). However that seemed like it
would involve more code and conditionals, and I don’t know if the dry-run mode
is even supported.
A benefit of this alternative approach would be to error out immediately instead
of opening the editor. But trying to create a tag which collides with an
existing “namespace” seems very unlikely to happen in practice.[2] Losing a file
is much worse than being inconvenienced to retry the command, so I decided to
just focus on the former problem.
Most importantly though this approach was within my ability to implement.
† 2: Just observe my “Reproduction script”: one tries to create `release` after
someone else made `release/v1`. But what is just “release”? What follows
(next version) that? But why am I arguing against my change…
§ Based on (base commit)
`v2.40.1`
§ Changes compared to the previous round
• Replace `! test_path_exists` with `test_path_is_missing`
Other:
• Drop “Cc” trailers (but keep Cc list)
§ Patches
Same number and order as last round.
1. Document `TAG_EDITMSG`
2. Test successful tag creation
• New: Use `test_path_is_missing`
3. The main change plus a regression test
Kristoffer Haugsbakk (3):
doc: tag: document `TAG_EDITMSG`
t/t7004-tag: add regression test for successful tag creation
tag: keep the message file in case ref transaction fails
Documentation/git-tag.txt | 10 ++++++++++
builtin/tag.c | 24 +++++++++++++++---------
t/t7004-tag.sh | 19 +++++++++++++++++++
3 files changed, 44 insertions(+), 9 deletions(-)
Range-diff against v2:
1: 0e0e592853d ! 1: 882008e32a4 doc: tag: document `TAG_EDITMSG`
@@ Commit message
in case of error.
Suggested-by: Junio C Hamano <gitster@pobox.com>
- Cc: Jeff King <peff@peff.net>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
2: aabeb4568ee ! 2: d1490b8f0b7 t/t7004-tag: add regression test for successful tag creation
@@ Commit message
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
## t/t7004-tag.sh ##
@@ t/t7004-tag.sh: test_expect_success 'Does --[no-]contains stop at commits? Yes!' '
test_cmp expected actual
@@ t/t7004-tag.sh: test_expect_success 'Does --[no-]contains stop at commits? Yes!'
+ echo Message >.git/TAG_EDITMSG
+ EOF
+ GIT_EDITOR=./fakeeditor git tag -a foo &&
-+ ! test_path_exists .git/TAG_EDITMSG
++ test_path_is_missing .git/TAG_EDITMSG
+'
+
test_done
3: e67b6416b7e ! 3: 81bca0673d8 tag: keep the message file in case ref transaction fails
@@ Commit message
Hold on to the message file for a little longer so that it is not
unlinked before the fatal error occurs.
- Cc: Jeff King <peff@peff.net>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
@@ builtin/tag.c: int cmd_tag(int argc, const char **argv, const char *prefix)
## t/t7004-tag.sh ##
@@ t/t7004-tag.sh: test_expect_success 'If tag is created then tag message file is unlinked' '
- ! test_path_exists .git/TAG_EDITMSG
+ test_path_is_missing .git/TAG_EDITMSG
'
+test_expect_success 'If tag cannot be created then tag message file is not unlinked' '
--
2.40.1
next prev parent reply other threads:[~2023-05-16 17:56 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-14 13:17 [PATCH 0/3] tag: keep the message file in case ref transaction fails Kristoffer Haugsbakk
2023-05-14 13:17 ` [PATCH 1/3] t/t7004-tag: add regression test for existing behavior Kristoffer Haugsbakk
2023-05-15 6:59 ` Junio C Hamano
2023-05-14 13:17 ` [PATCH 2/3] t/t7004-tag: add failing tag message file test Kristoffer Haugsbakk
2023-05-15 7:00 ` Junio C Hamano
2023-05-15 19:56 ` Kristoffer Haugsbakk
2023-05-15 7:02 ` Junio C Hamano
2023-05-14 13:18 ` [PATCH 3/3] tag: keep the message file in case ref transaction fails Kristoffer Haugsbakk
2023-05-15 6:59 ` Junio C Hamano
2023-05-15 20:29 ` [PATCH v2 0/3] " Kristoffer Haugsbakk
2023-05-15 20:29 ` [PATCH v2 1/3] doc: tag: document `TAG_EDITMSG` Kristoffer Haugsbakk
2023-05-15 21:59 ` Junio C Hamano
2023-05-15 20:29 ` [PATCH v2 2/3] t/t7004-tag: add regression test for successful tag creation Kristoffer Haugsbakk
2023-05-15 20:29 ` [PATCH v2 3/3] tag: keep the message file in case ref transaction fails Kristoffer Haugsbakk
2023-05-15 21:50 ` [PATCH v2 0/3] " Junio C Hamano
2023-05-16 17:55 ` Kristoffer Haugsbakk [this message]
2023-05-16 17:55 ` [PATCH v3 1/3] doc: tag: document `TAG_EDITMSG` Kristoffer Haugsbakk
2023-05-16 17:55 ` [PATCH v3 2/3] t/t7004-tag: add regression test for successful tag creation Kristoffer Haugsbakk
2023-05-16 17:55 ` [PATCH v3 3/3] tag: keep the message file in case ref transaction fails Kristoffer Haugsbakk
2023-05-16 18:39 ` [PATCH v3 0/3] " Junio C Hamano
2023-05-17 9:32 ` [PATCH " Jeff King
2023-05-17 16:00 ` Junio C Hamano
2023-05-17 17:37 ` Jeff King
2023-05-17 17:52 ` Eric Sunshine
2023-05-17 18:02 ` Kristoffer Haugsbakk
2023-05-17 19:58 ` Kristoffer Haugsbakk
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cover.1684258780.git.code@khaugsbakk.name \
--to=code@khaugsbakk.name \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).