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

> On Tue, May 2, 2017 at 10:25 AM, Brandon Williams <bmwill@google.com> wrote:
> > On 05/01, Stefan Beller wrote:
> >> 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.
> 
> Oh. In that case, why do we even check for its stdout?
> (from rev-lists man page)
>        --quiet
>            Don’t print anything to standard output. This form is primarily
>            meant to allow the caller to test the exit status to see if a range
>            of objects is fully connected (or not). It is faster than
>            redirecting stdout to /dev/null as the output does not have to be
>            formatted.
> 

> >
> > 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.
> 
> ok. I was just giving my thoughts on how I would approach it.
> 
> Thanks,
> Stefan