Re: [PATCH 02/13] Enable builds for z/OS.

[Junio C Hamano <gitster@pobox.com>]

"Haritha D via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Subject: Re: [PATCH 02/13] Enable builds for z/OS.

Documentation/CodingGuidelines and Documentation/SubmittingPatches
would help here, I think.

>  # 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.

The reason to exist for the _FOR_SCRIPTS variants is not justified
anywhere in the proposed log message.

The former should be sufficient, and our policy is to let the
builder specify exactly what binaries the build products depend on,
(instead of random $PATH interfere with the choice by using
"#!/bin/env tool" that also has to assume that everybody's "env" is
installed in "/bin").

This patch has too many #ifdefs in the primary codepaths.  Your
porting strategy may need to be rethought.  Our usual convention is
to encapsulate these platform differences as much as possible in
git-compat-util.h and platform specific files in compat/ directory.

> 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>
> +
> +   int setbinaryfd(int fd)
> +   {
> +     attrib_t attr;
> +     int rc;

Ahh, OK, I saw [03/13] first and was utterly confused by this thing.
Do not send in such a mess that introduces broken code in an early
step that you need to later say "oops that one was broken and I need
to fix it up with this patch".  "rebase -i" is your friend to clean
up your mess into a logical progression to help readers better
understand what you wrote.

I'll stop here.