On Sun, Jun 21, 2020 at 2:47 PM Emir Yâsin SARI <bitigchi@xxxxxx> wrote:
> Below you can find the current Haiku patches for upstream approval.
Thanks for volunteering to upstream Haiku-specific fixes. Please note
that it is unlikely that these changes will be accepted as-is, and
will require a fair bit of re-work to get them into shape for merging
into this project. If you're interested in pursuing this further, see
my comments below. Rather than submitting a single patch, when you
re-roll, you will almost certainly end up submitting a multi-patch
series.
> Currently Haiku is built with this [1] make configuration. Therefore
> it’s not possible to build the Haiku version with just ‘make’ yet.
> I need help in adapting this configuration to the current Git
> Makefile, I don’t have much experience in Makefiles. Or if it’s
> trivial, any quick-fix help is appreciated :).
These sort of 'make' tweaks are normally handled by adjusting Git's
config.mak.uname file. So, you will want to add a new section to
config.mak.uname which looks something like this:
ifeq ($(uname_S),Haiku)
PTHREAD_LIBS =
USE_LIBPCRE2 = YesPlease
NO_D_TYPE_IN_DIRENT = YesPlease
NO_MEMMEM = YesPlease
NEEDS_LIBICONV = YesPlease
GNU_ROFF = YesPlease
PERL_PATH = /bin/perl
NO_PYTHON = YesPlease
NO_TCLTK = YesPlease
OBJECT_CREATION_USES_RENAMES = YesPlease
NO_CROSS_DIRECTORY_HARDLINKS = YesPlease
NO_INSTALL_HARDLINKS = YesPlease
HAVE_DEV_TTY = YesPlease
DEFAULT_EDITOR = nano
DEFAULT_HELP_FORMAT = web
BASIC_LDFLAGS = -lnetwork -lbsd
endif
> In the meantime, please find the patchset below for review.
Right here is where your email commentary ends and the patch proper
begins. To allow "git am" to automatically pluck out the actual patch
portion out of the email, you should insert a "--- >8 ---" (scissors)
line here.
> From 2ac0c135d7c12a2581fe70ed1c8ffb4809950b55 Mon Sep 17 00:00:00 2001
> From: Emir Sarı <bitigchi@xxxxxx>
> Date: Sun, 21 Jun 2020 21:02:23 +0300
> Subject: [PATCH] haiku: add Haiku support
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
Except for From: and Subject:, all the other header lines are just
noise and of no interest to the project when submitting patches, thus
should not be included. (Better yet, if possible, use "git send-email"
or GitGitGadget" to send patches, as those tools will ensure the
correct headers are present.)
> This commit is a collection of minor patches previously applied as a
> Haikuports patchset at github.com/haikuports/haikuports repository for
> Git Haiku port.
>
> Patchset history:
>
> From 56acac1a903dcbdd37c3b57fc168ad20179596b1 Mon Sep 17 00:00:00 2001
> From: Ingo Weinhold <ingo_weinhold@xxxxxx>
> Date: Tue, 13 Aug 2013 08:07:25 +0200
> Subject: git-web--browse.sh: use "open" on Haiku
A few comments...
Drop the "From <hexstring> <date>" header; it is not of particular
interest in this context.
It probably would be a good idea to indent the entire header block
here as a precaution against it confusing tools which automate patch
application or which otherwise perform some sort of automated patch
processing.
I'm almost tempted to suggest submitting each of these changes as
separate patches in order to retain attribution of each of the
authors. On the other hand, to get these changes accepted into this
project, you almost certainly will end up rewriting them so
significantly that they may no longer resemble the changes from the
original authors. If that's the case, it might make more sense for you
to reference the original authors via "Based on a patch by..." in each
of your new patches.
> Signed-off-by: Emir Sarı <bitigchi@xxxxxx>
> ---
> diff --git a/builtin/config.c b/builtin/config.c
> @@ -645,6 +645,14 @@ int cmd_config(int argc, const char **argv, const char *prefix)
> char *user_config = expand_user_path("~/.gitconfig", 0);
> char *xdg_config = xdg_config_home("config");
>
> + #ifdef __HAIKU__
> + if (!xdg_config) {
> + given_config_source.file = user_config;
> + } else {
> + given_config_source.file = xdg_config;
> + if (user_config) free(user_config);
> + }
> + #else
This project frowns heavily on introducing platform-specific
conditional compilation like this. As such, this `#ifdef __HAIKU__`
will prevent this change from being accepted as-is. (It's true that
some such code exists already, but we want to avoid adding more.)
The commit[1] from which you derived this change doesn't explain why
the change was made, but rather only what it changes: specifically, it
forces use of XDG even if the XDG directory is missing rather than
falling back to ~/.gitconfig. This particular behavior change isn't
something that need be specific to Haiku -- it might make sense on any
platform -- so it doesn't make sense to bundle it inside an `#ifdef
__HAIKU__` conditional. The only way this sort of behavior change is
likely to be accepted into the project is if it re-worked to be
applicable to any platform _and_ if it can be properly justified to
convince people that it makes sense. Moreover, it probably would
require some sort of transition plan or mechanism.
Additionally, the change looks outright broken because it neglects to
do:
given_config_source.scope = CONFIG_SCOPE_GLOBAL;
like the code in the #else arm.
[1]: https://github.com/haikuports/haikuports/blob/02adc9b2c68a0ad51bc4db5699b46adc15c24c5c/dev-vcs/git/patches/git-2.27.0.patchset#L132
> diff --git a/config.c b/config.c
> @@ -2761,6 +2762,12 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
> if (!config_filename)
> config_filename = filename_buf = git_pathdup("config");
>
> + config_dir = xstrdup(config_filename);
> + * find_last_dir_sep(config_dir) = '\0';
> + if (access(config_dir, F_OK) != 0)
> + mkdir(config_dir, 0755);
> + free(config_dir);
This change comes from [2], which explains the change as:
Ensure config-directory exists before using it.
But what it fails to say is under what circumstances the directory
wouldn't exist. When would this code kick in? This is the sort of
thing which needs to be properly explained in the commit message for a
change like this to be accepted. Such a change would almost certainly
also be accompanied by a new test, perhaps added to t/t1300-config.sh.
There are other problems with this change, as well. For instance, why
is this "fix" made only to git_config_set_multivar_in_file_gently(),
but not to git_config_copy_or_rename_section_in_file() which almost
certainly suffers the same problem?
Moreover, this code:
* find_last_dir_sep(config_dir) = '\0';
is just outright dangerous considering that find_last_dir_sep() is
documented as possibly returning NULL. (A minor additional point is
that it violates project style convention by having a space after the
'*'.)
[2]: https://github.com/haikuports/haikuports/blob/02adc9b2c68a0ad51bc4db5699b46adc15c24c5c/dev-vcs/git/patches/git-2.27.0.patchset#L73
> diff --git a/credential-cache.c b/credential-cache.c
> @@ -87,7 +87,11 @@ static char *get_socket_path(void)
> {
> + #ifdef __HAIKU__
> + old_dir = xdg_config_home("credential-cache");
> + #else
> old_dir = expand_user_path("~/.git-credential-cache", 0);
> + #endif
This change comes from [3] which justifies it as:
Move credential cache to the config directory.
Do not clutter the home dir.
The same comments from above apply...
* don't insert platform-specific conditionals
* there is nothing Haiku-specific about this change, thus protecting
it inside a `#ifdef __HAIKU__` makes no sense
* if this sort of behavior change is indeed desirable, then it should
be implemented in a generic way with some sort of transition plan
from old to new behavior
[3]: https://github.com/haikuports/haikuports/blob/02adc9b2c68a0ad51bc4db5699b46adc15c24c5c/dev-vcs/git/patches/git-2.27.0.patchset#L108
> diff --git a/git-web--browse.sh b/git-web--browse.sh
> @@ -133,6 +133,11 @@ if test -z "$browser" ; then
> + # BEINCLUDES indicates Haiku
> + if test -f $BEINCLUDES; then
> + browser_candidates="open $browser_candidates"
> + fi
Haiku support was originally added to git-web--browse.sh by [4] which
triggered this conditional off existence of file
/boot/system/haiku_loader, however, that ended up being unreliable, so
[5] changed it to look for environment variable BEINCLUDES.
However, it feels somewhat fragile to depend upon an environment
variable which might or might not exist in the development environment
(even if it is present by default). A more robust alternative would be
to check against something not likely to be overridden by the user.
For instance:
if test "$(uname)" = Haiku; then
(Also note that placing 'then' on the same line as 'if' goes against
style in this project, however, this particular portion of
git-web--browse.sh already breaks style in that regard, so it's hard
to fault it here.)
[4]: https://github.com/haikuports/haikuports/blob/02adc9b2c68a0ad51bc4db5699b46adc15c24c5c/dev-vcs/git/patches/git-2.27.0.patchset#L1
[5]: https://github.com/haikuports/haikuports/blob/02adc9b2c68a0ad51bc4db5699b46adc15c24c5c/dev-vcs/git/patches/git-2.27.0.patchset#L175
> diff --git a/path.c b/path.c
> @@ -12,6 +12,10 @@
> +#ifdef __HAIKU__
> +#include <FindDirectory.h>
> +#include <StorageDefs.h>
> +#endif
> @@ -1500,16 +1504,22 @@ int looks_like_command_line_option(const char *str)
> char *xdg_config_home(const char *filename)
> {
> + #ifdef __HAIKU__
> + char settingsPath[B_PATH_NAME_LENGTH];
> + assert(filename);
> + if (find_directory(B_USER_SETTINGS_DIRECTORY, -1, true, settingsPath,
> + sizeof(settingsPath)) == B_OK)
> + return mkpathdup("%s/git/%s", settingsPath, filename);
> + #else
As noted earlier, don't introduce platform-specific compilation
sections like this. The way this is normally handled is to place
platform-specific implementation in a file in the compat/ directory
(perhaps "compat/haiku.c", for instance), and then add a #define to
git-compat-util.h to cause the platform-specific implementation to be
invoked in place of the generic implementation. See compat/mingw.[ch]
and git-compat-util.h for examples of how this is done.
To actually make that work, you might need to do a bit of preparation
to make it possible to swap in a different implementation of
xdg_config_home(), though.
A bit of commentary on the code itself: In this project, the variable
would be named "settings_path", not "settingsPath". That's something
to keep in mind when writing code for generic parts of the system. If
you're placing this code in a platform-specific compat/haiku.c file,
on the other hand, you may be able to get away with breaking style and
going with "settingsPath" (though if there is no strong need to break
style, then why bother?).
> const char *home, *config_home;
> -
> assert(filename);
> config_home = getenv("XDG_CONFIG_HOME");
> if (config_home && *config_home)
> return mkpathdup("%s/git/%s", config_home, filename);
> -
> home = getenv("HOME");
Don't make arbitrary changes like this unrelated to the purpose of a
patch. Doing so gives reviewers extra work with no benefit.
