git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 6/8] reflog_expire(): ignore --updateref for symbolic references

From
Stefan Beller <sbeller@google.com>
Date
Feb 11, 2015, 00:44 UTC
Message-ID
<CAGZ79kZvM4FeHQ074kh7qhsz8cHgGaHxOruP7CM2DgPJErkA-w@mail.gmail.com>
In-Reply-To
<1423473164-6011-7-git-send-email-mhagger@alum.mit.edu>
On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
Show 19 quoted lines
> If we are expiring reflog entries for a symbolic reference, then how
> should --updateref be handled if the newest reflog entry is expired?
>
> Option 1: Update the referred-to reference. (This is what the current
> code does.) This doesn't make sense, because the referred-to reference
> has its own reflog, which hasn't been rewritten.
>
> Option 2: Update the symbolic reference itself (as in, REF_NODEREF).
> This would convert the symbolic reference into a non-symbolic
> reference (e.g., detaching HEAD), which is surely not what a user
> would expect.
>
> Option 3: Error out. This is plausible, but it would make the
> following usage impossible:
>
>     git reflog expire ... --updateref --all
>
> Option 4: Ignore --updateref for symbolic references.
>
Ok let me ask a question first about the symbolic refs.

We used to use symbolic links for that, but because of portability issues we decided to not make it a link, but rather a standard file containing the pointing link (The content of .git/HEAD is "ref: refs/heads/master\n" except when detached)

So this is the only distinction? Or is there also a concept of symbolic links/pointers for the reflog handling?

> We choose to implement option 4.

You're only saying why the other options are insane, not why this is sane.

Also I'd rather tend for option 3 than 4, as it is a safety measure (assuming we give a hint to the user what the problem is, and how it is circumventable)

Show 77 quoted lines
>
> Note: there are still other problems in this code that will be fixed
> in a moment.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  Documentation/git-reflog.txt |  3 ++-
>  refs.c                       | 15 ++++++++++++---
>  2 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt
> index f15a48e..9b87b46 100644
> --- a/Documentation/git-reflog.txt
> +++ b/Documentation/git-reflog.txt
> @@ -85,7 +85,8 @@ them.
>
>  --updateref::
>         Update the ref with the sha1 of the top reflog entry (i.e.
> -       <ref>@\{0\}) after expiring or deleting.
> +       <ref>@\{0\}) after expiring or deleting.  (This option is
> +       ignored for symbolic references.)
>
>  --rewrite::
>         While expiring or deleting, adjust each reflog entry to ensure
> diff --git a/refs.c b/refs.c
> index b083858..c0001da 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -4025,6 +4025,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
>         struct ref_lock *lock;
>         char *log_file;
>         int status = 0;
> +       int type;
>
>         memset(&cb, 0, sizeof(cb));
>         cb.flags = flags;
> @@ -4036,7 +4037,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
>          * reference itself, plus we might need to update the
>          * reference if --updateref was specified:
>          */
> -       lock = lock_ref_sha1_basic(refname, sha1, NULL, 0, NULL);
> +       lock = lock_ref_sha1_basic(refname, sha1, NULL, 0, &type);
>         if (!lock)
>                 return error("cannot lock ref '%s'", refname);
>         if (!reflog_exists(refname)) {
> @@ -4073,10 +4074,18 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
>         (*cleanup_fn)(cb.policy_cb);
>
>         if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
> +               /*
> +                * It doesn't make sense to adjust a reference pointed
> +                * to by a symbolic ref based on expiring entries in
> +                * the symbolic reference's reflog.
> +                */
> +               int update = (flags & EXPIRE_REFLOGS_UPDATE_REF) &&
> +                       ~(type & REF_ISSYMREF);
> +
>                 if (close_lock_file(&reflog_lock)) {
>                         status |= error("couldn't write %s: %s", log_file,
>                                         strerror(errno));
> -               } else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&
> +               } else if (update &&
>                         (write_in_full(lock->lock_fd,
>                                 sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
>                          write_str_in_full(lock->lock_fd, "\n") != 1 ||
> @@ -4087,7 +4096,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
>                 } else if (commit_lock_file(&reflog_lock)) {
>                         status |= error("unable to commit reflog '%s' (%s)",
>                                         log_file, strerror(errno));
> -               } else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) && commit_ref(lock)) {
> +               } else if (update && commit_ref(lock)) {
>                         status |= error("couldn't set %s", lock->ref_name);
>                 }
>         }
> --
> 2.1.4
>
Previous: Michael HaggertyNext: Michael Haggerty
Message 19 of 46 in “Fix some problems with reflog expiration”
  1. 0/8 Fix some problems with reflog expirationMichael Haggerty, Feb 9, 2015
  2. 1/8 write_ref_sha1(): remove check for lock == NULLMichael Haggerty, Feb 9, 2015
  3. Stefan BellerFeb 10, 2015
  4. Jeff KingFeb 11, 2015
  5. 2/8 write_ref_sha1(): Move write elision test to callersMichael Haggerty, Feb 9, 2015
  6. Junio C HamanoFeb 12, 2015
  7. 3/8 lock_ref_sha1_basic(): do not set force_write for missing referencesMichael Haggerty, Feb 9, 2015
  8. Stefan BellerFeb 10, 2015
  9. Jeff KingFeb 11, 2015
  10. Stefan BellerFeb 11, 2015
  11. Michael HaggertyFeb 12, 2015
  12. 4/8 reflog: fix documentationMichael Haggerty, Feb 9, 2015
  13. Stefan BellerFeb 10, 2015
  14. 5/8 reflog: rearrange the manpageMichael Haggerty, Feb 9, 2015
  15. Stefan BellerFeb 10, 2015
  16. Michael HaggertyFeb 12, 2015
  17. Junio C HamanoFeb 12, 2015
  18. 6/8 reflog_expire(): ignore --updateref for symbolic referencesMichael Haggerty, Feb 9, 2015
  19. Stefan BellerFeb 11, 2015
  20. Michael HaggertyFeb 12, 2015
  21. Stefan BellerFeb 12, 2015
  22. Junio C HamanoFeb 12, 2015
  23. Jeff KingFeb 12, 2015
  24. Michael HaggertyFeb 13, 2015
  25. 7/8 reflog_expire(): never update a reference to null_sha1Michael Haggerty, Feb 9, 2015
  26. Eric SunshineFeb 9, 2015
  27. Michael HaggertyFeb 12, 2015
  28. 8/8 reflog_expire(): lock symbolic refs themselves, not their referentMichael Haggerty, Feb 9, 2015
  29. Stefan BellerFeb 11, 2015
  30. Junio C HamanoFeb 11, 2015
  31. Stefan BellerFeb 11, 2015
  32. Michael HaggertyFeb 12, 2015
  33. Stefan BellerFeb 12, 2015
  34. Michael HaggertyFeb 13, 2015
  35. Stefan BellerFeb 13, 2015
  36. Junio C HamanoFeb 13, 2015
  37. Stefan BellerFeb 13, 2015
  38. Junio C HamanoFeb 13, 2015
  39. Stefan BellerFeb 13, 2015
  40. Junio C HamanoFeb 13, 2015
  41. Michael HaggertyFeb 13, 2015
  42. Junio C HamanoFeb 13, 2015
  43. Michael HaggertyFeb 14, 2015
  44. Stefan BellerFeb 9, 2015
  45. refs.c: get rid of force_write flagStefan Beller, Feb 10, 2015
  46. Michael HaggertyFeb 12, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.