Skip to content

Initial commit for 3 way merge - #187

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

HarithaIBM wants to merge 125 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 review requested due to automatic review settings September 21, 2026 12:46

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

Critical correctness issues and multiple moderate build, test, and integration issues remain.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 20 High severity · 12 Medium severity · 7 Low severity

Open (39)

And 19 more that still need to be addressed.

Resolved since last review (23)

And 3 more resolved.

Comment on lines +20 to +21
+static int read_old_data(struct index_state *istate, struct stat *st, struct patch *patch,
+ const const char *path, struct strbuf *buf)
+ 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 +328 to +330
+#if 0 /* DISABLED - causes double conversion bug! */
+ encode_to_git(path, dst->buf, dst->len, dst, ca.working_tree_encoding, ca.attr_action, conv_flags);
+#endif
Comment thread stable-patches/diff.c.patch Outdated
Comment thread buildenv
export ZOPEN_INSTALL_OPTS=""
#export ZOPEN_CHECK_OPTS="-i test -j\$ZOPEN_NUM_JOBS"
export ZOPEN_CHECK_OPTS="test -j\$ZOPEN_NUM_JOBS"
export ZOPEN_CHECK_OPTS="-i test -j\$ZOPEN_NUM_JOBS"
# Reset and apply
"$GIT_BIN" checkout -f file.txt 2>/dev/null

if "$GIT_BIN" apply --3way patch.diff 2>&1 | grep -qv "^error:"; then

if "$GIT_BIN" apply patch.txt 2>/dev/null; then
TAG=$(chtag -p data.dat 2>/dev/null | awk '{print $2}')
if [ "$TAG" = "ISO8859-1" ] || [ "$TAG" = "UTF-8" ]; then
Comment on lines +140 to +145
# Merge A into B (conflict)
if ! "$GIT_BIN" merge branchA 2>/dev/null; then
# Resolve
echo "merged line" > file.txt
"$GIT_BIN" add file.txt
"$GIT_BIN" commit -q -m "Merge A into B" 2>/dev/null
Comment thread update_tests_tempdir.sh
Comment on lines +8 to +14
for test_file in test_*.sh; do
if [ -f "$test_file" ]; then
echo "Updating: $test_file"

# Replace /tmp with ./test_tmp_$$
sed -i 's|TEST_ROOT="$(mktemp -d /tmp/git_.*XXXXXX)"|TEST_ROOT="$(pwd)/test_tmp_$$"|g' "$test_file"
sed -i 's|mktemp -d /tmp/|mkdir -p ./test_tmp_$$ \&\& echo ./test_tmp_$$/|g' "$test_file"
Comment thread tests/TEST_STATUS.md
Comment on lines +3 to +5
## All Tests Now Pass ✅

After applying the parallel checkout timing fix (commit ca6a9e6), all tests now pass.
Remove OUTPUT file tagging code that was not needed or applied.
The patch now accurately reflects only the changes present in the source:
- Use read() instead of mmap() for files with working-tree-encoding
- Prevents z/OS kernel auto-conversion during file reading

This minimal patch is sufficient for all tested scenarios:
- git diff
- git stash
- git apply --3way
- git format-patch
Copilot AI review requested due to automatic review settings September 21, 2026 12:58

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, encoding-safety, and test-suite correctness findings remain.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 21 High severity · 12 Medium severity · 7 Low severity

Open (40)

And 20 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 Use build-marker presence instead of a hard-coded build identifier

.zoslib_hooks/​zoslib_env_hook.c:129

The build-time guard is hard-coded to one developer's build identifier. Any other zopen build will skip the guard and export source/build-time paths such as GIT_TEMPLATE_DIR and GIT_EXEC_PATH, contaminating the build and dependent tools; test for the build marker's presence instead of this literal value.

Medium severity Include the rerere test in Meson integration

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

The repository contains the z/OS rerere test patch, but this Meson integration list adds only t0082 and t0083. Under Meson, t0084-rerere-zos.sh will never run, leaving the new rerere coverage out of the configured test suite.

int close_file;

+#ifdef __MVS__
+ char *output_path; /* For z/OS file tagging */
+ if (lk->tempfile && fstat(lk->tempfile->fd, &st) >= 0 && S_ISREG(st.st_mode))
+ {
+ __chgfdccsid(lk->tempfile->fd, 819);
+ __chgfdccsid(lk->tempfile->fd, utf8_ccsid);
- Add test_encoding_pull_clone.sh to tests/ directory
  Tests git pull vs git clone encoding behavior
  Verifies files are tagged correctly with working-tree-encoding

- Add patch for git/t/ test files:
  - t0082-zos-encoding.sh: z/OS encoding failure handling
  - t0084-rerere-zos.sh: rerere with z/OS file tagging
  - t9001-zos-encoding-pull.sh: encoding pull/clone test
  - run-zos-tests.sh: wrapper to run z/OS tests cleanly

Tests verify fixes for:
- git diff garbled output
- git stash file tag corruption
- git apply --3way conflict markers
- git pull file tag updates

Issue: https://github.com/ZOSOpenTools/gitport/issues/255
Copilot AI review requested due to automatic review settings September 22, 2026 05:05

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 critical and moderate build, patch, test, and documentation issues remain.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 23 High severity · 14 Medium severity · 7 Low severity

Open (44)

And 24 more that still need to be addressed.

Previously missed (7)

In code that hasn't changed since last review

Medium severity Unsupported libiconv configure option is ignored

buildenv:34

The build log added in this change reports unrecognized options: --with-libiconv-prefix, so this newly configured option is ignored by configure and cannot select LIBICONV_HOME. Remove the unsupported option or pass the dependency through supported compiler/linker variables.

Medium severity Attribute lookup uses non-relative path

stable-patches/​builtin/​merge-file.c.patch:42

Attribute lookup receives argv[0] rather than the prefixed repository-relative path used for the output file. Invoking git merge-file from a subdirectory can therefore miss root/subdirectory .gitattributes patterns and apply the fallback tag; pass the normalized fpath (or another repository-relative path) to the tagging helper.

Medium severity Test runner still stores output under /tmp

tests/​run_all_tests.sh:29

The runner still writes every test's captured output to /tmp, even though this change claims to eliminate /tmp exhaustion and the committed logs show that /tmp was already a failure point. Store the per-test output under the repository-local test directory (and remove it afterward) instead.

Medium severity Wildcard executes helper scripts as tests

tests/​run_all_tests.sh:48

The wildcard includes every executable shell script, not just tests; for example it can run fix_all_test_scripts.sh and diagnose_codepage_mapping.sh as if they were test cases. That can mutate the suite during execution and count diagnostics/helpers as passing tests. Select an explicit test list or exclude helper scripts, and derive the TAP plan from the same selection.

Medium severity Pipeline masks git apply exit status

tests/​test_apply_3way_ebcdic.sh:71

This pipeline tests grep's output, not the exit status of git apply; any non-error warning can make the condition true even when git apply --3way failed. The test can therefore report a successful patch application incorrectly. Check the Git command's exit status directly (for this clean patch, redirect its output rather than piping it to grep).

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

Medium severity Test accepts incorrect UTF-8 attribute

tests/​test_apply_tagging.sh:61

The expected attribute is ISO8859-1, but accepting UTF-8 lets the test pass when .gitattributes is ignored or the wrong tag is selected. Assert exactly ISO8859-1 so this regression test verifies the requested encoding.

Low severity Strict mode documentation overstates conversion failure behavior

README.md:102

The documentation says strict mode will stop Git on an unconvertible character, but the changed conversion callers intentionally report the error and fall back to unconverted data. This overstates the guarantee and can mislead users about data-integrity behavior; describe the fallback explicitly.

Comment on lines +20 to +21
+static int read_old_data(struct index_state *istate, struct stat *st, struct patch *patch,
+ const const char *path, struct strbuf *buf)
Comment thread update_tests_tempdir.sh

# Replace /tmp with ./test_tmp_$$
sed -i 's|TEST_ROOT="$(mktemp -d /tmp/git_.*XXXXXX)"|TEST_ROOT="$(pwd)/test_tmp_$$"|g' "$test_file"
sed -i 's|mktemp -d /tmp/|mkdir -p ./test_tmp_$$ \&\& echo ./test_tmp_$$/|g' "$test_file"
Comment on lines +84 to +92
for patch in ../stable-patches/*.patch; do
git apply "$patch"
done
for patch in ../stable-patches/builtin/*.patch; do
git apply "$patch"
done
for patch in ../stable-patches/t/*.patch; do
git apply "$patch"
done
Comment on lines +18 to +19
TEST_DIR=$(mktemp -d /tmp/git-encoding-test.XXXXXX)
cd "$TEST_DIR"
1. Documentation Enhancement (README.md)
   - Added .gitattributes pattern ordering section
   - Explains: "Rules at BOTTOM take precedence over rules at TOP"
   - Includes correct vs incorrect ordering examples
   - Critical for users to avoid file tagging issues

2. Test Suite Fix (tests/test_3way_merge_encodings.sh)
   - Fixed Test 11 to SKIP instead of FAIL
   - Test expectation was incorrect - git behavior is correct
   - Git doesn't auto-retag files after manual .gitattributes merge resolution
   - Added explanation for why test is skipped

Related: https://github.com/ZOSOpenTools/gitport/issues/255
Copilot AI review requested due to automatic review settings September 22, 2026 11:46

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, documentation, and test reliability issues remain.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 24 High severity · 14 Medium severity · 7 Low severity

Open (45)

And 25 more that still need to be addressed.

Previously missed (1)

In code that hasn't changed since last review

Medium severity Run tests with the rebuilt Git binary

tests/​test_encoding_pull_clone.sh:28

Unlike the other new tests, this script invokes git from PATH throughout and never selects the rebuilt repository binary. Running it from run_all_tests.sh can therefore validate a system Git instead of the patched git/git; use the same repository-relative binary selection as the other tests.

Comment on lines +104 to +108
+ if (iconv_translit) {
+ out_encoding_translit = xstrfmt("%s//TRANSLIT", fallback_encoding(out_encoding));
+ conv = iconv_open(out_encoding_translit, in_encoding);
+ free(out_encoding_translit);
+ } else {
Test added 4 commits September 22, 2026 08:55
Test verifies that git correctly tags files with zos-working-tree-encoding
even when -text attribute is set.

Bug: -text + zos-working-tree-encoding results in binary tag (should be encoded)
Expected: -text only disables EOL conversion, not encoding/tagging

Test cases:
1. Baseline: text + encoding (passes)
2. Bug: -text + encoding (fails - tags as binary)
3. Use case: mixed-codepage file (fails - tags as binary)
4. Real-world: nickrayjones scenario (fails - tags as binary)

Integration: TAP format, auto-discovered by run_all_tests.sh

Status: Currently FAILS (3/4) - confirms bug exists
After fixing convert.c: Should PASS (4/4)
Copilot AI review requested due to automatic review settings September 23, 2026 04:43

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 critical and moderate findings affect build safety, patch application, runtime tagging, and test validity.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 27 High severity · 14 Medium severity · 7 Low severity

Open (48)

And 28 more that still need to be addressed.

Previously missed (4)

In code that hasn't changed since last review

Medium severity Initialize and clean up diff_options output_path

stable-patches/​diff.h.patch:11

This patch adds output_path to struct diff_options but the corresponding diff.c.patch never initializes or frees it. Once the field is used for the intended output tagging, ensure its lifetime is handled in every initialization/cleanup path; otherwise the new field remains dead state and the documented diff-output fix is incomplete.

Medium severity Register t0084 rerere regression test in Meson

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

The Meson test list registers the new t0082 and t0083 tests but omits the existing t/t0084-rerere-zos.sh.patch test that this patch series also documents. Meson builds will therefore silently skip the rerere z/OS regression test.

Medium severity Exclude network-dependent scripts from deterministic build checks

tests/​run_all_tests.sh:36

The wildcard now feeds every executable shell script into the build check, including basicclone.sh, stepwiseclone.sh, and testtags.sh, which clone external repositories over SSH. This makes the required test result depend on network access and developer credentials; use an explicit deterministic test list or isolate network/integration tests.

Medium severity Use the selected rebuilt Git binary consistently

tests/​test_encoding_pull_clone.sh:28

This test creates its repository with the ambient git command instead of the rebuilt binary selected by the other tests ($REPO_ROOT/git/git when available). It can therefore pass against an unrelated system Git and does not validate the patch under review; use one consistently selected GIT_BIN for every Git invocation.

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

Comment on lines +128 to +129
if ((envar_value = getenv("ZOPEN_IN_ZOPEN_BUILD")) &&
strcmp(envar_value, "HARITHA.4809.7513") == 0) {
Comment on lines +1 to +5
diff --git a/t/t0082-zos-encoding.sh b/t/t0082-zos-encoding.sh
new file mode 100755
index 0000000..9bfe25e
--- /dev/null
+++ b/t/t0082-zos-encoding.sh
Comment on lines +40 to +44
+char *reencode_string_len_translit(const char *in, size_t insz,
+ const char *out_encoding, const char *in_encoding,
+ size_t *outsz)
+{
+ iconv_t conv;
Use relative path ../git/git instead of system git to ensure
tests run against the locally built git with fixes applied.

This allows the test to work in zopen_check_results where GIT_BIN
may not be set.
Copilot AI review requested due to automatic review settings September 23, 2026 07:06

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 critical and moderate correctness issues remain across conversion, patch application, locking, build setup, and test coverage.

Review effort: Lite
Findings: 27 High severity · 14 Medium severity · 7 Low severity

Open (48)

And 28 more that still need to be addressed.

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

In code that hasn't changed since last review

Medium severity Unsupported libiconv configure option is ignored

buildenv:34

The checked-in configure log already reports configure: WARNING: unrecognized options: --with-libiconv-prefix, so this newly added option is not being consumed by configure. Remove it (the patch already supplies the libiconv include/library paths via the CPPFLAGS/LDFLAGS below), or pass a supported option so the build configuration is deterministic.

Medium severity Nested test patches are omitted by the glob

stable-patches/​PATCH_LIST.md:92

The nested patch files under stable-patches/t/ are not matched by ../stable-patches/t/*.patch; that glob only visits direct children, so t/t0083-apply-3way-zos.sh.patch, t/t0084-rerere-zos.sh.patch, and the nested t5801 patch listed above are silently omitted.

Medium severity Lock text flag is ignored

stable-patches/​lockfile.c.patch:35

LOCK_TAG_TEXT is introduced and passed by config.c, but this code ignores flags and tags every lockfile's descriptor with utf8_ccsid. That makes the new flag ineffective and can mark binary lockfiles as text; the main lockfile tagging condition should require (flags & LOCK_TAG_TEXT) (with PID-file handling kept separate).

Medium severity Test logs still use shared /tmp

tests/​run_all_tests.sh:29

The runner still creates every per-test log under /tmp. This is the same full filesystem that the new temp-directory documentation says has been eliminated, so the runner can fail before executing tests when /tmp is exhausted; store these logs under a managed directory below SCRIPT_DIR (or honor a configured temporary root).

Medium severity TAP plan counts skipped scripts

tests/​run_all_tests.sh:33

The TAP plan counts every *.sh, but the loop skips non-executable scripts. The repository already records a skipped non-executable test, so this produces a plan such as 1..17 with fewer result lines and invalid TAP output. Count only the scripts that the loop will actually run.

Medium severity Pipeline masks git apply failures

tests/​test_apply_3way_ebcdic.sh:71

Because this pipeline is not run with pipefail, its status is grep -qv's status rather than git apply's status. A failed apply that emits any non-error: line can enter the success branch and be counted as passed; test the command exit status directly and inspect output separately.

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

Medium severity Encoding assertion accepts incorrect UTF-8

tests/​test_apply_tagging.sh:61

The assertion accepts UTF-8 even though this test's .gitattributes explicitly requires ISO8859-1. A wrong UTF-8 tag would therefore be reported as a pass; require the configured encoding only.

Medium severity Test repository still uses shared /tmp

tests/​test_encoding_pull_clone.sh:18

The new test still allocates its repository under /tmp, so it remains vulnerable to the full-filesystem failure this PR claims to have fixed. Use the same repository-local temp-root convention as the other tests, or honor TMPDIR.

Medium severity Test invokes unrelated system Git

tests/​test_encoding_pull_clone.sh:28

This test invokes git directly for the entire scenario instead of selecting the repository's built binary (as the other tests do with $REPO_ROOT/git/git). It can therefore pass against an unrelated system Git and provide no coverage for these patches; resolve a GIT_BIN once and use it for every Git command.

Medium severity Patch file is created in shared /tmp

tests/​test_encoding_pull_clone.sh:126

The patch is written into the shared /tmp directory even though this PR introduces local test temp directories. This can collide with concurrent runs, select a stale 0001-*.patch, or fail when /tmp is full; keep the patch under TEST_DIR and apply that exact path.

Comment on lines +328 to +330
+#if 0 /* DISABLED - causes double conversion bug! */
+ encode_to_git(path, dst->buf, dst->len, dst, ca.working_tree_encoding, ca.attr_action, conv_flags);
+#endif
Test shows that git format-patch works correctly when files are
properly tagged before writing content (chtag -tc 1047 file).

Test 1: Improper tagging (without chtag) → garbled output (expected)
Test 2: Proper tagging (with chtag) → readable output (correct)

This demonstrates the issue is USER WORKFLOW, not a git bug.
Files must be tagged BEFORE writing content to avoid corruption.

Related: INVESTIGATION_FORMAT_PATCH_COMPLETE.md, USER_GUIDE_FILE_TAGGING.md
Copilot AI review requested due to automatic review settings September 23, 2026 10:04

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, patch correctness, runtime, and test reliability issues block approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 30 High severity · 15 Medium severity · 7 Low severity

Open (52)

And 32 more that still need to be addressed.

Previously missed (2)

In code that hasn't changed since last review

Low severity Document conversion fallback behavior instead of fail-fast semantics

README.md:131

This says strict mode stops Git on an unconvertible character, but the added conversion code reports the error and returns an unconverted fallback through several callers; t0082-zos-encoding.patch even expects checkout to continue after the error. Document the actual fallback semantics instead of promising fail-fast behavior.

Low severity Documentation references a nonexistent failure-analysis file

TEST_FIXES_SUMMARY.md:75

The referenced tests/WHY_TEST_FAILS.md file does not exist; the added file is tests/WHY_TEST_FAILED_BEFORE_FIX.md. This leaves the documented failure analysis link unusable.

+ * For files with working-tree-encoding, use read() instead to get raw bytes.
+ * See: MMAP_AUTOCONVERT_HYPOTHESIS.md for details.
+ */
+ if (would_convert_to_git(the_repository->index, s->path)) {
Comment on lines 10 to +17
+#ifdef __MVS__
+ if (!autocvtToASCII)
+ __disableautocvt(fd);
+#endif
+
+ /* On z/OS, mmap() ignores __disableautocvt() and auto-converts tagged files.
+ * For files with working-tree-encoding, use read() instead to get raw bytes.
+ * See: MMAP_AUTOCONVERT_HYPOTHESIS.md for details.
+ */
+ if (would_convert_to_git(the_repository->index, s->path)) {
+ /* Only disable autoconversion, don't change file tag */
+ __disableautocvt(fd);
Comment on lines +15 to +17
+ if (would_convert_to_git(the_repository->index, s->path)) {
+ /* Only disable autoconversion, don't change file tag */
+ __disableautocvt(fd);
rm -rf "$TEST_ROOT"

# Exit successfully (both tests behaved as expected)
exit 0
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