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

From: Junio C Hamano <[email-masked]>
Date: Mon, 20 Apr 2020 15:06:57 -0700

"Han-Wen Nienhuys via GitGitGadget" <[email-masked]> 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"?

> 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 &&?

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