Re: [PATCH 1/3] fast-export: allow dumping the refname mapping

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 19, 2020, 15:51 UTC
Message-ID: <CAPig+cRNem-S5LGX=v=1Tid64sXWBxNyWH4ffgLgF0o1yN=mtw@mail.gmail.com>
In-Reply-To: <20200619132546.GA2540774@coredump.intra.peff.net>

On Fri, Jun 19, 2020 at 9:25 AM Jeff King <peff@peff.net> wrote:

Show 18 quoted lines

> [...]
> Let's make it possible to dump the mapping separate from the output
> stream. This can be used by a bug reporter to modify their reproduction
> recipe without revealing the original names (see the example in the
> documentation).
> [...]
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> @@ -118,6 +120,32 @@ static int has_unshown_parent(struct commit *commit)
> +static void maybe_dump_anon(FILE *out, struct seen_set *seen,
> +                           const char *orig, const char *anon)
> +{
> +       if (!out)
> +               return;
> +       if (!check_and_mark_seen(seen, orig))
> +               fprintf(out, "%s %s\n", orig, anon);
> +}

Nit: For a function which has only a single caller in this patch and
one more caller added in 3/3, I wonder if the, um, convenience of this
function short-circuiting as the very first thing it does outweighs
the loss of clarity of having the callers make the check themselves.
That is, have the caller do this instead:

    if (anonymized_refnames_handle)
        dump_anon(anonymized_refnames_handle, ...);

> @@ -1213,6 +1249,9 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
> +       if (anonymized_refnames_file)
> +               anonymized_refnames_handle = xfopen(anonymized_refnames_file, "w");

For completeness, do you want to close this file at some point?

Show 11 quoted lines

> diff --git a/t/t9351-fast-export-anonymize.sh b/t/t9351-fast-export-anonymize.sh
> @@ -46,6 +46,18 @@ test_expect_success 'stream omits tag message' '
> +test_expect_success 'refname mapping can be dumped' '
> +       git fast-export --anonymize --all \
> +               --dump-anonymized-refnames=refs.out >/dev/null &&
> +       # we make no guarantees of the exact anonymized names,
> +       # so just check that we have the right number and
> +       # that a sample line looks sane.
> +       # Note that master is not anonymized, and so not included
> +       # in the mapping.
> +       test_line_count = 6 refs.out &&

This hard-coded "6" seems rather fragile, causing the test to break if other refs are ever added or removed. Perhaps just count the refs dynamically?

    git show-ref >refs &&
    nrefs=$(wc -l refs) &&
    # Note that master is not anonymized, and so not included
    # in the mapping.
    nrefs=$((nrefs - 1))
    test_line_count = $nrefs refs.out &&

and then drop the 'nrefs=$((nrefs - 1))' line and associated comment in patch 2/3 which removes the "master" special case.

> +       grep "^refs/heads/other refs/heads/" refs.out
> +'