| Message ID | 6bf1db2fd8e807f9f259787ef75651d43b61e03a.1619104091.git.matheus.bernardino@usp.br (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Parallel Checkout (part 3) | expand |
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(). 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. Thanks, -Stolee
On Fri, Apr 23, 2021 at 3:32 PM Derrick Stolee <stolee@gmail.com> 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.
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c index c0bf4ac1b2..e8a82ea9ed 100644 --- a/builtin/checkout-index.c +++ b/builtin/checkout-index.c @@ -12,6 +12,7 @@ #include "cache-tree.h" #include "parse-options.h" #include "entry.h" +#include "parallel-checkout.h" #define CHECKOUT_ALL 4 static int nul_term_line; @@ -115,7 +116,7 @@ static int checkout_file(const char *name, const char *prefix) return -1; } -static void checkout_all(const char *prefix, int prefix_length) +static int checkout_all(const char *prefix, int prefix_length) { int i, errs = 0; struct cache_entry *last_ce = NULL; @@ -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); + return !!errs; } static const char * const builtin_checkout_index_usage[] = { @@ -182,6 +179,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix) int force = 0, quiet = 0, not_new = 0; int index_opt = 0; int err = 0; + int pc_workers, pc_threshold; struct option builtin_checkout_index_options[] = { OPT_BOOL('a', "all", &all, N_("check out all files in the index")), @@ -236,6 +234,10 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix) hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR); } + get_parallel_checkout_configs(&pc_workers, &pc_threshold); + if (pc_workers > 1) + init_parallel_checkout(); + /* Check out named files first */ for (i = 0; i < argc; i++) { const char *arg = argv[i]; @@ -275,12 +277,16 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix) strbuf_release(&buf); } + 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; - if (all) - checkout_all(prefix, prefix_length); - if (is_lock_file_locked(&lock_file) && write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) die("Unable to write new index file");
Note: previously, `checkout_all()` would not return on errors, but instead call `exit()` with a non-zero code. However, it only did that after calling `checkout_entry()` for all index entries, thus not stopping on the first error, but attempting to write the maximum number of entries possible. In order to maintain this behavior we now propagate `checkout_all()`s error status to `cmd_checkout_index()`, so that it can call `run_parallel_checkout()` and attempt to write the queued entries before exiting with the error code. Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> --- builtin/checkout-index.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-)