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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote:

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

>
> Note: there are still other problems in this code that will be fixed
> in a moment.
>
> Signed-off-by: Michael Haggerty <mhagger@xxxxxxxxxxxx>
> ---
>  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
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]