* [PATCH 1/3] t/t7004-tag: add regression test for existing behavior
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 ` 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
` (4 subsequent siblings)
5 siblings, 1 reply; 26+ messages in thread
From: Kristoffer Haugsbakk @ 2023-05-14 13:17 UTC (permalink / raw)
To: git; +Cc: peff, Kristoffer Haugsbakk
The standard tag message file is unlinked if the tag is created.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
`test_when_finished` because this test interferes with the next one
without it.
t/t7004-tag.sh | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 9aa1660651..550b5b1cce 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -2127,4 +2127,13 @@ test_expect_success 'Does --[no-]contains stop at commits? Yes!' '
test_cmp expected actual
'
+test_expect_success 'If tag is created then tag message file is unlinked' '
+ test_when_finished "git tag -d foo" &&
+ write_script fakeeditor <<-\EOF &&
+ echo Message >.git/TAG_EDITMSG
+ EOF
+ GIT_EDITOR=./fakeeditor git tag -a foo &&
+ ! test -e .git/TAG_EDITMSG
+'
+
test_done
--
2.40.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 1/3] t/t7004-tag: add regression test for existing behavior
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
0 siblings, 0 replies; 26+ messages in thread
From: Junio C Hamano @ 2023-05-15 6:59 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git, peff
Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
> The standard tag message file is unlinked if the tag is created.
>
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---
>
> Notes (series):
> `test_when_finished` because this test interferes with the next one
> without it.
Use of when-finished to remove a tag makes sense.
Unlike COMMIT_EDITMSG that is documented in "git commit --help", we
do not promise the users that the temporary file that is used for
preparing a tag message is "$GIT_DIR/TAG_EDITMSG" in any of our
documentation. That needs to be done at the same time, or before,
this new test makes such a promise.
> t/t7004-tag.sh | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index 9aa1660651..550b5b1cce 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -2127,4 +2127,13 @@ test_expect_success 'Does --[no-]contains stop at commits? Yes!' '
> test_cmp expected actual
> '
>
> +test_expect_success 'If tag is created then tag message file is unlinked' '
> + test_when_finished "git tag -d foo" &&
> + write_script fakeeditor <<-\EOF &&
> + echo Message >.git/TAG_EDITMSG
> + EOF
> + GIT_EDITOR=./fakeeditor git tag -a foo &&
> + ! test -e .git/TAG_EDITMSG
I'd recommend using "test_path_is_missing" instead for better
test-debugging experience.
Thanks.
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/3] t/t7004-tag: add failing tag message file test
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-14 13:17 ` Kristoffer Haugsbakk
2023-05-15 7:00 ` Junio C Hamano
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
` (3 subsequent siblings)
5 siblings, 2 replies; 26+ messages in thread
From: Kristoffer Haugsbakk @ 2023-05-14 13:17 UTC (permalink / raw)
To: git; +Cc: peff, Kristoffer Haugsbakk
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
t/t7004-tag.sh | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 550b5b1cce..1e512dbe06 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -2136,4 +2136,14 @@ test_expect_success 'If tag is created then tag message file is unlinked' '
! test -e .git/TAG_EDITMSG
'
+test_expect_success 'If tag cannot be created then tag message file is not unlinked' '
+ test_when_finished "git tag -d foo/bar" &&
+ write_script fakeeditor <<-\EOF &&
+ echo Message >.git/TAG_EDITMSG
+ EOF
+ git tag foo/bar &&
+ ! GIT_EDITOR=./fakeeditor git tag -a foo &&
+ test -e .git/TAG_EDITMSG
+'
+
test_done
--
2.40.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 2/3] t/t7004-tag: add failing tag message file test
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
1 sibling, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2023-05-15 7:00 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git, peff
Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---
> t/t7004-tag.sh | 10 ++++++++++
> 1 file changed, 10 insertions(+)
Does this document the current behaviour, i.e. before applying the
patch [3/3]? Or is this a new test designed to fail until [3/3] is
applied?
If the latter, please don't [*].
Instead, combine this with [3/3] and make it [2/2] that changes the
behaviour of the command and protects the new behaviour from future
breakages in a single step. Those who are truly curious to see why
the code change in it is necessary can apply the "code change plus
new test" patch, and then temporarily revert only the code change
part in their working tree to see how the test breaks without the
code change.
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index 550b5b1cce..1e512dbe06 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -2136,4 +2136,14 @@ test_expect_success 'If tag is created then tag message file is unlinked' '
> ! test -e .git/TAG_EDITMSG
> '
>
> +test_expect_success 'If tag cannot be created then tag message file is not unlinked' '
> + test_when_finished "git tag -d foo/bar" &&
> + write_script fakeeditor <<-\EOF &&
> + echo Message >.git/TAG_EDITMSG
> + EOF
> + git tag foo/bar &&
> + ! GIT_EDITOR=./fakeeditor git tag -a foo &&
Imitate other tests that expect a controlled failure from our
command, and write something like
test_must_fail env GIT_EDITOR=./fakeeditor git tag -a foo
so that a segfaulting "git tag" will not count as "failing as
expected".
> + test -e .git/TAG_EDITMSG
Use "test_path_exists" instead.
Thanks.
[Footnote]
* Introducing this as a failing test "test_expect_failure" in [2/3]
and then flip it to "test_expect_success" in [3/3] would make
tests pass without applying [3/3], but generally it is not a
recommended practice.
This is because such a [3/3] patch would only show the line with
the test title and the behaviour of the test will not be shown in
the diff context. That hurts reviewability.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/3] t/t7004-tag: add failing tag message file test
2023-05-15 7:00 ` Junio C Hamano
@ 2023-05-15 19:56 ` Kristoffer Haugsbakk
0 siblings, 0 replies; 26+ messages in thread
From: Kristoffer Haugsbakk @ 2023-05-15 19:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
On Mon, May 15, 2023, at 09:00, Junio C Hamano wrote:
> Does this document the current behaviour, i.e. before applying the
> patch [3/3]? Or is this a new test designed to fail until [3/3] is
> applied?
>
> If the latter, please don't [*].
Yep, I was going for the latter. I’ll combine [2/3] and [3/3].
> Imitate other tests that expect a controlled failure from our
> command, and write something like
>
> test_must_fail env GIT_EDITOR=./fakeeditor git tag -a foo
>
> so that a segfaulting "git tag" will not count as "failing as
> expected".
Ah, I was wondering how to use `test_must_fail` on a command with an
environment variable. Thanks.
--
Kristoffer Haugsbakk
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/3] t/t7004-tag: add failing tag message file test
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 7:02 ` Junio C Hamano
1 sibling, 0 replies; 26+ messages in thread
From: Junio C Hamano @ 2023-05-15 7:02 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git, peff
Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---
> t/t7004-tag.sh | 10 ++++++++++
> 1 file changed, 10 insertions(+)
Does this document the current behaviour, i.e. before applying the
patch [3/3]? Or is this a new test designed to fail until [3/3] is
applied?
If the latter, please don't [*].
Instead, combine this with [3/3] and make it [2/2] that changes the
behaviour of the command and protects the new behaviour from future
breakages in a single step. Those who are truly curious to see why
the code change in it is necessary can apply the "code change plus
new test" patch, and then temporarily revert only the code change
part in their working tree to see how the test breaks without the
code change.
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index 550b5b1cce..1e512dbe06 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -2136,4 +2136,14 @@ test_expect_success 'If tag is created then tag message file is unlinked' '
> ! test -e .git/TAG_EDITMSG
> '
>
> +test_expect_success 'If tag cannot be created then tag message file is not unlinked' '
> + test_when_finished "git tag -d foo/bar" &&
> + write_script fakeeditor <<-\EOF &&
> + echo Message >.git/TAG_EDITMSG
> + EOF
> + git tag foo/bar &&
> + ! GIT_EDITOR=./fakeeditor git tag -a foo &&
Imitate other tests that expect a controlled failure from our
command, and write something like
test_must_fail env GIT_EDITOR=./fakeeditor git tag -a foo
so that a segfaulting "git tag" will not count as "failing as
expected".
> + test -e .git/TAG_EDITMSG
Use "test_path_exists" instead.
Thanks.
[Footnote]
* Introducing this as a failing test "test_expect_failure" in [2/3]
and then flip it to "test_expect_success" in [3/3] would make
tests pass without applying [3/3], but generally it is not a
recommended practice.
This is because such a [3/3] patch would only show the line with
the test title and the behaviour of the test will not be shown in
the diff context. That hurts reviewability.
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 3/3] tag: keep the message file in case ref transaction fails
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-14 13:17 ` [PATCH 2/3] t/t7004-tag: add failing tag message file test Kristoffer Haugsbakk
@ 2023-05-14 13:18 ` Kristoffer Haugsbakk
2023-05-15 6:59 ` Junio C Hamano
2023-05-15 20:29 ` [PATCH v2 0/3] " Kristoffer Haugsbakk
` (2 subsequent siblings)
5 siblings, 1 reply; 26+ messages in thread
From: Kristoffer Haugsbakk @ 2023-05-14 13:18 UTC (permalink / raw)
To: git; +Cc: peff, Kristoffer Haugsbakk
The ref transaction can fail after the user has written their tag message. In
particular, if there exists a tag `foo/bar` and `git tag -a foo` is said then
the command will only fail once it tries to write `refs/tags/foo`, which is
after one has closed the editor.
Hold on to the message file for a little longer so that it is not unlinked
before the fatal error occurs.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
I tried to maintain the proper formatting by using `clang-format` via Emacs on
the affected lines.
builtin/tag.c | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index d428c45dc8..7df9c143ac 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -266,11 +266,10 @@ static const char message_advice_nested_tag[] =
static void create_tag(const struct object_id *object, const char *object_ref,
const char *tag,
struct strbuf *buf, struct create_tag_options *opt,
- struct object_id *prev, struct object_id *result)
+ struct object_id *prev, struct object_id *result, char *path)
{
enum object_type type;
struct strbuf header = STRBUF_INIT;
- char *path = NULL;
type = oid_object_info(the_repository, object, NULL);
if (type <= OBJ_NONE)
@@ -294,7 +293,6 @@ static void create_tag(const struct object_id *object, const char *object_ref,
int fd;
/* write the template message before editing: */
- path = git_pathdup("TAG_EDITMSG");
fd = xopen(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);
if (opt->message_given) {
@@ -336,10 +334,6 @@ static void create_tag(const struct object_id *object, const char *object_ref,
path);
exit(128);
}
- if (path) {
- unlink_or_warn(path);
- free(path);
- }
}
static void create_reflog_msg(const struct object_id *oid, struct strbuf *sb)
@@ -487,6 +481,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
};
int ret = 0;
const char *only_in_list = NULL;
+ char *path = NULL;
setup_ref_filter_porcelain_msg();
@@ -621,7 +616,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
if (create_tag_object) {
if (force_sign_annotate && !annotate)
opt.sign = 1;
- create_tag(&object, object_ref, tag, &buf, &opt, &prev, &object);
+ path = git_pathdup("TAG_EDITMSG");
+ create_tag(&object, object_ref, tag, &buf, &opt, &prev, &object,
+ path);
}
transaction = ref_transaction_begin(&err);
@@ -629,8 +626,17 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
ref_transaction_update(transaction, ref.buf, &object, &prev,
create_reflog ? REF_FORCE_CREATE_REFLOG : 0,
reflog_msg.buf, &err) ||
- ref_transaction_commit(transaction, &err))
+ ref_transaction_commit(transaction, &err)) {
+ if (path)
+ fprintf(stderr,
+ _("The tag message has been left in %s\n"),
+ path);
die("%s", err.buf);
+ }
+ if (path) {
+ unlink_or_warn(path);
+ free(path);
+ }
ref_transaction_free(transaction);
if (force && !is_null_oid(&prev) && !oideq(&prev, &object))
printf(_("Updated tag '%s' (was %s)\n"), tag,
--
2.40.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 3/3] tag: keep the message file in case ref transaction fails
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
0 siblings, 0 replies; 26+ messages in thread
From: Junio C Hamano @ 2023-05-15 6:59 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git, peff
Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
> The ref transaction can fail after the user has written their tag message. In
> particular, if there exists a tag `foo/bar` and `git tag -a foo` is said then
> the command will only fail once it tries to write `refs/tags/foo`, which is
> after one has closed the editor.
>
> Hold on to the message file for a little longer so that it is not unlinked
> before the fatal error occurs.
>
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---
As I said in my review of [1/3], TAG_EDITMSG should be documented
just like COMMIT_EDITMSG is documented.
Other than that, looking OK to me (I didn't read the patch very
carefully so I may have missed leaking and/or use after free,
though).
Thanks for working on this.
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 0/3] tag: keep the message file in case ref transaction fails
2023-05-14 13:17 [PATCH 0/3] tag: keep the message file in case ref transaction fails Kristoffer Haugsbakk
` (2 preceding siblings ...)
2023-05-14 13:18 ` [PATCH 3/3] tag: keep the message file in case ref transaction fails Kristoffer Haugsbakk
@ 2023-05-15 20:29 ` Kristoffer Haugsbakk
2023-05-15 20:29 ` [PATCH v2 1/3] doc: tag: document `TAG_EDITMSG` Kristoffer Haugsbakk
` (3 more replies)
2023-05-16 17:55 ` [PATCH v3 " Kristoffer Haugsbakk
2023-05-17 9:32 ` [PATCH " Jeff King
5 siblings, 4 replies; 26+ messages in thread
From: Kristoffer Haugsbakk @ 2023-05-15 20:29 UTC (permalink / raw)
To: git; +Cc: gitster, Kristoffer Haugsbakk
§ Introduction (v2)
The following material is the same compared to v1 up to but not
including “Changes from the previous round (v1)”
(The CI link is also new.)
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…
§ CI
https://github.com/LemmingAvalanche/git/actions/runs/4984100467
§ Changes compared to the previous round
• Document `TAG_EDITMSG`
• Improve test structure
• Combine/squash the regression test for the change and the actual change
• In the previous round the regression test commit would fail the test
suite since it’s a `test_expect_success` and it comes before the
actual change
§ Patches (compared to previous round)
1. (new) Document `TAG_EDITMSG`
• So that we match `COMMIT_EDITMSG`
2. (was [1/3]) Test successful tag creation
• Tweak commit message subject
• Also remove `.git/TAG_EDITMSG` in `test_when_finished`
3. (was [2–3/3]) The main change plus a regression test
• Tweak commit message and reflow paragraphs
• Not a change but add a question for the reviewers (see the “Note”)
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 v1:
-: ---------- > 1: 0e0e592853 doc: tag: document `TAG_EDITMSG`
1: 87b709d856 ! 2: aabeb4568e t/t7004-tag: add regression test for existing behavior
@@ Metadata
Author: Kristoffer Haugsbakk <code@khaugsbakk.name>
## Commit message ##
- t/t7004-tag: add regression test for existing behavior
+ t/t7004-tag: add regression test for successful tag creation
The standard tag message file is unlinked if the tag is created.
@@ 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 -e .git/TAG_EDITMSG
++ ! test_path_exists .git/TAG_EDITMSG
+'
+
test_done
2: 1f24aa43f7 < -: ---------- t/t7004-tag: add failing tag message file test
3: 999af290af ! 3: e67b6416b7 tag: keep the message file in case ref transaction fails
@@ Metadata
## Commit message ##
tag: keep the message file in case ref transaction fails
- The ref transaction can fail after the user has written their tag message. In
- particular, if there exists a tag `foo/bar` and `git tag -a foo` is said then
- the command will only fail once it tries to write `refs/tags/foo`, which is
- after one has closed the editor.
+ The ref transaction can fail after the user has written their tag
+ message. In particular, if there exists a tag `foo/bar` and `git tag -a
+ foo` is said then the command will only fail once it tries to write
+ `refs/tags/foo`, which is after the file has been unlinked.
- Hold on to the message file for a little longer so that it is not unlinked
- before the fatal error occurs.
+ 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>
## Notes (series) ##
- I tried to maintain the proper formatting by using `clang-format` via Emacs on
- the affected lines.
+ I duplicated this message (this isn’t obvious in the diff):
+
+ fprintf(stderr,
+ _("The tag message has been left in %s\n"),
+ path);
+
+ Should this be factored into a static function instead?
+
+ § Changes from previous round
+
+ Squash (combine) the update to `tag.c` with the test so that the fix and
+ the regression test is added in one step.
+
+ This makes more sense than what I was going for since the test suite
+ would fail on patch 2/3 of the previous round.
+
+ Link: https://lore.kernel.org/git/xmqq4joeaxgw.fsf@gitster.g/T/#u
## builtin/tag.c ##
@@ builtin/tag.c: static const char message_advice_nested_tag[] =
@@ builtin/tag.c: int cmd_tag(int argc, const char **argv, const char *prefix)
ref_transaction_free(transaction);
if (force && !is_null_oid(&prev) && !oideq(&prev, &object))
printf(_("Updated tag '%s' (was %s)\n"), tag,
+
+ ## 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_expect_success 'If tag cannot be created then tag message file is not unlinked' '
++ test_when_finished "git tag -d foo/bar && rm .git/TAG_EDITMSG" &&
++ write_script fakeeditor <<-\EOF &&
++ echo Message >.git/TAG_EDITMSG
++ EOF
++ git tag foo/bar &&
++ test_must_fail env GIT_EDITOR=./fakeeditor git tag -a foo &&
++ test_path_exists .git/TAG_EDITMSG
++'
++
+ test_done
--
2.40.1
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH v2 1/3] doc: tag: document `TAG_EDITMSG`
2023-05-15 20:29 ` [PATCH v2 0/3] " Kristoffer Haugsbakk
@ 2023-05-15 20:29 ` 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
` (2 subsequent siblings)
3 siblings, 1 reply; 26+ messages in thread
From: Kristoffer Haugsbakk @ 2023-05-15 20:29 UTC (permalink / raw)
To: git; +Cc: gitster, Kristoffer Haugsbakk, Jeff King
Document `TAG_EDITMSG` which we have told the user about on unsuccessful
command invocations since commit 3927bbe9a4 (tag: delete TAG_EDITMSG
only on successful tag, 2008-12-06).
Introduce this documentation since we are going to add tests for the
lifetime of this file in the case of command failure and success.
Use the documentation for `COMMIT_EDITMSG` from `git-commit.txt` as a
template since these two files share the same purpose.[1]
† 1: from commit 3927bbe9a4:
“ This matches the behavior of COMMIT_EDITMSG, which stays around
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>
---
Notes (series):
• I changed (from `COMMIT_EDITMSG`) “will be overwritten” to “may” since
I don’t see the point in giving a guarantee
• Unsure if I was going to put this before or after “NOTES” as there seems
to be no precedence
Suggested by Junio: https://lore.kernel.org/git/xmqqy1lqaxln.fsf@gitster.g/T/#u
Documentation/git-tag.txt | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index fdc72b5875..46e56b0245 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -377,6 +377,16 @@ $ GIT_COMMITTER_DATE="2006-10-02 10:31" git tag -s v1.0.1
include::date-formats.txt[]
+FILES
+-----
+
+`$GIT_DIR/TAG_EDITMSG`::
+ This file contains the message of an in-progress annotated
+ tag. If `git tag` exits due to an error before creating an
+ annotated tag then the tag message that has been provided by the
+ user in an editor session will be available in this file, but
+ may be overwritten by the next invocation of `git tag`.
+
NOTES
-----
--
2.40.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 1/3] doc: tag: document `TAG_EDITMSG`
2023-05-15 20:29 ` [PATCH v2 1/3] doc: tag: document `TAG_EDITMSG` Kristoffer Haugsbakk
@ 2023-05-15 21:59 ` Junio C Hamano
0 siblings, 0 replies; 26+ messages in thread
From: Junio C Hamano @ 2023-05-15 21:59 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git, Jeff King
Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
> Document `TAG_EDITMSG` which we have told the user about on unsuccessful
> command invocations since commit 3927bbe9a4 (tag: delete TAG_EDITMSG
> only on successful tag, 2008-12-06).
>
> Introduce this documentation since we are going to add tests for the
> lifetime of this file in the case of command failure and success.
>
> Use the documentation for `COMMIT_EDITMSG` from `git-commit.txt` as a
> template since these two files share the same purpose.[1]
>
> † 1: from commit 3927bbe9a4:
>
> “ This matches the behavior of COMMIT_EDITMSG, which stays around
> 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>
> ---
Excellent.
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 2/3] t/t7004-tag: add regression test for successful tag creation
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 20:29 ` 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
3 siblings, 0 replies; 26+ messages in thread
From: Kristoffer Haugsbakk @ 2023-05-15 20:29 UTC (permalink / raw)
To: git; +Cc: gitster, Kristoffer Haugsbakk
The standard tag message file is unlinked if the tag is created.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
`test_when_finished` because this test interferes with the next one
without it.
t/t7004-tag.sh | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 9aa1660651..cd454acfed 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -2127,4 +2127,13 @@ test_expect_success 'Does --[no-]contains stop at commits? Yes!' '
test_cmp expected actual
'
+test_expect_success 'If tag is created then tag message file is unlinked' '
+ test_when_finished "git tag -d foo" &&
+ write_script fakeeditor <<-\EOF &&
+ echo Message >.git/TAG_EDITMSG
+ EOF
+ GIT_EDITOR=./fakeeditor git tag -a foo &&
+ ! test_path_exists .git/TAG_EDITMSG
+'
+
test_done
--
2.40.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 3/3] tag: keep the message file in case ref transaction fails
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 20:29 ` [PATCH v2 2/3] t/t7004-tag: add regression test for successful tag creation Kristoffer Haugsbakk
@ 2023-05-15 20:29 ` Kristoffer Haugsbakk
2023-05-15 21:50 ` [PATCH v2 0/3] " Junio C Hamano
3 siblings, 0 replies; 26+ messages in thread
From: Kristoffer Haugsbakk @ 2023-05-15 20:29 UTC (permalink / raw)
To: git; +Cc: gitster, Kristoffer Haugsbakk, Jeff King
The ref transaction can fail after the user has written their tag
message. In particular, if there exists a tag `foo/bar` and `git tag -a
foo` is said then the command will only fail once it tries to write
`refs/tags/foo`, which is after the file has been unlinked.
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>
---
Notes (series):
I duplicated this message (this isn’t obvious in the diff):
fprintf(stderr,
_("The tag message has been left in %s\n"),
path);
Should this be factored into a static function instead?
§ Changes from previous round
Squash (combine) the update to `tag.c` with the test so that the fix and
the regression test is added in one step.
This makes more sense than what I was going for since the test suite
would fail on patch 2/3 of the previous round.
Link: https://lore.kernel.org/git/xmqq4joeaxgw.fsf@gitster.g/T/#u
builtin/tag.c | 24 +++++++++++++++---------
t/t7004-tag.sh | 10 ++++++++++
2 files changed, 25 insertions(+), 9 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index d428c45dc8..7df9c143ac 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -266,11 +266,10 @@ static const char message_advice_nested_tag[] =
static void create_tag(const struct object_id *object, const char *object_ref,
const char *tag,
struct strbuf *buf, struct create_tag_options *opt,
- struct object_id *prev, struct object_id *result)
+ struct object_id *prev, struct object_id *result, char *path)
{
enum object_type type;
struct strbuf header = STRBUF_INIT;
- char *path = NULL;
type = oid_object_info(the_repository, object, NULL);
if (type <= OBJ_NONE)
@@ -294,7 +293,6 @@ static void create_tag(const struct object_id *object, const char *object_ref,
int fd;
/* write the template message before editing: */
- path = git_pathdup("TAG_EDITMSG");
fd = xopen(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);
if (opt->message_given) {
@@ -336,10 +334,6 @@ static void create_tag(const struct object_id *object, const char *object_ref,
path);
exit(128);
}
- if (path) {
- unlink_or_warn(path);
- free(path);
- }
}
static void create_reflog_msg(const struct object_id *oid, struct strbuf *sb)
@@ -487,6 +481,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
};
int ret = 0;
const char *only_in_list = NULL;
+ char *path = NULL;
setup_ref_filter_porcelain_msg();
@@ -621,7 +616,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
if (create_tag_object) {
if (force_sign_annotate && !annotate)
opt.sign = 1;
- create_tag(&object, object_ref, tag, &buf, &opt, &prev, &object);
+ path = git_pathdup("TAG_EDITMSG");
+ create_tag(&object, object_ref, tag, &buf, &opt, &prev, &object,
+ path);
}
transaction = ref_transaction_begin(&err);
@@ -629,8 +626,17 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
ref_transaction_update(transaction, ref.buf, &object, &prev,
create_reflog ? REF_FORCE_CREATE_REFLOG : 0,
reflog_msg.buf, &err) ||
- ref_transaction_commit(transaction, &err))
+ ref_transaction_commit(transaction, &err)) {
+ if (path)
+ fprintf(stderr,
+ _("The tag message has been left in %s\n"),
+ path);
die("%s", err.buf);
+ }
+ if (path) {
+ unlink_or_warn(path);
+ free(path);
+ }
ref_transaction_free(transaction);
if (force && !is_null_oid(&prev) && !oideq(&prev, &object))
printf(_("Updated tag '%s' (was %s)\n"), tag,
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index cd454acfed..37bfa63fe8 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -2136,4 +2136,14 @@ test_expect_success 'If tag is created then tag message file is unlinked' '
! test_path_exists .git/TAG_EDITMSG
'
+test_expect_success 'If tag cannot be created then tag message file is not unlinked' '
+ test_when_finished "git tag -d foo/bar && rm .git/TAG_EDITMSG" &&
+ write_script fakeeditor <<-\EOF &&
+ echo Message >.git/TAG_EDITMSG
+ EOF
+ git tag foo/bar &&
+ test_must_fail env GIT_EDITOR=./fakeeditor git tag -a foo &&
+ test_path_exists .git/TAG_EDITMSG
+'
+
test_done
--
2.40.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 0/3] tag: keep the message file in case ref transaction fails
2023-05-15 20:29 ` [PATCH v2 0/3] " Kristoffer Haugsbakk
` (2 preceding siblings ...)
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 ` Junio C Hamano
3 siblings, 0 replies; 26+ messages in thread
From: Junio C Hamano @ 2023-05-15 21:50 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git
Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
> + GIT_EDITOR=./fakeeditor git tag -a foo &&
> -+ ! test -e .git/TAG_EDITMSG
> ++ ! test_path_exists .git/TAG_EDITMSG
This is not quite right. test_path_exists is loud when its
expectation that the path _exists_ is not met, i.e.
test_path_exists () {
test "$#" -ne 1 && BUG "1 param"
if ! test -e "$1"
then
echo "Path $1 doesn't exist"
false
fi
}
But this test expects that .git/TAG_EDITMSG to be missing. When the
test is run with "-v" to make the output from this 'echo' visible,
we will keep getting the complaint when the test is happy, which is
not quite what we want.
What you want to use is test_path_is_missing, without "!".
> ## Notes (series) ##
> - I tried to maintain the proper formatting by using `clang-format` via Emacs on
> - the affected lines.
> + I duplicated this message (this isn’t obvious in the diff):
> +
> + fprintf(stderr,
> + _("The tag message has been left in %s\n"),
> + path);
> +
> + Should this be factored into a static function instead?
When the third copy is made, we would definitely insist avoiding
copies, but until then, I am indifferent. Others may have different
opinions, though.
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 0/3] tag: keep the message file in case ref transaction fails
2023-05-14 13:17 [PATCH 0/3] tag: keep the message file in case ref transaction fails Kristoffer Haugsbakk
` (3 preceding siblings ...)
2023-05-15 20:29 ` [PATCH v2 0/3] " Kristoffer Haugsbakk
@ 2023-05-16 17:55 ` Kristoffer Haugsbakk
2023-05-16 17:55 ` [PATCH v3 1/3] doc: tag: document `TAG_EDITMSG` Kristoffer Haugsbakk
` (3 more replies)
2023-05-17 9:32 ` [PATCH " Jeff King
5 siblings, 4 replies; 26+ messages in thread
From: Kristoffer Haugsbakk @ 2023-05-16 17:55 UTC (permalink / raw)
To: git; +Cc: peff, gitster, Kristoffer Haugsbakk
§ 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
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH v3 1/3] doc: tag: document `TAG_EDITMSG`
2023-05-16 17:55 ` [PATCH v3 " Kristoffer Haugsbakk
@ 2023-05-16 17:55 ` Kristoffer Haugsbakk
2023-05-16 17:55 ` [PATCH v3 2/3] t/t7004-tag: add regression test for successful tag creation Kristoffer Haugsbakk
` (2 subsequent siblings)
3 siblings, 0 replies; 26+ messages in thread
From: Kristoffer Haugsbakk @ 2023-05-16 17:55 UTC (permalink / raw)
To: git; +Cc: peff, gitster, Kristoffer Haugsbakk
Document `TAG_EDITMSG` which we have told the user about on unsuccessful
command invocations since commit 3927bbe9a4 (tag: delete TAG_EDITMSG
only on successful tag, 2008-12-06).
Introduce this documentation since we are going to add tests for the
lifetime of this file in the case of command failure and success.
Use the documentation for `COMMIT_EDITMSG` from `git-commit.txt` as a
template since these two files share the same purpose.[1]
† 1: from commit 3927bbe9a4:
“ This matches the behavior of COMMIT_EDITMSG, which stays around
in case of error.
Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
§ From v2
• I changed (from `COMMIT_EDITMSG`) “will be overwritten” to “may” since
I don’t see the point in giving a guarantee
• Unsure if I was going to put this before or after “NOTES” as there seems
to be no precedence
Suggested in: https://lore.kernel.org/git/xmqqy1lqaxln.fsf@gitster.g/T/#u
Documentation/git-tag.txt | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index fdc72b5875a..46e56b02455 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -377,6 +377,16 @@ $ GIT_COMMITTER_DATE="2006-10-02 10:31" git tag -s v1.0.1
include::date-formats.txt[]
+FILES
+-----
+
+`$GIT_DIR/TAG_EDITMSG`::
+ This file contains the message of an in-progress annotated
+ tag. If `git tag` exits due to an error before creating an
+ annotated tag then the tag message that has been provided by the
+ user in an editor session will be available in this file, but
+ may be overwritten by the next invocation of `git tag`.
+
NOTES
-----
--
2.40.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v3 2/3] t/t7004-tag: add regression test for successful tag creation
2023-05-16 17:55 ` [PATCH v3 " Kristoffer Haugsbakk
2023-05-16 17:55 ` [PATCH v3 1/3] doc: tag: document `TAG_EDITMSG` Kristoffer Haugsbakk
@ 2023-05-16 17:55 ` 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
3 siblings, 0 replies; 26+ messages in thread
From: Kristoffer Haugsbakk @ 2023-05-16 17:55 UTC (permalink / raw)
To: git; +Cc: peff, gitster, Kristoffer Haugsbakk
The standard tag message file is unlinked if the tag is created.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
See: https://lore.kernel.org/git/xmqq353x8de3.fsf@gitster.g/T/#m594378bc6fe2545e638ce2d5f9c11861d4e33082
> What you want to use is test_path_is_missing, without "!".
t/t7004-tag.sh | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 9aa1660651b..1cb738b00d2 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -2127,4 +2127,13 @@ test_expect_success 'Does --[no-]contains stop at commits? Yes!' '
test_cmp expected actual
'
+test_expect_success 'If tag is created then tag message file is unlinked' '
+ test_when_finished "git tag -d foo" &&
+ write_script fakeeditor <<-\EOF &&
+ echo Message >.git/TAG_EDITMSG
+ EOF
+ GIT_EDITOR=./fakeeditor git tag -a foo &&
+ test_path_is_missing .git/TAG_EDITMSG
+'
+
test_done
--
2.40.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v3 3/3] tag: keep the message file in case ref transaction fails
2023-05-16 17:55 ` [PATCH v3 " Kristoffer Haugsbakk
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 ` Kristoffer Haugsbakk
2023-05-16 18:39 ` [PATCH v3 0/3] " Junio C Hamano
3 siblings, 0 replies; 26+ messages in thread
From: Kristoffer Haugsbakk @ 2023-05-16 17:55 UTC (permalink / raw)
To: git; +Cc: peff, gitster, Kristoffer Haugsbakk
The ref transaction can fail after the user has written their tag
message. In particular, if there exists a tag `foo/bar` and `git tag -a
foo` is said then the command will only fail once it tries to write
`refs/tags/foo`, which is after the file has been unlinked.
Hold on to the message file for a little longer so that it is not
unlinked before the fatal error occurs.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
§ From v2
(Noting for other reviewers (after v2):)
I duplicated this message (this isn’t obvious in the diff):
fprintf(stderr,
_("The tag message has been left in %s\n"),
path);
builtin/tag.c | 24 +++++++++++++++---------
t/t7004-tag.sh | 10 ++++++++++
2 files changed, 25 insertions(+), 9 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index d428c45dc8d..7df9c143ac0 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -266,11 +266,10 @@ static const char message_advice_nested_tag[] =
static void create_tag(const struct object_id *object, const char *object_ref,
const char *tag,
struct strbuf *buf, struct create_tag_options *opt,
- struct object_id *prev, struct object_id *result)
+ struct object_id *prev, struct object_id *result, char *path)
{
enum object_type type;
struct strbuf header = STRBUF_INIT;
- char *path = NULL;
type = oid_object_info(the_repository, object, NULL);
if (type <= OBJ_NONE)
@@ -294,7 +293,6 @@ static void create_tag(const struct object_id *object, const char *object_ref,
int fd;
/* write the template message before editing: */
- path = git_pathdup("TAG_EDITMSG");
fd = xopen(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);
if (opt->message_given) {
@@ -336,10 +334,6 @@ static void create_tag(const struct object_id *object, const char *object_ref,
path);
exit(128);
}
- if (path) {
- unlink_or_warn(path);
- free(path);
- }
}
static void create_reflog_msg(const struct object_id *oid, struct strbuf *sb)
@@ -487,6 +481,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
};
int ret = 0;
const char *only_in_list = NULL;
+ char *path = NULL;
setup_ref_filter_porcelain_msg();
@@ -621,7 +616,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
if (create_tag_object) {
if (force_sign_annotate && !annotate)
opt.sign = 1;
- create_tag(&object, object_ref, tag, &buf, &opt, &prev, &object);
+ path = git_pathdup("TAG_EDITMSG");
+ create_tag(&object, object_ref, tag, &buf, &opt, &prev, &object,
+ path);
}
transaction = ref_transaction_begin(&err);
@@ -629,8 +626,17 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
ref_transaction_update(transaction, ref.buf, &object, &prev,
create_reflog ? REF_FORCE_CREATE_REFLOG : 0,
reflog_msg.buf, &err) ||
- ref_transaction_commit(transaction, &err))
+ ref_transaction_commit(transaction, &err)) {
+ if (path)
+ fprintf(stderr,
+ _("The tag message has been left in %s\n"),
+ path);
die("%s", err.buf);
+ }
+ if (path) {
+ unlink_or_warn(path);
+ free(path);
+ }
ref_transaction_free(transaction);
if (force && !is_null_oid(&prev) && !oideq(&prev, &object))
printf(_("Updated tag '%s' (was %s)\n"), tag,
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 1cb738b00d2..e6f9579cff7 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -2136,4 +2136,14 @@ test_expect_success 'If tag is created then tag message file is unlinked' '
test_path_is_missing .git/TAG_EDITMSG
'
+test_expect_success 'If tag cannot be created then tag message file is not unlinked' '
+ test_when_finished "git tag -d foo/bar && rm .git/TAG_EDITMSG" &&
+ write_script fakeeditor <<-\EOF &&
+ echo Message >.git/TAG_EDITMSG
+ EOF
+ git tag foo/bar &&
+ test_must_fail env GIT_EDITOR=./fakeeditor git tag -a foo &&
+ test_path_exists .git/TAG_EDITMSG
+'
+
test_done
--
2.40.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v3 0/3] tag: keep the message file in case ref transaction fails
2023-05-16 17:55 ` [PATCH v3 " Kristoffer Haugsbakk
` (2 preceding siblings ...)
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 ` Junio C Hamano
3 siblings, 0 replies; 26+ messages in thread
From: Junio C Hamano @ 2023-05-16 18:39 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git, peff
Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
> § 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.
Thanks, will queue. Let's merge it down to 'next' soonish.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/3] tag: keep the message file in case ref transaction fails
2023-05-14 13:17 [PATCH 0/3] tag: keep the message file in case ref transaction fails Kristoffer Haugsbakk
` (4 preceding siblings ...)
2023-05-16 17:55 ` [PATCH v3 " Kristoffer Haugsbakk
@ 2023-05-17 9:32 ` Jeff King
2023-05-17 16:00 ` Junio C Hamano
2023-05-17 19:58 ` Kristoffer Haugsbakk
5 siblings, 2 replies; 26+ messages in thread
From: Jeff King @ 2023-05-17 9:32 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Junio C Hamano, git
On Sun, May 14, 2023 at 03:17:57PM +0200, Kristoffer Haugsbakk wrote:
> 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.)
> [...]
>
> So I added only Jeff King based on commit 3927bbe9a4 (tag: delete TAG_EDITMSG
> only on successful tag, 2008-12-06).
It has definitely been a while since that one. :) I agree what you are
doing here is in the same spirit as that commit, though there is one
small difference. 3927bbe9a4 was about saving the message file if we
failed to create the object, since otherwise there is no record of it.
Your patch is about saving it from a failed ref update. But in that case
we _do_ have the data saved in the object. You can find it with
git-fsck:
[after running something like your test script]
$ git fsck
Checking object directories: 100% (256/256), done.
dangling tag b9bde516952a863690027a611575a79d4c786b8d
$ git cat-file tag b9bde516952a863690027a611575a79d4c786b8d
object 8b1e708e32ae3af17825b3c713a3ab317824b932
type commit
tag foo/bar
tagger Jeff King <peff@peff.net> 1684314980 -0400
my message
That said, I'm willing to believe that most users wouldn't figure this
out on their own, and saving TAG_EDITMSG might be more friendly. But one
other alternative might be to mention the hash of that tag object, and
possibly give advice on recovering it.
It's too bad we do not have "git tag -c" to match "git commit -c", which
would allow us to say something like:
A tag object was created, but we failed to update the ref.
After correcting any errors, you can recover the original tag
message with:
git tag -c $oid [other options]
(where we'd replace $oid with the hash of the created tag object). The
best alternatives I could come up with were:
# this is kind of obscure advice to give somebody, plus
# it makes a weird tag if you originally tried to tag "foo/bar"
# but then later switch to some other name. The "tag" field
# in the object won't match the ref.
git update-ref $ref $oid
# this saves just the message but is horribly complicated
git cat-file tag $oid | sed '1,/^$/d' |
git tag -F -
I dunno. There is a certain elegance to telling the user about what
progress we _did_ make, but if there isn't an easy way to turn that into
a retry of their command, it may not be all that useful.
So I'm not really opposed to your patch, but mostly just brainstorming
alternatives.
I gave v3 a brief look, and it seems OK to me. It might be nice to
factor out the duplicated advice message, but since we cannot share any
of the other lines (one spot calls exit() and the other uses die()), I
am OK with it as-is.
-Peff
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 0/3] tag: keep the message file in case ref transaction fails
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 19:58 ` Kristoffer Haugsbakk
1 sibling, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2023-05-17 16:00 UTC (permalink / raw)
To: Jeff King; +Cc: Kristoffer Haugsbakk, git
Jeff King <peff@peff.net> writes:
> That said, I'm willing to believe that most users wouldn't figure this
> out on their own, and saving TAG_EDITMSG might be more friendly. But one
> other alternative might be to mention the hash of that tag object, and
> possibly give advice on recovering it.
Hmph, then the advice message would suggest "update-ref"?
Ah, no. Because the message may be reused to create a tag with
different tagname, which is very likely because one reason for the
refusal to update the ref could be that the name was already taken,
and that would create a mismatch between tagname and refname.
OK, so ...
> It's too bad we do not have "git tag -c" to match "git commit -c", which
> would allow us to say something like:
>
> A tag object was created, but we failed to update the ref.
> After correcting any errors, you can recover the original tag
> message with:
>
> git tag -c $oid [other options]
>
> (where we'd replace $oid with the hash of the created tag object). The
> best alternatives I could come up with were:
>
> # this is kind of obscure advice to give somebody, plus
> # it makes a weird tag if you originally tried to tag "foo/bar"
> # but then later switch to some other name. The "tag" field
> # in the object won't match the ref.
> git update-ref $ref $oid
... I agree that this is not a very good advice to give, and ...
> # this saves just the message but is horribly complicated
> git cat-file tag $oid | sed '1,/^$/d' |
> git tag -F -
... this is a reasonable thing to have in a more user-friendly
feature, like your -c above.
> I dunno. There is a certain elegance to telling the user about what
> progress we _did_ make, but if there isn't an easy way to turn that into
> a retry of their command, it may not be all that useful.
Yeah, I am OK with "leaving TAG_EDITMSG behind" and a future "tag
-c/-C $another" to coexist.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/3] tag: keep the message file in case ref transaction fails
2023-05-17 16:00 ` Junio C Hamano
@ 2023-05-17 17:37 ` Jeff King
2023-05-17 17:52 ` Eric Sunshine
0 siblings, 1 reply; 26+ messages in thread
From: Jeff King @ 2023-05-17 17:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kristoffer Haugsbakk, git
On Wed, May 17, 2023 at 09:00:10AM -0700, Junio C Hamano wrote:
> > I dunno. There is a certain elegance to telling the user about what
> > progress we _did_ make, but if there isn't an easy way to turn that into
> > a retry of their command, it may not be all that useful.
>
> Yeah, I am OK with "leaving TAG_EDITMSG behind" and a future "tag
> -c/-C $another" to coexist.
Me too.
One thing I wondered is whether the obvious command to retry:
git tag -F .git/TAG_EDITMSG foo
would work, or if we would overwrite the file before it is read. But it
does work, which is good. I wonder if we:
a. want to protect that with a test (since I could imagine a
refactoring where we try to copy the "-F" contents from file to
file, rather than reading it into a memory buffer ahead of time)
b. want to tell users that is a good way to recover (though maybe that
is a rabbit hole of details as one subtlety is that it will be
overwritten by an unrelated tag command).
But I am also happy to leave it to the user's imagination to pull the
contents from the file with "cp" or their editor. :)
-Peff
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 0/3] tag: keep the message file in case ref transaction fails
2023-05-17 17:37 ` Jeff King
@ 2023-05-17 17:52 ` Eric Sunshine
2023-05-17 18:02 ` Kristoffer Haugsbakk
0 siblings, 1 reply; 26+ messages in thread
From: Eric Sunshine @ 2023-05-17 17:52 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Kristoffer Haugsbakk, git
On Wed, May 17, 2023 at 1:40 PM Jeff King <peff@peff.net> wrote:
> One thing I wondered is whether the obvious command to retry:
>
> git tag -F .git/TAG_EDITMSG foo
>
> would work, or if we would overwrite the file before it is read. But it
> does work, which is good. I wonder if we:
>
> b. want to tell users that is a good way to recover (though maybe that
> is a rabbit hole of details as one subtlety is that it will be
> overwritten by an unrelated tag command).
If that route is taken, then making the advice worktree-friendly would
probably be a good idea:
git tag -F $(git rev-parse --git-path TAG_EDITMSG) foo
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 0/3] tag: keep the message file in case ref transaction fails
2023-05-17 17:52 ` Eric Sunshine
@ 2023-05-17 18:02 ` Kristoffer Haugsbakk
0 siblings, 0 replies; 26+ messages in thread
From: Kristoffer Haugsbakk @ 2023-05-17 18:02 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Junio C Hamano, git, Jeff King
On Wed, May 17, 2023, at 19:52, Eric Sunshine wrote:
> If that route is taken, then making the advice worktree-friendly would
> probably be a good idea:
>
> git tag -F $(git rev-parse --git-path TAG_EDITMSG) foo
The current advice works with worktrees since it just uses `path`.
--
Kristoffer Haugsbakk
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/3] tag: keep the message file in case ref transaction fails
2023-05-17 9:32 ` [PATCH " Jeff King
2023-05-17 16:00 ` Junio C Hamano
@ 2023-05-17 19:58 ` Kristoffer Haugsbakk
1 sibling, 0 replies; 26+ messages in thread
From: Kristoffer Haugsbakk @ 2023-05-17 19:58 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
On Wed, May 17, 2023, at 11:32, Jeff King wrote:
> It has definitely been a while since that one. :) I agree what you are
> doing here is in the same spirit as that commit, though there is one
> small difference. 3927bbe9a4 was about saving the message file if we
> failed to create the object, since otherwise there is no record of it.
>
> Your patch is about saving it from a failed ref update. But in that case
> we _do_ have the data saved in the object. You can find it with
> git-fsck:
I didn’t even think of that. :) Too much tunnel-focus on that text file…
>
> [after running something like your test script]
> $ git fsck
> Checking object directories: 100% (256/256), done.
> dangling tag b9bde516952a863690027a611575a79d4c786b8d
>
> $ git cat-file tag b9bde516952a863690027a611575a79d4c786b8d
> object 8b1e708e32ae3af17825b3c713a3ab317824b932
> type commit
> tag foo/bar
> tagger Jeff King <peff@peff.net> 1684314980 -0400
>
> my message
>
> That said, I'm willing to believe that most users wouldn't figure this
> out on their own, and saving TAG_EDITMSG might be more friendly.
The current error says:
```
fatal: cannot lock ref 'refs/tags/release': 'refs/tags/release/v1' exists; cannot create 'refs/tags/release'
```
As a user, I have no idea if anything (the tag object) was even
created. The error just says that the tag couldn’t be created. (As a
user the ref pointing to the annotated tag object and the object itself
sound like basically the same thing.)
I might be able to guess that it *had* created something tangible like
an object in the database since it got all the way to update the ref to
point to it (just one more step…). But that feels like a stretch.
> But one other alternative might be to mention the hash of that tag
> object, and possibly give advice on recovering it.
That could work. Like:
```
hint: git tag <new name> f73e152cb1560aff1446b208affba4cdcf5bea36
```
So I copy that into my prompt and update the name. All is good.
But I might decide to be just copy the hash, press “Up” in the terminal,
copy the hash at the end, and change the name:
```
git tag -a release-it f73e152cb1560aff1446b208affba4cdcf5bea36
```
Next I’m in my text editor again. Not sure what that is about but okay.
*closes it*
```
hint: You have created a nested tag. The object referred to by your new tag is
hint: already a tag. If you meant to tag the object that it points to, use:
hint:
hint: git tag -f release-it f73e152cb1560aff1446b208affba4cdcf5bea36^{}
hint: Disable this message with "git config advice.nestedTag false"
```
> It's too bad we do not have "git tag -c" to match "git commit -c", which
> would allow us to say something like:
Sure. I appreciate that kind of symmetry. (I guess that’s not the most
precise word in this context.)
> I dunno. There is a certain elegance to telling the user about what
> progress we _did_ make, but if there isn't an easy way to turn that into
> a retry of their command, it may not be all that useful.
As a user, the only expensive part (potentially; refer back to footnote
№ 2 of the CV) is writing the tag message. So fudging around a bit with
copying the backup text file into the new message (I would probably just
copy it into the editor) is not a big deal. And retrying the command is
just “Up” and a little line editing.
> So I'm not really opposed to your patch, but mostly just brainstorming
> alternatives.
It’s very much appreciated.
> I gave v3 a brief look, and it seems OK to me. It might be nice to
> factor out the duplicated advice message, but since we cannot share any
> of the other lines (one spot calls exit() and the other uses die()), I
> am OK with it as-is.
>
> -Peff
I’ll make a function for the message in the next round. It’s nice to not
have to duplicate such strings/message ids, I think.
Cheers
--
Kristoffer Haugsbakk
^ permalink raw reply [flat|nested] 26+ messages in thread