Initial commit for 3 way merge - #187
Open
HarithaIBM wants to merge 46 commits into
Open
Conversation
with the target encoding if the conversion fails
…ring add and clone
…rsion-2.54.0 Update git-version to 2.54.0 from 2.53.0
skip tagging the file
handled ref file tag
patch failure fix
Added explicit zoslib linking
The fail_pipe label was incorrectly placed inside the 'if (cmd->pid < 0)' block, causing the pipe error message to be printed even when commands succeeded. This happened because 'str' retained its value from earlier external command checks. The fix moves the fail_pipe label before the pid check and restores the proper cleanup and return logic (child_process_clear, errno, return -1). This fixes the spurious error message: error: cannot create pipe for git-<alias>: EDC5129I No such file or directory. That appeared for all git aliases (both shell '!' and non-shell) even though the commands executed successfully. Regression introduced in: b6fe124 (enable more descriptive error reporting) Affects: Git 2.54.0, 2.55.0 Fix verified: alias execution now silent when successful
There was a problem hiding this comment.
🟡 Changes recommended
There are multiple correctness issues that will break tests/builds (including a shell syntax error, an invalid diff header in a test, and a lockfile patch that disables PID lockfile creation on non-z/OS builds).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
stable-patches/lockfile.c.patch:37
- This hunk accidentally moves
create_lock_pid_file()inside#ifdef __MVS__, so non-z/OS builds will no longer create PID lockfiles whenlockfile_pid_enabledis on. It also unconditionally retags all lockfiles as UTF-8 on z/OS, ignoring the newLOCK_TAG_TEXTintent; only text lockfiles (e.g., .git/config) should be tagged as UTF-8.
lk->tempfile = create_tempfile_mode(lock_path.buf, mode);
- if (lk->tempfile)
+#ifdef __MVS__
+ if (lk->tempfile && fstat(lk->tempfile->fd, &st) >= 0 && S_ISREG(st.st_mode))
+ {
- Files reviewed: 49/49 changed files
- Comments generated: 3
- Review effort level: Lite
Comment on lines
+20
to
+23
| +if ! uname | grep -q "OS/390"; then | ||
| + echo "DEBUG: Not on z/OS, skipping test" >&2 | ||
| +fi | ||
| +echo "DEBUG: On z/OS, proceeding with tests" >&2 |
| +diff --git a/merged.txt b/merged.txt | ||
| +index ${BASE_OID:0:7}..${THEIRS_OID:0:7} 100644 | ||
| +--- a/merged.txt | ||
| ++++ b/merged.txt |
Comment on lines
+123
to
+125
| + size_t line = 1, col = 0; | ||
| + size_t fail_pos = src_len; | ||
| + iconv_t cd = iconv_open(to, from); |
…ive tests - Restore parallel test execution (-j$ZOPEN_NUM_JOBS) to match working Aug 24 config - Add zopen_check() function to run both Git community tests and z/OS-specific tests - Add zopen_check_results() to count results from both test suites - z/OS comprehensive tests cover UTF-8 multi-byte characters using real Git workflows - Both test suites provide complete coverage for EBCDIC and UTF-8 encodings
There was a problem hiding this comment.
🟡 Changes recommended
There are confirmed functional issues in the added patches/tests (notably incorrect test skipping off z/OS and problematic lockfile PID-lock semantics) that should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (6)
tests/test_3way_merge_encodings.sh:36
- Progress label uses "[Test 1/7]" but the script sets
TOTAL=16and later uses other denominators (e.g. "/14"); this makes the output misleading when triaging failures.
stable-patches/t0083-apply-3way-zos.patch:23 - The test does not actually skip on non-z/OS: it prints a message but continues running and even logs "proceeding with tests" unconditionally. This will cause failures on other platforms.
stable-patches/lockfile.c.patch:37 - This hunk removes the generic
if (lk->tempfile)guard and moves PID lockfile creation under#ifdef __MVS__and anfstat()/S_ISREG()check. If this patch is applied on non-z/OS builds,lk->pid_tempfilewill no longer be created (and the existingcreate_lock_pid_file(...)line becomes unconditional), changing behavior and potentially dereferencing/using state whenlk->tempfileis NULL. Even on z/OS, PID lock creation becomes dependent onfstat()success, which is unrelated to whether the lock was created.
lk->tempfile = create_tempfile_mode(lock_path.buf, mode);
- if (lk->tempfile)
+#ifdef __MVS__
+ if (lk->tempfile && fstat(lk->tempfile->fd, &st) >= 0 && S_ISREG(st.st_mode))
+ {
buildenv:194
- Avoid non-ASCII emoji in status output to prevent encoding/display issues in some environments.
echo "❌ FAILED: Comprehensive tests failed"
stable-patches/entry.c.patch:49
fcntl_retis assigned but never used, which can trigger unused-variable warnings (and potentially fail builds that treat warnings as errors).
+ int fcntl_ret = fcntl(fd, F_CONTROL_CVT, &query);
stable-patches/entry.c.patch:87
fcntl_retis assigned but never used, which can trigger unused-variable warnings (and potentially fail builds that treat warnings as errors).
+ int fcntl_ret = fcntl(fd, F_CONTROL_CVT, &query);
- Files reviewed: 49/49 changed files
- Comments generated: 9
- Review effort level: Lite
Comment on lines
+34
to
36
| + if (!strcmp(var, "core.iconvtranslit")) { | ||
| + iconv_translit = git_config_bool(var, value); | ||
| + return 0; |
Comment on lines
+9
to
+12
| +/* | ||
| + * Tag the lockfile as text (UTF-8) on z/OS. If not set, the default is binary. | ||
| + */ | ||
| +#define LOCK_TAG_TEXT 8 |
Comment on lines
+19
to
+20
| TEST_ROOT="$(mktemp -d /tmp/git_3way_test.XXXXXX)" | ||
|
|
|
|
||
| **Using environment variable (takes precedence):** | ||
| - `export GIT_ICONV_TRANSLIT=false` (Default): Git will stop with an error if a character cannot be converted. | ||
| - `export GIT_ICONV_TRANSLIT=true`: Git will use iconv's transliteration feature to substitute the character with a similar-looking one (e.g., `é` becomes `e`), and will issue a warning. |
Comment on lines
+25
to
+29
| + if (must_checkout(ce)) { | ||
| + const char *basename = strrchr(ce->name, '/'); | ||
| + basename = basename ? basename + 1 : ce->name; | ||
| + if (!strcmp(basename, GITATTRIBUTES_FILE)) | ||
| + gitattributes_updated = 1; |
| elif [ -x "$REPO_ROOT/git_working_4aug/git" ]; then | ||
| GIT_BIN="$REPO_ROOT/git_working_4aug/git" | ||
| else | ||
| GIT_BIN="$(which git)" |
- Set default JOBS=4 if ZOPEN_NUM_JOBS is not set - Replace $ZOPEN_NUM_JOBS placeholder with actual job count in make command - Prevents 'make: the -j option requires a positive integer argument' error
There was a problem hiding this comment.
🟡 Changes recommended
There are confirmed correctness issues in newly added/changed artifacts (a syntax error in the new test script, a broken/non-portable lockfile patch flow, and a z/OS test that does not actually skip off-platform).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
tests/test_3way_merge_encodings.sh:961
- The final summary banner has an unterminated string literal (
echo "========================================================================) which will cause the whole script to fail with a syntax error before printing results.
stable-patches/t0083-apply-3way-zos.patch:6 - This patch creates
t/t0083-apply-3way-zos.sh, but the repository also containsstable-patches/t/t0083-apply-3way-zos.sh.patchwhich adds the same file. Keeping both patches in the stable-patches series will make patch application order-dependent and likely fail with an "already exists" conflict.
stable-patches/t0083-apply-3way-zos.patch:23 - The test claims to skip when not on z/OS, but it currently only prints a message and then continues running. It also always prints "On z/OS" even when the platform check fails, which will produce confusing failures on non-z/OS CI.
stable-patches/lockfile.c.patch:37 - This patch accidentally moves the
if (lk->tempfile)block (including creation oflk->pid_tempfile) under#ifdef __MVS__. On non-z/OS builds that would skip creating the pid lock file entirely, breaking lockfile semantics. Also,LOCK_TAG_TEXTis defined/used by callers but not honored here.
lk->tempfile = create_tempfile_mode(lock_path.buf, mode);
- if (lk->tempfile)
+#ifdef __MVS__
+ if (lk->tempfile && fstat(lk->tempfile->fd, &st) >= 0 && S_ISREG(st.st_mode))
+ {
- Files reviewed: 49/49 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
+19
to
+24
| if (convert_to_git(rev->diffopt.repo->index, | ||
| - elem->path, result, len, &buf, global_conv_flags_eol)) { | ||
| + elem->path, result, len, &buf, global_conv_flags_eol) > 0) { | ||
| free(result); | ||
| result = strbuf_detach(&buf, &len); | ||
| result_size = len; |
Comment on lines
+36
to
+40
| - if (convert_to_git(r->index, s->path, s->data, s->size, &buf, conv_flags)) { | ||
| + if (convert_to_git(r->index, s->path, s->data, s->size, &buf, conv_flags) > 0) { | ||
| size_t size = 0; | ||
| munmap(s->data, s->size); | ||
| s->should_munmap = 0; |
- Replace complex parameter substitution with simple make command
- Use 'make -i test -j${JOBS}' directly instead of modifying ZOPEN_CHECK_OPTS
- Avoids 'FSUM7728 bad ${} modifier' error in older shells
- More portable and easier to understand
- Use : ${ZOPEN_NUM_JOBS:=4} to set default if not set
- Consistent with how other zopen projects handle ZOPEN_NUM_JOBS
- make -i test -j${ZOPEN_NUM_JOBS} directly
- Replace checkmarks and crosses with plain text - Better compatibility with different terminals and log files
There was a problem hiding this comment.
🟡 Changes recommended
It contains several confirmed correctness issues (including a shell syntax error, a non-skipping platform-specific test, missing error handling for new negative return paths, and a lockfile pid creation regression outside z/OS) that should be fixed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (6)
Previously missed (1) — in code that hasn't changed since the last review.
tests/test_3way_merge_encodings.sh:837
- This uses fixed filenames under /tmp for the hex dumps, which can collide when tests run concurrently and also leaves artifacts behind; use files under $TEST_ROOT (or mktemp) instead.
tests/test_3way_merge_encodings.sh:962
- The final summary banner has an unterminated string literal (missing closing quote), which will cause the script to exit with a syntax error before printing the summary.
stable-patches/apply.c.patch:38 - convert_to_git() can now return a negative value on encoding failure, but this call ignores the return value and still returns success, which can make apply continue after a conversion error.
+
+ convert_to_git(istate, path, buf->buf, buf->len, buf, conv_flags);
return 0;
stable-patches/utf8.c.patch:126
- bad_char_out is only assigned when an encoding failure is detected; when iconv_open() fails or no failure is found, callers may print an uninitialized byte value. Initialize *bad_char_out to 0 at function entry.
stable-patches/t0083-apply-3way-zos.patch:24 - This test does not actually skip when not running on z/OS: it prints a debug message and then continues, which will likely fail on other platforms due to missing chtag and z/OS-specific behavior.
stable-patches/lockfile.c.patch:37 - This change makes creation of lk->pid_tempfile conditional on MVS (because the create_lock_pid_file() call is now inside the MVS block). On non-z/OS builds, the pid lockfile will no longer be created, breaking LOCKFILE_PID behavior.
lk->tempfile = create_tempfile_mode(lock_path.buf, mode);
- if (lk->tempfile)
+#ifdef __MVS__
+ if (lk->tempfile && fstat(lk->tempfile->fd, &st) >= 0 && S_ISREG(st.st_mode))
+ {
- Files reviewed: 49/49 changed files
- Comments generated: 1
- Review effort level: Lite
| + TEST_SHELL_PATH = $(SHELL_PATH) | ||
| + SHELL_PATH_FOR_SCRIPTS = /bin/env bash | ||
| + PYTHON_PATH = python | ||
| + PYTHON_PATH := $(PYTHON_PATH)/python3 |
There was a problem hiding this comment.
🟡 Changes recommended
There are several correctness issues in the patch set (notably lockfile control-flow changes and uninitialized/unsafe state usage) plus a conflicting duplicate test patch that must be resolved before safe approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (8)
Previously missed (3) — in code that hasn't changed since the last review.
stable-patches/read-cache.c.patch:43
- ie_match_stat() sets the global attribute direction to GIT_ATTR_CHECKIN and never restores it. Because attribute direction is global process state, this can leak into unrelated callers and cause later attribute lookups to consult the wrong source (working tree vs index).
tests/test_3way_merge_encodings.sh:19 - If any test fails, the script exits early and leaves the temporary directory behind. Add an EXIT trap so $TEST_ROOT is always cleaned up, and honor $TMPDIR when creating the temp directory.
tests/test_3way_merge_encodings.sh:17 - The script falls back to
which git, which is not reliable/portable (and produces confusing errors if git is missing). Prefercommand -vand fail fast when git cannot be found.
stable-patches/utf8.c.patch:125
- find_first_encoding_error() does not initialize *bad_char_out when iconv_open() fails or when no failure is detected, but callers use the value in error/warning messages. This can surface uninitialized stack data in output and produce nondeterministic diagnostics.
stable-patches/lockfile.c.patch:40 - This patch removes the original
if (lk->tempfile)guard for non-z/OS builds (it is replaced by an#ifdef __MVS__-only condition). As a result, on non-MVS platformslk->pid_tempfile = create_lock_pid_file(...)becomes unconditional and can run even whencreate_tempfile_mode()failed (changing behavior and risking later errors). Also, tagging the lockfile unconditionally ignores the new LOCK_TAG_TEXT flag semantics (lockfiles should not all be tagged as UTF-8).
lk->tempfile = create_tempfile_mode(lock_path.buf, mode);
- if (lk->tempfile)
+#ifdef __MVS__
+ if (lk->tempfile && fstat(lk->tempfile->fd, &st) >= 0 && S_ISREG(st.st_mode))
+ {
+ __chgfdccsid(lk->tempfile->fd, utf8_ccsid);
+
lk->pid_tempfile = create_lock_pid_file(pid_path.buf, mode);
stable-patches/environment.c.patch:37
- README.md states that GIT_ICONV_TRANSLIT takes precedence over core.iconvtranslit, but git_default_core_config() always overwrites iconv_translit from config even when the environment variable is set (common-init.c initializes it earlier). This makes runtime behavior contradict the documented precedence.
+ if (!strcmp(var, "core.iconvtranslit")) {
+ iconv_translit = git_config_bool(var, value);
+ return 0;
+ }
stable-patches/t0083-apply-3way-zos.patch:23
- This test does not actually skip when not running on z/OS; it only prints a debug message and continues. That will make the test run (and likely fail) on non-z/OS platforms. Also, there is another patch in this PR that adds the same test file (t/t0083-apply-3way-zos.sh) with proper skip behavior, so keeping both patch files will conflict.
stable-patches/config.mak.uname.patch:14 - The OS/390 section sets
PYTHON_PATH := $(PYTHON_PATH)/python3, which expands to an invalid path if PYTHON_PATH is empty (and is also self-referential/fragile). It should point directly to the python3 interpreter, consistent with how PERL_PATH and SHELL_PATH are set.
+ PYTHON_PATH := $(PYTHON_PATH)/python3
- Files reviewed: 49/49 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+60
to
+67
| +fail_pipe: | ||
| + if (str) { | ||
| + error("cannot create %s pipe for %s: %s", | ||
| + str, cmd->args.v[0], strerror(failed_errno)); | ||
| + child_process_clear(cmd); | ||
| + errno = failed_errno; | ||
| + return -1; | ||
| + } |
- Skip t0300, t0301, t0302 (credential tests require interactive TTY) - Skip t0021, t1051, t2003, t2082, t3400, t4150 (other TTY/interactive issues) - These tests hang waiting for password prompts in non-interactive build environment - Consistent with Error 130 failures seen in Aug 31 logs
…access" This reverts commit 6986913.
There was a problem hiding this comment.
🔵 Needs a closer look
There are confirmed correctness/build issues in newly added tests and patches (e.g., a shell syntax error, missing non-z/OS test skipping, unused variables, and z/OS-specific logic bugs) that need fixing before it can be safely validated.
Review details
Suppressed comments (6)
Previously missed (1) — in code that hasn't changed since the last review.
stable-patches/utf8.c.patch:156
find_first_encoding_error()computes line/column using only ASCII LF (\n). On z/OS inputs may use the EBCDIC newline byte (0x15), which will produce incorrect line/col in error messages. Consider treatingis_ebcdic_newline()as a newline too (similar tofind_first_non_ascii()).
tests/test_3way_merge_encodings.sh:961
- The final summary echo is missing a closing quote, which makes the script a syntax error and prevents the test suite from running.
stable-patches/t0083-apply-3way-zos.patch:24 - This test does not actually skip on non-z/OS platforms (it only prints a debug line). It will continue and likely fail due to missing z/OS utilities like
chtag. Useskip_all+test_donelike other Git tests.
stable-patches/entry.c.patch:49 fcntl_retis assigned but never used. With common build flags (e.g., -Wall/-Werror), this can fail the build. If the return value is intentionally ignored, cast the call to void instead of storing it.
+ int fcntl_ret = fcntl(fd, F_CONTROL_CVT, &query);
stable-patches/entry.c.patch:87
fcntl_retis assigned but never used here as well; this can trigger -Wunused-variable and fail builds that treat warnings as errors.
+ int fcntl_ret = fcntl(fd, F_CONTROL_CVT, &query);
stable-patches/lockfile.c.patch:41
- On z/OS,
lk->pid_tempfileis now created only whenfstat(lk->tempfile->fd)succeeds and reports a regular file. That changes behavior from the original code (which created the PID lockfile wheneverlk->tempfileexists) and can break lockfile PID support in edge cases (e.g., fstat failure). Also,LOCK_TAG_TEXTis introduced but not consulted here.
lk->tempfile = create_tempfile_mode(lock_path.buf, mode);
- if (lk->tempfile)
+#ifdef __MVS__
+ if (lk->tempfile && fstat(lk->tempfile->fd, &st) >= 0 && S_ISREG(st.st_mode))
+ {
+ __chgfdccsid(lk->tempfile->fd, utf8_ccsid);
+
lk->pid_tempfile = create_lock_pid_file(pid_path.buf, mode);
+ if (lk->pid_tempfile && fstat(lk->pid_tempfile->fd, &st) >= 0 && S_ISREG(st.st_mode))
- Files reviewed: 49/49 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
There are confirmed correctness issues in added tests/patches (e.g., a syntax error in the new test script, incorrect non-z/OS skip logic, and lockfile patch behavior affecting non-z/OS) that would cause failures or unintended behavior.
Review details
Suppressed comments (10)
Previously missed (2) — in code that hasn't changed since the last review.
stable-patches/t0082-zos-encoding.patch:37
- The test name says checkout should fail due to an encoding error, but the command is chained with
&&, so a non-zero exit from checkout-index will abort the test immediately. Make the test tolerate either exit status while still asserting the error text was emitted.
tests/test_3way_merge_encodings.sh:837 - These hex-dump artifacts are written to fixed paths in /tmp, which can collide across parallel runs and may be cleaned by the system mid-run. Prefer writing under $TEST_ROOT (or use mktemp).
This issue also appears on line 933 of the same file.
tests/test_3way_merge_encodings.sh:961
- The script ends with an unterminated string literal (missing closing quote), which will cause the entire test suite to fail to run.
tests/test_3way_merge_encodings.sh:20 - Temporary test directory cleanup only happens on the happy-path; any earlier failure will leak $TEST_ROOT under /tmp. Add an EXIT trap so the directory is always removed.
tests/test_3way_merge_encodings.sh:936 - This test writes its hex dump to /tmp/test16_hex.txt, which is a shared global path and can be clobbered by parallel runs. Use a per-run path under $TEST_ROOT instead.
stable-patches/t0083-apply-3way-zos.patch:23 - The test claims it is skipping when not on z/OS, but it doesn’t actually skip; it continues running (and even prints "On z/OS" unconditionally). This will produce noisy failures on other platforms.
stable-patches/t0083-apply-3way-zos.patch:5 - This patch adds t/t0083-apply-3way-zos.sh, but stable-patches/t/t0083-apply-3way-zos.sh.patch also adds the same file with different content. Having both patches will either conflict at apply time or make the final result ambiguous—please keep only one source of truth.
stable-patches/apply.c.patch:38 - convert_to_git() can now fail (e.g., encoding errors), but its return value is ignored here. That can cause
git applyto proceed with unconverted/invalid data instead of surfacing the failure.
+
+ convert_to_git(istate, path, buf->buf, buf->len, buf, conv_flags);
return 0;
stable-patches/lockfile.c.patch:37
- This change makes pid lockfile creation z/OS-only by moving it inside
#ifdef __MVS__. That would unintentionally disable pid-based locking on all other platforms. Also, LOCK_TAG_TEXT is introduced but not used here, so lockfiles are always tagged as text on z/OS regardless of flags.
lk->tempfile = create_tempfile_mode(lock_path.buf, mode);
- if (lk->tempfile)
+#ifdef __MVS__
+ if (lk->tempfile && fstat(lk->tempfile->fd, &st) >= 0 && S_ISREG(st.st_mode))
+ {
stable-patches/config.mak.uname.patch:14
PYTHON_PATH := $(PYTHON_PATH)/python3is self-referential (and has a trailing space). If PYTHON_PATH is unset, this becomes/python3; if it is set, it appends another segment. Set PYTHON_PATH explicitly to the intended interpreter.
+ PYTHON_PATH := $(PYTHON_PATH)/python3
- Files reviewed: 49/49 changed files
- Comments generated: 0 new
- Review effort level: Lite
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.