Re: [PATCH 3/7] checkout-index: add parallel checkout support

From: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
Date: Apr 26, 2021, 22:30 UTC
Message-ID: <CAHd-oW547EXAg6bCFx84pKX0j_8Ry2RraYxVscNbK17+0ObkRA@mail.gmail.com>
In-Reply-To: <36615f6a-ce58-3737-8c1d-fa1253a85439@gmail.com>

On Fri, Apr 23, 2021 at 3:32 PM Derrick Stolee <stolee@gmail.com> wrote:

Show 28 quoted lines

>
> On 4/22/2021 11:17 AM, Matheus Tavares wrote:
> > @@ -142,11 +143,7 @@ static void checkout_all(const char *prefix, int prefix_length)
> >       }
> >       if (last_ce && to_tempfile)
> >               write_tempfile_record(last_ce->name, prefix);
> > -     if (errs)
> > -             /* we have already done our error reporting.
> > -              * exit with the same code as die().
> > -              */
> > -             exit(128);
>
> Here, it makes note of returning 128 as if it were a die(), but
>
> > +     if (all)
> > +             err |= checkout_all(prefix, prefix_length);
> > +
> > +     if (pc_workers > 1)
> > +             err |= run_parallel_checkout(&state, pc_workers, pc_threshold,
> > +                                          NULL, NULL);
> > +
> >       if (err)
> >               return 1;
>
> This returns 1 instead. Should we `return err` and use an error
> code specific to the response? I imagine there are other reasons
> that could cause a non-zero return for checkout_all() and
> run_parallel_checkout().

Hmm, I think checkout_entry() returns -1 for any kind of error, and checkout_all()'s `err` only holds the number of checkout_entry()'s failures. run_parallel_checkout() also always use -1 for errors.

> I suppose: is there a value in persisting the 128 response code
> here, or is an exit of 1 sufficient? There is no documentation
> about the exit code, so any dependence on 128 is not based on a
> "written promise" but I thought it was worth mentioning.

Yeah, I also wondered if we should keep the 128 exit code here... But I couldn't see much value in doing so, unless there are scripts actually expecting 128. But I guess this is unlikely, right?

The additional bonus of unifying the exit path for `git checkout-index --all` and `git checkout-index <path>` is that the code flow becomes a little easier to read, especially now that they both need to [conditionally] call run_parallel_checkout() before exiting.