Re: [PATCH v2] This PR enables a successful git build on z/OS.
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Dec 4, 2023, 21:46 UTC
- Message-ID
- <CAPig+cQ8eEU0TOoBf2KavTyf0OLhNtmOzs8+WqZy9JMXa=ydPQ@mail.gmail.com>
- In-Reply-To
- <pull.1537.v2.git.git.1701699574054.gitgitgadget@gmail.com>
On Mon, Dec 4, 2023 at 9:19 AM Haritha via GitGitGadget <gitgitgadget@gmail.com> wrote:
Show 17 quoted lines
> Rename functions like "release" and "fetch" > due to conflict in z/OS standard C libraries. > Also disables autoconversion facility on z/OS > and relies on iconv. > New files created in binary format are also > tagged as binary. > > Signed-off-by: Haritha D <harithamma.d@ibm.com> > --- > Enabling z/OS workflow for git > > z/OS is an IBM mainframe operating system, also known as OS/390. Our > team has been actively involved in porting Git to z/OS and we have made > significant modifications to facilitate this process. The patch below is > the initial configuration for z/OS. I also have few follow up changes > and I will send that after these changes are approved. Please let me > know if there are any concerns.
It's fairly unlikely that this patch will be accepted as-is. Please see brian's[1] and Junio's[2] valuable review comments in response to your v1. They contain important suggestions which will give you a better chance of landing these changes.
Generally speaking, the patch's commit message lacks sufficient detail to allow a reviewer (or future reader) to understand why the changes are being made. Moreover, this single patch seems to be addressing at least three separate issues, hence should be split into three or more patches, each standalone and tackling a single issue, and each easily digested by a reviewer. The commit message of each patch should fully explain and justify the changes made by the patch, keeping in mind that most reviewers probably aren't familiar with z/OS, thus will need extra hand-holding. More below...
[1]: https://lore.kernel.org/git/ZVKrWSv7JguKTSYw@tapette.crustytoothpaste.net/ [2]: https://lore.kernel.org/git/xmqqcywd2m9i.fsf@gitster.g/
Show 14 quoted lines
> diff --git a/builtin/archive.c b/builtin/archive.c
> @@ -14,6 +14,15 @@
> static void create_output_file(const char *output_file)
> {
> int output_fd = xopen(output_file, O_CREAT | O_WRONLY | O_TRUNC, 0666);
> +#ifdef __MVS__
> + /*
> + * Since the data is in binary format,
> + * we need to set the z/OS file tag
> + * to binary to disable autoconversion
> + */
> + if (__setfdbinary(output_fd))
> + die_errno(_("could not tag archive file '%s'"), output_file);
> +#endifAs mentioned in an earlier review, the project generally doesn't want #ifdef's littering the code and prefer that this sort of platform-specific specialization be wrapped up in its own "compat" file/function. For instance, perhaps you could create a platform-specific specialization of xopen() and then `#define xopen` to reference your specialized version. Your custom xopen() might first call the xopen() which Git defines and then perform whatever extra special work is needed for your platform. That way, you would not have to muck around either in the code which calls xopen() or in the Git-supplied xopen(). See, for example, how git-compat-util.h overriedes certain functions, such as stat(), fstat(), etc. using an #undefine/#define dance.
Show 10 quoted lines
> diff --git a/combine-diff.c b/combine-diff.c > @@ -1082,6 +1082,14 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent, > + #ifdef __MVS__ > + /* > + * Disable implicit autconversion on z/os, > + * rely on conversion from iconv > + */ > + __disableautocvt(fd); > + #endif > elem->mode = canon_mode(st.st_mode);
Similar comment. Try to find an abstraction which allows you to perform this specialization in a way which does not require #ifdef's within the main source code if possible.
Show 21 quoted lines
> diff --git a/fetch-negotiator.h b/fetch-negotiator.h
> @@ -47,7 +47,7 @@ struct fetch_negotiator {
> - void (*release)(struct fetch_negotiator *);
> + void (*release_negotiator)(struct fetch_negotiator *);> diff --git a/fetch-pack.c b/fetch-pack.c
> @@ -1232,7 +1232,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
> if (negotiator)
> - negotiator->release(negotiator);
> + negotiator->release_negotiator(negotiator);
> return ref;
> }
> diff --git a/git-compat-util.h b/git-compat-util.h
> @@ -223,7 +223,15 @@ struct strbuf;
> +#ifdef __MVS__
> + #define release stdlib_release
> + #define fetch stdlib_fetch
> +#endif
> #include <stdlib.h>
> +#ifdef __MVS__
> + #undef fetch
> + #undef release
> +#endifSo, the problem is that z/OS is polluting the C namespace or the preprocessor namespace with names "release" and "fetch"? When we've run across this problem on other platforms, we modify git-compat-util.h or some other files in compat/ to suppress the pollution introduced by the platform headers rather than "fixing" the Git source code. For instance, if "release" and "fetch" are macros on z/OS, then you may be able to simply #undef them after pulling in whichever z/OS header defines them. If they are actual system functions (rather than macros), you may be able to employ the #undef/#define dance to rename them to something else, such as "zos_release" and "zos_fetch" _before_ including the system header which declares those functions.
Show 12 quoted lines
> diff --git a/read-cache.c b/read-cache.c
> @@ -205,6 +205,14 @@ static int ce_compare_data(struct index_state *istate,
> int fd = git_open_cloexec(ce->name, O_RDONLY);
> if (fd >= 0) {
> + #ifdef __MVS__
> + /*
> + * Since the data is in binary format,
> + * we need to set the z/OS file tag to
> + * binary to disable autoconversion
> + */
> + __disableautocvt(fd);
> + #endifSame comment as above about encapsulating this in a platform-specific specialization function in compat/ rather than polluting the code with #ifdef.