patch, 3 partstag: keep the message file in case ref transaction fails

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/4972149825

§ Carbon copy

The suggested contacts seemed too long:

``` $ ./contrib/contacts/git-contacts v2.40.1.. Junio C Hamano <> René Scharfe <> Jonathan Tan <> Jonathan Nieder <> Jeff King <> Stefan Beller <> Denton Liu <> Robert Dailey <> ```

So I added only Jeff King based on commit 3927bbe9a4 (tag: delete TAG_EDITMSG only on successful tag, 2008-12-06).

§ Patches

1. Test the happy case
2. Test the unhappy case (fails without the next patch)
3. The change

—

Cheers.

Kristoffer Haugsbakk (3):
  t/t7004-tag: add regression test for existing behavior
  t/t7004-tag: add failing tag message file test
  tag: keep the message file in case ref transaction fails

 builtin/tag.c  | 24 +++++++++++++++---------
 t/t7004-tag.sh | 19 +++++++++++++++++++
 2 files changed, 34 insertions(+), 9 deletions(-)

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(+)

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

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.

I'd recommend using "test_path_is_missing" instead for better test-debugging experience.

Thanks.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 t/t7004-tag.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

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.

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".

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.

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

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.

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".

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.

On Mon, May 15, 2023, at 09:00, Junio C Hamano wrote:

Yep, I was going for the latter. I’ll combine [2/3] and [3/3].

Ah, I was wondering how to use `test_must_fail` on a command with an environment variable. Thanks.

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(-)

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

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.

§ 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

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(+)

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

Excellent.

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(+)

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(-)

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

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 "!".

When the third copy is made, we would definitely insist avoiding copies, but until then, I am indifferent. Others may have different opinions, though.

§ 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' '

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(+)

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(+)

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(-)

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

Thanks, will queue.  Let's merge it down to 'next' soonish.

On Sun, May 14, 2023 at 03:17:57PM +0200, Kristoffer Haugsbakk 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:

  [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

Jeff King <peff@peff.net> writes:

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 ...

... I agree that this is not a very good advice to give, and ...

... this is a reasonable thing to have in a more user-friendly feature, like your -c above.

Yeah, I am OK with "leaving TAG_EDITMSG behind" and a future "tag -c/-C $another" to coexist.

On Wed, May 17, 2023 at 09:00:10AM -0700, Junio C Hamano wrote:

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

On Wed, May 17, 2023 at 1:40 PM Jeff King <peff@peff.net> 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

On Wed, May 17, 2023, at 19:52, Eric Sunshine wrote:

The current advice works with worktrees since it just uses `path`.

On Wed, May 17, 2023, at 11:32, Jeff King wrote:

I didn’t even think of that. :) Too much tunnel-focus on that text file…

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.

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" ```

Sure. I appreciate that kind of symmetry. (I guess that’s not the most precise word in this context.)

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.

It’s very much appreciated.

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