[PATCH 00/29] Yet more preparation for reference backends

classic Classic list List threaded Threaded
70 messages Options
1234
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 05/29] refname_is_safe(): insist that the refname already be normalized

Jeff King
On Wed, Apr 27, 2016 at 04:34:53PM -0400, David Turner wrote:

> > I thought the point is that one is a lesser check than the other, and
> > we
> > need different rules for different situations. So we might allow
> > deletion on a refname that does not pass check_refname_format(), but
> > we
> > must make sure it is not going to cause any mischief (e.g., escaping
> > ".git" and deleting random files).
> >
> > But anything writing a _new_ refname (whether the actual ref, or
> > referencing it via a symref) should be using check_refname_format()
> > before writing.
>
> Unfortunately, neither check is lesser -- refname_is_safe allows
> refs/heads//foo but not a/b while check_refname_format allows a/b but
> not refs/heads//foo.  So sometimes we need both, while other times we
> just need one :(

IMHO, that sounds like a bug. check_refname_format() should
conceptually[1] be a superset of refname_is_safe(). Is there a case
where we would want to _allow_ a refname that is not safe to look at on
disk?

-Peff

[1] The implementation can be a direct call, or can simply implement a
    superset of the rules, if that's more efficient.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning

Junio C Hamano
In reply to this post by David Turner
David Turner <[hidden email]> writes:

> On Wed, 2016-04-27 at 11:47 -0700, Junio C Hamano wrote:
>> Michael Haggerty <[hidden email]> writes:
>>
>> > It is nonsensical (and a little bit dangerous) to use REF_ISPRUNING
>> > without REF_NODEREF. Forbid it explicitly. Change the one
>> > REF_ISPRUNING
>> > caller to pass REF_NODEREF too.
>> >
>> > Signed-off-by: Michael Haggerty <[hidden email]>
>> > ---
>> > This also makes later patches a bit clearer.
>>
>> I wonder if it is more future-proof to solve this by doing
>>
>>     -#define REF_ISPRUNING 0x04
>>     +#define REF_ISPRUNING (0x04 | REF_NODEREF)
>>
>> instead.  It makes the intention clear that pruning is always about
>> the single level (i.e. no-deref).
>
> I think the approach in Michael's patch might be clearer than yours,
> since someone reading the code doesn't have to look at the definition
> of REF_ISPRUNING to understand what is going on.

I have to strongly disagree, assuming that my understanding of the
point of this patch correctly.

If a casual reader sees this code:

    ref_transaction_delete(transaction, r->name, r->sha1,
                           REF_ISPRUNING | REF_NODEREF, NULL, &err)

it gives an incorrect impression that there may also be a valid case
to make a "delete" call with ISPRUNING alone without NODEREF, in
other codepaths and under certain conditions, and write an incorrect

    ref_transaction_delete(transaction, refname, sha1,
                           REF_ISPRUNING, NULL, &err)

in her new code.  Or a careless programmer and reviewer may not even
memorize and remember what the new world order is when they see such
a code and let it pass.

As I understand that we declare that "to prune a ref from set of
loose refs is to prune the named one, never following a symbolic
ref" is the new world order with this patch, making sure that
ISPRUNING automatically and always mean NODEREF will eliminate the
possibility that any new code makes an incorrect call to "delete",
which I think is much better.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 08/29] ref_transaction_commit(): remove local variable n

Junio C Hamano
In reply to this post by Junio C Hamano
Junio C Hamano <[hidden email]> writes:

> I expect that somewhere in this series transaction->nr will not stay

s/series/& it is documented that/

> constant even if the client code of ref-transaction API makes no
> direct call that adds a new update[] element, though, even if it is
> not done in this patch.
>
> Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning

Junio C Hamano
In reply to this post by Junio C Hamano
Junio C Hamano <[hidden email]> writes:

> If a casual reader sees this code:
>
>     ref_transaction_delete(transaction, r->name, r->sha1,
>   REF_ISPRUNING | REF_NODEREF, NULL, &err)
>
> it gives an incorrect impression that there may also be a valid case
> to make a "delete" call with ISPRUNING alone without NODEREF, in
> other codepaths and under certain conditions, and write an incorrect
>
>     ref_transaction_delete(transaction, refname, sha1,
>   REF_ISPRUNING, NULL, &err)
>
> in her new code.  Or a careless programmer and reviewer may not even
> memorize and remember what the new world order is when they see such
> a code and let it pass.
>
> As I understand that we declare that "to prune a ref from set of
> loose refs is to prune the named one, never following a symbolic
> ref" is the new world order with this patch, making sure that
> ISPRUNING automatically and always mean NODEREF will eliminate the
> possibility that any new code makes an incorrect call to "delete",
> which I think is much better.

... but my understanding of the point of this patch may be flawed,
in which case I of course am willing to be enlightened ;-)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 05/29] refname_is_safe(): insist that the refname already be normalized

Jeff King
In reply to this post by Jeff King
On Wed, Apr 27, 2016 at 04:37:28PM -0400, Jeff King wrote:

> > > But anything writing a _new_ refname (whether the actual ref, or
> > > referencing it via a symref) should be using check_refname_format()
> > > before writing.
> >
> > Unfortunately, neither check is lesser -- refname_is_safe allows
> > refs/heads//foo but not a/b while check_refname_format allows a/b but
> > not refs/heads//foo.  So sometimes we need both, while other times we
> > just need one :(
>
> IMHO, that sounds like a bug. check_refname_format() should
> conceptually[1] be a superset of refname_is_safe(). Is there a case
> where we would want to _allow_ a refname that is not safe to look at on
> disk?

BTW, if there isn't a superset relationship here, then I suspect I may
have introduced some loosening or inconsistencies in 03afcbe
(read_packed_refs: avoid double-checking sane refs, 2015-04-16). So any
tightening now may simply be fixing that (which doesn't change anything
with respect to correctness, but may give people more confidence that
the tightening is not breaking something people have been depending on).

-Peff

PS Reading over that commit message, I think it mis-states
   "refname_is_safe() can only be true if check_refname_format() also
   failed". It should be "!refname_is_safe() ...". I.e., the condition can
   only trigger if...
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 05/29] refname_is_safe(): insist that the refname already be normalized

David Turner
In reply to this post by Jeff King
On Wed, 2016-04-27 at 16:37 -0400, Jeff King wrote:

> On Wed, Apr 27, 2016 at 04:34:53PM -0400, David Turner wrote:
>
> > > I thought the point is that one is a lesser check than the other,
> > > and
> > > we
> > > need different rules for different situations. So we might allow
> > > deletion on a refname that does not pass check_refname_format(),
> > > but
> > > we
> > > must make sure it is not going to cause any mischief (e.g.,
> > > escaping
> > > ".git" and deleting random files).
> > >
> > > But anything writing a _new_ refname (whether the actual ref, or
> > > referencing it via a symref) should be using
> > > check_refname_format()
> > > before writing.
> >
> > Unfortunately, neither check is lesser -- refname_is_safe allows
> > refs/heads//foo but not a/b while check_refname_format allows a/b
> > but
> > not refs/heads//foo.  So sometimes we need both, while other times
> > we
> > just need one :(
>
> IMHO, that sounds like a bug. check_refname_format() should
> conceptually[1] be a superset of refname_is_safe(). Is there a case
> where we would want to _allow_ a refname that is not safe to look at
> on
> disk?

The only such case I can think of is the case where there is a symref
to such a bad refname, and we want to delete said symref.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning

David Turner
In reply to this post by Junio C Hamano
On Wed, 2016-04-27 at 14:15 -0700, Junio C Hamano wrote:

> Junio C Hamano <[hidden email]> writes:
>
> > If a casual reader sees this code:
> >
> >     ref_transaction_delete(transaction, r->name, r->sha1,
> >   REF_ISPRUNING | REF_NODEREF, NULL, &err)
> >
> > it gives an incorrect impression that there may also be a valid
> > case
> > to make a "delete" call with ISPRUNING alone without NODEREF, in
> > other codepaths and under certain conditions, and write an
> > incorrect
> >
> >     ref_transaction_delete(transaction, refname, sha1,
> >   REF_ISPRUNING, NULL, &err)
> >
> > in her new code.  Or a careless programmer and reviewer may not
> > even
> > memorize and remember what the new world order is when they see
> > such
> > a code and let it pass.
> >
> > As I understand that we declare that "to prune a ref from set of
> > loose refs is to prune the named one, never following a symbolic
> > ref" is the new world order with this patch, making sure that
> > ISPRUNING automatically and always mean NODEREF will eliminate the
> > possibility that any new code makes an incorrect call to "delete",
> > which I think is much better.
>
> ... but my understanding of the point of this patch may be flawed,
> in which case I of course am willing to be enlightened ;-)

Since there is a manual check for that case, the code will fail at test
time.

But I don't have strong feelings and am happy to go either way on this.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning

Junio C Hamano
David Turner <[hidden email]> writes:

> Since there is a manual check for that case, the code will fail at test
> time.

I see a difference between "We make it hard to write incorrect code"
and "We keep it easy to write incorrect code but a runtime check
will catch mistakes anyway", and I tend to prefer the former.

I do think the "manual check" needs to be kept in an adjusted form
even if we decide to take the suggested update.  A caller can pass
0x04 instead of REF_ISPRUNING after all---but that is exactly the
case where you have to work hard to write incorrect code.

One existing check that is not touched with 15/29 also needs to be
adjusted.  An incremental update relative to 15/29 would look like
this:

 refs.c               | 4 ++--
 refs/files-backend.c | 4 ++--
 refs/refs-internal.h | 6 ++++--
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/refs.c b/refs.c
index 5dc2473..bb81dfd 100644
--- a/refs.c
+++ b/refs.c
@@ -790,8 +790,8 @@ int ref_transaction_update(struct ref_transaction *transaction,
  if (transaction->state != REF_TRANSACTION_OPEN)
  die("BUG: update called for transaction that is not open");
 
- if ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF))
- die("BUG: REF_ISPRUNING set without REF_NODEREF");
+ if ((flags & REF_ISPRUNING_) && !(flags & REF_NODEREF))
+ die("BUG: REF_ISPRUNING_ set without REF_NODEREF");
 
  if (new_sha1 && !is_null_sha1(new_sha1) &&
     check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 8fcbd7d..012e3d0 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2116,7 +2116,7 @@ static void prune_ref(struct ref_to_prune *r)
  transaction = ref_transaction_begin(&err);
  if (!transaction ||
     ref_transaction_delete(transaction, r->name, r->sha1,
-   REF_ISPRUNING | REF_NODEREF, NULL, &err) ||
+   REF_ISPRUNING, NULL, &err) ||
     ref_transaction_commit(transaction, &err)) {
  ref_transaction_free(transaction);
  error("%s", err.buf);
@@ -3178,7 +3178,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
  goto cleanup;
  }
 
- if (!(update->flags & REF_ISPRUNING))
+ if (!(update->flags & REF_ISPRUNING_))
  string_list_append(&refs_to_delete,
    update->lock->ref_name);
  }
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 37a1a37..0a2bf1f 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -15,9 +15,11 @@
 
 /*
  * Used as a flag in ref_update::flags when a loose ref is being
- * pruned.
+ * pruned.  As this must always be done with REF_NODEREF, we make
+ * the public version REF_ISPRUNING to contain REF_NODEREF.
  */
-#define REF_ISPRUNING 0x04
+#define REF_ISPRUNING_ 0x04
+#define REF_ISPRUNING (REF_ISPRUNING_ | REF_NODEREF)
 
 /*
  * Used as a flag in ref_update::flags when the reference should be
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 25/29] refs: resolve symbolic refs first

David Turner
In reply to this post by Michael Haggerty-2
On Wed, 2016-04-27 at 18:57 +0200, Michael Haggerty wrote:
+retry:
...
> + if (--attempts_remaining > 0)
> + goto retry;

could this be a loop instead of using gotos?

> + /*
> + * There is a directory in the way,
> + * but we don't know of any references
> + * that it should contain. This might
> + * be a directory that used to contain
> + * references but is now empty. Try to
> + * remove it; otherwise it might cause
> + * trouble when we try to rename the
> + * lockfile into place.
> + */
> + strbuf_addf(err, "there is a non-empty directory '%s' "
> +          "blocking reference '%s'",
> +  ref_file.buf,refname);
> + goto error_return;

We don't actually try to remove anything here, so that comment seems
wrong?

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 00/29] Yet more preparation for reference backends

David Turner
In reply to this post by Michael Haggerty-2
On Wed, 2016-04-27 at 18:57 +0200, Michael Haggerty wrote:
> This started as a modest attempt to move the last couple of patches
> mentioned here [1] to before the vtable patches. I wanted to do that
> because having real changes mixed with the vtable refactoring was
> making rebasing and stuff awkward.
>
> But then it snowballed. A lot of what's new is pretty trivial (though
> in some cases fixes minor bugs). But a few of the later patches are
> pretty major.

Apart from the comments on one patch that I sent, this looks good to
me.  I'm a bit worried about the impact on lmdb (which doesn't do
"locking" the same way), but I think it will probably work out.  Since
Michael has been rebasing the pluggable-refs-backend code on top of
this, he's probably got a good sense that it doesn't do too much
violence to the plugin infrastructure.



>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning

Michael Haggerty-2
In reply to this post by Junio C Hamano
On 04/27/2016 11:15 PM, Junio C Hamano wrote:

> Junio C Hamano <[hidden email]> writes:
>
>> If a casual reader sees this code:
>>
>>     ref_transaction_delete(transaction, r->name, r->sha1,
>>   REF_ISPRUNING | REF_NODEREF, NULL, &err)
>>
>> it gives an incorrect impression that there may also be a valid case
>> to make a "delete" call with ISPRUNING alone without NODEREF, in
>> other codepaths and under certain conditions, and write an incorrect
>>
>>     ref_transaction_delete(transaction, refname, sha1,
>>   REF_ISPRUNING, NULL, &err)
>>
>> in her new code.  Or a careless programmer and reviewer may not even
>> memorize and remember what the new world order is when they see such
>> a code and let it pass.
>>
>> As I understand that we declare that "to prune a ref from set of
>> loose refs is to prune the named one, never following a symbolic
>> ref" is the new world order with this patch, making sure that
>> ISPRUNING automatically and always mean NODEREF will eliminate the
>> possibility that any new code makes an incorrect call to "delete",
>> which I think is much better.
>
> ... but my understanding of the point of this patch may be flawed,
> in which case I of course am willing to be enlightened ;-)

I was thinking of this patch as documenting and enforcing a limitation
in the current implementation of pruning. But to be honest I can't think
of a reason that we would ever want to remove this limitation, so I am
OK with changing the policy to "REF_ISPRUNING always implies
REF_NODEREF" as you have suggested.

But I think it would be cleaner to achieve that goal with the following
change:

diff --git a/refs.c b/refs.c
index 5dc2473..1d4c12a 100644
--- a/refs.c
+++ b/refs.c
@@ -790,8 +790,10 @@ int ref_transaction_update(struct ref_transaction
*transaction,
  if (transaction->state != REF_TRANSACTION_OPEN)
  die("BUG: update called for transaction that is not open");

- if ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF))
- die("BUG: REF_ISPRUNING set without REF_NODEREF");
+ if (flags & REF_ISPRUNING) {
+ /* Pruning is always non-recursive */
+ flags |= REF_NODEREF;
+ }

  if (new_sha1 && !is_null_sha1(new_sha1) &&
     check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 8fcbd7d..9faf17c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2116,7 +2116,7 @@ static void prune_ref(struct ref_to_prune *r)
  transaction = ref_transaction_begin(&err);
  if (!transaction ||
     ref_transaction_delete(transaction, r->name, r->sha1,
-   REF_ISPRUNING | REF_NODEREF, NULL, &err) ||
+   REF_ISPRUNING, NULL, &err) ||
     ref_transaction_commit(transaction, &err)) {
  ref_transaction_free(transaction);
  error("%s", err.buf);
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 37a1a37..704eea7 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -15,7 +15,7 @@

 /*
  * Used as a flag in ref_update::flags when a loose ref is being
- * pruned.
+ * pruned. This flag implies REF_NODEREF.
  */
 #define REF_ISPRUNING 0x04


Note that patch "add_update(): initialize the whole ref_update" should
then be adjusted to do the flag-tweak in the add_update() function.

If there are no objections, I will implement these changes in v2.

Michael

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 19/29] refs: don't dereference on rename

Michael Haggerty-2
In reply to this post by Junio C Hamano
On 04/27/2016 08:55 PM, Junio C Hamano wrote:

> Michael Haggerty <[hidden email]> writes:
>
>> @@ -2380,8 +2381,8 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
>>   goto rollback;
>>   }
>>  
>> - if (!read_ref_full(newrefname, RESOLVE_REF_READING, sha1, NULL) &&
>> -    delete_ref(newrefname, sha1, REF_NODEREF)) {
>> + if (!read_ref_full(newrefname, resolve_flags, sha1, NULL) &&
>> +    delete_ref(newrefname, NULL, REF_NODEREF)) {
>
> Could you explain s/sha1/NULL/ here in the proposed log message?

Good question.

Passing sha1 to delete_ref() doesn't add any safety, because the same
sha1 was just read a moment before, and it is not used for anything
else. So the check only protects us from a concurrent update to
newrefname between the call to read_ref_full() and the call to
delete_ref(). But such a race is indistinguishable from the case that a
modification to newrefname happens just before the call to
read_ref_full(), which would have the same outcome as the new code. So
the "sha1" check only adds ways for the rename() to fail in situations
where nothing harmful would have happened anyway.

That being said, this is a very unlikely race, and I don't think it
matters much either way. In any case, the change s/sha1/NULL/ here seems
orthogonal to the rest of the patch.

David, you wrote the original version of this patch. Am I overlooking
something?

Michael

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 24/29] ref_transaction_update(): check refname_is_safe() at a minimum

Michael Haggerty-2
In reply to this post by Junio C Hamano
On 04/27/2016 10:14 PM, Junio C Hamano wrote:
> Michael Haggerty <[hidden email]> writes:
>
>> If the user has asked that a new value be set for a reference, we use
>> check_refname_format() to verify that the reference name satisfies all
>> of the rules. But in other cases, at least check that refname_is_safe().
>
> It isn't clear to me what "in other cases" exactly refers to.  A
> request to delete a ref would obviously one of those that do not
> "ask that a new value be set", but are there other cases?

The other case is `verify`, which can be used to check the old value of
a reference without modifying it. `verify` is exposed via `git
update-ref --stdin`

Michael

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning

Junio C Hamano
In reply to this post by Michael Haggerty-2
Michael Haggerty <[hidden email]> writes:

> But I think it would be cleaner to achieve that goal with the following
> change:
>
> diff --git a/refs.c b/refs.c
> index 5dc2473..1d4c12a 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -790,8 +790,10 @@ int ref_transaction_update(struct ref_transaction
> *transaction,
>   if (transaction->state != REF_TRANSACTION_OPEN)
>   die("BUG: update called for transaction that is not open");
>
> - if ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF))
> - die("BUG: REF_ISPRUNING set without REF_NODEREF");
> + if (flags & REF_ISPRUNING) {
> + /* Pruning is always non-recursive */
> + flags |= REF_NODEREF;
> + }
>
>   if (new_sha1 && !is_null_sha1(new_sha1) &&
>      check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 8fcbd7d..9faf17c 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2116,7 +2116,7 @@ static void prune_ref(struct ref_to_prune *r)
>   transaction = ref_transaction_begin(&err);
>   if (!transaction ||
>      ref_transaction_delete(transaction, r->name, r->sha1,
> -   REF_ISPRUNING | REF_NODEREF, NULL, &err) ||
> +   REF_ISPRUNING, NULL, &err) ||
>      ref_transaction_commit(transaction, &err)) {
>   ref_transaction_free(transaction);
>   error("%s", err.buf);
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index 37a1a37..704eea7 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -15,7 +15,7 @@
>
>  /*
>   * Used as a flag in ref_update::flags when a loose ref is being
> - * pruned.
> + * pruned. This flag implies REF_NODEREF.
>   */
>  #define REF_ISPRUNING 0x04
>
>
> Note that patch "add_update(): initialize the whole ref_update" should
> then be adjusted to do the flag-tweak in the add_update() function.
> ...
> If there are no objections, I will implement these changes in v2.

One worrysome point the above approach leaves is that nothing
guarantees that nobody in the codepath from the callsite of
ref-transaction-delete to ref-transaction-update looks at the flag
and acts differently depending on REF_NODEREF bit.  During that
time, REF_ISPRUNING call would not trigger (flag & REF_NODEREF)
check, because "pruning implies no-deref" is done only after you
reach transaction-update (i.e. the code you added in the first
hunk), but any code after that "implie no-deref" happens will see
REF_NODEREF bit in the flag word.

As long as you can add some mechanism to make that a non-issue, I am
fine with that approach.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning

Junio C Hamano
In reply to this post by Michael Haggerty-2
Two things I forgot to say.

Michael Haggerty <[hidden email]> writes:

> I was thinking of this patch as documenting and enforcing a limitation
> in the current implementation of pruning.

This actually is an excellent point, and is the reason why I
repeatedly made my suggestion conditional on "If my understanding of
the purpose of this patch correctly" ;-) And I personally am not
convinced that this limitation is fundamental and will not be
lifted.  It may be better to treat this as a limitation of the
current implementation of pruning, and in that case I do agree that
the original patch that started this thread is the best way to
express it.

I said that it makes harder to write incorrect client code of this
API to make REF_ISPRUNING contain both 0x04 and REF_NODEREF, and I
still think that is true, but at the same time, it makes it easier
to write incorrect code in the API implementation.  We no longer can
check if we are in the pruning codepath with

    if (flag & REF_ISPRUNING)

but have to write

    if ((flag & REF_ISPRUNING) == REF_ISPRUNING)

instead.  This "& with the constant and compare the result with that
same constant" pattern could also be used for a single-bit constant,
so if there were short-and-sweet syntax to express that pattern in
the language, consistently using that for all bit checks would make
it less likely for us to write incorrect code, but but there is no
short-and-sweet syntax to do so in C, and spelling it out like the
above is too cumbersome to be practical.  The suggested squash did

    if (flag & REF_ISPRUNING_)

to check only for 0x04 bit, but that is also error prone; it is too
easy to forget the difference between the two and drop the trailing
underscore by mistake.

Even though it is generally a good trade-off to make it harder to
make mistakes in the "user of the API" code even if it makes it
easier to make mistakes in the "implementation of the API" code,
because the "user of the API" in this case is actually still inside
the ref API implementation, I do not think either way makes too much
of a difference.  So perhaps your original might be the best version
among those that have been discussed in this thread.



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 19/29] refs: don't dereference on rename

Junio C Hamano
In reply to this post by Michael Haggerty-2
Michael Haggerty <[hidden email]> writes:

>> Could you explain s/sha1/NULL/ here in the proposed log message?
>
> Good question.
>
> Passing sha1 to delete_ref() doesn't add any safety, because the same
> sha1 was just read a moment before, and it is not used for anything
> else.

"... and it is guaranteed that no other process in the meantime
wanted to update the ref we are trying to delete because it wants to
see the ref with its updated value." is something I expected to see
at the end.

> So the check only protects us from a concurrent update to
> newrefname between the call to read_ref_full() and the call to
> delete_ref(). But such a race is indistinguishable from the case that a
> modification to newrefname happens just before the call to
> read_ref_full(), which would have the same outcome as the new code.

In other words, when a transaction that contains a deletion of a ref
races with another one that updates the ref, the latter transaction
may come after the former one, but the ref may not survive in the
end result and can be left deleted?

Puzzled...
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 24/29] ref_transaction_update(): check refname_is_safe() at a minimum

Junio C Hamano
In reply to this post by Michael Haggerty-2
Michael Haggerty <[hidden email]> writes:

> On 04/27/2016 10:14 PM, Junio C Hamano wrote:
>> Michael Haggerty <[hidden email]> writes:
>>
>>> If the user has asked that a new value be set for a reference, we use
>>> check_refname_format() to verify that the reference name satisfies all
>>> of the rules. But in other cases, at least check that refname_is_safe().
>>
>> It isn't clear to me what "in other cases" exactly refers to.  A
>> request to delete a ref would obviously one of those that do not
>> "ask that a new value be set", but are there other cases?
>
> The other case is `verify`, which can be used to check the old value of
> a reference without modifying it. `verify` is exposed via `git
> update-ref --stdin`

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 25/29] refs: resolve symbolic refs first

Michael Haggerty-2
In reply to this post by David Turner
On 04/29/2016 01:40 AM, David Turner wrote:
> On Wed, 2016-04-27 at 18:57 +0200, Michael Haggerty wrote:
> +retry:
> ...
>> + if (--attempts_remaining > 0)
>> + goto retry;
>
> could this be a loop instead of using gotos?

It certainly could. The goto-vs-loop question was debated on the mailing
list when I first added very similar code elsewhere (unfortunately I'm
unable to find a link to that conversation). I was persuaded to change
my loop into gotos, the argument being that the "retry" case is
exceptional and shouldn't be such a dominant part of the function
structure. Plus the goto code is briefer and feels less awkward to me in
this case (that's subjective, of course).

>> + /*
>> + * There is a directory in the way,
>> + * but we don't know of any references
>> + * that it should contain. This might
>> + * be a directory that used to contain
>> + * references but is now empty. Try to
>> + * remove it; otherwise it might cause
>> + * trouble when we try to rename the
>> + * lockfile into place.
>> + */
>> + strbuf_addf(err, "there is a non-empty directory '%s' "
>> +          "blocking reference '%s'",
>> +  ref_file.buf,refname);
>> + goto error_return;
>
> We don't actually try to remove anything here, so that comment seems
> wrong?

Thanks, will fix.

Michael

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 19/29] refs: don't dereference on rename

Michael Haggerty-2
In reply to this post by Junio C Hamano
On 04/29/2016 10:53 AM, Junio C Hamano wrote:

> Michael Haggerty <[hidden email]> writes:
>
>>> Could you explain s/sha1/NULL/ here in the proposed log message?
>>
>> Good question.
>>
>> Passing sha1 to delete_ref() doesn't add any safety, because the same
>> sha1 was just read a moment before, and it is not used for anything
>> else.
>
> "... and it is guaranteed that no other process in the meantime
> wanted to update the ref we are trying to delete because it wants to
> see the ref with its updated value." is something I expected to see
> at the end.
>
>> So the check only protects us from a concurrent update to
>> newrefname between the call to read_ref_full() and the call to
>> delete_ref(). But such a race is indistinguishable from the case that a
>> modification to newrefname happens just before the call to
>> read_ref_full(), which would have the same outcome as the new code.
>
> In other words, when a transaction that contains a deletion of a ref
> races with another one that updates the ref, the latter transaction
> may come after the former one, but the ref may not survive in the
> end result and can be left deleted?
>
> Puzzled...

Remember, we're talking about rename_ref() only, not reference deletion
in general. rename_ref() is not very robust anyway--it doesn't happen in
a single transaction, and it is vulnerable to being defeated by
simultaneous reference updates by other processes. It *does* wrap the
deletion of newrefname in a transaction; the only question is whether an
old_sha1 is supplied to that transaction.

So, suppose that newrefname starts at value A, and there are two updates
running simultaneously:

1. An update of reference newrefname from A -> B

2. A rename of reference oldrefname to newrefname, which includes
   a. read_ref_full("newrefname") and
   b. delete_ref("newrefname").

It is not possible for (1) to happen after (2b) because the former's
check of the old value of newrefname would fail. So there are two
possible interleavings:

* 1, 2a, 2b
* 2a, 1, 2b

With the new code, both of these interleavings end up with newrefname
deleted.

With the old code, the second interleaving would fail.

But the only difference is the relative order of the read-only operation
(2a), whose SHA-1 result is never used. So neither process actually
cares which of those two interleavings occurred, and it is legitimate to
treat them the same.

Note that the first transaction *did* successfully set newrefname to
value B in both cases and indeed knows for sure that the update was
successful. It's just that newrefname was deleted immediately afterwards.

Michael

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 19/29] refs: don't dereference on rename

Jeff King
On Fri, Apr 29, 2016 at 12:57:29PM +0200, Michael Haggerty wrote:

> Remember, we're talking about rename_ref() only, not reference deletion
> in general. rename_ref() is not very robust anyway--it doesn't happen in
> a single transaction, and it is vulnerable to being defeated by
> simultaneous reference updates by other processes. It *does* wrap the
> deletion of newrefname in a transaction; the only question is whether an
> old_sha1 is supplied to that transaction.
>
> So, suppose that newrefname starts at value A, and there are two updates
> running simultaneously:
>
> 1. An update of reference newrefname from A -> B
>
> 2. A rename of reference oldrefname to newrefname, which includes
>    a. read_ref_full("newrefname") and
>    b. delete_ref("newrefname").

I wondered at first if you meant "oldrefname" in 2b. That is, a rename
would involve writing to "newrefname" and then deleting "oldrefname",
and not doing the old_sha1 and normal locking on the deletion of
oldrefname would be bad (in case somebody else updated it while we were
operating).

But reading the patch again, that's not what's going on. You're talking
just about the case where we force-overwrite an existing newrefname, and
delete it first to do so. But what I don't understand is:

  1. Why do we read_ref_full() at all? Since it is not done under lock
     anyway, it is useless for helping with race conditions, and I think
     that is what you are arguing (that we should just be deleting
     regardless). But then why not just call delete_ref(), and handle
     the ENOENT case as a noop (which closes another race if somebody
     deletes it between your 2a and 2b).

  2. Why delete it at all? The point is to overwrite, so I guess it is
     trying to make room. But we could just rename the loose ref file
     and reflog overtop the old, couldn't we?

Or am I totally misreading the purpose of this code?

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
1234