@matheusbsbs / matheus-bagliano / commits / b34ab4a43b

stash: remove unnecessary process forking

When stash was converted from shell to a builtin, it merely transliterated the forking of various git commands from shell to a C program that would fork the same commands. Some of those were converted over to actual library calls, but much of the pipeline-of-commands design still remains. Fix some of this by replacing the portion corresponding to git diff-index --cached --name-only --diff-filter=A $CTREE >"$a" git read-tree --reset $CTREE git update-index --add --stdin <"$a" rm -f "$a" into a library function that does the same thing. (The read-tree --reset was already partially converted over to a library call, but as an independent piece.) Note here that this came after a merge operation was performed. The merge machinery always stages anything that cleanly merges, and the above code only runs if there are no conflicts. Its purpose is to make it so that when there are no conflicts, all the changes from the stash are unstaged. However, that causes brand new files from the stash to become untracked, so the code above first saves those files off and then re-adds them afterwards. We replace the whole series of commands with a simple function that will unstage files that are not newly added. This doesn't fix any bugs in the usage of these commands, it simply matches the existing behavior but makes it into a single atomic operation that we can then operate on as a whole. A subsequent commit will take advantage of this to fix issues with these commands in sparse-checkouts. This conversion incidentally fixes t3906.1, because the separate update-index process would die with the following error messages: error: uninitialized_sub: is a directory - add files inside instead fatal: Unable to process path uninitialized_sub The unstaging of the directory as a submodule meant it was no longer tracked, and thus as an uninitialized directory it could not be added back using `git update-index --add`, thus resulting in this error and early abort. Most of the submodule tests in 3906 continue to fail after this change, this change was just enough to push the first of those tests to success. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Elijah Newren committed Dec 1, 2020 at 22:25 UTC b34ab4a43ba68dd4bba66d765368a37d99890ab2

2 files changed +78 -57

builtin/stash.c

+70 -49

@@ -325,35 +325,6 @@ static void add_diff_to_buf(struct diff_queue_struct *q,

325 }

326 }

327

328 -static int get_newly_staged(struct strbuf *out, struct object_id *c_tree)

329 -{

330 - struct child_process cp = CHILD_PROCESS_INIT;

331 - const char *c_tree_hex = oid_to_hex(c_tree);

332 -

333 - /*

334 - * diff-index is very similar to diff-tree above, and should be

335 - * converted together with update_index.

336 - */

337 - cp.git_cmd = 1;

338 - strvec_pushl(&cp.args, "diff-index", "--cached", "--name-only",

339 - "--diff-filter=A", NULL);

340 - strvec_push(&cp.args, c_tree_hex);

341 - return pipe_command(&cp, NULL, 0, out, 0, NULL, 0);

342 -}

343 -

344 -static int update_index(struct strbuf *out)

345 -{

346 - struct child_process cp = CHILD_PROCESS_INIT;

347 -

348 - /*

349 - * Update-index is very complicated and may need to have a public

350 - * function exposed in order to remove this forking.

351 - */

352 - cp.git_cmd = 1;

353 - strvec_pushl(&cp.args, "update-index", "--add", "--stdin", NULL);

354 - return pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0);

355 -}

356 -

328 static int restore_untracked(struct object_id *u_tree)

329 {

330 int res;

@@ -385,6 +356,75 @@ static int restore_untracked(struct object_id *u_tree)

356 return res;

357 }

358

359 +static void unstage_changes_unless_new(struct object_id *orig_tree)

360 +{

361 + /*

362 + * When we enter this function, there has been a clean merge of

363 + * relevant trees, and the merge logic always stages whatever merges

364 + * cleanly. We want to unstage those changes, unless it corresponds

365 + * to a file that didn't exist as of orig_tree.

366 + */

367 +

368 + struct diff_options diff_opts;

369 + struct lock_file lock = LOCK_INIT;

370 + int i;

371 +

372 + /*

373 + * Step 1: get a difference between orig_tree (which corresponding

374 + * to the index before a merge was run) and the current index

375 + * (reflecting the changes brought in by the merge).

376 + */

377 + diff_setup(&diff_opts);

378 + diff_opts.flags.recursive = 1;

379 + diff_opts.detect_rename = 0;

380 + diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT;

381 + diff_setup_done(&diff_opts);

382 +

383 + do_diff_cache(orig_tree, &diff_opts);

384 + diffcore_std(&diff_opts);

385 +

386 + /* Iterate over the paths that changed due to the merge... */

387 + for (i = 0; i < diff_queued_diff.nr; i++) {

388 + struct diff_filepair *p;

389 + struct cache_entry *ce;

390 + int pos;

391 +

392 + /* Look up the path's position in the current index. */

393 + p = diff_queued_diff.queue[i];

394 + pos = index_name_pos(&the_index, p->two->path,

395 + strlen(p->two->path));

396 +

397 + /*

398 + * Step 2: "unstage" changes, as long as they are still tracked

399 + */

400 + if (p->one->oid_valid) {

401 + /*

402 + * Path existed in orig_tree; restore index entry

403 + * from that tree in order to "unstage" the changes.

404 + */

405 + int option = ADD_CACHE_OK_TO_REPLACE;

406 + if (pos < 0)

407 + option = ADD_CACHE_OK_TO_ADD;

408 +

409 + ce = make_cache_entry(&the_index,

410 + p->one->mode,

411 + &p->one->oid,

412 + p->one->path,

413 + 0, 0);

414 + add_index_entry(&the_index, ce, option);

415 + }

416 + }

417 + diff_flush(&diff_opts);

418 +

419 + /*

420 + * Step 3: write the new index to disk

421 + */

422 + repo_hold_locked_index(the_repository, &lock, LOCK_DIE_ON_ERROR);

423 + if (write_locked_index(&the_index, &lock,

424 + COMMIT_LOCK | SKIP_IF_UNCHANGED))

425 + die(_("Unable to write index."));

426 +}

427 +

428 static int do_apply_stash(const char *prefix, struct stash_info *info,

429 int index, int quiet)

430 {

@@ -467,26 +507,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,

507 if (reset_tree(&index_tree, 0, 0))

508 return -1;

509 } else {

470 - struct strbuf out = STRBUF_INIT;

471 -

472 - if (get_newly_staged(&out, &c_tree)) {

473 - strbuf_release(&out);

474 - return -1;

475 - }

476 -

477 - if (reset_tree(&c_tree, 0, 1)) {

478 - strbuf_release(&out);

479 - return -1;

480 - }

481 -

482 - ret = update_index(&out);

483 - strbuf_release(&out);

484 - if (ret)

485 - return -1;

486 -

487 - /* read back the result of update_index() back from the disk */

488 - discard_cache();

489 - read_cache();

510 + unstage_changes_unless_new(&c_tree);

511 }

512

513 if (!quiet) {

t/lib-submodule-update.sh

+8 -8

@@ -316,14 +316,7 @@ test_submodule_switch_common () {

316 command="$1"

317 ######################### Appearing submodule #########################

318 # Switching to a commit letting a submodule appear creates empty dir ...

319 - if test "$KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES" = 1

320 - then

321 - # Restoring stash fails to restore submodule index entry

322 - RESULT="failure"

323 - else

324 - RESULT="success"

325 - fi

326 - test_expect_$RESULT "$command: added submodule creates empty directory" '

319 + test_expect_success "$command: added submodule creates empty directory" '

320 prolog &&

321 reset_work_tree_to no_submodule &&

322 (

@@ -337,6 +330,13 @@ test_submodule_switch_common () {

330 )

331 '

332 # ... and doesn't care if it already exists.

333 + if test "$KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES" = 1

334 + then

335 + # Restoring stash fails to restore submodule index entry

336 + RESULT="failure"

337 + else

338 + RESULT="success"

339 + fi

340 test_expect_$RESULT "$command: added submodule leaves existing empty directory alone" '

341 prolog &&

342 reset_work_tree_to no_submodule &&