On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhag...@alum.mit.edu> 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 <mhag...@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
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
