Re: [PATCH v9 09/10] Add reftable library

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 20, 2020, 22:06 UTC
Message-ID: <xmqqeeshrev2.fsf@gitster.c.googlers.com>
In-Reply-To: <a30001ad1e8f94a0f3901c1694c3dd660ba1e7c0.1587417295.git.gitgitgadget@gmail.com>

"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/reftable/update.sh b/reftable/update.sh
> new file mode 100644

Shouldn't this be executable, even though the readme tells the reader to run "sh update.sh"?

Show 7 quoted lines

> index 00000000000..0fd90f89675
> --- /dev/null
> +++ b/reftable/update.sh
> @@ -0,0 +1,14 @@
> +#!/bin/sh
> +
> +set -eux

Use of -e and -u are understandable but -x is a bit unusual unless one is debugging...

> +((cd reftable-repo && git fetch origin && git checkout origin/master ) ||
> +git clone https://github.com/google/reftable reftable-repo) && \

I think you can discard most of the '\' at the end of the line, as the shell knows, when you stop a line with &&, you haven't finished talking to it yet. That would make the result slightly less noisy to follow. We don't rely on "set -e" in this project, but this is a borrowed file from our point of view---as long as we are using -e, wouldn't it make more sense to take full advantage of it and do without &&?

Show 5 quoted lines

> +cp reftable-repo/c/*.[ch] reftable/ && \
> +cp reftable-repo/c/include/*.[ch] reftable/ && \
> +cp reftable-repo/LICENSE reftable/ &&
> +git --git-dir reftable-repo/.git show --no-patch origin/master \
> +> reftable/VERSION && \

> +sed -i~ 's|if REFTABLE_IN_GITCORE|if 1 /* REFTABLE_IN_GITCORE */|' reftable/system.h

"sed -i" is probably GNUism that does not port well.

> +rm reftable/*_test.c reftable/test_framework.* reftable/compat.*

I presume that this is because we copied everything that match c/*.[ch] and is not about removing what used to exist in reftable/ directory locally? Not complaining, but checking if I am reading the intention of the code correctly.

> +git add reftable/*.[ch]

As we flattened when copying from c/include/*.[ch], this "git add" can also be flat, which makes sense. Don't you need to also add the LICENSE file in there?

Two comments on the design:

 - The list of "what to copy in" largely depends on the upstream
   reftable library; hardcoding the patterns here would make it
   harder to reorganize the files for the upstream project.  I
   wonder if it makes more sense to reserve a single path in the
   reftable-repo and hardcode the knowledge of what to copy out
   there?  That would mean the resulting update.sh here can become

	(git -C reftable-repo pull --ff-only && git clone ...) &&
	reftable-repo/copy-out-to-git-core.sh reftable/

   and the copy-out-to-git-core.sh script that comes with the
   upstream reftable library is the only one that needs to know
   where the license and C source files are in the same version as
   itself, and how to produce $1/VERSION file using the commit log
   message of the commit it comes from.

 - The above procedure (with or without the "who should know what to
   copy" change) would cope well with modified and new files in the
   reftable upstream, but does not remove a library file that used
   to exist (and copied to the git-core project) that no longer is
   needed and shipped.  It appears that there has to be some way to
   remove them.

Thanks.