On Tue, Apr 21, 2020 at 12:07 AM Junio C Hamano <gitster@xxxxxxxxx> wrote: > Shouldn't this be executable, even though the readme tells the > reader to run "sh update.sh"? Done. (should I post an updated version of the patch series for these minor changes, or is it OK to fold them with the next major update?) > > +set -eux > > Use of -e and -u are understandable but -x is a bit unusual unless > one is debugging... Done. > > +((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 &&? Done. > > +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. done. > > +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. correct. > > +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? Done. > 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. You are correct, but once this is working to the extent that it's accepted into git-core, I expect imports to be infrequent, and I think it's not worth the effort to automate this further. If files are renamed, the result will likely not compile, so this mistake will be easy to catch. -- Han-Wen Nienhuys - Google Munich I work 80%. Don't expect answers from me on Fridays. -- Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
