Re: [PATCH 3/8] lock_ref_sha1_basic(): do not set force_write for missing references
On 02/11/2015 01:05 AM, Jeff King wrote:
> On Tue, Feb 10, 2015 at 03:24:47PM -0800, Stefan Beller wrote:
>
>> On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty
>> wrote:
>>> If a reference is missing, its SHA-1 will be null_sha1, which can't
>>> possibly match a new value that ref_transaction_commit() is trying to
>>> update it to. So there is no need to set force_write in this scenario.
>>>
>>
>> This commit reverts half the lines of 5bdd8d4a3062a (2008-11, do not
>> force write of packed refs). And reading both commit messages, they
>> seem to contradict each other. (Both agree on "If a reference is
>> missing, its SHA-1 will be null_sha1 as provided by resolve_ref", but
>> the conclusion seems to be different.)
>
> Most of the lines of 5bdd8d4a3062a that are being reverted here are
> caching the is_null_sha1() check in the "missing" variable. And that's
> a cleanup in this patch that is not strictly necessary ("missing" would
> only be used once, so it becomes noise).
>
> The interesting thing in the earlier commit was to use the null sha1 to
> cause a force-write, rather than lstat()ing the filesystem. And here we
> are saying the force-write is not necessary at all, no matter what
> storage scheme is used. So I don't think there is any contradiction
> between the two.
>
> Is this patch correct that the force-write is not necessary? I think so.
> The force-write flag comes from:
>
> commit 732232a123e1e61e38babb1c572722bb8a189ba3
> Author: Shawn Pearce
> Date: Fri May 19 03:29:05 2006 -0400
>
> Force writing ref if it doesn't exist.
>
> Normally we try to skip writing a ref if its value hasn't changed
> but in the special case that the ref doesn't exist but the new
> value is going to be 0{40} then force writing the ref anyway.
>
> but I am not sure that logic still holds (if it ever did). We do not ever
> write
> 0{40} into a ref value.
I don't understand that old commit, either. Maybe there was an idea of
storing 0{40} in a loose ref file to mark a packed reference as deleted?
CC to Shawn Pearce in case he can shed some light on the situation.
I still think that my change is OK, because we definitely don't want to
write 0{40} to any loose reference file. The reference-reading code
can't deal with it, so this would break the repository.
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
--
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
Re: [PATCH 3/8] lock_ref_sha1_basic(): do not set force_write for missing references
On Tue, Feb 10, 2015 at 4:05 PM, Jeff King wrote:
> On Tue, Feb 10, 2015 at 03:24:47PM -0800, Stefan Beller wrote:
>
>> On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty
>> wrote:
>> > If a reference is missing, its SHA-1 will be null_sha1, which can't
>> > possibly match a new value that ref_transaction_commit() is trying to
>> > update it to. So there is no need to set force_write in this scenario.
>> >
>>
>> This commit reverts half the lines of 5bdd8d4a3062a (2008-11, do not
>> force write of packed refs). And reading both commit messages, they
>> seem to contradict each other. (Both agree on "If a reference is
>> missing, its SHA-1 will be null_sha1 as provided by resolve_ref", but
>> the conclusion seems to be different.)
>
> Most of the lines of 5bdd8d4a3062a that are being reverted here are
> caching the is_null_sha1() check in the "missing" variable. And that's
> a cleanup in this patch that is not strictly necessary ("missing" would
> only be used once, so it becomes noise).
>
> The interesting thing in the earlier commit was to use the null sha1 to
> cause a force-write, rather than lstat()ing the filesystem. And here we
> are saying the force-write is not necessary at all, no matter what
> storage scheme is used. So I don't think there is any contradiction
> between the two.
>
> Is this patch correct that the force-write is not necessary? I think so.
> The force-write flag comes from:
>
> commit 732232a123e1e61e38babb1c572722bb8a189ba3
> Author: Shawn Pearce
> Date: Fri May 19 03:29:05 2006 -0400
>
> Force writing ref if it doesn't exist.
>
> Normally we try to skip writing a ref if its value hasn't changed
> but in the special case that the ref doesn't exist but the new
> value is going to be 0{40} then force writing the ref anyway.
>
> but I am not sure that logic still holds (if it ever did). We do not ever
> write
> 0{40} into a ref value.
>
> -Peff
Makes sense.
--
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
Re: [PATCH 3/8] lock_ref_sha1_basic(): do not set force_write for missing references
On Tue, Feb 10, 2015 at 03:24:47PM -0800, Stefan Beller wrote:
> On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty wrote:
> > If a reference is missing, its SHA-1 will be null_sha1, which can't
> > possibly match a new value that ref_transaction_commit() is trying to
> > update it to. So there is no need to set force_write in this scenario.
> >
>
> This commit reverts half the lines of 5bdd8d4a3062a (2008-11, do not
> force write of packed refs). And reading both commit messages, they
> seem to contradict each other. (Both agree on "If a reference is
> missing, its SHA-1 will be null_sha1 as provided by resolve_ref", but
> the conclusion seems to be different.)
Most of the lines of 5bdd8d4a3062a that are being reverted here are
caching the is_null_sha1() check in the "missing" variable. And that's
a cleanup in this patch that is not strictly necessary ("missing" would
only be used once, so it becomes noise).
The interesting thing in the earlier commit was to use the null sha1 to
cause a force-write, rather than lstat()ing the filesystem. And here we
are saying the force-write is not necessary at all, no matter what
storage scheme is used. So I don't think there is any contradiction
between the two.
Is this patch correct that the force-write is not necessary? I think so.
The force-write flag comes from:
commit 732232a123e1e61e38babb1c572722bb8a189ba3
Author: Shawn Pearce
Date: Fri May 19 03:29:05 2006 -0400
Force writing ref if it doesn't exist.
Normally we try to skip writing a ref if its value hasn't changed
but in the special case that the ref doesn't exist but the new
value is going to be 0{40} then force writing the ref anyway.
but I am not sure that logic still holds (if it ever did). We do not ever write
0{40} into a ref value.
-Peff
--
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
Re: [PATCH 3/8] lock_ref_sha1_basic(): do not set force_write for missing references
On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty wrote:
> If a reference is missing, its SHA-1 will be null_sha1, which can't
> possibly match a new value that ref_transaction_commit() is trying to
> update it to. So there is no need to set force_write in this scenario.
>
This commit reverts half the lines of 5bdd8d4a3062a (2008-11, do not
force write of packed refs). And reading both commit messages, they
seem to contradict each other. (Both agree on "If a reference is
missing, its SHA-1 will be null_sha1 as provided by resolve_ref", but
the conclusion seems to be different.)
On the other hand, there is more than 6 years difference, so I guess
the meaning and implications of some variables and functions may have
slightly changed.
> Signed-off-by: Michael Haggerty
> ---
> refs.c | 15 ++-
> 1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 651e37e..b083858 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2259,7 +2259,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char
> *refname,
> int type, lflags;
> int mustexist = (old_sha1 && !is_null_sha1(old_sha1));
> int resolve_flags = 0;
> - int missing = 0;
> int attempts_remaining = 3;
>
> lock = xcalloc(1, sizeof(struct ref_lock));
> @@ -2298,13 +2297,13 @@ static struct ref_lock *lock_ref_sha1_basic(const
> char *refname,
> orig_refname, strerror(errno));
> goto error_return;
> }
> - missing = is_null_sha1(lock->old_sha1);
> - /* When the ref did not exist and we are creating it,
> -* make sure there is no existing ref that is packed
> -* whose name begins with our refname, nor a ref whose
> -* name is a proper prefix of our refname.
> + /*
> +* When the ref did not exist and we are creating it, make
> +* sure there is no existing packed ref whose name begins with
> +* our refname, nor a packed ref whose name is a proper prefix
> +* of our refname.
> */
> - if (missing &&
> + if (is_null_sha1(lock->old_sha1) &&
> !is_refname_available(refname, skip,
> get_packed_refs(&ref_cache))) {
> last_errno = ENOTDIR;
> goto error_return;
> @@ -2320,8 +2319,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char
> *refname,
> lock->ref_name = xstrdup(refname);
> lock->orig_ref_name = xstrdup(orig_refname);
> ref_file = git_path("%s", refname);
> - if (missing)
> - lock->force_write = 1;
> if ((flags & REF_NODEREF) && (type & REF_ISSYMREF))
> lock->force_write = 1;
>
> --
> 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
