On 2023-11-13 at 10:24:04, Haritha D via GitGitGadget wrote:
> From: Haritha D <harithamma.d@xxxxxxx>
>
> This commit enables git to build on z/OS.
> It takes advantage of enahanced ASCII
> services on z/OS to auto-convert input
> files to ASCII
In general, we don't want to convert files to ASCII, since we assume
text files are UTF-8 unless otherwise specified. If you're suggesting
that your system is normally EBCDIC, changing that should be an initial
piece of setup code or something used in `xopen` on your platform so it
applies everywhere with minimal changes.
> It also adds support for
> [platform]-working-tree-encoding.
> Platform is substituted with uname_info.sysname,
> so it will only apply to the given platform.
I think this is going to need to be a separate patch and feature with
its own explanation of why it's valuable.
> Also adds support for scripts that are not in
> standard locations so that /bin/env bash
> can be specified.
> Signed-off-by: Harithamma D <harithamma.d@xxxxxxx>
> ---
> Makefile | 21 +++++++++---
> builtin.h | 3 ++
> builtin/archive.c | 6 ++++
> builtin/hash-object.c | 28 +++++++++++++++
> combine-diff.c | 4 +++
> config.c | 7 ++++
> configure.ac | 3 ++
> convert.c | 44 ++++++++++++++++++++----
> copy.c | 3 ++
> diff.c | 11 ++++++
> entry.c | 26 ++++++++++++++
> environment.c | 3 ++
> git-compat-util.h | 8 +++++
> negotiator/default.c | 4 +--
> negotiator/noop.c | 4 +--
> negotiator/skipping.c | 4 +--
> object-file.c | 80 ++++++++++++++++++++++++++++++++++++++++++-
> read-cache.c | 3 ++
> utf8.c | 11 ++++++
> 19 files changed, 255 insertions(+), 18 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 9c6a2f125f8..30aa76da4f4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -20,6 +20,8 @@ include shared.mak
> #
> # Define SHELL_PATH to a POSIX shell if your /bin/sh is broken.
> #
> +# Define SHELL_PATH_FOR_SCRIPTS to a POSIX shell if your /bin/sh is broken.
> +#
> # Define SANE_TOOL_PATH to a colon-separated list of paths to prepend
> # to PATH if your tools in /usr/bin are broken.
> #
> @@ -215,6 +217,8 @@ include shared.mak
> #
> # Define PERL_PATH to the path of your Perl binary (usually /usr/bin/perl).
> #
> +# Define PERL_PATH_FOR_SCRIPTS to a Perl binary if your /usr/bin/perl is broken.
You will probably want to explain in your commit message why these two
are required to be different from the standard values and explain here
what the relevant difference is so that users can set them
appropriately.
> diff --git a/builtin/archive.c b/builtin/archive.c
> index 90761fdfee0..53ec794356f 100644
> --- a/builtin/archive.c
> +++ b/builtin/archive.c
> @@ -14,6 +14,12 @@
> static void create_output_file(const char *output_file)
> {
> int output_fd = xopen(output_file, O_CREAT | O_WRONLY | O_TRUNC, 0666);
> +#ifdef __MVS__
> + #if (__CHARSET_LIB == 1)
> + if (setbinaryfd(output_fd))
> + die_errno(_("could not tag archive file '%s'"), output_file);
> + #endif
> +#endif
This would be better to place in `xopen` itself so that all files are
correctly configured. That would do well as its own patch, and you'd
want to explain well in the commit message what this function does, why
it's necessary, and what the consequences of not using it are, as well
as any alternatives that you've rejected.
> if (output_fd != 1) {
> if (dup2(output_fd, 1) < 0)
> die_errno(_("could not redirect output"));
> diff --git a/builtin/hash-object.c b/builtin/hash-object.c
> index 5ffec99dcea..b33b32ff977 100644
> --- a/builtin/hash-object.c
> +++ b/builtin/hash-object.c
> @@ -57,11 +57,39 @@ static void hash_fd(int fd, const char *type, const char *path, unsigned flags,
> maybe_flush_or_die(stdout, "hash to stdout");
> }
>
> +#ifdef __MVS__
> +# if (__CHARSET_LIB == 1)
> +# include <stdio.h>
> +# include <stdlib.h>
We typically don't include the standard headers here. Instead, they're
included by git-compat-util.h because on some systems they have to be
included in a certain order with certain options. If you include that
header instead at the top of the file, or one of the headers that
includes it, then typically that should do the right thing.
> + int setbinaryfd(int fd)
> + {
> + attrib_t attr;
> + int rc;
> +
> + memset(&attr, 0, sizeof(attr));
> + attr.att_filetagchg = 1;
> + attr.att_filetag.ft_ccsid = FT_BINARY;
> + attr.att_filetag.ft_txtflag = 0;
> +
> + rc = __fchattr(fd, &attr, sizeof(attr));
> + return rc;
> + }
> +# endif
> +#endif
I would think a comment explaining what this function does and why it's
necessary would be appropriate, since it's not in POSIX and isn't
typically necessary on POSIX systems.
> diff --git a/combine-diff.c b/combine-diff.c
> index f90f4424829..73445a517c7 100644
> --- a/combine-diff.c
> +++ b/combine-diff.c
> @@ -1082,6 +1082,10 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
> ssize_t done;
> int is_file, i;
>
> +#ifdef __MVS__
> + __disableautocvt(fd);
> +#endif
Again, if this can be centralized, it should be, and it should be
explained well in the commit message. I'm uncertain what it does or
what value it provides.
> diff --git a/config.c b/config.c
> index f9a1cca4e8a..37c124a37c0 100644
> --- a/config.c
> +++ b/config.c
> @@ -1521,6 +1521,13 @@ static int git_default_core_config(const char *var, const char *value,
> return 0;
> }
>
> + #ifdef __MVS__
> + if (!strcmp(var, "core.ignorefiletags")) {
> + ignore_file_tags = git_config_bool(var, value);
> + return 0;
> + }
> + #endif
This should also live in its own patch and the commit message should
explain what it does. We'd also want it to be documented in the config
options in the Documentation directory.
> diff --git a/convert.c b/convert.c
> index a8870baff36..4f14ff6f1ed 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -377,12 +377,15 @@ static int check_roundtrip(const char *enc_name)
> static const char *default_encoding = "UTF-8";
>
> static int encode_to_git(const char *path, const char *src, size_t src_len,
> - struct strbuf *buf, const char *enc, int conv_flags)
> + struct strbuf *buf, const char *enc, enum convert_crlf_action attr_action, int conv_flags)
> {
> char *dst;
> size_t dst_len;
> int die_on_error = conv_flags & CONV_WRITE_OBJECT;
>
> + if (attr_action == CRLF_BINARY) {
> + return 0;
> + }
I'm pretty sure this is a change in behaviour from what we had before.
It should live in its own patch, with an explanation in the commit
message why it's a compelling and correct change overall, and with
suitable tests.
> /*
> * No encoding is specified or there is nothing to encode.
> * Tell the caller that the content was not modified.
> @@ -403,6 +406,11 @@ static int encode_to_git(const char *path, const char *src, size_t src_len,
> return 0;
>
> trace_encoding("source", path, enc, src, src_len);
> +#ifdef __MVS__
> + // Don't convert ISO8859-1 on z/OS
> + if (strcasecmp("ISO8859-1", enc) == 0)
> + return 0;
> +#endif
This definitely needs explanation in the commit message and should
probably be its own patch, explaining why z/OS has this compelling need
to not convert ISO8859-1. Note that ISO8859-1 is not the same as "no
binary conversion", since it doesn't include many control codes.
Note that if, as it says later on, this really means "UTF-8", that's a
platform wart you'd want to paper over in a file in the compat code. In
general, the compat directory is a good place to put anything that your
platform needs specifically.
> +static const char* get_platform() {
> + struct utsname uname_info;
> +
> + if (uname(&uname_info))
> + die(_("uname() failed with error '%s' (%d)\n"),
> + strerror(errno),
> + errno);
> +
> + if (!strcmp(uname_info.sysname, "OS/390"))
> + return "zos";
> + return uname_info.sysname;
> +}
This is definitely a new feature, and I'm not sure why it's necessary or
useful. I suspect there's something about z/OS that makes it valuable,
but I don't know what it is since the commit message doesn't tell me.
I'm also not sure that these values will be correct on Windows.
I think I could go on to make similar comments about the rest of this
series. I'm not opposed to seeing z/OS changes come in, but you've
amalgamated at least a half-dozen separate patches into one and haven't
explained them very thoroughly in the commit message.
I'd generally want to look at the commit message and understand the
problem the code is trying to solve and then look at the code and think,
"Oh, yes, this seems like the obvious and logical way to solve this
problem," or at least think, "Oh, no, I think we should solve this
problem in a different way," so I can help make a thoughtful review
comment. Right now, I lack the information to have an informed opinion
and so I can't provide any helpful feedback or analysis of the patches.
When you're adding new features or fixing bugs, we'll also want tests
for those cases to help us avoid regressing that code in the future.
Even if we don't normally run the testsuite on z/OS, at least _you_ will
notice that the tests have failed and then we'll be able to address the
bugs in a timely manner.
I also noted that there were some fixup commits later on in the series
that address whitespace issues. Typically, we'd want to squash those in
to the earlier patches. Nobody expects perfection, but squashing errors
into earlier patches helps us keep the history neat and let us pretend
like you never made those errors at all. It also lets tools like git
blame and git bisect work more nicely for users.
I'd recommend a quick pass over the SubmittingPatches file, which is
also available at https://git-scm.com/docs/SubmittingPatches. The
sections on making separate commits for separate changes and describing
changes well come to mind as places to focus.
I know this may seem overwhelming and like I'm upset or disappointed;
I'm definitely not. I'm very much interested in seeing Git available
for more platforms, but right now it's too hard for me to reason about
the changes for z/OS to provide helpful feedback, so I'm hoping you can
send a fixed v2 that helps me (and everyone else) understand these
changes better so you can get a helpful review.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
Attachment:
signature.asc
Description: PGP signature
