Re: [PATCH v2 5/6] submodule: improve submodule_has_commits

From: Brandon Williams <bmwill@google.com>
Date: May 2, 2017, 17:25 UTC
Message-ID: <20170502172555.GA181268@google.com>
In-Reply-To: <CAGZ79kbbz3AAjbg_dV9RVS8kgLs-zWZxt5tsFbQczCm78LcTVw@mail.gmail.com>

On 05/01, Stefan Beller wrote:

Show 9 quoted lines

> On Mon, May 1, 2017 at 6:02 PM, Brandon Williams <bmwill@google.com> wrote:
> > +
> > +               if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) || out.len)
> 
> eh, I gave too much and self-contradicting feedback here earlier,
> ideally I'd like to review this to be similar as:
> 
>     if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1)
>         die("cannot capture git-rev-list in submodule '%s', sub->path);

This wouldn't really work because if you provide a SHA1 to rev-list which it isn't able to find then it returns a non-zero exit code which would cause this to die, which isn't the desired behavior.

Show 53 quoted lines

> 
>     if (out.len)
>         has_commit = 0;
> 
> instead as that does not have a silent error. (though it errs
> on the safe side, so maybe it is not to bad.)
> 
> I could understand if the callers do not want to have
> `submodule_has_commits` die()-ing on them, so maybe
> 
>     if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) {
>         warning("cannot capture git-rev-list in submodule '%s', sub->path);
>         has_commit = -1;
>         /* this would require auditing all callers and handling -1 though */
>     }
> 
>     if (out.len)
>         has_commit = 0;
> 
> As the comment eludes, we'd then have
>  0 -> has no commits
>  1 -> has commits
> -1 -> error
> 
> So to group (error || has_no_commits), we could write
> 
>     if (submodule_has_commits(..) <= 0)
> 
> which is awkward. So maybe we can rename the function
> to misses_submodule_commits instead, as then we could
> flip the return value as well and have
> 
>  0 -> has commits
>  1 -> has no commits
> -1 -> error
> 
> and the lazy invoker could just go with
> 
>     if (!misses_submodule_commits(..))
>         proceed();
>     else
>         die("missing submodule commits or errors; I don't care");
> 
> whereas the careful invoker could go with
> 
>     switch (misses_submodule_commits(..)) {
>     case 0:
>         proceed(); break;
>     case 1:
>         pull_magic_trick(); break;
>     case -1:
>         make_errors_go_away_and_retry(); break;
>     }

I feel like you're making this a little too complicated, as all I'm doing is shuffling around already existing logic. I understand the want to make things more robust but this seems unnecessarily complex.

Show 17 quoted lines

> ---
> On the longer term plan:
> As you wrote about costs. Maybe instead of invoking rev-list,
> we could try to have this in-core as a first try-out for
> "classified-repos", looking at refs.h there is e.g.
> 
>     int for_each_ref_submodule(const char *submodule_path,
>           each_ref_fn fn, void *cb_data);
> 
> which we could use to obtain all submodule refs and then
> use the revision walking machinery to find out ourselves if
> we have or do not have the commits. (As we loaded the
> odb of the submodule, this would *just work*, building one
> kludgy hack upon the next.)
> 
> Thanks,
> Stefan

-- 
Brandon Williams