git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Michael Haggerty <mhagger@alum.mit.edu>
To: Stefan Beller <sbeller@google.com>
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"Ronnie Sahlberg" <ronniesahlberg@gmail.com>,
	"Jonathan Nieder" <jrnieder@gmail.com>,
	"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
	"git@vger.kernel.org" <git@vger.kernel.org>
Subject: Re: [PATCH 6/8] reflog_expire(): ignore --updateref for symbolic references
Date: Thu, 12 Feb 2015 17:08:53 +0100	[thread overview]
Message-ID: <54DCD015.2080002@alum.mit.edu> (raw)
In-Reply-To: <CAGZ79kZvM4FeHQ074kh7qhsz8cHgGaHxOruP7CM2DgPJErkA-w@mail.gmail.com>

On 02/11/2015 01:44 AM, Stefan Beller wrote:
> On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@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?

A symbolic reference can have a reflog just like a normal reference can.

When a reference is updated through a symbolic reference, then
write_ref_sha1() writes a reflog entry for both the reference and the
symbolic reference. Also (as an extra kludge), if *any* reference is
updated directly and it happens to be the current HEAD reference, then
an entry is added to HEAD's reflog.

"HEAD" is the only symbolic reference that is ever transferred across
repositories.

Symbolic references are always stored loose (i.e., not in packed-refs).

Does that answer your questions?

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

This is a pretty exotic usage. I can't think of any real-life use case
for using "--updateref" together with a symbolic reference. In our
entire code base, "--updateref" is only used a single time, in
"git-stash.sh", and that is always for "refs/stash", which is never a
symbolic reference. "git-stash" itself is implemented in a very stylized
way ("stylized" being a polite way of saying "bizarre"), and I doubt
that there are many more users of this option in the wild, let alone
"--updateref" together with a symbolic reference.

So, honestly, I don't think it is worth the effort of deciding between 3
vs. 4. Since 4 is easier to implement (and already implemented), I'd
rather leave it as is. If you want to submit a patch implementing 3, I
won't argue against it.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

  reply	other threads:[~2015-02-12 16:09 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-02-09  9:12 [PATCH 0/8] Fix some problems with reflog expiration Michael Haggerty
2015-02-09  9:12 ` [PATCH 1/8] write_ref_sha1(): remove check for lock == NULL Michael Haggerty
2015-02-10 22:52   ` Stefan Beller
2015-02-11  0:06     ` Jeff King
2015-02-09  9:12 ` [PATCH 2/8] write_ref_sha1(): Move write elision test to callers Michael Haggerty
2015-02-12 19:58   ` Junio C Hamano
2015-02-09  9:12 ` [PATCH 3/8] lock_ref_sha1_basic(): do not set force_write for missing references Michael Haggerty
2015-02-10 23:24   ` Stefan Beller
2015-02-11  0:05     ` Jeff King
2015-02-11  0:07       ` Stefan Beller
2015-02-12 12:09       ` Michael Haggerty
2015-02-09  9:12 ` [PATCH 4/8] reflog: fix documentation Michael Haggerty
2015-02-10 23:25   ` Stefan Beller
2015-02-09  9:12 ` [PATCH 5/8] reflog: rearrange the manpage Michael Haggerty
2015-02-10 23:42   ` Stefan Beller
2015-02-12 15:17     ` Michael Haggerty
2015-02-12 20:09       ` Junio C Hamano
2015-02-09  9:12 ` [PATCH 6/8] reflog_expire(): ignore --updateref for symbolic references Michael Haggerty
2015-02-11  0:44   ` Stefan Beller
2015-02-12 16:08     ` Michael Haggerty [this message]
2015-02-12 17:04       ` Stefan Beller
2015-02-12 20:16       ` Junio C Hamano
2015-02-12 21:54   ` Jeff King
2015-02-13 14:34     ` Michael Haggerty
2015-02-09  9:12 ` [PATCH 7/8] reflog_expire(): never update a reference to null_sha1 Michael Haggerty
2015-02-09 20:55   ` Eric Sunshine
2015-02-12 11:51     ` Michael Haggerty
2015-02-09  9:12 ` [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent Michael Haggerty
2015-02-11  0:49   ` Stefan Beller
2015-02-11 22:49     ` Junio C Hamano
2015-02-11 23:25       ` Stefan Beller
2015-02-12 16:52         ` Michael Haggerty
2015-02-12 18:04           ` Stefan Beller
2015-02-13 16:26             ` Michael Haggerty
2015-02-13 17:16               ` Stefan Beller
2015-02-13 18:05               ` Junio C Hamano
2015-02-13 18:21                 ` Stefan Beller
2015-02-13 18:26                   ` Junio C Hamano
2015-02-13 18:32                     ` Stefan Beller
2015-02-13 19:12                       ` Junio C Hamano
2015-02-13 20:11                         ` Michael Haggerty
2015-02-13 21:53                           ` Junio C Hamano
2015-02-14  5:58                             ` Michael Haggerty
2015-02-09 18:57 ` [PATCH 0/8] Fix some problems with reflog expiration Stefan Beller
2015-02-10 23:12 ` [PATCH] refs.c: get rid of force_write flag Stefan Beller
2015-02-12 15:35   ` Michael Haggerty

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=54DCD015.2080002@alum.mit.edu \
    --to=mhagger@alum.mit.edu \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jrnieder@gmail.com \
    --cc=pclouds@gmail.com \
    --cc=ronniesahlberg@gmail.com \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).