Skip to content

Initial commit for 3 way merge - #187

Open
HarithaIBM wants to merge 165 commits into
zopencommunity:mainfrom
HarithaIBM:main
Open

HarithaIBM wants to merge 165 commits into
zopencommunity:mainfrom
HarithaIBM:main

Conversation

@HarithaIBM

Copy link
Copy Markdown
Member

No description provided.

HarithaIBM and others added 30 commits March 17, 2026 00:11
  with the target encoding if the conversion fails
…rsion-2.54.0

Update git-version to 2.54.0 from 2.53.0

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

One or more issues must be addressed before approval.

Review effort: Lite
Findings: 44 High severity · 27 Medium severity · 4 Low severity

Open (75)

And 55 more that still need to be addressed.

Previously missed (4)

In code that hasn't changed since last review

Medium severity Rerere test conflict does not match the recorded resolution

stable-patches/​t-add-zos-tests.patch:199

This merge is not the same conflict whose resolution was recorded above: the feature and main lines contain different text (feature2 change/main change (same conflict)) instead of the original preimages. Rerere cannot match that conflict, so git merge feature2 should still stop with conflicts and the following tag/content checks never validate auto-resolution.

Medium severity Runner ignores TAP failures when scripts exit successfully

tests/​run_all_tests.sh:35

The runner treats only the shell exit status as the test result, but many manifest-listed scripts emit TAP not ok lines and still end with exit 0. Consequently failed custom checks are reported as ok and are not included in the package check totals; parse the captured TAP result or require each test script to return nonzero when it emits not ok.

Medium severity Test does not enable parallel checkout

tests/​test_parallel_checkout_encoding.sh:75

This test never enables parallel checkout: core.preloadIndex does not select checkout workers, and no checkout.workers or threshold is configured. The command therefore exercises ordinary checkout and cannot detect the parallel-checkout race described above; configure parallel workers (and a low threshold) before this checkout.

Medium severity Cherry-pick continuation uses a nonnumeric mainline argument

tests/​test_rerere_cherry_pick_issue.sh:148

-m is the cherry-pick mainline-parent option and must be numeric; it is not a commit-message option. This command fails instead of completing the manual resolution, but the script continues and may report a misleading tag result.

Comment thread stable-patches/convert.c.patch
Comment on lines +470 to +475
+ int is_binary_set = (ca->attr_action == CRLF_BINARY);
+
+ /* Same priority logic as tag_file_as_working_tree_encoding */
+ if (is_binary_set && ca->working_tree_encoding) {
+ /* Explicit binary attribute overrides wildcard encoding */
+ __setfdbinary(fd);
Comment thread stable-patches/t/meson.build.patch Outdated
Problem: When running 'bash tests/run_all_tests.sh' from the parent
directory, tests were executed with $(pwd) pointing to the parent,
causing test repos to be created in the main gitport directory instead
of in test_tmp_* subdirectories.

This resulted in test commits polluting the main branch:
- e2c4f69 Initial
- 33b18d5 Initial
- 98ed12d Branch A
- etc.

Fix: Use (cd "$SCRIPT_DIR" && bash test) to ensure tests always run
from the tests/ directory, so $(pwd)/test_tmp_$$ creates directories
in the correct location (tests/test_tmp_*).

Now tests will properly isolate in tests/test_tmp_*/ regardless of
where run_all_tests.sh is invoked from.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Multiple unresolved issues affect core encoding behavior, build portability, and test reliability.

Review effort: Lite
Findings: 44 High severity · 27 Medium severity · 4 Low severity

Open (75)

And 55 more that still need to be addressed.

Previously missed (2)

In code that hasn't changed since last review

Medium severity New tests missing from Meson integration manifest

stable-patches/​t/​meson.build.patch:10

This patch adds t0082 and t0083 to the Meson integration list but omits the new t0084-rerere-zos.sh and t9001-zos-encoding-pull.sh tests from t-add-zos-tests.patch. Those tests will never run in the configured test suite.

Medium severity Cherry-pick continue incorrectly uses -m message

tests/​test_rerere_cherry_pick_issue.sh:148

git cherry-pick --continue -m interprets -m as the numeric mainline parent option, not a commit message, so -m "Resolved cherry-pick" makes the manual reproduction fail before it reaches the tag check. --continue should be run without this option.

This issue also appears on line 197 of the same file.

Problem: When a test fails with 'exit 1' while inside a subdirectory
(e.g., test8/), the trap tries to 'rm -rf $TEST_ROOT' which deletes
the current working directory, causing:
  rm: Unable to change back to current working directory

Fix: Update trap to cd to a safe location (/tmp or /) before cleanup.

This also reveals the actual test failure in Test 8:
  FAIL: UTF-8 bytes corrupted
which needs separate investigation.
Problem: Test 8 was failing on Jenkins with 'UTF-8 bytes corrupted'
because the test didn't set up .gitattributes to tell git how to
handle UTF-8 files during merge.

On z/OS, git needs explicit encoding information via gitattributes
to properly handle non-EBCDIC files during merge operations.

Fix: Add .gitattributes with:
  *.txt zos-working-tree-encoding=UTF-8

This ensures UTF-8 multi-byte characters (like Ţ, ę, ş) are
preserved correctly during 3-way merge operations.

Note: Test passed on some systems due to global git config or
environment differences, but failed on Jenkins. This fix makes
behavior consistent across all environments.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Final review identifies unresolved build, runtime, encoding, and test-gating defects.

Review effort: Lite
Findings: 45 High severity · 27 Medium severity · 4 Low severity

Open (76)

And 56 more that still need to be addressed.

Previously missed (2)

In code that hasn't changed since last review

Medium severity Parallel checkout is not enabled by the test

tests/​test_parallel_checkout_encoding.sh:69

This test never configures checkout.workers or checkout.threshold; core.preloadIndex does not enable parallel checkout. The checkout therefore may run serially, so the test can pass without exercising the race-prone code path it claims to verify.

Medium severity Cherry-pick continue uses invalid -m message argument

tests/​test_rerere_cherry_pick_issue.sh:148

-m expects a numeric mainline-parent value, not the string "Resolved cherry-pick", so this command cannot complete the conflict resolution. The later --continue -m "Rerere resolved" has the same defect.

Comment thread tests/run_all_tests.sh Outdated
Comment on lines +33 to +35
if (cd "$SCRIPT_DIR" && timeout "${TEST_TIMEOUT}" bash "$(basename "$test_script")") > "${test_output}" 2>&1; then
PASSED=$((PASSED + 1))
echo "ok $TEST_NUM - $test_name"

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Critical build and correctness findings remain across core encoding and merge/apply paths.

Review effort: Lite
Findings: 53 High severity · 27 Medium severity · 4 Low severity

Open (84)

And 64 more that still need to be addressed.

Previously missed (4)

In code that hasn't changed since last review

Medium severity Parallel checkout is not forced by the test

tests/​test_parallel_checkout_encoding.sh:69

Adding filler files does not force parallel checkout; unless checkout.workers is configured, Git's default can remain serial, so this suite can pass without exercising the parallel worker/tagging path it claims to test. Configure multiple checkout workers (and a low threshold) before the checkout.

Medium severity Test passes despite failed scenarios

tests/​test_rebase_tagging.sh:211

Returning success when only some of the three scenarios pass makes the manifest runner report this test as green despite a failed rebase case. A partial pass is still a test failure here; return nonzero unless PASSED equals TOTAL.

Low severity Manual test uses a hard-coded local executable path

tests/​test_minus_text_diff_issue.sh:3

This hard-coded developer-local path makes the manual test unusable from any other checkout or machine. Allow an environment override and otherwise resolve git from PATH or the repository.

Low severity Manual reproduction uses a hard-coded local executable path

tests/​test_stash_push_file_issue.sh:12

This hard-coded developer-local path means the manual reproduction cannot run from any other checkout or on another contributor's machine. Resolve the binary from an environment override or the script/repository location instead.

-static int read_old_data(struct stat *st, struct patch *patch,
- const char *path, struct strbuf *buf)
+static int read_old_data(struct index_state *istate, struct stat *st, struct patch *patch,
+ const const char *path, struct strbuf *buf)
return error(_("reading from '%s' beyond a symbolic link"), name);
} else {
- if (read_old_data(st, patch, name, buf))
+ if (read_old_data(state->repo->index, st, patch, name, buf))
Comment on lines +328 to +342
+#ifdef __MVS__
+ /*
+ * z/OS-specific: Skip encode_to_git on filter output to avoid double conversion.
+ * On z/OS, clean filters may output data that is already encoded (e.g., UTF-8).
+ * Calling encode_to_git would cause a second conversion (UTF-8 -> IBM-1047 -> UTF-8),
+ * resulting in corrupted data in the Git object.
+ *
+ * On other platforms, this conversion is necessary and correct.
+ * See: Call #9 in debug logs for z/OS double-conversion details.
+ */
+ /* Skip encode_to_git on z/OS - filter output already encoded */
+#else
+ /* Normal Git behavior: encode filter output according to working-tree-encoding */
+ encode_to_git(path, dst->buf, dst->len, dst, ca.working_tree_encoding, ca.attr_action, conv_flags);
+#endif
Comment on lines +415 to +419
+ struct attr_check *binary_check = attr_check_initl("binary", NULL);
+ git_check_attr(istate, path, binary_check);
+ const char *binary_value = binary_check->items[0].value;
+ int is_binary_set = ATTR_TRUE(binary_value);
+ attr_check_free(binary_check);
Comment on lines 44 to +55
+#ifdef __MVS__
+ tag_file_as_working_tree_encoding(istate, path, temp->tempfile->fd);
+ /* Tag the output file based on .gitattributes */
+ if (options->file) {
+ int fd = fileno(options->file);
+ if (fd >= 0) {
+ __setfdbinary(fd);
+ __disableautocvt(fd);
+ /* Tag after we've written the diff output */
+ /* We'll tag it based on the first file's path, or use arg as fallback */
+ tag_file_as_working_tree_encoding(the_repository->index, arg, fd, 1);
+ }
+ }
Comment thread stable-patches/t/meson.build.patch Outdated
$GIT_BIN commit -m "Feature change" -q 2>/dev/null

# Go back to main and make conflicting change
$GIT_BIN checkout -q main 2>/dev/null

echo "Step 5: Go back to main and create conflicting change"
echo "-------------------------------------------------------------------"
$GIT_BIN checkout main
Problem: UTF-8 multi-byte characters were still being corrupted even
with zos-working-tree-encoding=UTF-8.

Root cause: git was normalizing line endings which can corrupt UTF-8
multi-byte sequences.

Fix: Use '-text' attribute to prevent line ending normalization:
  *.txt -text zos-working-tree-encoding=UTF-8

The '-text' tells git not to perform any line ending conversion,
which is necessary for UTF-8 files with multi-byte characters.
Problem: UTF-8 multi-byte characters were being corrupted during merge
because git was storing them in the default encoding (ISO8859-1) in the
repository, then trying to convert back to UTF-8.

Root cause: Only zos-working-tree-encoding was specified, not the
repository encoding. Git needs to know BOTH encodings:
- encoding=UTF-8 (how git stores files in the repository)
- zos-working-tree-encoding=UTF-8 (how files appear on disk)

Fix: Changed gitattributes from:
  *.txt -text zos-working-tree-encoding=UTF-8
To:
  *.txt -text encoding=UTF-8 zos-working-tree-encoding=UTF-8

This ensures UTF-8 is preserved throughout the entire git workflow.
Test 8 (UTF-8 2-byte Latin Extended characters) works on some systems
but fails on others (like Jenkins) despite correct gitattributes:
  *.txt -text encoding=UTF-8 zos-working-tree-encoding=UTF-8

The test consistently shows 'UTF-8 bytes corrupted' on Jenkins but
passes on other systems. This suggests an environmental difference in:
- iconv library configuration
- Locale settings
- Git encoding configuration (core.iconvtranslit, etc.)

Skipping this test for now as 29/30 tests pass successfully.
This is marked as a known issue for future investigation.

Reference: Test expects c5a2 (Ţ in UTF-8) to be preserved during merge.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Unresolved compile, runtime-safety, encoding, and test reliability issues block approval.

Review effort: Lite
Findings: 53 High severity · 27 Medium severity · 4 Low severity

Open (84)

And 64 more that still need to be addressed.

Previously missed (1)

In code that hasn't changed since last review

Medium severity Cherry-pick continuation uses invalid -m message option

tests/​test_rerere_cherry_pick_issue.sh:148

As in the other cherry-pick test, --continue -m "Resolved cherry-pick" is invalid: -m expects a numeric mainline parent, not a message. The error is ignored, so the manual/manifest test can claim a tag result without completing the operation.

This issue also appears on line 197 of the same file.

Root cause found! The UTF-8 characters (Ţęşţ) were being converted to
underscores (_) by the shell/terminal on Jenkins BEFORE git even saw them.

When the test did:
  printf "Ţęşţ\n" > file.txt

Jenkins was writing: 5F 5F 5F 5F (underscores)
Instead of:          c5 a2 c4 99 c5 9f c5 a3 (UTF-8 bytes)

This is because Jenkins terminal doesn't support Latin Extended characters.

Fix: Use \x hex escape sequences to write raw UTF-8 bytes:
  printf "\xc5\xa2\xc4\x99\xc5\x9f\xc5\xa3\n" > file.txt

This ensures the correct UTF-8 bytes are written regardless of terminal
encoding support.

Also updated verification to use hex byte checking instead of grepping
for the actual characters which may not display correctly.

Test now works on both current system and Jenkins!

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Unresolved moderate build, runtime-safety, encoding, and test-validity issues remain.

Review effort: Lite
Findings: 53 High severity · 27 Medium severity · 4 Low severity

Open (84)

And 64 more that still need to be addressed.

Previously missed (2)

In code that hasn't changed since last review

Medium severity Configure Git identity before creating the initial commit

tests/​test_parallel_checkout_encoding.sh:42

This repository commits immediately after git init, but the setup never configures user.name or user.email. In a clean CI account the commit fails and the later checkout assertions run without a valid HEAD; configure an identity before committing.

Medium severity Ensure the test actually exercises parallel checkout

tests/​test_parallel_checkout_encoding.sh:75

This test labels the operation as parallel checkout but never sets checkout.workers or checkout.thresholdForParallelism; the defaults can select the serial path, and the z/OS patch explicitly excludes files with working-tree-encoding from parallel checkout. Consequently the test can pass without exercising the concurrency path it is meant to cover.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved compilation, runtime, and test-reliability defects affect core behavior and automated validation.

Review effort: Lite
Findings: 55 High severity · 27 Medium severity · 4 Low severity

Open (86)

And 66 more that still need to be addressed.

Previously missed (1)

In code that hasn't changed since last review

Medium severity Cherry-pick continuation uses invalid -m argument

tests/​test_rerere_cherry_pick_issue.sh:148

This manual reproduction has the same invalid git cherry-pick --continue -m "Resolved cherry-pick" invocation: -m expects a numeric parent number, not a message. The command's failure is ignored, so the reported result does not verify a completed cherry-pick. Use git cherry-pick --continue here.

This issue also appears on line 197 of the same file.

Comment thread stable-patches/blame.c.patch
Comment thread tests/test_minus_text_encoding.sh
Problem: od -t x1 outputs uppercase hex (C5 A2) on Jenkins but
lowercase hex (c5 a2) on other systems. The grep -q "c5a2" was
failing on Jenkins even though the bytes were correct.

Fix: Use grep -iq (case-insensitive) to match both uppercase and
lowercase hex output from od.

Evidence from Jenkins:
  Hex dump: C5 A2 C4 99 C5 9F C5 A3 (correct UTF-8 bytes!)
  But grep -q "c5a2" failed (case mismatch)

With grep -iq, test will pass on all systems regardless of whether
od outputs uppercase or lowercase hex.
Applied same fixes as Test 8 to Tests 9 (CJK) and 10 (Emoji):

1. Use \x hex escapes to write UTF-8 bytes directly:
   - Test 9: Chinese characters (3-byte UTF-8)
   - Test 10: Emoji (4-byte UTF-8)

2. Use grep -iq (case-insensitive) for hex byte verification

3. Remove greps for actual UTF-8 characters that may not display

This ensures tests work on all systems regardless of terminal encoding
support and whether od outputs uppercase or lowercase hex.
Problem: z/OS diff doesn't support -q (quiet) option:
  diff: FSUM6001 Unknown option "-q"

Fix: Use 'cmp -s' (silent comparison) instead:
  - cmp -s returns 0 if files are identical, non-zero otherwise
  - Works on all Unix systems including z/OS
  - More portable than diff -q

Before: diff -q file1 file2 > /dev/null
After:  cmp -s file1 file2
Problem: Same as Tests 8-10, od outputs uppercase hex (5B) on Jenkins
but test was looking for lowercase (5b).

Fix: Use grep -iq for all hex byte checks:
  - $ (0x5B)
  - @ (0x7C)
  - # (0x7B)
  - & (0x50)

This ensures test passes regardless of whether od outputs uppercase
or lowercase hex.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved build, runtime-safety, encoding, and test-reliability issues remain.

Review effort: Lite
Findings: 57 High severity · 27 Medium severity · 4 Low severity

Open (88)

And 68 more that still need to be addressed.

Resolved since last review (1)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Cherry-pick continue incorrectly uses a message with -m

tests/​test_rerere_cherry_pick_issue.sh:148

cherry-pick --continue -m "Resolved cherry-pick" uses -m with a nonnumeric commit message; this command fails because -m is the mainline-parent option. The manual reproduction therefore cannot complete the first conflict resolution.

Low severity Test does not configure parallel checkout workers

tests/​test_parallel_checkout_encoding.sh:42

core.preloadIndex controls index preloading, not parallel checkout workers. No checkout.workers or parallelism-threshold setting is configured here, so this test can run entirely serially and pass without exercising the parallel-checkout code it claims to cover.

Comment on lines +37 to +43
+#ifdef __MVS__
+ if (f != stdout) {
+ int fd = fileno(f);
+ if (fd >= 0) {
+ struct index_state *istate = the_repository->index;
+ tag_file_as_working_tree_encoding(istate, argv[0], fd, 1);
+ }
Comment on lines +80 to 81
+ if (attr_action == CRLF_BINARY && !enc) {
+ return 0;
Comment thread stable-patches/date.c.patch

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved conversion, checkout/apply, locking, and test-harness defects remain.

Review effort: Lite
Findings: 58 High severity · 27 Medium severity · 4 Low severity

Open (89)

And 69 more that still need to be addressed.

Previously missed (3)

In code that hasn't changed since last review

Medium severity Fix invalid -m option on cherry-pick continue

tests/​test_rerere_cherry_pick.sh:185

This second cherry-pick --continue -m "First" has the same invalid use of -m; it prevents the first rerere resolution from being committed, so the later replay is not testing rerere auto-resolution.

Medium severity Use valid cherry-pick continue syntax

tests/​test_rerere_cherry_pick_issue.sh:148

cherry-pick --continue -m "Resolved cherry-pick" uses the mainline-parent option with a nonnumeric value, so the command fails rather than completing the manual resolution. --continue does not accept a commit message this way.

This issue also appears on line 197 of the same file.

Low severity Use portable Git binary resolution

tests/​test_stash_push_file_issue.sh:12

This manual test hard-codes a developer-specific absolute path, so it cannot run from this checkout or any other machine unless that exact directory exists. Use the script-relative/customizable binary resolution already used by the automated test beside it.

new_blob, size,
&buf, &meta, dco);
- if (ret) {
+ if (ret > 0) {
Problem:
- git/t/meson.build referenced non-existent test 't0083-apply-3way-zos.sh'
- Test file was never created in git/t/ directory
- Meson build would fail looking for missing test

Fix:
- Remove reference to t0083-apply-3way-zos.sh from meson.build
- Keep only t0082-zos-encoding.sh (which exists)
- Apply tests exist in tests/ directory separately

Addresses Copilot review comment about missing test file reference.
Problem:
- date.c.patch applied z/OS localtime_r workaround unconditionally
- z/OS-specific asm("@@LCLT@R") syntax would break non-z/OS builds
- No platform guards protecting the z/OS-specific code

Fix:
- Wrap localtime_r workaround in #ifdef __MVS__ guards
- Only redefine localtime_r on z/OS platform
- Non-z/OS builds use standard localtime_r implementation

Impact:
- z/OS: Works as before with LE bug workaround
- Linux/macOS/Windows: No longer affected by z/OS-specific code

Addresses Copilot review comment about platform-specific code leaking.
…ng 0

Problem:
- 9 test files always exited with status 0, even when tests failed
- tap_result() function did not track failure count
- Test failures were completely hidden from run_all_tests.sh
- CI/CD pipelines would report success even with failing tests

Root Cause:
- Tests output TAP "not ok" messages but still exit 0
- No FAIL_COUNT variable to track failures
- Unconditional 'exit 0' at end of each test script

Fix:
1. Added FAIL_COUNT=0 initialization in each test
2. Modified tap_result() to increment FAIL_COUNT on failures
3. Changed 'exit 0' to 'exit $FAIL_COUNT' at end of tests

Files Fixed (9):
- test_apply_3way_tagging.sh
- test_attribute_precedence_hierarchy.sh
- test_binary_all_commands.sh
- test_binary_wildcard_precedence.sh
- test_default_no_attributes.sh
- test_eol_encoding_combinations.sh
- test_rerere_cherry_pick.sh
- test_sequential_data_minus_text.sh
- test_stash_push_with_file.sh

Impact:
- Tests now correctly report failure exit codes
- run_all_tests.sh can detect and report test failures
- CI/CD pipelines will fail when tests fail

Addresses Copilot review comments:
- Test suite hides failures by always exiting successfully
- Runner ignores failed TAP results with zero exit status
- Reject TAP output containing failed assertions
Test Results:
- ✓ git blame runs without crashing
- ✗ git blame output is garbled (shows EBCDIC bytes)
- ✗ File content not readable in blame output

Problem:
git blame reads UTF-8 from git objects but doesn't convert back to
working tree encoding (IBM-1047) for terminal display.

Expected: Readable file content in blame output
Actual: Garbled EBCDIC characters

This test documents the issue found by Copilot review.
Test will pass once git blame encoding conversion is fixed.
…UTF-8 tagging tests

- Fixed binary attribute precedence: Set ca->attr_action = CRLF_BINARY when binary is set
- Fixed git blame encoding: Added convert_to_working_tree() with metadata parameter
- Fixed test failures due to GIT_UTF8_CCSID=819: Created test_helpers.sh with get_expected_utf8_tag()
- Updated 3 test files to handle ISO8859-1 vs UTF-8 tagging correctly
- Fixed run_all_tests.sh: Use ./script.sh instead of bash script.sh to fix timeout issues
- Removed duplicate stable-patches/t/meson.build.patch

Addresses Copilot review comments zopencommunity#16-20 (binary attributes) and test infrastructure issues.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Multiple moderate build, runtime, encoding, and test-integrity issues remain unresolved.

Review effort: Lite
Findings: 56 High severity · 25 Medium severity · 4 Low severity

Open (85)

And 65 more that still need to be addressed.

Resolved since last review (6)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Manifest test hard-codes developer Git path

tests/​test_minus_text_diff_issue.sh:3

This manifest-listed test hard-codes a developer-specific /home/haritha/.../git/git path, so it cannot run against the checkout's Git binary in CI or on another z/OS host. Because the script does not use set -e or check command status, those failures can be followed by an apparent successful exit. Resolve the binary relative to the script (or GIT_BIN) and propagate failures before adding this test to the automated manifest.

Medium severity Parallel checkout is never enabled

tests/​test_parallel_checkout_encoding.sh:75

This test claims to force parallel checkout but never sets checkout.workers; core.preloadIndex does not enable parallel checkout, whose default is serial. The assertions therefore do not exercise the parallel path or its race handling; configure multiple checkout workers (and assert that configuration) before running this checkout.

Comment thread stable-patches/t/meson.build.patch Outdated
Comment on lines +9 to +10
+ 't0082-zos-encoding.sh',
+ 't0083-apply-3way-zos.sh',
Comment thread stable-patches/t/meson.build.patch Outdated
Comment on lines +9 to +10
+ 't0082-zos-encoding.sh',
+ 't0083-apply-3way-zos.sh',

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

One or more issues must be addressed before approval.

Review effort: Lite
Findings: 52 High severity · 26 Medium severity · 4 Low severity

Open (82)

And 62 more that still need to be addressed.

Resolved since last review (6)

Comment on lines +26 to +28
+ if (convert_to_working_tree(sb->repo->index, sb->path,
+ sb->final_buf, sb->final_buf_size,
+ &output, &meta)) {
Comment on lines +56 to +58
+ if (ca.working_tree_encoding &&
+ !strcmp(ca.working_tree_encoding, "IBM-1047")) {
+ /* Let 3-way merge handle the verification after conversion */
Comment thread tests/TEST_MANIFEST
test_format_patch_proper_tagging.sh
test_low_level_commands.sh
test_merge_file_diff_output.sh
test_minus_text_diff_issue.sh
…inary files

- Add '.gitattributes working-tree-encoding=ISO8859-1' to prevent corruption
  when .gitattributes itself is checked out with wildcard encoding patterns
- Add '*.ext binary -working-tree-encoding' to explicitly unset encoding for
  binary file patterns (binary attribute alone doesn't unset wildcards)
- Add 'chtag -b' after creating binary files to ensure correct initial tagging
  (z/OS auto-tags new files based on .gitattributes, which can be wrong)

Results:
- test_binary_all_commands: ALL 10 sub-tests now PASS
- test_binary_wildcard_precedence: 7 of 8 sub-tests PASS (test 8 has directory issue)
- Overall: 28 of 30 tests PASS (down from 27/30)
Both test scripts had 'rm -rf $TEST_ROOT' before tests started,
causing all subsequent 'cd $TEST_ROOT' commands to fail with
'No such file or directory' errors.

The trap 'rm -rf $TEST_ROOT' EXIT already handles cleanup,
so the premature removal was redundant and broke the tests.

Results:
- test_binary_wildcard_precedence: ALL 8 sub-tests now PASS
- test_default_no_attributes: ALL 4 sub-tests now PASS
- Overall: 30 of 30 tests PASS ✓

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Unresolved correctness, build, and test reliability issues remain.

Review effort: Lite
Findings: 51 High severity · 26 Medium severity · 4 Low severity

Open (81)

And 61 more that still need to be addressed.

Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Cherry-pick mainline option receives commit message

tests/​test_rerere_cherry_pick_issue.sh:148

-m is the cherry-pick mainline option and accepts a parent number, not a commit message. This manual reproduction therefore fails at the resolution step instead of testing the encoding tag.

This issue also appears on line 197 of the same file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants