On Fri, Apr 23, 2021 at 3:32 PM Derrick Stolee <stolee@xxxxxxxxx> wrote: > > 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.
