Re: [PATCH 07/16] update submodules: introduce submodule_is_interesting

2016-11-17 Thread Stefan Beller 
On Thu, Nov 17, 2016 at 2:57 AM, Heiko Voigt  wrote:

> It seems that you are only looking at the submodule config from a
> commit. Should a user be able to override this with local configuration?
> Haven't looked further in the patchseries so maybe that is somewhere
> else?

It turns out that in later patches we pass in null_sha1 only, which is
looking at the config and possible overrides.

I'll refactor to take no sha1 argument and use null_sha1 here directly.

Re: [PATCH 07/16] update submodules: introduce submodule_is_interesting

2016-11-17 Thread Stefan Beller 
On Tue, Nov 15, 2016 at 4:14 PM, David Turner  wrote:

>> +int submodule_is_interesting(const char *path, const unsigned char
>> +*sha1) {
>
> This is apparently only ever (in this series) called with null_sha1.  So 
> either this arg is unnecessary, or there are bugs elsewhere in the code.

I was torn when writing the series, as I initially had submodule_is_interesting
with no sha1 argument and it turned out to be buggy in my first
initial implementation,
which lead me to thinking the sha1 actually matters.

The line of thinking was similar to loading the submodules from the
submodule-config cache as that also has different values for different sha1s,
e.g. a submodule is only interesting if submodule..update != none,
which can have changed with different sha1s.

I refactored the series since then to call the _is_initeresting method
at different times
(before and after the actual checkout), such that we implicitly have
the correct sha1
while calling it.

So I would argue the sha1 argument is not needed. I'll remove it.

RE: [PATCH 07/16] update submodules: introduce submodule_is_interesting

2016-11-15 Thread David Turner 
> -Original Message-
> From: Stefan Beller [mailto:sbel...@google.com]
> Sent: Tuesday, November 15, 2016 6:07 PM
> Cc: git@vger.kernel.org; bmw...@google.com; gits...@pobox.com;
> jrnie...@gmail.com; mogulgu...@gmail.com; David Turner; Stefan Beller
> Subject: [PATCH 07/16] update submodules: introduce
> submodule_is_interesting
> +int submodule_is_interesting(const char *path, const unsigned char
> +*sha1) {

This is apparently only ever (in this series) called with null_sha1.  So either 
this arg is unnecessary, or there are bugs elsewhere in the code.

Re: [PATCH 07/16] update submodules: introduce submodule_is_interesting

2016-11-15 Thread Brandon Williams 
On 11/15, Stefan Beller wrote:
> +/**
> + * When updating the working tree, do we need to check if the submodule needs
> + * updating. We do not require a check if we are already sure that the
> + * submodule doesn't need updating, e.g. when we are not interested in 
> submodules
> + * or the submodule is marked uninteresting by being not initialized.
> + */

The first sentence seems a bit awkward.  It seems like its worded as a
question, maybe drop the 'do'?

-- 
Brandon Williams