git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Michael Haggerty <mhagger@alum.mit.edu>
To: Jeff King <peff@peff.net>
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
	"David Turner" <novalis@novalis.org>,
	git@vger.kernel.org
Subject: Re: [PATCH 04/20] refs_verify_refname_available(): implement once for all backends
Date: Mon, 20 Mar 2017 23:20:40 +0100	[thread overview]
Message-ID: <6ff4a9f9-23a5-89c9-f478-00449132d410@alum.mit.edu> (raw)
In-Reply-To: <20170320174240.gykldqzbqyva6kbs@sigill.intra.peff.net>

On 03/20/2017 06:42 PM, Jeff King wrote:
> On Mon, Mar 20, 2017 at 05:33:09PM +0100, Michael Haggerty wrote:
> 
>> It turns out that we can now implement
>> `refs_verify_refname_available()` based on the other virtual
>> functions, so there is no need for it to be defined at the backend
>> level. Instead, define it once in `refs.c` and remove the
>> `files_backend` definition.
> 
> Does this mean that backends can no longer provide storage for
> D/F-conflicted refnames (i.e., "refs/foo" and "refs/foo/bar")? It looks
> like the global verify_refname_available() has that logic baked in.

The `verify_refname_available()` function implements the "no D/F
conflict" policy. But it's called from the backends, not from the common
code, and nobody says that a backend needs to call the function.

> [...]
>>  int refs_verify_refname_available(struct ref_store *refs,
>>  				  const char *refname,
>> -				  const struct string_list *extra,
>> +				  const struct string_list *extras,
>>  				  const struct string_list *skip,
>>  				  struct strbuf *err)
>>  {
>> [...]
>> +		/*
>> +		 * We are still at a leading dir of the refname (e.g.,
>> +		 * "refs/foo"; if there is a reference with that name,
>> +		 * it is a conflict, *unless* it is in skip.
>> +		 */
>> +		if (skip && string_list_has_string(skip, dirname.buf))
>> +			continue;
>> +
>> +		if (!refs_read_raw_ref(refs, dirname.buf, oid.hash, &referent, &type)) {
>> +			strbuf_addf(err, "'%s' exists; cannot create '%s'",
>> +				    dirname.buf, refname);
>> +			goto cleanup;
>> +		}
> 
> We don't really care about reading the ref value here; we just care if
> it exists. Does that matter for efficiency (e.g., for the files backend
> it's a stat() versus an open/read/close)? I guess the difference only
> matters when it _does_ exist, which is the uncommon error case.

Yes, I assume that the conflict cases are unusual so I wasn't worrying
too much about their performance.

Not quite so obvious is that the new code sometimes checks for conflicts
against both loose and packed references in situations where the old
code only checked against packed references. Specifically, this happens
when the code has just read, or locked, or failed to find a loose
reference, so it could infer that there are no loose references that
could conflict. I don't think that will be noticeable, because it is the
reading of the whole `packed-refs` file that is a big expensive
operation that it is important to avoid. Anyway, later patches (i.e.,
after there is a `packed_ref_store`) can switch back to checking only
packed references and get back the old optimization.

> (Also, I suspect the loose ref cache always just reads everything in the
> current code, though with the iterator approach in theory we could stop
> doing that).

The loose ref cache reads directories into the cache lazily,
breadth-first. So it reads all of the entries in the directories leading
to the reference being looked up, but no others. When iterating, it
reads the parent directories of the prefix that is being iterated over
plus the whole subtree under the prefix, and that's it (though this
optimization is not yet wired through to `git for-each-ref`).

Michael


  reply	other threads:[~2017-03-20 22:20 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-20 16:33 [PATCH 00/20] Separate `ref_cache` into a separate module Michael Haggerty
2017-03-20 16:33 ` [PATCH 01/20] get_ref_dir(): don't call read_loose_refs() for "refs/bisect" Michael Haggerty
2017-03-20 16:33 ` [PATCH 02/20] refs_read_raw_ref(): new function Michael Haggerty
2017-03-20 16:33 ` [PATCH 03/20] refs_ref_iterator_begin(): " Michael Haggerty
2017-03-20 16:33 ` [PATCH 04/20] refs_verify_refname_available(): implement once for all backends Michael Haggerty
2017-03-20 17:42   ` Jeff King
2017-03-20 22:20     ` Michael Haggerty [this message]
2017-03-20 16:33 ` [PATCH 05/20] refs_verify_refname_available(): use function in more places Michael Haggerty
2017-03-20 16:33 ` [PATCH 06/20] Rename `add_ref()` to `add_ref_entry()` Michael Haggerty
2017-03-20 16:33 ` [PATCH 07/20] Rename `find_ref()` to `find_ref_entry()` Michael Haggerty
2017-03-20 16:33 ` [PATCH 08/20] Rename `remove_entry()` to `remove_entry_from_dir()` Michael Haggerty
2017-03-20 16:33 ` [PATCH 09/20] refs: split `ref_cache` code into separate files Michael Haggerty
2017-03-20 17:49   ` Jeff King
2017-03-20 19:47     ` Junio C Hamano
2017-03-20 20:35       ` Stefan Beller
2017-03-20 22:40         ` Junio C Hamano
2017-03-20 16:33 ` [PATCH 10/20] ref-cache: introduce a new type, ref_cache Michael Haggerty
2017-03-20 16:33 ` [PATCH 11/20] refs: record the ref_store in ref_cache, not ref_dir Michael Haggerty
2017-03-20 17:51   ` Jeff King
2017-03-20 22:39     ` Michael Haggerty
2017-03-20 16:33 ` [PATCH 12/20] ref-cache: use a callback function to fill the cache Michael Haggerty
2017-03-20 16:33 ` [PATCH 13/20] refs: handle "refs/bisect/" in `loose_fill_ref_dir()` Michael Haggerty
2017-03-20 16:33 ` [PATCH 14/20] do_for_each_entry_in_dir(): eliminate `offset` argument Michael Haggerty
2017-03-20 16:33 ` [PATCH 15/20] get_loose_ref_dir(): function renamed from get_loose_refs() Michael Haggerty
2017-03-20 16:33 ` [PATCH 16/20] get_loose_ref_cache(): new function Michael Haggerty
2017-03-20 16:33 ` [PATCH 17/20] cache_ref_iterator_begin(): make function smarter Michael Haggerty
2017-03-20 16:33 ` [PATCH 18/20] commit_packed_refs(): use reference iteration Michael Haggerty
2017-03-20 18:05   ` Jeff King
2017-03-22  8:42     ` Michael Haggerty
2017-03-22 13:06       ` Jeff King
2017-03-20 16:33 ` [PATCH 19/20] files_pack_refs(): " Michael Haggerty
2017-03-20 16:33 ` [PATCH 20/20] do_for_each_entry_in_dir(): delete function Michael Haggerty
2017-03-20 17:25 ` [PATCH 00/20] Separate `ref_cache` into a separate module Junio C Hamano
2017-03-20 18:12 ` Jeff King
2017-03-20 18:24 ` Ævar Arnfjörð Bjarmason
2017-03-20 18:30   ` Jeff King
2017-03-20 22:32 ` Junio C Hamano
2017-03-20 22:48   ` 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

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=6ff4a9f9-23a5-89c9-f478-00449132d410@alum.mit.edu \
    --to=mhagger@alum.mit.edu \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=novalis@novalis.org \
    --cc=pclouds@gmail.com \
    --cc=peff@peff.net \
    /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.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).