git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Heiko Voigt <hvoigt@hvoigt.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Nick Townsend" <nick.townsend@mac.com>,
	"René Scharfe" <l.s.r@web.de>,
	"Jens Lehmann" <Jens.Lehmann@web.de>,
	git@vger.kernel.org, "Jeff King" <peff@peff.net>
Subject: Re: Re: [RFC/WIP PATCH] implement reading of submodule .gitmodules configuration into cache
Date: Thu, 12 Dec 2013 14:03:07 +0100	[thread overview]
Message-ID: <20131212130307.GA6183@t2784.greatnet.de> (raw)
In-Reply-To: <xmqqppp5vbn5.fsf@gitster.dls.corp.google.com>

On Mon, Dec 09, 2013 at 03:37:50PM -0800, Junio C Hamano wrote:
> > +void submodule_config_cache_free(struct submodule_config_cache *cache)
> > +{
> > +	/* NOTE: its important to iterate over the name hash here
> > +	 * since paths might have multiple entries */
> 
> Style (multi-line comments).

Will fix.

> This is interesting.  I wonder what the practical consequence is to
> have a single submodule bound to the top-level tree more than once.
> Updating from one of the working tree will make the other working
> tree out of sync because the ultimate location of the submodule
> directory pointed at by the two .git gitdirs can only have a single
> HEAD, be it detached or on a branch, and a single index.

To clarify, when writing this comment I was not thinking about the same
submodule with multiple paths in the same tree but rather with the same
name under different paths in different commits.

> Not that the decision to enforce that names are unique in the
> top-level .gitmodules, and follow that decision in this part of the
> code to be defensive (not rely on the "one submodule can be bound
> only once to a top-level tree"), but shouldn't such a configuration
> to have a single submodule bound to more than one place in the
> top-level tree be forbidden?

Yes IMO, that should be forbidden currently. I do not think we actually
prevent the user from doing so but it can not happen by accident since
we derive the initial name from the local path. Maybe we should be more
strict about that and put more guards in place to avoid such
configurations from entering the database.

> > +	for_each_hash(&cache->for_name, free_one_submodule_config, NULL);
> > +	free_hash(&cache->for_path);
> > +	free_hash(&cache->for_name);
> > +}
> > +
> > +static unsigned int hash_sha1_string(const unsigned char *sha1, const char *string)
> > +{
> > +	int c;
> > +	unsigned int hash, string_hash = 5381;
> > +	memcpy(&hash, sha1, sizeof(hash));
> > +
> > +	/* djb2 hash */
> > +	while ((c = *string++))
> > +		string_hash = ((string_hash << 5) + hash) + c; /* hash * 33 + c */
> 
> Hmm, the comment and the code does not seem to match in math here...

Yeah sorry that was a leftover from the code I started with. In the
beginning it was a pure string hash. Will remove both comments (since
its also not a pure djb2 hash anymore).

Cheers Heiko

  reply	other threads:[~2013-12-12 13:03 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-11-26  0:04 [PATCH] submodule recursion in git-archive Nick Townsend
2013-11-26 15:17 ` René Scharfe
2013-11-26 18:57   ` Jens Lehmann
2013-11-26 22:18   ` Junio C Hamano
2013-11-27  0:28     ` René Scharfe
2013-11-27  3:28       ` Nick Townsend
2013-11-27 19:05       ` Junio C Hamano
2013-11-27  3:55     ` Nick Townsend
2013-11-27 19:43       ` Junio C Hamano
2013-11-29 22:38         ` Heiko Voigt
     [not found]           ` <3C71BC83-4DD0-43F8-9E36-88594CA63FC5@mac.com>
2013-12-03  0:05             ` Nick Townsend
2013-12-03 18:33             ` Heiko Voigt
2013-12-09 20:55               ` [RFC/WIP PATCH] implement reading of submodule .gitmodules configuration into cache Heiko Voigt
2013-12-09 23:37                 ` Junio C Hamano
2013-12-12 13:03                   ` Heiko Voigt [this message]
2013-12-03  0:00         ` [PATCH] submodule recursion in git-archive Nick Townsend
2013-12-03  0:03           ` Fwd: " Nick Townsend
2013-11-26 22:38   ` Heiko Voigt
2013-11-27  3:33     ` Nick Townsend

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=20131212130307.GA6183@t2784.greatnet.de \
    --to=hvoigt@hvoigt.net \
    --cc=Jens.Lehmann@web.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=l.s.r@web.de \
    --cc=nick.townsend@mac.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).