* [PATCH 01/10] t9351: derive anonymized tree checks from original repo
2020-06-23 15:24 ` [alternative 0/10] fast-export: allow seeding the anonymized mapping Jeff King
@ 2020-06-23 15:24 ` Jeff King
2020-06-23 15:24 ` [PATCH 02/10] fast-export: use xmemdupz() for anonymizing oids Jeff King
` (10 subsequent siblings)
11 siblings, 0 replies; 64+ messages in thread
From: Jeff King @ 2020-06-23 15:24 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Junio C Hamano, Johannes Schindelin
Our tests of the anonymized repo just hard-code the expected set of
objects in the root and subdirectory trees. This makes them brittle to
the test setup changing (e.g., adding new paths that need tested).
Let's look at the original repo to compute our expected set of objects.
Note that this isn't completely perfect (e.g., we still rely on there
being only one tree in the root), but it does simplify later patches.
Signed-off-by: Jeff King <peff@peff.net>
---
t/t9351-fast-export-anonymize.sh | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/t/t9351-fast-export-anonymize.sh b/t/t9351-fast-export-anonymize.sh
index 897dc50907..e772cf9930 100755
--- a/t/t9351-fast-export-anonymize.sh
+++ b/t/t9351-fast-export-anonymize.sh
@@ -71,22 +71,18 @@ test_expect_success 'repo has original shape and timestamps' '
test_expect_success 'root tree has original shape' '
# the output entries are not necessarily in the same
- # order, but we know at least that we will have one tree
- # and one blob, so just check the sorted order
- cat >expect <<-\EOF &&
- blob
- tree
- EOF
+ # order, but we should at least have the same set of
+ # object types.
+ git -C .. ls-tree HEAD >orig-root &&
+ cut -d" " -f2 <orig-root | sort >expect &&
git ls-tree $other_branch >root &&
cut -d" " -f2 <root | sort >actual &&
test_cmp expect actual
'
test_expect_success 'paths in subdir ended up in one tree' '
- cat >expect <<-\EOF &&
- blob
- blob
- EOF
+ git -C .. ls-tree other:subdir >orig-subdir &&
+ cut -d" " -f2 <orig-subdir | sort >expect &&
tree=$(grep tree root | cut -f2) &&
git ls-tree $other_branch:$tree >tree &&
cut -d" " -f2 <tree >actual &&
--
2.27.0.517.gbc32778fa3
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH 02/10] fast-export: use xmemdupz() for anonymizing oids
2020-06-23 15:24 ` [alternative 0/10] fast-export: allow seeding the anonymized mapping Jeff King
2020-06-23 15:24 ` [PATCH 01/10] t9351: derive anonymized tree checks from original repo Jeff King
@ 2020-06-23 15:24 ` Jeff King
2020-06-23 15:24 ` [PATCH 03/10] fast-export: store anonymized oids as hex strings Jeff King
` (9 subsequent siblings)
11 siblings, 0 replies; 64+ messages in thread
From: Jeff King @ 2020-06-23 15:24 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Junio C Hamano, Johannes Schindelin
Our anonymize_mem() function is careful to take a ptr/len pair to allow
storing binary tokens like object ids, as well as partial strings (e.g.,
just "foo" of "foo/bar"). But it duplicates the hash key using
xstrdup()! That means that:
- for a partial string, we'd store all bytes up to the NUL, even
though we'd never look at anything past "len". This didn't produce
wrong behavior, but was wasteful.
- for a binary oid that doesn't contain a zero byte, we'd copy garbage
bytes off the end of the array (though as long as nothing complained
about reading uninitialized bytes, further reads would be limited by
"len", and we'd produce the correct results)
- for a binary oid that does contain a zero byte, we'd copy _fewer_
bytes than intended into the hashmap struct. When we later try to
look up a value, we'd access uninitialized memory and potentially
falsely claim that a particular oid is not present.
The most common reason to store an oid is an anonymized gitlink, but our
test case doesn't have any gitlinks at all. So let's add one whose oid
contains a NUL and is present at two different paths. ASan catches the
memory error, but even without it we can detect the bug because the oid
is not anonymized the same way for both paths.
And of course the fix is to copy the correct number of bytes. We don't
technically need the appended NUL from xmemdupz(), but it doesn't hurt
as an extra protection against anybody treating it like a string (plus a
future patch will push us more in that direction).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/fast-export.c | 2 +-
t/t9351-fast-export-anonymize.sh | 15 +++++++++++++++
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 85868162ee..289395a131 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -162,7 +162,7 @@ static const void *anonymize_mem(struct hashmap *map,
if (!ret) {
ret = xmalloc(sizeof(*ret));
hashmap_entry_init(&ret->hash, key.hash.hash);
- ret->orig = xstrdup(orig);
+ ret->orig = xmemdupz(orig, *len);
ret->orig_len = *len;
ret->anon = generate(orig, len);
ret->anon_len = *len;
diff --git a/t/t9351-fast-export-anonymize.sh b/t/t9351-fast-export-anonymize.sh
index e772cf9930..dc5d75cd19 100755
--- a/t/t9351-fast-export-anonymize.sh
+++ b/t/t9351-fast-export-anonymize.sh
@@ -10,6 +10,10 @@ test_expect_success 'setup simple repo' '
mkdir subdir &&
test_commit subdir/bar &&
test_commit subdir/xyzzy &&
+ fake_commit=$(echo $ZERO_OID | sed s/0/a/) &&
+ git update-index --add --cacheinfo 160000,$fake_commit,link1 &&
+ git update-index --add --cacheinfo 160000,$fake_commit,link2 &&
+ git commit -m "add gitlink" &&
git tag -m "annotated tag" mytag
'
@@ -26,6 +30,12 @@ test_expect_success 'stream omits path names' '
! grep xyzzy stream
'
+test_expect_success 'stream omits gitlink oids' '
+ # avoid relying on the whole oid to remain hash-agnostic; this is
+ # plenty to be unique within our test case
+ ! grep a000000000000000000 stream
+'
+
test_expect_success 'stream allows master as refname' '
grep master stream
'
@@ -89,6 +99,11 @@ test_expect_success 'paths in subdir ended up in one tree' '
test_cmp expect actual
'
+test_expect_success 'identical gitlinks got identical oid' '
+ awk "/commit/ { print \$3 }" <root | sort -u >commits &&
+ test_line_count = 1 commits
+'
+
test_expect_success 'tag points to branch tip' '
git rev-parse $other_branch >expect &&
git for-each-ref --format="%(*objectname)" | grep . >actual &&
--
2.27.0.517.gbc32778fa3
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH 03/10] fast-export: store anonymized oids as hex strings
2020-06-23 15:24 ` [alternative 0/10] fast-export: allow seeding the anonymized mapping Jeff King
2020-06-23 15:24 ` [PATCH 01/10] t9351: derive anonymized tree checks from original repo Jeff King
2020-06-23 15:24 ` [PATCH 02/10] fast-export: use xmemdupz() for anonymizing oids Jeff King
@ 2020-06-23 15:24 ` Jeff King
2020-06-24 11:43 ` SZEDER Gábor
2020-06-23 15:24 ` [PATCH 04/10] fast-export: tighten anonymize_mem() interface to handle only strings Jeff King
` (8 subsequent siblings)
11 siblings, 1 reply; 64+ messages in thread
From: Jeff King @ 2020-06-23 15:24 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Junio C Hamano, Johannes Schindelin
When fast-export stores anonymized oids, it does so as binary strings.
And while the anonymous mapping storage is binary-clean (at least as of
the previous commit), this will become awkward when we start exposing
more of it to the user. In particular, if we allow a method for
retaining token "foo", then users may want to specify a hex oid as such
a token.
Let's just switch to storing the hex strings. The difference in memory
usage is negligible (especially considering how infrequently we'd
generally store an oid compared to, say, path components).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/fast-export.c | 28 ++++++++++++++++------------
1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 289395a131..4a3a4c933e 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -387,16 +387,19 @@ static void *generate_fake_oid(const void *old, size_t *len)
{
static uint32_t counter = 1; /* avoid null oid */
const unsigned hashsz = the_hash_algo->rawsz;
- unsigned char *out = xcalloc(hashsz, 1);
- put_be32(out + hashsz - 4, counter++);
- return out;
+ struct object_id oid;
+ char *hex = xmallocz(GIT_MAX_HEXSZ);
+
+ oidclr(&oid);
+ put_be32(oid.hash + hashsz - 4, counter++);
+ return oid_to_hex_r(hex, &oid);
}
-static const struct object_id *anonymize_oid(const struct object_id *oid)
+static const char *anonymize_oid(const char *oid_hex)
{
static struct hashmap objs;
- size_t len = the_hash_algo->rawsz;
- return anonymize_mem(&objs, generate_fake_oid, oid, &len);
+ size_t len = strlen(oid_hex);
+ return anonymize_mem(&objs, generate_fake_oid, oid_hex, &len);
}
static void show_filemodify(struct diff_queue_struct *q,
@@ -455,9 +458,9 @@ static void show_filemodify(struct diff_queue_struct *q,
*/
if (no_data || S_ISGITLINK(spec->mode))
printf("M %06o %s ", spec->mode,
- oid_to_hex(anonymize ?
- anonymize_oid(&spec->oid) :
- &spec->oid));
+ anonymize ?
+ anonymize_oid(oid_to_hex(&spec->oid)) :
+ oid_to_hex(&spec->oid));
else {
struct object *object = lookup_object(the_repository,
&spec->oid);
@@ -712,9 +715,10 @@ static void handle_commit(struct commit *commit, struct rev_info *rev,
if (mark)
printf(":%d\n", mark);
else
- printf("%s\n", oid_to_hex(anonymize ?
- anonymize_oid(&obj->oid) :
- &obj->oid));
+ printf("%s\n",
+ anonymize ?
+ anonymize_oid(oid_to_hex(&obj->oid)) :
+ oid_to_hex(&obj->oid));
i++;
}
--
2.27.0.517.gbc32778fa3
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [PATCH 03/10] fast-export: store anonymized oids as hex strings
2020-06-23 15:24 ` [PATCH 03/10] fast-export: store anonymized oids as hex strings Jeff King
@ 2020-06-24 11:43 ` SZEDER Gábor
2020-06-24 15:54 ` Jeff King
0 siblings, 1 reply; 64+ messages in thread
From: SZEDER Gábor @ 2020-06-24 11:43 UTC (permalink / raw)
To: Jeff King; +Cc: git, Eric Sunshine, Junio C Hamano, Johannes Schindelin
On Tue, Jun 23, 2020 at 11:24:51AM -0400, Jeff King wrote:
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index 289395a131..4a3a4c933e 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -387,16 +387,19 @@ static void *generate_fake_oid(const void *old, size_t *len)
> {
> static uint32_t counter = 1; /* avoid null oid */
> const unsigned hashsz = the_hash_algo->rawsz;
> - unsigned char *out = xcalloc(hashsz, 1);
> - put_be32(out + hashsz - 4, counter++);
> - return out;
> + struct object_id oid;
> + char *hex = xmallocz(GIT_MAX_HEXSZ);
> +
> + oidclr(&oid);
> + put_be32(oid.hash + hashsz - 4, counter++);
> + return oid_to_hex_r(hex, &oid);
> }
GCC 4.8 complains about this change in our CI job:
$ make CC=gcc-4.8 builtin/fast-export.o
CC builtin/fast-export.o
builtin/fast-export.c: In function ‘generate_fake_oid’:
builtin/fast-export.c:394:2: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
put_be32(oid.hash + hashsz - 4, counter++);
^
cc1: all warnings being treated as errors
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [PATCH 03/10] fast-export: store anonymized oids as hex strings
2020-06-24 11:43 ` SZEDER Gábor
@ 2020-06-24 15:54 ` Jeff King
2020-06-25 15:49 ` Jeff King
0 siblings, 1 reply; 64+ messages in thread
From: Jeff King @ 2020-06-24 15:54 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git, Eric Sunshine, Junio C Hamano, Johannes Schindelin
On Wed, Jun 24, 2020 at 01:43:13PM +0200, SZEDER Gábor wrote:
> > static uint32_t counter = 1; /* avoid null oid */
> > const unsigned hashsz = the_hash_algo->rawsz;
> > - unsigned char *out = xcalloc(hashsz, 1);
> > - put_be32(out + hashsz - 4, counter++);
> > - return out;
> > + struct object_id oid;
> > + char *hex = xmallocz(GIT_MAX_HEXSZ);
> > +
> > + oidclr(&oid);
> > + put_be32(oid.hash + hashsz - 4, counter++);
> > + return oid_to_hex_r(hex, &oid);
> > }
>
> GCC 4.8 complains about this change in our CI job:
>
> $ make CC=gcc-4.8 builtin/fast-export.o
> CC builtin/fast-export.o
> builtin/fast-export.c: In function ‘generate_fake_oid’:
> builtin/fast-export.c:394:2: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
> put_be32(oid.hash + hashsz - 4, counter++);
> ^
> cc1: all warnings being treated as errors
Interesting. The only change on this line is swapping out the local
"unsigned char *" for an object_id, which also stores an "unsigned
char" (though as an array). And while put_be32() takes a void pointer,
it's inlined and treats it the argument an "unsigned char *" internally.
So I'm not sure I see that any type punning is going on at all.
I'll see if I can appease the compiler, though.
-Peff
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH 03/10] fast-export: store anonymized oids as hex strings
2020-06-24 15:54 ` Jeff King
@ 2020-06-25 15:49 ` Jeff King
2020-06-25 20:45 ` SZEDER Gábor
0 siblings, 1 reply; 64+ messages in thread
From: Jeff King @ 2020-06-25 15:49 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git, Eric Sunshine, Junio C Hamano, Johannes Schindelin
On Wed, Jun 24, 2020 at 11:54:20AM -0400, Jeff King wrote:
> > GCC 4.8 complains about this change in our CI job:
> >
> > $ make CC=gcc-4.8 builtin/fast-export.o
> > CC builtin/fast-export.o
> > builtin/fast-export.c: In function ‘generate_fake_oid’:
> > builtin/fast-export.c:394:2: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
> > put_be32(oid.hash + hashsz - 4, counter++);
> > ^
> > cc1: all warnings being treated as errors
>
> Interesting. The only change on this line is swapping out the local
> "unsigned char *" for an object_id, which also stores an "unsigned
> char" (though as an array). And while put_be32() takes a void pointer,
> it's inlined and treats it the argument an "unsigned char *" internally.
> So I'm not sure I see that any type punning is going on at all.
>
> I'll see if I can appease the compiler, though.
Hmm. Switching to a local array makes the warning go away:
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 4a3a4c93..eae2ec5 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -387,12 +387,12 @@ static void *generate_fake_oid(const void *old, size_t *len)
{
static uint32_t counter = 1; /* avoid null oid */
const unsigned hashsz = the_hash_algo->rawsz;
- struct object_id oid;
+ unsigned char out[GIT_MAX_RAWSZ];
char *hex = xmallocz(GIT_MAX_HEXSZ);
- oidclr(&oid);
- put_be32(oid.hash + hashsz - 4, counter++);
- return oid_to_hex_r(hex, &oid);
+ hashclr(out);
+ put_be32(out + hashsz - 4, counter++);
+ return hash_to_hex_algop_r(hex, out, the_hash_algo);
}
static const char *anonymize_oid(const char *oid_hex)
at the cost of some uglier use of the_hash_algo and RAWSZ. But
curiously, even with the array, if I adjust the write only slightly to
write at the begining instead of the end (we don't really care where our
counter appears in the hash, since the point is to anonymize):
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index eae2ec5..1f52247 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -386,12 +386,11 @@ static void print_path(const char *path)
static void *generate_fake_oid(const void *old, size_t *len)
{
static uint32_t counter = 1; /* avoid null oid */
- const unsigned hashsz = the_hash_algo->rawsz;
unsigned char out[GIT_MAX_RAWSZ];
char *hex = xmallocz(GIT_MAX_HEXSZ);
hashclr(out);
- put_be32(out + hashsz - 4, counter++);
+ put_be32(out, counter++);
return hash_to_hex_algop_r(hex, out, the_hash_algo);
}
...then the warning comes back. I tried that because I was wondering
whether the same thing might make the warning go away on the object_id
version, but it doesn't.
So this really seems like a pointless false positive from the compiler,
and it's a rather old compiler (the warning no longer triggers in gcc 6
and up). Is it worth caring about? Ubuntu Trusty is out of standard
support but not fully EOL'd until 2022. Debian jessie has gcc 4.9
which triggers the warning, but will hit EOL in 5 days. If it were an
actual breakage I'd be more concerned, but keeping such an old compiler
-Werror clean doesn't seem that important.
I'd note also that none of the Actions CI jobs run with this compiler
version. If we _do_ want to care about it, it might be worth covering it
there.
-Peff
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [PATCH 03/10] fast-export: store anonymized oids as hex strings
2020-06-25 15:49 ` Jeff King
@ 2020-06-25 20:45 ` SZEDER Gábor
2020-06-25 21:15 ` Jeff King
0 siblings, 1 reply; 64+ messages in thread
From: SZEDER Gábor @ 2020-06-25 20:45 UTC (permalink / raw)
To: Jeff King; +Cc: git, Eric Sunshine, Junio C Hamano, Johannes Schindelin
On Thu, Jun 25, 2020 at 11:49:48AM -0400, Jeff King wrote:
> On Wed, Jun 24, 2020 at 11:54:20AM -0400, Jeff King wrote:
>
> > > GCC 4.8 complains about this change in our CI job:
> > >
> > > $ make CC=gcc-4.8 builtin/fast-export.o
> > > CC builtin/fast-export.o
> > > builtin/fast-export.c: In function ‘generate_fake_oid’:
> > > builtin/fast-export.c:394:2: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
> > > put_be32(oid.hash + hashsz - 4, counter++);
> > > ^
> > > cc1: all warnings being treated as errors
> So this really seems like a pointless false positive from the compiler,
> and it's a rather old compiler (the warning no longer triggers in gcc 6
> and up). Is it worth caring about? Ubuntu Trusty is out of standard
> support but not fully EOL'd until 2022. Debian jessie has gcc 4.9
> which triggers the warning, but will hit EOL in 5 days. If it were an
> actual breakage I'd be more concerned, but keeping such an old compiler
> -Werror clean doesn't seem that important.
>
> I'd note also that none of the Actions CI jobs run with this compiler
> version. If we _do_ want to care about it, it might be worth covering it
> there.
C99 style 'for' loop initial declarations are still frowned upon in
Git's codebase, and as far as we know it GCC 4.8 is the the most
recent compiler that can reasonably detect it; see fb9d7431cf
(travis-ci: build with GCC 4.8 as well, 2019-07-18).
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH 03/10] fast-export: store anonymized oids as hex strings
2020-06-25 20:45 ` SZEDER Gábor
@ 2020-06-25 21:15 ` Jeff King
2020-06-29 13:17 ` Johannes Schindelin
0 siblings, 1 reply; 64+ messages in thread
From: Jeff King @ 2020-06-25 21:15 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git, Eric Sunshine, Junio C Hamano, Johannes Schindelin
On Thu, Jun 25, 2020 at 10:45:32PM +0200, SZEDER Gábor wrote:
> > So this really seems like a pointless false positive from the compiler,
> > and it's a rather old compiler (the warning no longer triggers in gcc 6
> > and up). Is it worth caring about? Ubuntu Trusty is out of standard
> > support but not fully EOL'd until 2022. Debian jessie has gcc 4.9
> > which triggers the warning, but will hit EOL in 5 days. If it were an
> > actual breakage I'd be more concerned, but keeping such an old compiler
> > -Werror clean doesn't seem that important.
> >
> > I'd note also that none of the Actions CI jobs run with this compiler
> > version. If we _do_ want to care about it, it might be worth covering it
> > there.
>
> C99 style 'for' loop initial declarations are still frowned upon in
> Git's codebase, and as far as we know it GCC 4.8 is the the most
> recent compiler that can reasonably detect it; see fb9d7431cf
> (travis-ci: build with GCC 4.8 as well, 2019-07-18).
TBH, that does not seem like that compelling a reason to me to keep it
around. If no compiler is complaining of C99 for-loop declarations, then
maybe we should consider loosening our style. Or are we trying to be
kind of some unknown set of platform-specific compilers that we can't
feasibly hit in our CI?
I also note that fb9d7431cf mentions that -std=c90 doesn't work, because
there are other spots where we violate the standard (looks like "inline"
is a big one). But gcc with -std=gnu89 seems to compile just fine for
me, and does notice for-loop declarations. That's obviously totally
unportable, but it would be sufficient for a CI test.
-Peff
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH 03/10] fast-export: store anonymized oids as hex strings
2020-06-25 21:15 ` Jeff King
@ 2020-06-29 13:17 ` Johannes Schindelin
2020-06-30 19:35 ` Jeff King
0 siblings, 1 reply; 64+ messages in thread
From: Johannes Schindelin @ 2020-06-29 13:17 UTC (permalink / raw)
To: Jeff King; +Cc: SZEDER Gábor, git, Eric Sunshine, Junio C Hamano
[-- Attachment #1: Type: text/plain, Size: 1899 bytes --]
Hi Peff & Gábor,
On Thu, 25 Jun 2020, Jeff King wrote:
> On Thu, Jun 25, 2020 at 10:45:32PM +0200, SZEDER Gábor wrote:
>
> > > So this really seems like a pointless false positive from the compiler,
> > > and it's a rather old compiler (the warning no longer triggers in gcc 6
> > > and up). Is it worth caring about? Ubuntu Trusty is out of standard
> > > support but not fully EOL'd until 2022. Debian jessie has gcc 4.9
> > > which triggers the warning, but will hit EOL in 5 days. If it were an
> > > actual breakage I'd be more concerned, but keeping such an old compiler
> > > -Werror clean doesn't seem that important.
> > >
> > > I'd note also that none of the Actions CI jobs run with this compiler
> > > version. If we _do_ want to care about it, it might be worth covering it
> > > there.
> >
> > C99 style 'for' loop initial declarations are still frowned upon in
> > Git's codebase, and as far as we know it GCC 4.8 is the the most
> > recent compiler that can reasonably detect it; see fb9d7431cf
> > (travis-ci: build with GCC 4.8 as well, 2019-07-18).
>
> TBH, that does not seem like that compelling a reason to me to keep it
> around. If no compiler is complaining of C99 for-loop declarations, then
> maybe we should consider loosening our style. Or are we trying to be
> kind of some unknown set of platform-specific compilers that we can't
> feasibly hit in our CI?
FWIW _iff_ we decide to loosen our style, I would like to propose doing it
in one place first, and keep it that way for two or three major versions.
Traditionally, people stuck on platforms such as IRIX or HP/UX with
proprietary C compilers (and remember: I once was one of those people)
often lack the luxury of upgrading frequently. And if it turns out that we
want to revert the style-loosening, it is much easier to do it in one
place than in many.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH 03/10] fast-export: store anonymized oids as hex strings
2020-06-29 13:17 ` Johannes Schindelin
@ 2020-06-30 19:35 ` Jeff King
0 siblings, 0 replies; 64+ messages in thread
From: Jeff King @ 2020-06-30 19:35 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: SZEDER Gábor, git, Eric Sunshine, Junio C Hamano
On Mon, Jun 29, 2020 at 03:17:00PM +0200, Johannes Schindelin wrote:
> > TBH, that does not seem like that compelling a reason to me to keep it
> > around. If no compiler is complaining of C99 for-loop declarations, then
> > maybe we should consider loosening our style. Or are we trying to be
> > kind of some unknown set of platform-specific compilers that we can't
> > feasibly hit in our CI?
>
> FWIW _iff_ we decide to loosen our style, I would like to propose doing it
> in one place first, and keep it that way for two or three major versions.
>
> Traditionally, people stuck on platforms such as IRIX or HP/UX with
> proprietary C compilers (and remember: I once was one of those people)
> often lack the luxury of upgrading frequently. And if it turns out that we
> want to revert the style-loosening, it is much easier to do it in one
> place than in many.
Yeah, I definitely agree that would be the way to do it. I admit I don't
even _really_ care that much about allowing the feature. More that it
might not be worth trying to crack down on it so aggressively, and
just letting it get picked up by review (or if it slips through, letting
the poor souls stuck on HP/UX complain).
(And I say "worth" because now the use of gcc 4.8 as the checking tool
has demonstrably cost a bunch of human time, so it comes with a cost).
-Peff
^ permalink raw reply [flat|nested] 64+ messages in thread
* [PATCH 04/10] fast-export: tighten anonymize_mem() interface to handle only strings
2020-06-23 15:24 ` [alternative 0/10] fast-export: allow seeding the anonymized mapping Jeff King
` (2 preceding siblings ...)
2020-06-23 15:24 ` [PATCH 03/10] fast-export: store anonymized oids as hex strings Jeff King
@ 2020-06-23 15:24 ` Jeff King
2020-06-23 15:24 ` [PATCH 05/10] fast-export: stop storing lengths in anonymized hashmaps Jeff King
` (7 subsequent siblings)
11 siblings, 0 replies; 64+ messages in thread
From: Jeff King @ 2020-06-23 15:24 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Junio C Hamano, Johannes Schindelin
While the anonymize_mem() interface _can_ store arbitrary byte
sequences, none of the callers uses this feature (as of the previous
commit). We'd like to keep it that way, as we'll be exposing the
string-like nature of the anonymization routines to the user. So let's
tighten up the interface a bit:
- don't treat "len" as an out-parameter from anonymize_mem(); this
ensures callers treat the pointer result as a NUL-terminated string
- likewise, don't treat "len" as an out-parameter from generator
functions
- swap out "void *" for "char *" as appropriate to signal that we
don't handle arbitrary memory
- rename the function to anonymize_str()
This will also open up some optimization opportunities in a future
patch.
Note that we can't drop the "len" parameter entirely. Some callers do
pass in partial strings (e.g., "foo/bar", len=3) to avoid copying, and
we need to handle those still.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/fast-export.c | 53 +++++++++++++++++++++----------------------
1 file changed, 26 insertions(+), 27 deletions(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 4a3a4c933e..d8ea067630 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -145,31 +145,30 @@ static int anonymized_entry_cmp(const void *unused_cmp_data,
* the same anonymized string with another. The actual generation
* is farmed out to the generate function.
*/
-static const void *anonymize_mem(struct hashmap *map,
- void *(*generate)(const void *, size_t *),
- const void *orig, size_t *len)
+static const char *anonymize_str(struct hashmap *map,
+ char *(*generate)(const char *, size_t),
+ const char *orig, size_t len)
{
struct anonymized_entry key, *ret;
if (!map->cmpfn)
hashmap_init(map, anonymized_entry_cmp, NULL, 0);
- hashmap_entry_init(&key.hash, memhash(orig, *len));
+ hashmap_entry_init(&key.hash, memhash(orig, len));
key.orig = orig;
- key.orig_len = *len;
+ key.orig_len = len;
ret = hashmap_get_entry(map, &key, hash, NULL);
if (!ret) {
ret = xmalloc(sizeof(*ret));
hashmap_entry_init(&ret->hash, key.hash.hash);
- ret->orig = xmemdupz(orig, *len);
- ret->orig_len = *len;
+ ret->orig = xmemdupz(orig, len);
+ ret->orig_len = len;
ret->anon = generate(orig, len);
- ret->anon_len = *len;
+ ret->anon_len = strlen(ret->anon);
hashmap_put(map, &ret->hash);
}
- *len = ret->anon_len;
return ret->anon;
}
@@ -181,13 +180,13 @@ static const void *anonymize_mem(struct hashmap *map,
*/
static void anonymize_path(struct strbuf *out, const char *path,
struct hashmap *map,
- void *(*generate)(const void *, size_t *))
+ char *(*generate)(const char *, size_t))
{
while (*path) {
const char *end_of_component = strchrnul(path, '/');
size_t len = end_of_component - path;
- const char *c = anonymize_mem(map, generate, path, &len);
- strbuf_add(out, c, len);
+ const char *c = anonymize_str(map, generate, path, len);
+ strbuf_addstr(out, c);
path = end_of_component;
if (*path)
strbuf_addch(out, *path++);
@@ -361,12 +360,12 @@ static void print_path_1(const char *path)
printf("%s", path);
}
-static void *anonymize_path_component(const void *path, size_t *len)
+static char *anonymize_path_component(const char *path, size_t len)
{
static int counter;
struct strbuf out = STRBUF_INIT;
strbuf_addf(&out, "path%d", counter++);
- return strbuf_detach(&out, len);
+ return strbuf_detach(&out, NULL);
}
static void print_path(const char *path)
@@ -383,7 +382,7 @@ static void print_path(const char *path)
}
}
-static void *generate_fake_oid(const void *old, size_t *len)
+static char *generate_fake_oid(const char *old, size_t len)
{
static uint32_t counter = 1; /* avoid null oid */
const unsigned hashsz = the_hash_algo->rawsz;
@@ -399,7 +398,7 @@ static const char *anonymize_oid(const char *oid_hex)
{
static struct hashmap objs;
size_t len = strlen(oid_hex);
- return anonymize_mem(&objs, generate_fake_oid, oid_hex, &len);
+ return anonymize_str(&objs, generate_fake_oid, oid_hex, len);
}
static void show_filemodify(struct diff_queue_struct *q,
@@ -496,12 +495,12 @@ static const char *find_encoding(const char *begin, const char *end)
return bol;
}
-static void *anonymize_ref_component(const void *old, size_t *len)
+static char *anonymize_ref_component(const char *old, size_t len)
{
static int counter;
struct strbuf out = STRBUF_INIT;
strbuf_addf(&out, "ref%d", counter++);
- return strbuf_detach(&out, len);
+ return strbuf_detach(&out, NULL);
}
static const char *anonymize_refname(const char *refname)
@@ -550,13 +549,13 @@ static char *anonymize_commit_message(const char *old)
}
static struct hashmap idents;
-static void *anonymize_ident(const void *old, size_t *len)
+static char *anonymize_ident(const char *old, size_t len)
{
static int counter;
struct strbuf out = STRBUF_INIT;
strbuf_addf(&out, "User %d <user%d@example.com>", counter, counter);
counter++;
- return strbuf_detach(&out, len);
+ return strbuf_detach(&out, NULL);
}
/*
@@ -591,9 +590,9 @@ static void anonymize_ident_line(const char **beg, const char **end)
size_t len;
len = split.mail_end - split.name_begin;
- ident = anonymize_mem(&idents, anonymize_ident,
- split.name_begin, &len);
- strbuf_add(out, ident, len);
+ ident = anonymize_str(&idents, anonymize_ident,
+ split.name_begin, len);
+ strbuf_addstr(out, ident);
strbuf_addch(out, ' ');
strbuf_add(out, split.date_begin, split.tz_end - split.date_begin);
} else {
@@ -733,12 +732,12 @@ static void handle_commit(struct commit *commit, struct rev_info *rev,
show_progress();
}
-static void *anonymize_tag(const void *old, size_t *len)
+static char *anonymize_tag(const char *old, size_t len)
{
static int counter;
struct strbuf out = STRBUF_INIT;
strbuf_addf(&out, "tag message %d", counter++);
- return strbuf_detach(&out, len);
+ return strbuf_detach(&out, NULL);
}
static void handle_tail(struct object_array *commits, struct rev_info *revs,
@@ -808,8 +807,8 @@ static void handle_tag(const char *name, struct tag *tag)
name = anonymize_refname(name);
if (message) {
static struct hashmap tags;
- message = anonymize_mem(&tags, anonymize_tag,
- message, &message_size);
+ message = anonymize_str(&tags, anonymize_tag,
+ message, message_size);
}
}
--
2.27.0.517.gbc32778fa3
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH 05/10] fast-export: stop storing lengths in anonymized hashmaps
2020-06-23 15:24 ` [alternative 0/10] fast-export: allow seeding the anonymized mapping Jeff King
` (3 preceding siblings ...)
2020-06-23 15:24 ` [PATCH 04/10] fast-export: tighten anonymize_mem() interface to handle only strings Jeff King
@ 2020-06-23 15:24 ` Jeff King
2020-06-23 15:24 ` [PATCH 06/10] fast-export: use a flex array to store anonymized entries Jeff King
` (6 subsequent siblings)
11 siblings, 0 replies; 64+ messages in thread
From: Jeff King @ 2020-06-23 15:24 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Junio C Hamano, Johannes Schindelin
Now that the anonymize_str() interface is restricted to NUL-terminated
strings, there's no need for us to keep track of the length of each
entry in the hashmap. This simplifies the code and saves a bit of
memory.
Note that we do still need to compare the stored results to partial
strings passed in by the callers. We can do that by using hashmap's
keydata feature to get the ptr/len pair into the comparison function,
and then using strncmp().
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/fast-export.c | 28 ++++++++++++++++++----------
1 file changed, 18 insertions(+), 10 deletions(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index d8ea067630..5df2ada47d 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -121,23 +121,32 @@ static int has_unshown_parent(struct commit *commit)
struct anonymized_entry {
struct hashmap_entry hash;
const char *orig;
- size_t orig_len;
const char *anon;
- size_t anon_len;
+};
+
+struct anonymized_entry_key {
+ struct hashmap_entry hash;
+ const char *orig;
+ size_t orig_len;
};
static int anonymized_entry_cmp(const void *unused_cmp_data,
const struct hashmap_entry *eptr,
const struct hashmap_entry *entry_or_key,
- const void *unused_keydata)
+ const void *keydata)
{
const struct anonymized_entry *a, *b;
a = container_of(eptr, const struct anonymized_entry, hash);
- b = container_of(entry_or_key, const struct anonymized_entry, hash);
+ if (keydata) {
+ const struct anonymized_entry_key *key = keydata;
+ int equal = !strncmp(a->orig, key->orig, key->orig_len) &&
+ !a->orig[key->orig_len];
+ return !equal;
+ }
- return a->orig_len != b->orig_len ||
- memcmp(a->orig, b->orig, a->orig_len);
+ b = container_of(entry_or_key, const struct anonymized_entry, hash);
+ return strcmp(a->orig, b->orig);
}
/*
@@ -149,23 +158,22 @@ static const char *anonymize_str(struct hashmap *map,
char *(*generate)(const char *, size_t),
const char *orig, size_t len)
{
- struct anonymized_entry key, *ret;
+ struct anonymized_entry_key key;
+ struct anonymized_entry *ret;
if (!map->cmpfn)
hashmap_init(map, anonymized_entry_cmp, NULL, 0);
hashmap_entry_init(&key.hash, memhash(orig, len));
key.orig = orig;
key.orig_len = len;
- ret = hashmap_get_entry(map, &key, hash, NULL);
+ ret = hashmap_get_entry(map, &key, hash, &key);
if (!ret) {
ret = xmalloc(sizeof(*ret));
hashmap_entry_init(&ret->hash, key.hash.hash);
ret->orig = xmemdupz(orig, len);
- ret->orig_len = len;
ret->anon = generate(orig, len);
- ret->anon_len = strlen(ret->anon);
hashmap_put(map, &ret->hash);
}
--
2.27.0.517.gbc32778fa3
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH 06/10] fast-export: use a flex array to store anonymized entries
2020-06-23 15:24 ` [alternative 0/10] fast-export: allow seeding the anonymized mapping Jeff King
` (4 preceding siblings ...)
2020-06-23 15:24 ` [PATCH 05/10] fast-export: stop storing lengths in anonymized hashmaps Jeff King
@ 2020-06-23 15:24 ` Jeff King
2020-06-23 15:25 ` [PATCH 07/10] fast-export: move global "idents" anonymize hashmap into function Jeff King
` (5 subsequent siblings)
11 siblings, 0 replies; 64+ messages in thread
From: Jeff King @ 2020-06-23 15:24 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Junio C Hamano, Johannes Schindelin
Now that we're using a separate keydata struct for hash lookups, we have
more flexibility in how we allocate anonymized_entry structs. Let's push
the "orig" key into a flex member within the struct. That should save us
a few bytes of memory per entry (a pointer plus any malloc overhead),
and may make lookups a little faster (since it's one less pointer to
chase in the comparison function).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/fast-export.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 5df2ada47d..99d4a2b404 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -120,8 +120,8 @@ static int has_unshown_parent(struct commit *commit)
struct anonymized_entry {
struct hashmap_entry hash;
- const char *orig;
const char *anon;
+ const char orig[FLEX_ARRAY];
};
struct anonymized_entry_key {
@@ -170,9 +170,8 @@ static const char *anonymize_str(struct hashmap *map,
ret = hashmap_get_entry(map, &key, hash, &key);
if (!ret) {
- ret = xmalloc(sizeof(*ret));
+ FLEX_ALLOC_MEM(ret, orig, orig, len);
hashmap_entry_init(&ret->hash, key.hash.hash);
- ret->orig = xmemdupz(orig, len);
ret->anon = generate(orig, len);
hashmap_put(map, &ret->hash);
}
--
2.27.0.517.gbc32778fa3
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH 07/10] fast-export: move global "idents" anonymize hashmap into function
2020-06-23 15:24 ` [alternative 0/10] fast-export: allow seeding the anonymized mapping Jeff King
` (5 preceding siblings ...)
2020-06-23 15:24 ` [PATCH 06/10] fast-export: use a flex array to store anonymized entries Jeff King
@ 2020-06-23 15:25 ` Jeff King
2020-06-23 15:25 ` [PATCH 08/10] fast-export: add a "data" callback parameter to anonymize_str() Jeff King
` (4 subsequent siblings)
11 siblings, 0 replies; 64+ messages in thread
From: Jeff King @ 2020-06-23 15:25 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Junio C Hamano, Johannes Schindelin
All of the other anonymization functions keep their static mappings
inside the function to avoid polluting the global namespace. Let's do
the same for "idents", as nobody needs it outside of
anonymize_ident_line().
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/fast-export.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 99d4a2b404..16a1563e49 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -555,7 +555,6 @@ static char *anonymize_commit_message(const char *old)
return xstrfmt("subject %d\n\nbody\n", counter++);
}
-static struct hashmap idents;
static char *anonymize_ident(const char *old, size_t len)
{
static int counter;
@@ -572,6 +571,7 @@ static char *anonymize_ident(const char *old, size_t len)
*/
static void anonymize_ident_line(const char **beg, const char **end)
{
+ static struct hashmap idents;
static struct strbuf buffers[] = { STRBUF_INIT, STRBUF_INIT };
static unsigned which_buffer;
--
2.27.0.517.gbc32778fa3
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH 08/10] fast-export: add a "data" callback parameter to anonymize_str()
2020-06-23 15:24 ` [alternative 0/10] fast-export: allow seeding the anonymized mapping Jeff King
` (6 preceding siblings ...)
2020-06-23 15:25 ` [PATCH 07/10] fast-export: move global "idents" anonymize hashmap into function Jeff King
@ 2020-06-23 15:25 ` Jeff King
2020-06-24 19:58 ` Junio C Hamano
2020-06-23 15:25 ` [PATCH 09/10] fast-export: allow seeding the anonymized mapping Jeff King
` (3 subsequent siblings)
11 siblings, 1 reply; 64+ messages in thread
From: Jeff King @ 2020-06-23 15:25 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Junio C Hamano, Johannes Schindelin
The anonymize_str() function takes a generator callback, but there's no
way to pass extra context to it. Let's add the usual "void *data"
parameter to the generator interface and pass it along.
This is mildly annoying for existing callers, all of which pass NULL,
but is necessary to avoid extra globals in some cases we'll add in a
subsequent patch.
While we're touching each of these callbacks, we can further observe
that none of them use the existing orig/len parameters at all. This
makes sense, since the point is for their output to have no discernable
basis in the original (my original version had some notion that we might
use a one-way function to obfuscate the names, but it was never
implemented). So let's drop those extra parameters. If a caller really
wants to do something with them, it can pass a struct through the new
data parameter.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/fast-export.c | 27 ++++++++++++++-------------
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 16a1563e49..1cbca5b4b4 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -155,8 +155,9 @@ static int anonymized_entry_cmp(const void *unused_cmp_data,
* is farmed out to the generate function.
*/
static const char *anonymize_str(struct hashmap *map,
- char *(*generate)(const char *, size_t),
- const char *orig, size_t len)
+ char *(*generate)(void *),
+ const char *orig, size_t len,
+ void *data)
{
struct anonymized_entry_key key;
struct anonymized_entry *ret;
@@ -172,7 +173,7 @@ static const char *anonymize_str(struct hashmap *map,
if (!ret) {
FLEX_ALLOC_MEM(ret, orig, orig, len);
hashmap_entry_init(&ret->hash, key.hash.hash);
- ret->anon = generate(orig, len);
+ ret->anon = generate(data);
hashmap_put(map, &ret->hash);
}
@@ -187,12 +188,12 @@ static const char *anonymize_str(struct hashmap *map,
*/
static void anonymize_path(struct strbuf *out, const char *path,
struct hashmap *map,
- char *(*generate)(const char *, size_t))
+ char *(*generate)(void *))
{
while (*path) {
const char *end_of_component = strchrnul(path, '/');
size_t len = end_of_component - path;
- const char *c = anonymize_str(map, generate, path, len);
+ const char *c = anonymize_str(map, generate, path, len, NULL);
strbuf_addstr(out, c);
path = end_of_component;
if (*path)
@@ -367,7 +368,7 @@ static void print_path_1(const char *path)
printf("%s", path);
}
-static char *anonymize_path_component(const char *path, size_t len)
+static char *anonymize_path_component(void *data)
{
static int counter;
struct strbuf out = STRBUF_INIT;
@@ -389,7 +390,7 @@ static void print_path(const char *path)
}
}
-static char *generate_fake_oid(const char *old, size_t len)
+static char *generate_fake_oid(void *data)
{
static uint32_t counter = 1; /* avoid null oid */
const unsigned hashsz = the_hash_algo->rawsz;
@@ -405,7 +406,7 @@ static const char *anonymize_oid(const char *oid_hex)
{
static struct hashmap objs;
size_t len = strlen(oid_hex);
- return anonymize_str(&objs, generate_fake_oid, oid_hex, len);
+ return anonymize_str(&objs, generate_fake_oid, oid_hex, len, NULL);
}
static void show_filemodify(struct diff_queue_struct *q,
@@ -502,7 +503,7 @@ static const char *find_encoding(const char *begin, const char *end)
return bol;
}
-static char *anonymize_ref_component(const char *old, size_t len)
+static char *anonymize_ref_component(void *data)
{
static int counter;
struct strbuf out = STRBUF_INIT;
@@ -555,7 +556,7 @@ static char *anonymize_commit_message(const char *old)
return xstrfmt("subject %d\n\nbody\n", counter++);
}
-static char *anonymize_ident(const char *old, size_t len)
+static char *anonymize_ident(void *data)
{
static int counter;
struct strbuf out = STRBUF_INIT;
@@ -598,7 +599,7 @@ static void anonymize_ident_line(const char **beg, const char **end)
len = split.mail_end - split.name_begin;
ident = anonymize_str(&idents, anonymize_ident,
- split.name_begin, len);
+ split.name_begin, len, NULL);
strbuf_addstr(out, ident);
strbuf_addch(out, ' ');
strbuf_add(out, split.date_begin, split.tz_end - split.date_begin);
@@ -739,7 +740,7 @@ static void handle_commit(struct commit *commit, struct rev_info *rev,
show_progress();
}
-static char *anonymize_tag(const char *old, size_t len)
+static char *anonymize_tag(void *data)
{
static int counter;
struct strbuf out = STRBUF_INIT;
@@ -815,7 +816,7 @@ static void handle_tag(const char *name, struct tag *tag)
if (message) {
static struct hashmap tags;
message = anonymize_str(&tags, anonymize_tag,
- message, message_size);
+ message, message_size, NULL);
}
}
--
2.27.0.517.gbc32778fa3
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [PATCH 08/10] fast-export: add a "data" callback parameter to anonymize_str()
2020-06-23 15:25 ` [PATCH 08/10] fast-export: add a "data" callback parameter to anonymize_str() Jeff King
@ 2020-06-24 19:58 ` Junio C Hamano
0 siblings, 0 replies; 64+ messages in thread
From: Junio C Hamano @ 2020-06-24 19:58 UTC (permalink / raw)
To: Jeff King; +Cc: git, Eric Sunshine, Johannes Schindelin
Jeff King <peff@peff.net> writes:
> The anonymize_str() function takes a generator callback, but there's no
> way to pass extra context to it. Let's add the usual "void *data"
> parameter to the generator interface and pass it along.
>
> This is mildly annoying for existing callers, all of which pass NULL,
> but is necessary to avoid extra globals in some cases we'll add in a
> subsequent patch.
> While we're touching each of these callbacks, we can further observe
> that none of them use the existing orig/len parameters at all. This
> makes sense, since the point is for their output to have no discernable
> basis in the original (my original version had some notion that we might
> use a one-way function to obfuscate the names, but it was never
> implemented). So let's drop those extra parameters. If a caller really
> wants to do something with them, it can pass a struct through the new
> data parameter.
I guess it was giving a perfect proof that the anonymization is
good---you cannot leak the info in data you did not even look at
;-).
And we must keep passing the <orig,len> pair to the anonymize API,
of course, because that would be used as a look-up key for the
customized/seeded mapping, which makes sense. But of course it is
not necessary to pass them to the lower-level "generate" callbacks.
^ permalink raw reply [flat|nested] 64+ messages in thread
* [PATCH 09/10] fast-export: allow seeding the anonymized mapping
2020-06-23 15:24 ` [alternative 0/10] fast-export: allow seeding the anonymized mapping Jeff King
` (7 preceding siblings ...)
2020-06-23 15:25 ` [PATCH 08/10] fast-export: add a "data" callback parameter to anonymize_str() Jeff King
@ 2020-06-23 15:25 ` Jeff King
2020-06-23 17:16 ` Eric Sunshine
2020-06-23 18:11 ` Eric Sunshine
2020-06-23 15:25 ` [PATCH 10/10] fast-export: anonymize "master" refname Jeff King
` (2 subsequent siblings)
11 siblings, 2 replies; 64+ messages in thread
From: Jeff King @ 2020-06-23 15:25 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Junio C Hamano, Johannes Schindelin
After you anonymize a repository, it can be hard to find which commits
correspond between the original and the result, and thus hard to
reproduce commands that triggered bugs in the original.
Let's make it possible to seed the anonymization map. This lets users
either:
- mark names to be retained as-is, if they don't consider them secret
(in which case their original commands would just work)
- map names to new values, which lets them adapt the reproduction
recipe to the new names without revealing the originals
The implementation is fairly straight-forward. We already store each
anonymized token in a hashmap (so that the same token appearing twice is
converted to the same result). We can just introduce a new "seed"
hashmap which is consulted first.
This does make a few more promises to the user about how we'll anonymize
things (e.g., token-splitting pathnames). But it's unlikely that we'd
want to change those rules, even if the actual anonymization of a single
token changes. And it makes things much easier for the user, who can
unblind only a directory name without having to specify each path within
it.
One alternative to this approach would be to anonymize as we see fit,
and then dump the whole refname and pathname mappings to a file. This
does work, but it's a bit awkward to use (you have to manually dig the
items you care about out of the mapping).
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/git-fast-export.txt | 24 ++++++++++++++++
builtin/fast-export.c | 47 ++++++++++++++++++++++++++++++-
t/t9351-fast-export-anonymize.sh | 11 +++++++-
3 files changed, 80 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
index e8950de3ba..2d7b62e835 100644
--- a/Documentation/git-fast-export.txt
+++ b/Documentation/git-fast-export.txt
@@ -119,6 +119,11 @@ by keeping the marks the same across runs.
the shape of the history and stored tree. See the section on
`ANONYMIZING` below.
+--seed-anonymized=<from>[:<to>]::
+ Convert token `<from>` to `<to>` in the anonymized output. If
+ `<to>` is omitted, map `<from>` to itself (i.e., do not
+ anonymize it). See the section on `ANONYMIZING` below.
+
--reference-excluded-parents::
By default, running a command such as `git fast-export
master~5..master` will not include the commit master{tilde}5
@@ -238,6 +243,25 @@ collapse "User 0", "User 1", etc into "User X"). This produces a much
smaller output, and it is usually easy to quickly confirm that there is
no private data in the stream.
+Reproducing some bugs may require referencing particular commits or
+paths, which becomes challenging after refnames and paths have been
+anonymized. You can ask for a particular token to be left as-is or
+mapped to a new value. For example, if you have a bug which reproduces
+with `git rev-list mybranch -- foo.c`, you can run:
+
+---------------------------------------------------
+$ git fast-export --anonymize --all \
+ --seed-anonymized=foo.c:secret.c \
+ --seed-anonymized=mybranch \
+ >stream
+---------------------------------------------------
+
+After importing the stream, you can then run `git rev-list mybranch --
+secret.c` in the anonymized repository.
+
+Note that paths and refnames are split into tokens at slash boundaries.
+The command above would anonymize `subdir/foo.c` as something like
+`path123/secret.c`.
LIMITATIONS
-----------
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 1cbca5b4b4..ef82497bbf 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -45,6 +45,7 @@ static struct string_list extra_refs = STRING_LIST_INIT_NODUP;
static struct string_list tag_refs = STRING_LIST_INIT_NODUP;
static struct refspec refspecs = REFSPEC_INIT_FETCH;
static int anonymize;
+static struct hashmap anonymized_seeds;
static struct revision_sources revision_sources;
static int parse_opt_signed_tag_mode(const struct option *opt,
@@ -168,8 +169,18 @@ static const char *anonymize_str(struct hashmap *map,
hashmap_entry_init(&key.hash, memhash(orig, len));
key.orig = orig;
key.orig_len = len;
- ret = hashmap_get_entry(map, &key, hash, &key);
+ /* First check if it's a token the user configured manually... */
+ if (anonymized_seeds.cmpfn)
+ ret = hashmap_get_entry(&anonymized_seeds, &key, hash, &key);
+ else
+ ret = NULL;
+
+ /* ...otherwise check if we've already seen it in this context... */
+ if (!ret)
+ ret = hashmap_get_entry(map, &key, hash, &key);
+
+ /* ...and finally generate a new mapping if necessary */
if (!ret) {
FLEX_ALLOC_MEM(ret, orig, orig, len);
hashmap_entry_init(&ret->hash, key.hash.hash);
@@ -1147,6 +1158,37 @@ static void handle_deletes(void)
}
}
+static char *anonymize_seed(void *data)
+{
+ return xstrdup(data);
+}
+
+static int parse_opt_seed_anonymized(const struct option *opt,
+ const char *arg, int unset)
+{
+ struct hashmap *map = opt->value;
+ const char *delim, *value;
+ size_t keylen;
+
+ BUG_ON_OPT_NEG(unset);
+
+ delim = strchr(arg, ':');
+ if (delim) {
+ keylen = delim - arg;
+ value = delim + 1;
+ } else {
+ keylen = strlen(arg);
+ value = arg;
+ }
+
+ if (!keylen || !*value)
+ return error(_("--seed-anonymized token cannot be empty"));
+
+ anonymize_str(map, anonymize_seed, arg, keylen, (void *)value);
+
+ return 0;
+}
+
int cmd_fast_export(int argc, const char **argv, const char *prefix)
{
struct rev_info revs;
@@ -1188,6 +1230,9 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
OPT_STRING_LIST(0, "refspec", &refspecs_list, N_("refspec"),
N_("Apply refspec to exported refs")),
OPT_BOOL(0, "anonymize", &anonymize, N_("anonymize output")),
+ OPT_CALLBACK_F(0, "seed-anonymized", &anonymized_seeds, N_("from:to"),
+ N_("convert <from> to <to> in anonymized output"),
+ PARSE_OPT_NONEG, parse_opt_seed_anonymized),
OPT_BOOL(0, "reference-excluded-parents",
&reference_excluded_commits, N_("Reference parents which are not in fast-export stream by object id")),
OPT_BOOL(0, "show-original-ids", &show_original_ids,
diff --git a/t/t9351-fast-export-anonymize.sh b/t/t9351-fast-export-anonymize.sh
index dc5d75cd19..d84eec9bab 100755
--- a/t/t9351-fast-export-anonymize.sh
+++ b/t/t9351-fast-export-anonymize.sh
@@ -6,6 +6,7 @@ test_description='basic tests for fast-export --anonymize'
test_expect_success 'setup simple repo' '
test_commit base &&
test_commit foo &&
+ test_commit retain-me &&
git checkout -b other HEAD^ &&
mkdir subdir &&
test_commit subdir/bar &&
@@ -18,7 +19,10 @@ test_expect_success 'setup simple repo' '
'
test_expect_success 'export anonymized stream' '
- git fast-export --anonymize --all >stream
+ git fast-export --anonymize --all \
+ --seed-anonymized=retain-me \
+ --seed-anonymized=xyzzy:custom-name \
+ >stream
'
# this also covers commit messages
@@ -30,6 +34,11 @@ test_expect_success 'stream omits path names' '
! grep xyzzy stream
'
+test_expect_success 'stream contains user-specified names' '
+ grep retain-me stream &&
+ grep custom-name stream
+'
+
test_expect_success 'stream omits gitlink oids' '
# avoid relying on the whole oid to remain hash-agnostic; this is
# plenty to be unique within our test case
--
2.27.0.517.gbc32778fa3
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [PATCH 09/10] fast-export: allow seeding the anonymized mapping
2020-06-23 15:25 ` [PATCH 09/10] fast-export: allow seeding the anonymized mapping Jeff King
@ 2020-06-23 17:16 ` Eric Sunshine
2020-06-23 18:30 ` Jeff King
2020-06-23 18:11 ` Eric Sunshine
1 sibling, 1 reply; 64+ messages in thread
From: Eric Sunshine @ 2020-06-23 17:16 UTC (permalink / raw)
To: Jeff King; +Cc: Git List, Junio C Hamano, Johannes Schindelin
On Tue, Jun 23, 2020 at 11:25 AM Jeff King <peff@peff.net> wrote:
> diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
> @@ -238,6 +243,25 @@ collapse "User 0", "User 1", etc into "User X"). This produces a much
> +[...] For example, if you have a bug which reproduces
> +with `git rev-list mybranch -- foo.c`, you can run:
> +
> +---------------------------------------------------
> +$ git fast-export --anonymize --all \
> + --seed-anonymized=foo.c:secret.c \
> + --seed-anonymized=mybranch \
> + >stream
> +---------------------------------------------------
> +
> +After importing the stream, you can then run `git rev-list mybranch --
> +secret.c` in the anonymized repository.
I understand that your intention here is to demonstrate both forms of
--seed-anonymized, but I'm slightly concerned that people may
interpret this example as meaning that you are not allowed to
anonymize the refname when anonymizing a pathname. It might be less
ambiguous to avoid the "short form" in the example; people who have
read the description of --seed-anonymized will know that the short
form can be used without having to see it in an example.
> +Note that paths and refnames are split into tokens at slash boundaries.
> +The command above would anonymize `subdir/foo.c` as something like
> +`path123/secret.c`.
Confusing. This seems to be saying that anonymizing filenames in
subdirectories is pointless because you can't know how the leading
directory names will be anonymized. That leaves the reader wondering
how to deal with the situation. Does it require using
--seed-anonymized for each path component leading up to the filename?
Or can --seed-anonymized take an full pathname (leading directory
components and filename) in one shot?
> @@ -168,8 +169,18 @@ static const char *anonymize_str(struct hashmap *map,
> - ret = hashmap_get_entry(map, &key, hash, &key);
>
> + /* First check if it's a token the user configured manually... */
> + if (anonymized_seeds.cmpfn)
> + ret = hashmap_get_entry(&anonymized_seeds, &key, hash, &key);
> + else
> + ret = NULL;
> +
> + /* ...otherwise check if we've already seen it in this context... */
> + if (!ret)
> + ret = hashmap_get_entry(map, &key, hash, &key);
> +
> + /* ...and finally generate a new mapping if necessary */
I was a bit surprised to see that --seed-anonymized values are stored
in a separate hash map rather than simply being used to (literally)
seed the existing anonymization hash map. I guess there's a good
technical reason for doing it this way, such as the normal
anonymization hash map not yet being in existence at the time the
--seed-anonymized option is processed? (I haven't checked because I'm
too lazy, so it may not be worth spending time answering me.)
> @@ -1188,6 +1230,9 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
> OPT_BOOL(0, "anonymize", &anonymize, N_("anonymize output")),
> + OPT_CALLBACK_F(0, "seed-anonymized", &anonymized_seeds, N_("from:to"),
> + N_("convert <from> to <to> in anonymized output"),
> + PARSE_OPT_NONEG, parse_opt_seed_anonymized),
Would it be worthwhile to add a check somewhere after the
parse_options() invocation and complain if --seed-anonymized was used
without --anonymize? (Or should --seed-anonymized perhaps imply
--anonymize?)
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [PATCH 09/10] fast-export: allow seeding the anonymized mapping
2020-06-23 17:16 ` Eric Sunshine
@ 2020-06-23 18:30 ` Jeff King
2020-06-23 20:30 ` Eric Sunshine
0 siblings, 1 reply; 64+ messages in thread
From: Jeff King @ 2020-06-23 18:30 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List, Junio C Hamano, Johannes Schindelin
On Tue, Jun 23, 2020 at 01:16:05PM -0400, Eric Sunshine wrote:
> On Tue, Jun 23, 2020 at 11:25 AM Jeff King <peff@peff.net> wrote:
> > diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
> > @@ -238,6 +243,25 @@ collapse "User 0", "User 1", etc into "User X"). This produces a much
> > +[...] For example, if you have a bug which reproduces
> > +with `git rev-list mybranch -- foo.c`, you can run:
> > +
> > +---------------------------------------------------
> > +$ git fast-export --anonymize --all \
> > + --seed-anonymized=foo.c:secret.c \
> > + --seed-anonymized=mybranch \
> > + >stream
> > +---------------------------------------------------
> > +
> > +After importing the stream, you can then run `git rev-list mybranch --
> > +secret.c` in the anonymized repository.
>
> I understand that your intention here is to demonstrate both forms of
> --seed-anonymized, but I'm slightly concerned that people may
> interpret this example as meaning that you are not allowed to
> anonymize the refname when anonymizing a pathname. It might be less
> ambiguous to avoid the "short form" in the example; people who have
> read the description of --seed-anonymized will know that the short
> form can be used without having to see it in an example.
I'm not sure what you'd write, then. You can't mention "mybranch"
anymore if it was anonymized. Are you suggesting to make the example:
git rev-list -- foo.c
by itself?
> > +Note that paths and refnames are split into tokens at slash boundaries.
> > +The command above would anonymize `subdir/foo.c` as something like
> > +`path123/secret.c`.
>
> Confusing. This seems to be saying that anonymizing filenames in
> subdirectories is pointless because you can't know how the leading
> directory names will be anonymized. That leaves the reader wondering
> how to deal with the situation. Does it require using
> --seed-anonymized for each path component leading up to the filename?
You can do that, but I think it would be simpler to just find "secret.c"
in the anonymized repo (either in the checkout, or just "git ls-tree
-r").
> Or can --seed-anonymized take an full pathname (leading directory
> components and filename) in one shot?
No, it can't. Suggested wording? That's what I was trying to say with
the above sentence.
> > + /* First check if it's a token the user configured manually... */
> > + if (anonymized_seeds.cmpfn)
> > + ret = hashmap_get_entry(&anonymized_seeds, &key, hash, &key);
> > + else
> > + ret = NULL;
> > +
> > + /* ...otherwise check if we've already seen it in this context... */
> > + if (!ret)
> > + ret = hashmap_get_entry(map, &key, hash, &key);
> > +
> > + /* ...and finally generate a new mapping if necessary */
>
> I was a bit surprised to see that --seed-anonymized values are stored
> in a separate hash map rather than simply being used to (literally)
> seed the existing anonymization hash map. I guess there's a good
> technical reason for doing it this way, such as the normal
> anonymization hash map not yet being in existence at the time the
> --seed-anonymized option is processed? (I haven't checked because I'm
> too lazy, so it may not be worth spending time answering me.)
The reason is that there isn't one anonymization hash map. There's a
separate one for each generator (so refs become "refs/heads/ref123" and
paths become "path123/path456").
> > @@ -1188,6 +1230,9 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
> > OPT_BOOL(0, "anonymize", &anonymize, N_("anonymize output")),
> > + OPT_CALLBACK_F(0, "seed-anonymized", &anonymized_seeds, N_("from:to"),
> > + N_("convert <from> to <to> in anonymized output"),
> > + PARSE_OPT_NONEG, parse_opt_seed_anonymized),
>
> Would it be worthwhile to add a check somewhere after the
> parse_options() invocation and complain if --seed-anonymized was used
> without --anonymize? (Or should --seed-anonymized perhaps imply
> --anonymize?)
I thought about implying, but I have a slight preference to err on the
side of making things less magical. I don't mind triggering a warning or
error, but it's not like anything _bad_ happens if you don't say
--anonymize. It just doesn't do anything, which seems like a perfectly
logical outcome.
-Peff
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [PATCH 09/10] fast-export: allow seeding the anonymized mapping
2020-06-23 18:30 ` Jeff King
@ 2020-06-23 20:30 ` Eric Sunshine
2020-06-24 15:47 ` Jeff King
0 siblings, 1 reply; 64+ messages in thread
From: Eric Sunshine @ 2020-06-23 20:30 UTC (permalink / raw)
To: Jeff King; +Cc: Git List, Junio C Hamano, Johannes Schindelin
On Tue, Jun 23, 2020 at 2:31 PM Jeff King <peff@peff.net> wrote:
> On Tue, Jun 23, 2020 at 01:16:05PM -0400, Eric Sunshine wrote:
> > On Tue, Jun 23, 2020 at 11:25 AM Jeff King <peff@peff.net> wrote:
> > I understand that your intention here is to demonstrate both forms of
> > --seed-anonymized, but I'm slightly concerned that people may
> > interpret this example as meaning that you are not allowed to
> > anonymize the refname when anonymizing a pathname. It might be less
> > ambiguous to avoid the "short form" in the example; people who have
> > read the description of --seed-anonymized will know that the short
> > form can be used without having to see it in an example.
>
> I'm not sure what you'd write, then. You can't mention "mybranch"
> anymore if it was anonymized. Are you suggesting to make the example:
>
> git rev-list -- foo.c
>
> by itself?
Sorry, I meant to provide an example like this:
For example, if you have a bug which reproduces with `git rev-list
sensitive -- secret.c`, you can run:
$ git fast-export --anonymize --all \
--seed-anonymized=sensitive:foo \
--seed-anonymized=secret.c:bar.c \
>stream
After importing the stream, you can then run `git rev-list foo --
bar.c` in the anonymized repository.
> > > +Note that paths and refnames are split into tokens at slash boundaries.
> > > +The command above would anonymize `subdir/foo.c` as something like
> > > +`path123/secret.c`.
> >
> > Confusing. This seems to be saying that anonymizing filenames in
> > subdirectories is pointless because you can't know how the leading
> > directory names will be anonymized. That leaves the reader wondering
> > how to deal with the situation. Does it require using
> > --seed-anonymized for each path component leading up to the filename?
>
> You can do that, but I think it would be simpler to just find "secret.c"
> in the anonymized repo (either in the checkout, or just "git ls-tree
> -r").
>
> > Or can --seed-anonymized take an full pathname (leading directory
> > components and filename) in one shot?
>
> No, it can't. Suggested wording? That's what I was trying to say with
> the above sentence.
Hmm, perhaps your original attempt can be extended slightly to state
it more explicitly?
Note that paths and refnames are split into tokens at slash
boundaries. The command above would anonymize `subdir/foo.c` as
something like `path123/secret.c`; you could then search for
`secret.c` in the anonymized repository to determine the final
pathname.
To make referencing the final pathname simpler, you can seed
anonymization for each path component; so, if you also anonymize
`subdir` to `publicdir`, then the final pathname would be
`publicdir/secret.c`.
This makes me wonder if --seed-anonymized should do its own
tokenization so that --seed-anonymized=subdir/foo:public/bar is
automatically understood as anonymizing "subdir" to "public" _and_
"foo" to "bar". But that potentially gets weird if you say:
--seed-anonymized=a/b:q/p --seed-anonymized=a/c:y/z
in which case you've given conflicting replacements for "a". (I
suppose it could issue a warning message in that case.)
> > Would it be worthwhile to add a check somewhere after the
> > parse_options() invocation and complain if --seed-anonymized was used
> > without --anonymize? (Or should --seed-anonymized perhaps imply
> > --anonymize?)
>
> I thought about implying, but I have a slight preference to err on the
> side of making things less magical. I don't mind triggering a warning or
> error, but it's not like anything _bad_ happens if you don't say
> --anonymize. It just doesn't do anything, which seems like a perfectly
> logical outcome.
Lack of a warning or error could be kind of bad if the person doesn't
check the fast-export file before sending it out and only discovers
later that:
git fast-export --seed-anonymized=foo:bar
didn't perform _any_ anonymization at all.
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [PATCH 09/10] fast-export: allow seeding the anonymized mapping
2020-06-23 20:30 ` Eric Sunshine
@ 2020-06-24 15:47 ` Jeff King
0 siblings, 0 replies; 64+ messages in thread
From: Jeff King @ 2020-06-24 15:47 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List, Junio C Hamano, Johannes Schindelin
On Tue, Jun 23, 2020 at 04:30:23PM -0400, Eric Sunshine wrote:
> > I'm not sure what you'd write, then. You can't mention "mybranch"
> > anymore if it was anonymized. Are you suggesting to make the example:
> >
> > git rev-list -- foo.c
> >
> > by itself?
>
> Sorry, I meant to provide an example like this:
>
> For example, if you have a bug which reproduces with `git rev-list
> sensitive -- secret.c`, you can run:
>
> $ git fast-export --anonymize --all \
> --seed-anonymized=sensitive:foo \
> --seed-anonymized=secret.c:bar.c \
> >stream
>
> After importing the stream, you can then run `git rev-list foo --
> bar.c` in the anonymized repository.
Thanks, that makes sense. I took this as-is for my reroll (modulo the
change of option name discussed elsewhere).
> Hmm, perhaps your original attempt can be extended slightly to state
> it more explicitly?
>
> Note that paths and refnames are split into tokens at slash
> boundaries. The command above would anonymize `subdir/foo.c` as
> something like `path123/secret.c`; you could then search for
> `secret.c` in the anonymized repository to determine the final
> pathname.
>
> To make referencing the final pathname simpler, you can seed
> anonymization for each path component; so, if you also anonymize
> `subdir` to `publicdir`, then the final pathname would be
> `publicdir/secret.c`.
Thanks, I took this modulo some fixups to match the example above, and
to avoid the use of the word "seed" based on our other discussion.
> This makes me wonder if --seed-anonymized should do its own
> tokenization so that --seed-anonymized=subdir/foo:public/bar is
> automatically understood as anonymizing "subdir" to "public" _and_
> "foo" to "bar". But that potentially gets weird if you say:
>
> --seed-anonymized=a/b:q/p --seed-anonymized=a/c:y/z
>
> in which case you've given conflicting replacements for "a". (I
> suppose it could issue a warning message in that case.)
Right, I think you get into weird corner cases. Another issue is that
not all items are tokenized (e.g., if your author name was foo/bar,
you'd want that replaced as a whole). Probably you could add both the
broken-down and full inputs. Yet another issue is that you can't add a
token with a ":" due to the syntax.
This is an infrequently-enough-used feature that I think it's worth
keeping things simple, even if they're a little less convenient to
invoke.
> Lack of a warning or error could be kind of bad if the person doesn't
> check the fast-export file before sending it out and only discovers
> later that:
>
> git fast-export --seed-anonymized=foo:bar
>
> didn't perform _any_ anonymization at all.
Good point. I'd hope people would glance at the output before sending it
out, but given that it's a potential safety issue, it probably is worth
detecting this case. I'll add it to my re-roll.
-Peff
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH 09/10] fast-export: allow seeding the anonymized mapping
2020-06-23 15:25 ` [PATCH 09/10] fast-export: allow seeding the anonymized mapping Jeff King
2020-06-23 17:16 ` Eric Sunshine
@ 2020-06-23 18:11 ` Eric Sunshine
2020-06-23 18:35 ` Jeff King
1 sibling, 1 reply; 64+ messages in thread
From: Eric Sunshine @ 2020-06-23 18:11 UTC (permalink / raw)
To: Jeff King; +Cc: Git List, Junio C Hamano, Johannes Schindelin
On Tue, Jun 23, 2020 at 11:25 AM Jeff King <peff@peff.net> wrote:
> Let's make it possible to seed the anonymization map. This lets users
> either:
> [...]
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
> @@ -119,6 +119,11 @@ by keeping the marks the same across runs.
> +--seed-anonymized=<from>[:<to>]::
> + Convert token `<from>` to `<to>` in the anonymized output. If
> + `<to>` is omitted, map `<from>` to itself (i.e., do not
> + anonymize it). See the section on `ANONYMIZING` below.
By the way (possible bikeshedding ahead), "seed anonymous" seems
overly technical. I wonder if a name such as
'--anonymize-to=<from>[:<to>]' might be clearer and easier for people
to understand.
In fact, in an earlier email, I asked whether --seed-anonymized should
imply --anonymize. Thinking further on this, I wonder if we even need
the second option name. It should be possible to overload the existing
--anonymize to handle all functions. For instance:
'--anonymize' would anonymize everything
'--anonymize=<from>[:<to>]' would anonymize and map <from> to <to>
So, the example you give in the documentation would become:
git fast-export --all \
--anonymize=foo.c:secret.c \
--anonymize=mybranch >stream
Or is that too cryptic?
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [PATCH 09/10] fast-export: allow seeding the anonymized mapping
2020-06-23 18:11 ` Eric Sunshine
@ 2020-06-23 18:35 ` Jeff King
2020-06-23 20:35 ` Eric Sunshine
0 siblings, 1 reply; 64+ messages in thread
From: Jeff King @ 2020-06-23 18:35 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List, Junio C Hamano, Johannes Schindelin
On Tue, Jun 23, 2020 at 02:11:51PM -0400, Eric Sunshine wrote:
> On Tue, Jun 23, 2020 at 11:25 AM Jeff King <peff@peff.net> wrote:
> > Let's make it possible to seed the anonymization map. This lets users
> > either:
> > [...]
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> > diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
> > @@ -119,6 +119,11 @@ by keeping the marks the same across runs.
> > +--seed-anonymized=<from>[:<to>]::
> > + Convert token `<from>` to `<to>` in the anonymized output. If
> > + `<to>` is omitted, map `<from>` to itself (i.e., do not
> > + anonymize it). See the section on `ANONYMIZING` below.
>
> By the way (possible bikeshedding ahead), "seed anonymous" seems
> overly technical. I wonder if a name such as
> '--anonymize-to=<from>[:<to>]' might be clearer and easier for people
> to understand.
I wrestled with the name, and I agree "seed" is overly technical. And I
came up with many similar variations of "anonymize-to", but they all
seemed ambiguous (e.g., it could be "to" a file that we're storing the
data in).
Perhaps "--anonymize-map" would be less technical?
> In fact, in an earlier email, I asked whether --seed-anonymized should
> imply --anonymize. Thinking further on this, I wonder if we even need
> the second option name. It should be possible to overload the existing
> --anonymize to handle all functions. For instance:
>
> '--anonymize' would anonymize everything
>
> '--anonymize=<from>[:<to>]' would anonymize and map <from> to <to>
>
> So, the example you give in the documentation would become:
>
> git fast-export --all \
> --anonymize=foo.c:secret.c \
> --anonymize=mybranch >stream
>
> Or is that too cryptic?
Yeah, that was another one I considered, but it both seemed cryptic
(after all, we're saying what _not_ to anonymize), and it squats on the
"anonymize" option. So imagine we had another option later, like
"anonymize blobs and paths, but not refs", that could easily be
"--anonymize=blobs,path" or "--anonymize=!refs". I'd rather not paint
ourselves in a corner.
-Peff
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH 09/10] fast-export: allow seeding the anonymized mapping
2020-06-23 18:35 ` Jeff King
@ 2020-06-23 20:35 ` Eric Sunshine
2020-06-24 15:48 ` Jeff King
0 siblings, 1 reply; 64+ messages in thread
From: Eric Sunshine @ 2020-06-23 20:35 UTC (permalink / raw)
To: Jeff King; +Cc: Git List, Junio C Hamano, Johannes Schindelin
On Tue, Jun 23, 2020 at 2:35 PM Jeff King <peff@peff.net> wrote:
> On Tue, Jun 23, 2020 at 02:11:51PM -0400, Eric Sunshine wrote:
> > By the way (possible bikeshedding ahead), "seed anonymous" seems
> > overly technical. I wonder if a name such as
> > '--anonymize-to=<from>[:<to>]' might be clearer and easier for people
> > to understand.
>
> I wrestled with the name, and I agree "seed" is overly technical. And I
> came up with many similar variations of "anonymize-to", but they all
> seemed ambiguous (e.g., it could be "to" a file that we're storing the
> data in).
>
> Perhaps "--anonymize-map" would be less technical?
That's not too bad. It is better than --seed-anonymized. I haven't
come up with any name which improves upon it.
> > In fact, in an earlier email, I asked whether --seed-anonymized should
> > imply --anonymize. Thinking further on this, I wonder if we even need
> > the second option name. It should be possible to overload the existing
> > --anonymize to handle all functions. For instance:
> >
> > git fast-export --all \
> > --anonymize=foo.c:secret.c \
> > --anonymize=mybranch >stream
> >
> > Or is that too cryptic?
>
> Yeah, that was another one I considered, but it both seemed cryptic
> (after all, we're saying what _not_ to anonymize), and it squats on the
> "anonymize" option. So imagine we had another option later, like
> "anonymize blobs and paths, but not refs", that could easily be
> "--anonymize=blobs,path" or "--anonymize=!refs". I'd rather not paint
> ourselves in a corner.
Okay, makes sense.
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH 09/10] fast-export: allow seeding the anonymized mapping
2020-06-23 20:35 ` Eric Sunshine
@ 2020-06-24 15:48 ` Jeff King
0 siblings, 0 replies; 64+ messages in thread
From: Jeff King @ 2020-06-24 15:48 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List, Junio C Hamano, Johannes Schindelin
On Tue, Jun 23, 2020 at 04:35:37PM -0400, Eric Sunshine wrote:
> > I wrestled with the name, and I agree "seed" is overly technical. And I
> > came up with many similar variations of "anonymize-to", but they all
> > seemed ambiguous (e.g., it could be "to" a file that we're storing the
> > data in).
> >
> > Perhaps "--anonymize-map" would be less technical?
>
> That's not too bad. It is better than --seed-anonymized. I haven't
> come up with any name which improves upon it.
I went with that in my reroll, and avoided using the word "seed" at all
in the documentation (I did keep the name "anonymized_seeds" as the
internal variable name for the hashmap, since just calling it "map"
there is ambiguous with all of the other maps).
-Peff
^ permalink raw reply [flat|nested] 64+ messages in thread
* [PATCH 10/10] fast-export: anonymize "master" refname
2020-06-23 15:24 ` [alternative 0/10] fast-export: allow seeding the anonymized mapping Jeff King
` (8 preceding siblings ...)
2020-06-23 15:25 ` [PATCH 09/10] fast-export: allow seeding the anonymized mapping Jeff King
@ 2020-06-23 15:25 ` Jeff King
2020-06-23 19:34 ` [alternative 0/10] fast-export: allow seeding the anonymized mapping Junio C Hamano
2020-06-25 19:48 ` [PATCH v2 0/11] " Jeff King
11 siblings, 0 replies; 64+ messages in thread
From: Jeff King @ 2020-06-23 15:25 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Junio C Hamano, Johannes Schindelin
Running "fast-export --anonymize" will leave "refs/heads/master"
untouched in the output, for two reasons:
- it helped to have some known reference point between the original
and anonymized repository
- since it's historically the default branch name, it doesn't leak any
information
Now that we can ask fast-export to retain particular tokens, we have a
much better tool for the first one (because it works for any ref, not
just master).
For the second, the notion of "default branch name" is likely to become
configurable soon, at which point the name _does_ leak information.
Let's drop this special case in preparation.
Note that we have to adjust the test a bit, since it relied on using the
name "master" in the anonymized repos. We could just use
--seed-anonymized=master to keep the same output, but then we wouldn't
know if it works because of our hard-coded master or because of the
seeding.
So let's flip the test a bit, and confirm that we anonymize "master",
but keep "other" in the output.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/fast-export.c | 7 -------
t/t9351-fast-export-anonymize.sh | 12 +++++++-----
2 files changed, 7 insertions(+), 12 deletions(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index ef82497bbf..7e0e1770cf 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -538,13 +538,6 @@ static const char *anonymize_refname(const char *refname)
static struct strbuf anon = STRBUF_INIT;
int i;
- /*
- * We also leave "master" as a special case, since it does not reveal
- * anything interesting.
- */
- if (!strcmp(refname, "refs/heads/master"))
- return refname;
-
strbuf_reset(&anon);
for (i = 0; i < ARRAY_SIZE(prefixes); i++) {
if (skip_prefix(refname, prefixes[i], &refname)) {
diff --git a/t/t9351-fast-export-anonymize.sh b/t/t9351-fast-export-anonymize.sh
index d84eec9bab..6e2041865c 100755
--- a/t/t9351-fast-export-anonymize.sh
+++ b/t/t9351-fast-export-anonymize.sh
@@ -22,6 +22,7 @@ test_expect_success 'export anonymized stream' '
git fast-export --anonymize --all \
--seed-anonymized=retain-me \
--seed-anonymized=xyzzy:custom-name \
+ --seed-anonymized=other \
>stream
'
@@ -45,12 +46,12 @@ test_expect_success 'stream omits gitlink oids' '
! grep a000000000000000000 stream
'
-test_expect_success 'stream allows master as refname' '
- grep master stream
+test_expect_success 'stream retains other as refname' '
+ grep other stream
'
test_expect_success 'stream omits other refnames' '
- ! grep other stream &&
+ ! grep master stream &&
! grep mytag stream
'
@@ -76,15 +77,16 @@ test_expect_success 'import stream to new repository' '
test_expect_success 'result has two branches' '
git for-each-ref --format="%(refname)" refs/heads >branches &&
test_line_count = 2 branches &&
- other_branch=$(grep -v refs/heads/master branches)
+ other_branch=refs/heads/other &&
+ main_branch=$(grep -v $other_branch branches)
'
test_expect_success 'repo has original shape and timestamps' '
shape () {
git log --format="%m %ct" --left-right --boundary "$@"
} &&
(cd .. && shape master...other) >expect &&
- shape master...$other_branch >actual &&
+ shape $main_branch...$other_branch >actual &&
test_cmp expect actual
'
--
2.27.0.517.gbc32778fa3
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [alternative 0/10] fast-export: allow seeding the anonymized mapping
2020-06-23 15:24 ` [alternative 0/10] fast-export: allow seeding the anonymized mapping Jeff King
` (9 preceding siblings ...)
2020-06-23 15:25 ` [PATCH 10/10] fast-export: anonymize "master" refname Jeff King
@ 2020-06-23 19:34 ` Junio C Hamano
2020-06-23 19:44 ` Jeff King
2020-06-25 19:48 ` [PATCH v2 0/11] " Jeff King
11 siblings, 1 reply; 64+ messages in thread
From: Junio C Hamano @ 2020-06-23 19:34 UTC (permalink / raw)
To: Jeff King; +Cc: git, Eric Sunshine, Johannes Schindelin
Jeff King <peff@peff.net> writes:
> On Mon, Jun 22, 2020 at 05:47:46PM -0400, Jeff King wrote:
>
>> Here's a v2 which I think addresses all of the comments. I have to admit
>> that after writing my last email to Junio, I am wondering whether it
>> would be sufficient and simpler to let the user specify a static mapping
>> of tokens (that could just be applied anywhere).
Yeah, dumping the random mapping created and telling the user to
figure out what the tool did is less nice than allowing the user
to enumerate what tokens are sensitible and need to be replaced.
> Both of these techniques _could_ live side-by-side within fast-export,
> as they have slightly different strengths and weaknesses. But I'd prefer
> to just go with one (this one) in the name of simplicity, and I strongly
> suspect nobody will ever ask for the other.
OK. So should we revert the merge of the other one into 'next'?
That is easy enough ;-)
Thanks.
> [01/10]: t9351: derive anonymized tree checks from original repo
> [02/10]: fast-export: use xmemdupz() for anonymizing oids
>
> These first two are actually a bug-fix that I noticed while writing
> it.
>
> [03/10]: fast-export: store anonymized oids as hex strings
> [04/10]: fast-export: tighten anonymize_mem() interface to handle only strings
> [05/10]: fast-export: stop storing lengths in anonymized hashmaps
> [06/10]: fast-export: use a flex array to store anonymized entries
> [07/10]: fast-export: move global "idents" anonymize hashmap into function
> [08/10]: fast-export: add a "data" callback parameter to anonymize_str()
>
> This is all cleanup and prep. More than is strictly necessary for
> this series, but it does simplify things and reduce the memory
> footprint (only a few megabytes in git.git, but more in larger
> repos).
>
> [09/10]: fast-export: allow seeding the anonymized mapping
>
> And then this is the actual feature...
>
> [10/10]: fast-export: anonymize "master" refname
>
> ...which finally lets us drop the special name rule.
>
> Documentation/git-fast-export.txt | 24 +++++
> builtin/fast-export.c | 161 +++++++++++++++++++-----------
> t/t9351-fast-export-anonymize.sh | 54 +++++++---
> 3 files changed, 167 insertions(+), 72 deletions(-)
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [alternative 0/10] fast-export: allow seeding the anonymized mapping
2020-06-23 19:34 ` [alternative 0/10] fast-export: allow seeding the anonymized mapping Junio C Hamano
@ 2020-06-23 19:44 ` Jeff King
0 siblings, 0 replies; 64+ messages in thread
From: Jeff King @ 2020-06-23 19:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Eric Sunshine, Johannes Schindelin
On Tue, Jun 23, 2020 at 12:34:47PM -0700, Junio C Hamano wrote:
> > Both of these techniques _could_ live side-by-side within fast-export,
> > as they have slightly different strengths and weaknesses. But I'd prefer
> > to just go with one (this one) in the name of simplicity, and I strongly
> > suspect nobody will ever ask for the other.
>
> OK. So should we revert the merge of the other one into 'next'?
> That is easy enough ;-)
Yes, please. :)
-Peff
^ permalink raw reply [flat|nested] 64+ messages in thread
* [PATCH v2 0/11] fast-export: allow seeding the anonymized mapping
2020-06-23 15:24 ` [alternative 0/10] fast-export: allow seeding the anonymized mapping Jeff King
` (10 preceding siblings ...)
2020-06-23 19:34 ` [alternative 0/10] fast-export: allow seeding the anonymized mapping Junio C Hamano
@ 2020-06-25 19:48 ` Jeff King
2020-06-25 19:48 ` [PATCH v2 01/11] t9351: derive anonymized tree checks from original repo Jeff King
` (11 more replies)
11 siblings, 12 replies; 64+ messages in thread
From: Jeff King @ 2020-06-25 19:48 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Junio C Hamano, Johannes Schindelin, SZEDER Gábor
Here's a re-roll of the "--seed-anonymize" feature, incorporating
changes from Eric's review.
Here's a summary of the changes. Patches 1-8 are unchanged:
[01/11]: t9351: derive anonymized tree checks from original repo
[02/11]: fast-export: use xmemdupz() for anonymizing oids
[03/11]: fast-export: store anonymized oids as hex strings
[04/11]: fast-export: tighten anonymize_mem() interface to handle only strings
[05/11]: fast-export: stop storing lengths in anonymized hashmaps
[06/11]: fast-export: use a flex array to store anonymized entries
[07/11]: fast-export: move global "idents" anonymize hashmap into function
[08/11]: fast-export: add a "data" callback parameter to anonymize_str()
[09/11]: fast-export: allow seeding the anonymized mapping
- the option is now called "--anonymize-map", and the word "seed"
isn't used in any user-facing documentation
- incorporated Eric's documentation suggestions
- --anonymize-map without --anonymize now triggers an error
[10/11]: fast-export: anonymize "master" refname
- minor adjustments to handle change of option name
[11/11]: fast-export: use local array to store anonymized oid
- new in this iteration; this address the gcc-4 warning that Gábor
mentioned. I prepared it on top since I think we'd eventually want
to revert it once we decide that compiler is too old (and I'd be
perfectly fine to declare that it's so now, and just never apply
it at all).
Full range diff is below.
1: 65dca0c684 = 1: dd2668d7fe t9351: derive anonymized tree checks from original repo
2: bc79b92850 = 2: 46a07c62e1 fast-export: use xmemdupz() for anonymizing oids
3: f5b34b3730 = 3: a7be6c4703 fast-export: store anonymized oids as hex strings
4: 5982a5c764 = 4: 712e481e4e fast-export: tighten anonymize_mem() interface to handle only strings
5: d6b7a75a13 = 5: 1ad28f560a fast-export: stop storing lengths in anonymized hashmaps
6: f897a949b1 = 6: b6456c0c7b fast-export: use a flex array to store anonymized entries
7: e5465fb8d8 = 7: 81d478c12b fast-export: move global "idents" anonymize hashmap into function
8: 8dd74b95b7 = 8: 0d34f57a94 fast-export: add a "data" callback parameter to anonymize_str()
9: c7aec82ba5 ! 9: 64e3afe46f fast-export: allow seeding the anonymized mapping
@@ Commit message
does work, but it's a bit awkward to use (you have to manually dig the
items you care about out of the mapping).
+ Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Jeff King <peff@peff.net>
## Documentation/git-fast-export.txt ##
@@ Documentation/git-fast-export.txt: by keeping the marks the same across runs.
the shape of the history and stored tree. See the section on
`ANONYMIZING` below.
-+--seed-anonymized=<from>[:<to>]::
++--anonymize-map=<from>[:<to>]::
+ Convert token `<from>` to `<to>` in the anonymized output. If
+ `<to>` is omitted, map `<from>` to itself (i.e., do not
+ anonymize it). See the section on `ANONYMIZING` below.
@@ Documentation/git-fast-export.txt: collapse "User 0", "User 1", etc into "User X
+paths, which becomes challenging after refnames and paths have been
+anonymized. You can ask for a particular token to be left as-is or
+mapped to a new value. For example, if you have a bug which reproduces
-+with `git rev-list mybranch -- foo.c`, you can run:
++with `git rev-list sensitive -- secret.c`, you can run:
+
+---------------------------------------------------
+$ git fast-export --anonymize --all \
-+ --seed-anonymized=foo.c:secret.c \
-+ --seed-anonymized=mybranch \
++ --anonymize-map=sensitive:foo \
++ --anonymize-map=secret.c:bar.c \
+ >stream
+---------------------------------------------------
+
-+After importing the stream, you can then run `git rev-list mybranch --
-+secret.c` in the anonymized repository.
++After importing the stream, you can then run `git rev-list foo -- bar.c`
++in the anonymized repository.
+
+Note that paths and refnames are split into tokens at slash boundaries.
-+The command above would anonymize `subdir/foo.c` as something like
-+`path123/secret.c`.
++The command above would anonymize `subdir/secret.c` as something like
++`path123/bar.c`; you could then search for `bar.c` in the anonymized
++repository to determine the final pathname.
++
++To make referencing the final pathname simpler, you can map each path
++component; so if you also anonymize `subdir` to `publicdir`, then the
++final pathname would be `publicdir/bar.c`.
LIMITATIONS
-----------
@@ builtin/fast-export.c: static void handle_deletes(void)
+ return xstrdup(data);
+}
+
-+static int parse_opt_seed_anonymized(const struct option *opt,
-+ const char *arg, int unset)
++static int parse_opt_anonymize_map(const struct option *opt,
++ const char *arg, int unset)
+{
+ struct hashmap *map = opt->value;
+ const char *delim, *value;
@@ builtin/fast-export.c: static void handle_deletes(void)
+ }
+
+ if (!keylen || !*value)
-+ return error(_("--seed-anonymized token cannot be empty"));
++ return error(_("--anonymize-map token cannot be empty"));
+
+ anonymize_str(map, anonymize_seed, arg, keylen, (void *)value);
+
@@ builtin/fast-export.c: int cmd_fast_export(int argc, const char **argv, const ch
OPT_STRING_LIST(0, "refspec", &refspecs_list, N_("refspec"),
N_("Apply refspec to exported refs")),
OPT_BOOL(0, "anonymize", &anonymize, N_("anonymize output")),
-+ OPT_CALLBACK_F(0, "seed-anonymized", &anonymized_seeds, N_("from:to"),
++ OPT_CALLBACK_F(0, "anonymize-map", &anonymized_seeds, N_("from:to"),
+ N_("convert <from> to <to> in anonymized output"),
-+ PARSE_OPT_NONEG, parse_opt_seed_anonymized),
++ PARSE_OPT_NONEG, parse_opt_anonymize_map),
OPT_BOOL(0, "reference-excluded-parents",
&reference_excluded_commits, N_("Reference parents which are not in fast-export stream by object id")),
OPT_BOOL(0, "show-original-ids", &show_original_ids,
+@@ builtin/fast-export.c: int cmd_fast_export(int argc, const char **argv, const char *prefix)
+ if (argc > 1)
+ usage_with_options (fast_export_usage, options);
+
++ if (anonymized_seeds.cmpfn && !anonymize)
++ die(_("--anonymize-map without --anonymize does not make sense"));
++
+ if (refspecs_list.nr) {
+ int i;
+
## t/t9351-fast-export-anonymize.sh ##
@@ t/t9351-fast-export-anonymize.sh: test_description='basic tests for fast-export --anonymize'
@@ t/t9351-fast-export-anonymize.sh: test_expect_success 'setup simple repo' '
test_expect_success 'export anonymized stream' '
- git fast-export --anonymize --all >stream
+ git fast-export --anonymize --all \
-+ --seed-anonymized=retain-me \
-+ --seed-anonymized=xyzzy:custom-name \
++ --anonymize-map=retain-me \
++ --anonymize-map=xyzzy:custom-name \
+ >stream
'
10: 59737b97de ! 10: 103f5bcd65 fast-export: anonymize "master" refname
@@ Commit message
Note that we have to adjust the test a bit, since it relied on using the
name "master" in the anonymized repos. We could just use
- --seed-anonymized=master to keep the same output, but then we wouldn't
+ --anonymize-map=master to keep the same output, but then we wouldn't
know if it works because of our hard-coded master or because of the
- seeding.
+ explicit map.
So let's flip the test a bit, and confirm that we anonymize "master",
but keep "other" in the output.
@@ builtin/fast-export.c: static const char *anonymize_refname(const char *refname)
## t/t9351-fast-export-anonymize.sh ##
@@ t/t9351-fast-export-anonymize.sh: test_expect_success 'export anonymized stream' '
git fast-export --anonymize --all \
- --seed-anonymized=retain-me \
- --seed-anonymized=xyzzy:custom-name \
-+ --seed-anonymized=other \
+ --anonymize-map=retain-me \
+ --anonymize-map=xyzzy:custom-name \
++ --anonymize-map=other \
>stream
'
-: ---------- > 11: f2640407c5 fast-export: use local array to store anonymized oid
^ permalink raw reply [flat|nested] 64+ messages in thread* [PATCH v2 01/11] t9351: derive anonymized tree checks from original repo
2020-06-25 19:48 ` [PATCH v2 0/11] " Jeff King
@ 2020-06-25 19:48 ` Jeff King
2020-06-25 19:48 ` [PATCH v2 02/11] fast-export: use xmemdupz() for anonymizing oids Jeff King
` (10 subsequent siblings)
11 siblings, 0 replies; 64+ messages in thread
From: Jeff King @ 2020-06-25 19:48 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Junio C Hamano, Johannes Schindelin, SZEDER Gábor
Our tests of the anonymized repo just hard-code the expected set of
objects in the root and subdirectory trees. This makes them brittle to
the test setup changing (e.g., adding new paths that need tested).
Let's look at the original repo to compute our expected set of objects.
Note that this isn't completely perfect (e.g., we still rely on there
being only one tree in the root), but it does simplify later patches.
Signed-off-by: Jeff King <peff@peff.net>
---
t/t9351-fast-export-anonymize.sh | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/t/t9351-fast-export-anonymize.sh b/t/t9351-fast-export-anonymize.sh
index 897dc50907..e772cf9930 100755
--- a/t/t9351-fast-export-anonymize.sh
+++ b/t/t9351-fast-export-anonymize.sh
@@ -71,22 +71,18 @@ test_expect_success 'repo has original shape and timestamps' '
test_expect_success 'root tree has original shape' '
# the output entries are not necessarily in the same
- # order, but we know at least that we will have one tree
- # and one blob, so just check the sorted order
- cat >expect <<-\EOF &&
- blob
- tree
- EOF
+ # order, but we should at least have the same set of
+ # object types.
+ git -C .. ls-tree HEAD >orig-root &&
+ cut -d" " -f2 <orig-root | sort >expect &&
git ls-tree $other_branch >root &&
cut -d" " -f2 <root | sort >actual &&
test_cmp expect actual
'
test_expect_success 'paths in subdir ended up in one tree' '
- cat >expect <<-\EOF &&
- blob
- blob
- EOF
+ git -C .. ls-tree other:subdir >orig-subdir &&
+ cut -d" " -f2 <orig-subdir | sort >expect &&
tree=$(grep tree root | cut -f2) &&
git ls-tree $other_branch:$tree >tree &&
cut -d" " -f2 <tree >actual &&
--
2.27.0.593.gb3082a2aaf
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH v2 02/11] fast-export: use xmemdupz() for anonymizing oids
2020-06-25 19:48 ` [PATCH v2 0/11] " Jeff King
2020-06-25 19:48 ` [PATCH v2 01/11] t9351: derive anonymized tree checks from original repo Jeff King
@ 2020-06-25 19:48 ` Jeff King
2020-06-25 19:48 ` [PATCH v2 03/11] fast-export: store anonymized oids as hex strings Jeff King
` (9 subsequent siblings)
11 siblings, 0 replies; 64+ messages in thread
From: Jeff King @ 2020-06-25 19:48 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Junio C Hamano, Johannes Schindelin, SZEDER Gábor
Our anonymize_mem() function is careful to take a ptr/len pair to allow
storing binary tokens like object ids, as well as partial strings (e.g.,
just "foo" of "foo/bar"). But it duplicates the hash key using
xstrdup()! That means that:
- for a partial string, we'd store all bytes up to the NUL, even
though we'd never look at anything past "len". This didn't produce
wrong behavior, but was wasteful.
- for a binary oid that doesn't contain a zero byte, we'd copy garbage
bytes off the end of the array (though as long as nothing complained
about reading uninitialized bytes, further reads would be limited by
"len", and we'd produce the correct results)
- for a binary oid that does contain a zero byte, we'd copy _fewer_
bytes than intended into the hashmap struct. When we later try to
look up a value, we'd access uninitialized memory and potentially
falsely claim that a particular oid is not present.
The most common reason to store an oid is an anonymized gitlink, but our
test case doesn't have any gitlinks at all. So let's add one whose oid
contains a NUL and is present at two different paths. ASan catches the
memory error, but even without it we can detect the bug because the oid
is not anonymized the same way for both paths.
And of course the fix is to copy the correct number of bytes. We don't
technically need the appended NUL from xmemdupz(), but it doesn't hurt
as an extra protection against anybody treating it like a string (plus a
future patch will push us more in that direction).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/fast-export.c | 2 +-
t/t9351-fast-export-anonymize.sh | 15 +++++++++++++++
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 85868162ee..289395a131 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -162,7 +162,7 @@ static const void *anonymize_mem(struct hashmap *map,
if (!ret) {
ret = xmalloc(sizeof(*ret));
hashmap_entry_init(&ret->hash, key.hash.hash);
- ret->orig = xstrdup(orig);
+ ret->orig = xmemdupz(orig, *len);
ret->orig_len = *len;
ret->anon = generate(orig, len);
ret->anon_len = *len;
diff --git a/t/t9351-fast-export-anonymize.sh b/t/t9351-fast-export-anonymize.sh
index e772cf9930..dc5d75cd19 100755
--- a/t/t9351-fast-export-anonymize.sh
+++ b/t/t9351-fast-export-anonymize.sh
@@ -10,6 +10,10 @@ test_expect_success 'setup simple repo' '
mkdir subdir &&
test_commit subdir/bar &&
test_commit subdir/xyzzy &&
+ fake_commit=$(echo $ZERO_OID | sed s/0/a/) &&
+ git update-index --add --cacheinfo 160000,$fake_commit,link1 &&
+ git update-index --add --cacheinfo 160000,$fake_commit,link2 &&
+ git commit -m "add gitlink" &&
git tag -m "annotated tag" mytag
'
@@ -26,6 +30,12 @@ test_expect_success 'stream omits path names' '
! grep xyzzy stream
'
+test_expect_success 'stream omits gitlink oids' '
+ # avoid relying on the whole oid to remain hash-agnostic; this is
+ # plenty to be unique within our test case
+ ! grep a000000000000000000 stream
+'
+
test_expect_success 'stream allows master as refname' '
grep master stream
'
@@ -89,6 +99,11 @@ test_expect_success 'paths in subdir ended up in one tree' '
test_cmp expect actual
'
+test_expect_success 'identical gitlinks got identical oid' '
+ awk "/commit/ { print \$3 }" <root | sort -u >commits &&
+ test_line_count = 1 commits
+'
+
test_expect_success 'tag points to branch tip' '
git rev-parse $other_branch >expect &&
git for-each-ref --format="%(*objectname)" | grep . >actual &&
--
2.27.0.593.gb3082a2aaf
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH v2 03/11] fast-export: store anonymized oids as hex strings
2020-06-25 19:48 ` [PATCH v2 0/11] " Jeff King
2020-06-25 19:48 ` [PATCH v2 01/11] t9351: derive anonymized tree checks from original repo Jeff King
2020-06-25 19:48 ` [PATCH v2 02/11] fast-export: use xmemdupz() for anonymizing oids Jeff King
@ 2020-06-25 19:48 ` Jeff King
2020-06-25 19:48 ` [PATCH v2 04/11] fast-export: tighten anonymize_mem() interface to handle only strings Jeff King
` (8 subsequent siblings)
11 siblings, 0 replies; 64+ messages in thread
From: Jeff King @ 2020-06-25 19:48 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Junio C Hamano, Johannes Schindelin, SZEDER Gábor
When fast-export stores anonymized oids, it does so as binary strings.
And while the anonymous mapping storage is binary-clean (at least as of
the previous commit), this will become awkward when we start exposing
more of it to the user. In particular, if we allow a method for
retaining token "foo", then users may want to specify a hex oid as such
a token.
Let's just switch to storing the hex strings. The difference in memory
usage is negligible (especially considering how infrequently we'd
generally store an oid compared to, say, path components).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/fast-export.c | 28 ++++++++++++++++------------
1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 289395a131..4a3a4c933e 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -387,16 +387,19 @@ static void *generate_fake_oid(const void *old, size_t *len)
{
static uint32_t counter = 1; /* avoid null oid */
const unsigned hashsz = the_hash_algo->rawsz;
- unsigned char *out = xcalloc(hashsz, 1);
- put_be32(out + hashsz - 4, counter++);
- return out;
+ struct object_id oid;
+ char *hex = xmallocz(GIT_MAX_HEXSZ);
+
+ oidclr(&oid);
+ put_be32(oid.hash + hashsz - 4, counter++);
+ return oid_to_hex_r(hex, &oid);
}
-static const struct object_id *anonymize_oid(const struct object_id *oid)
+static const char *anonymize_oid(const char *oid_hex)
{
static struct hashmap objs;
- size_t len = the_hash_algo->rawsz;
- return anonymize_mem(&objs, generate_fake_oid, oid, &len);
+ size_t len = strlen(oid_hex);
+ return anonymize_mem(&objs, generate_fake_oid, oid_hex, &len);
}
static void show_filemodify(struct diff_queue_struct *q,
@@ -455,9 +458,9 @@ static void show_filemodify(struct diff_queue_struct *q,
*/
if (no_data || S_ISGITLINK(spec->mode))
printf("M %06o %s ", spec->mode,
- oid_to_hex(anonymize ?
- anonymize_oid(&spec->oid) :
- &spec->oid));
+ anonymize ?
+ anonymize_oid(oid_to_hex(&spec->oid)) :
+ oid_to_hex(&spec->oid));
else {
struct object *object = lookup_object(the_repository,
&spec->oid);
@@ -712,9 +715,10 @@ static void handle_commit(struct commit *commit, struct rev_info *rev,
if (mark)
printf(":%d\n", mark);
else
- printf("%s\n", oid_to_hex(anonymize ?
- anonymize_oid(&obj->oid) :
- &obj->oid));
+ printf("%s\n",
+ anonymize ?
+ anonymize_oid(oid_to_hex(&obj->oid)) :
+ oid_to_hex(&obj->oid));
i++;
}
--
2.27.0.593.gb3082a2aaf
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH v2 04/11] fast-export: tighten anonymize_mem() interface to handle only strings
2020-06-25 19:48 ` [PATCH v2 0/11] " Jeff King
` (2 preceding siblings ...)
2020-06-25 19:48 ` [PATCH v2 03/11] fast-export: store anonymized oids as hex strings Jeff King
@ 2020-06-25 19:48 ` Jeff King
2020-06-25 19:48 ` [PATCH v2 05/11] fast-export: stop storing lengths in anonymized hashmaps Jeff King
` (7 subsequent siblings)
11 siblings, 0 replies; 64+ messages in thread
From: Jeff King @ 2020-06-25 19:48 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Junio C Hamano, Johannes Schindelin, SZEDER Gábor
While the anonymize_mem() interface _can_ store arbitrary byte
sequences, none of the callers uses this feature (as of the previous
commit). We'd like to keep it that way, as we'll be exposing the
string-like nature of the anonymization routines to the user. So let's
tighten up the interface a bit:
- don't treat "len" as an out-parameter from anonymize_mem(); this
ensures callers treat the pointer result as a NUL-terminated string
- likewise, don't treat "len" as an out-parameter from generator
functions
- swap out "void *" for "char *" as appropriate to signal that we
don't handle arbitrary memory
- rename the function to anonymize_str()
This will also open up some optimization opportunities in a future
patch.
Note that we can't drop the "len" parameter entirely. Some callers do
pass in partial strings (e.g., "foo/bar", len=3) to avoid copying, and
we need to handle those still.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/fast-export.c | 53 +++++++++++++++++++++----------------------
1 file changed, 26 insertions(+), 27 deletions(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 4a3a4c933e..d8ea067630 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -145,31 +145,30 @@ static int anonymized_entry_cmp(const void *unused_cmp_data,
* the same anonymized string with another. The actual generation
* is farmed out to the generate function.
*/
-static const void *anonymize_mem(struct hashmap *map,
- void *(*generate)(const void *, size_t *),
- const void *orig, size_t *len)
+static const char *anonymize_str(struct hashmap *map,
+ char *(*generate)(const char *, size_t),
+ const char *orig, size_t len)
{
struct anonymized_entry key, *ret;
if (!map->cmpfn)
hashmap_init(map, anonymized_entry_cmp, NULL, 0);
- hashmap_entry_init(&key.hash, memhash(orig, *len));
+ hashmap_entry_init(&key.hash, memhash(orig, len));
key.orig = orig;
- key.orig_len = *len;
+ key.orig_len = len;
ret = hashmap_get_entry(map, &key, hash, NULL);
if (!ret) {
ret = xmalloc(sizeof(*ret));
hashmap_entry_init(&ret->hash, key.hash.hash);
- ret->orig = xmemdupz(orig, *len);
- ret->orig_len = *len;
+ ret->orig = xmemdupz(orig, len);
+ ret->orig_len = len;
ret->anon = generate(orig, len);
- ret->anon_len = *len;
+ ret->anon_len = strlen(ret->anon);
hashmap_put(map, &ret->hash);
}
- *len = ret->anon_len;
return ret->anon;
}
@@ -181,13 +180,13 @@ static const void *anonymize_mem(struct hashmap *map,
*/
static void anonymize_path(struct strbuf *out, const char *path,
struct hashmap *map,
- void *(*generate)(const void *, size_t *))
+ char *(*generate)(const char *, size_t))
{
while (*path) {
const char *end_of_component = strchrnul(path, '/');
size_t len = end_of_component - path;
- const char *c = anonymize_mem(map, generate, path, &len);
- strbuf_add(out, c, len);
+ const char *c = anonymize_str(map, generate, path, len);
+ strbuf_addstr(out, c);
path = end_of_component;
if (*path)
strbuf_addch(out, *path++);
@@ -361,12 +360,12 @@ static void print_path_1(const char *path)
printf("%s", path);
}
-static void *anonymize_path_component(const void *path, size_t *len)
+static char *anonymize_path_component(const char *path, size_t len)
{
static int counter;
struct strbuf out = STRBUF_INIT;
strbuf_addf(&out, "path%d", counter++);
- return strbuf_detach(&out, len);
+ return strbuf_detach(&out, NULL);
}
static void print_path(const char *path)
@@ -383,7 +382,7 @@ static void print_path(const char *path)
}
}
-static void *generate_fake_oid(const void *old, size_t *len)
+static char *generate_fake_oid(const char *old, size_t len)
{
static uint32_t counter = 1; /* avoid null oid */
const unsigned hashsz = the_hash_algo->rawsz;
@@ -399,7 +398,7 @@ static const char *anonymize_oid(const char *oid_hex)
{
static struct hashmap objs;
size_t len = strlen(oid_hex);
- return anonymize_mem(&objs, generate_fake_oid, oid_hex, &len);
+ return anonymize_str(&objs, generate_fake_oid, oid_hex, len);
}
static void show_filemodify(struct diff_queue_struct *q,
@@ -496,12 +495,12 @@ static const char *find_encoding(const char *begin, const char *end)
return bol;
}
-static void *anonymize_ref_component(const void *old, size_t *len)
+static char *anonymize_ref_component(const char *old, size_t len)
{
static int counter;
struct strbuf out = STRBUF_INIT;
strbuf_addf(&out, "ref%d", counter++);
- return strbuf_detach(&out, len);
+ return strbuf_detach(&out, NULL);
}
static const char *anonymize_refname(const char *refname)
@@ -550,13 +549,13 @@ static char *anonymize_commit_message(const char *old)
}
static struct hashmap idents;
-static void *anonymize_ident(const void *old, size_t *len)
+static char *anonymize_ident(const char *old, size_t len)
{
static int counter;
struct strbuf out = STRBUF_INIT;
strbuf_addf(&out, "User %d <user%d@example.com>", counter, counter);
counter++;
- return strbuf_detach(&out, len);
+ return strbuf_detach(&out, NULL);
}
/*
@@ -591,9 +590,9 @@ static void anonymize_ident_line(const char **beg, const char **end)
size_t len;
len = split.mail_end - split.name_begin;
- ident = anonymize_mem(&idents, anonymize_ident,
- split.name_begin, &len);
- strbuf_add(out, ident, len);
+ ident = anonymize_str(&idents, anonymize_ident,
+ split.name_begin, len);
+ strbuf_addstr(out, ident);
strbuf_addch(out, ' ');
strbuf_add(out, split.date_begin, split.tz_end - split.date_begin);
} else {
@@ -733,12 +732,12 @@ static void handle_commit(struct commit *commit, struct rev_info *rev,
show_progress();
}
-static void *anonymize_tag(const void *old, size_t *len)
+static char *anonymize_tag(const char *old, size_t len)
{
static int counter;
struct strbuf out = STRBUF_INIT;
strbuf_addf(&out, "tag message %d", counter++);
- return strbuf_detach(&out, len);
+ return strbuf_detach(&out, NULL);
}
static void handle_tail(struct object_array *commits, struct rev_info *revs,
@@ -808,8 +807,8 @@ static void handle_tag(const char *name, struct tag *tag)
name = anonymize_refname(name);
if (message) {
static struct hashmap tags;
- message = anonymize_mem(&tags, anonymize_tag,
- message, &message_size);
+ message = anonymize_str(&tags, anonymize_tag,
+ message, message_size);
}
}
--
2.27.0.593.gb3082a2aaf
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH v2 05/11] fast-export: stop storing lengths in anonymized hashmaps
2020-06-25 19:48 ` [PATCH v2 0/11] " Jeff King
` (3 preceding siblings ...)
2020-06-25 19:48 ` [PATCH v2 04/11] fast-export: tighten anonymize_mem() interface to handle only strings Jeff King
@ 2020-06-25 19:48 ` Jeff King
2020-06-25 19:48 ` [PATCH v2 06/11] fast-export: use a flex array to store anonymized entries Jeff King
` (6 subsequent siblings)
11 siblings, 0 replies; 64+ messages in thread
From: Jeff King @ 2020-06-25 19:48 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Junio C Hamano, Johannes Schindelin, SZEDER Gábor
Now that the anonymize_str() interface is restricted to NUL-terminated
strings, there's no need for us to keep track of the length of each
entry in the hashmap. This simplifies the code and saves a bit of
memory.
Note that we do still need to compare the stored results to partial
strings passed in by the callers. We can do that by using hashmap's
keydata feature to get the ptr/len pair into the comparison function,
and then using strncmp().
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/fast-export.c | 28 ++++++++++++++++++----------
1 file changed, 18 insertions(+), 10 deletions(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index d8ea067630..5df2ada47d 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -121,23 +121,32 @@ static int has_unshown_parent(struct commit *commit)
struct anonymized_entry {
struct hashmap_entry hash;
const char *orig;
- size_t orig_len;
const char *anon;
- size_t anon_len;
+};
+
+struct anonymized_entry_key {
+ struct hashmap_entry hash;
+ const char *orig;
+ size_t orig_len;
};
static int anonymized_entry_cmp(const void *unused_cmp_data,
const struct hashmap_entry *eptr,
const struct hashmap_entry *entry_or_key,
- const void *unused_keydata)
+ const void *keydata)
{
const struct anonymized_entry *a, *b;
a = container_of(eptr, const struct anonymized_entry, hash);
- b = container_of(entry_or_key, const struct anonymized_entry, hash);
+ if (keydata) {
+ const struct anonymized_entry_key *key = keydata;
+ int equal = !strncmp(a->orig, key->orig, key->orig_len) &&
+ !a->orig[key->orig_len];
+ return !equal;
+ }
- return a->orig_len != b->orig_len ||
- memcmp(a->orig, b->orig, a->orig_len);
+ b = container_of(entry_or_key, const struct anonymized_entry, hash);
+ return strcmp(a->orig, b->orig);
}
/*
@@ -149,23 +158,22 @@ static const char *anonymize_str(struct hashmap *map,
char *(*generate)(const char *, size_t),
const char *orig, size_t len)
{
- struct anonymized_entry key, *ret;
+ struct anonymized_entry_key key;
+ struct anonymized_entry *ret;
if (!map->cmpfn)
hashmap_init(map, anonymized_entry_cmp, NULL, 0);
hashmap_entry_init(&key.hash, memhash(orig, len));
key.orig = orig;
key.orig_len = len;
- ret = hashmap_get_entry(map, &key, hash, NULL);
+ ret = hashmap_get_entry(map, &key, hash, &key);
if (!ret) {
ret = xmalloc(sizeof(*ret));
hashmap_entry_init(&ret->hash, key.hash.hash);
ret->orig = xmemdupz(orig, len);
- ret->orig_len = len;
ret->anon = generate(orig, len);
- ret->anon_len = strlen(ret->anon);
hashmap_put(map, &ret->hash);
}
--
2.27.0.593.gb3082a2aaf
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH v2 06/11] fast-export: use a flex array to store anonymized entries
2020-06-25 19:48 ` [PATCH v2 0/11] " Jeff King
` (4 preceding siblings ...)
2020-06-25 19:48 ` [PATCH v2 05/11] fast-export: stop storing lengths in anonymized hashmaps Jeff King
@ 2020-06-25 19:48 ` Jeff King
2020-06-25 19:48 ` [PATCH v2 07/11] fast-export: move global "idents" anonymize hashmap into function Jeff King
` (5 subsequent siblings)
11 siblings, 0 replies; 64+ messages in thread
From: Jeff King @ 2020-06-25 19:48 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Junio C Hamano, Johannes Schindelin, SZEDER Gábor
Now that we're using a separate keydata struct for hash lookups, we have
more flexibility in how we allocate anonymized_entry structs. Let's push
the "orig" key into a flex member within the struct. That should save us
a few bytes of memory per entry (a pointer plus any malloc overhead),
and may make lookups a little faster (since it's one less pointer to
chase in the comparison function).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/fast-export.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 5df2ada47d..99d4a2b404 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -120,8 +120,8 @@ static int has_unshown_parent(struct commit *commit)
struct anonymized_entry {
struct hashmap_entry hash;
- const char *orig;
const char *anon;
+ const char orig[FLEX_ARRAY];
};
struct anonymized_entry_key {
@@ -170,9 +170,8 @@ static const char *anonymize_str(struct hashmap *map,
ret = hashmap_get_entry(map, &key, hash, &key);
if (!ret) {
- ret = xmalloc(sizeof(*ret));
+ FLEX_ALLOC_MEM(ret, orig, orig, len);
hashmap_entry_init(&ret->hash, key.hash.hash);
- ret->orig = xmemdupz(orig, len);
ret->anon = generate(orig, len);
hashmap_put(map, &ret->hash);
}
--
2.27.0.593.gb3082a2aaf
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH v2 07/11] fast-export: move global "idents" anonymize hashmap into function
2020-06-25 19:48 ` [PATCH v2 0/11] " Jeff King
` (5 preceding siblings ...)
2020-06-25 19:48 ` [PATCH v2 06/11] fast-export: use a flex array to store anonymized entries Jeff King
@ 2020-06-25 19:48 ` Jeff King
2020-06-25 19:48 ` [PATCH v2 08/11] fast-export: add a "data" callback parameter to anonymize_str() Jeff King
` (4 subsequent siblings)
11 siblings, 0 replies; 64+ messages in thread
From: Jeff King @ 2020-06-25 19:48 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Junio C Hamano, Johannes Schindelin, SZEDER Gábor
All of the other anonymization functions keep their static mappings
inside the function to avoid polluting the global namespace. Let's do
the same for "idents", as nobody needs it outside of
anonymize_ident_line().
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/fast-export.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 99d4a2b404..16a1563e49 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -555,7 +555,6 @@ static char *anonymize_commit_message(const char *old)
return xstrfmt("subject %d\n\nbody\n", counter++);
}
-static struct hashmap idents;
static char *anonymize_ident(const char *old, size_t len)
{
static int counter;
@@ -572,6 +571,7 @@ static char *anonymize_ident(const char *old, size_t len)
*/
static void anonymize_ident_line(const char **beg, const char **end)
{
+ static struct hashmap idents;
static struct strbuf buffers[] = { STRBUF_INIT, STRBUF_INIT };
static unsigned which_buffer;
--
2.27.0.593.gb3082a2aaf
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH v2 08/11] fast-export: add a "data" callback parameter to anonymize_str()
2020-06-25 19:48 ` [PATCH v2 0/11] " Jeff King
` (6 preceding siblings ...)
2020-06-25 19:48 ` [PATCH v2 07/11] fast-export: move global "idents" anonymize hashmap into function Jeff King
@ 2020-06-25 19:48 ` Jeff King
2020-06-25 19:48 ` [PATCH v2 09/11] fast-export: allow seeding the anonymized mapping Jeff King
` (3 subsequent siblings)
11 siblings, 0 replies; 64+ messages in thread
From: Jeff King @ 2020-06-25 19:48 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Junio C Hamano, Johannes Schindelin, SZEDER Gábor
The anonymize_str() function takes a generator callback, but there's no
way to pass extra context to it. Let's add the usual "void *data"
parameter to the generator interface and pass it along.
This is mildly annoying for existing callers, all of which pass NULL,
but is necessary to avoid extra globals in some cases we'll add in a
subsequent patch.
While we're touching each of these callbacks, we can further observe
that none of them use the existing orig/len parameters at all. This
makes sense, since the point is for their output to have no discernable
basis in the original (my original version had some notion that we might
use a one-way function to obfuscate the names, but it was never
implemented). So let's drop those extra parameters. If a caller really
wants to do something with them, it can pass a struct through the new
data parameter.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/fast-export.c | 27 ++++++++++++++-------------
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 16a1563e49..1cbca5b4b4 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -155,8 +155,9 @@ static int anonymized_entry_cmp(const void *unused_cmp_data,
* is farmed out to the generate function.
*/
static const char *anonymize_str(struct hashmap *map,
- char *(*generate)(const char *, size_t),
- const char *orig, size_t len)
+ char *(*generate)(void *),
+ const char *orig, size_t len,
+ void *data)
{
struct anonymized_entry_key key;
struct anonymized_entry *ret;
@@ -172,7 +173,7 @@ static const char *anonymize_str(struct hashmap *map,
if (!ret) {
FLEX_ALLOC_MEM(ret, orig, orig, len);
hashmap_entry_init(&ret->hash, key.hash.hash);
- ret->anon = generate(orig, len);
+ ret->anon = generate(data);
hashmap_put(map, &ret->hash);
}
@@ -187,12 +188,12 @@ static const char *anonymize_str(struct hashmap *map,
*/
static void anonymize_path(struct strbuf *out, const char *path,
struct hashmap *map,
- char *(*generate)(const char *, size_t))
+ char *(*generate)(void *))
{
while (*path) {
const char *end_of_component = strchrnul(path, '/');
size_t len = end_of_component - path;
- const char *c = anonymize_str(map, generate, path, len);
+ const char *c = anonymize_str(map, generate, path, len, NULL);
strbuf_addstr(out, c);
path = end_of_component;
if (*path)
@@ -367,7 +368,7 @@ static void print_path_1(const char *path)
printf("%s", path);
}
-static char *anonymize_path_component(const char *path, size_t len)
+static char *anonymize_path_component(void *data)
{
static int counter;
struct strbuf out = STRBUF_INIT;
@@ -389,7 +390,7 @@ static void print_path(const char *path)
}
}
-static char *generate_fake_oid(const char *old, size_t len)
+static char *generate_fake_oid(void *data)
{
static uint32_t counter = 1; /* avoid null oid */
const unsigned hashsz = the_hash_algo->rawsz;
@@ -405,7 +406,7 @@ static const char *anonymize_oid(const char *oid_hex)
{
static struct hashmap objs;
size_t len = strlen(oid_hex);
- return anonymize_str(&objs, generate_fake_oid, oid_hex, len);
+ return anonymize_str(&objs, generate_fake_oid, oid_hex, len, NULL);
}
static void show_filemodify(struct diff_queue_struct *q,
@@ -502,7 +503,7 @@ static const char *find_encoding(const char *begin, const char *end)
return bol;
}
-static char *anonymize_ref_component(const char *old, size_t len)
+static char *anonymize_ref_component(void *data)
{
static int counter;
struct strbuf out = STRBUF_INIT;
@@ -555,7 +556,7 @@ static char *anonymize_commit_message(const char *old)
return xstrfmt("subject %d\n\nbody\n", counter++);
}
-static char *anonymize_ident(const char *old, size_t len)
+static char *anonymize_ident(void *data)
{
static int counter;
struct strbuf out = STRBUF_INIT;
@@ -598,7 +599,7 @@ static void anonymize_ident_line(const char **beg, const char **end)
len = split.mail_end - split.name_begin;
ident = anonymize_str(&idents, anonymize_ident,
- split.name_begin, len);
+ split.name_begin, len, NULL);
strbuf_addstr(out, ident);
strbuf_addch(out, ' ');
strbuf_add(out, split.date_begin, split.tz_end - split.date_begin);
@@ -739,7 +740,7 @@ static void handle_commit(struct commit *commit, struct rev_info *rev,
show_progress();
}
-static char *anonymize_tag(const char *old, size_t len)
+static char *anonymize_tag(void *data)
{
static int counter;
struct strbuf out = STRBUF_INIT;
@@ -815,7 +816,7 @@ static void handle_tag(const char *name, struct tag *tag)
if (message) {
static struct hashmap tags;
message = anonymize_str(&tags, anonymize_tag,
- message, message_size);
+ message, message_size, NULL);
}
}
--
2.27.0.593.gb3082a2aaf
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH v2 09/11] fast-export: allow seeding the anonymized mapping
2020-06-25 19:48 ` [PATCH v2 0/11] " Jeff King
` (7 preceding siblings ...)
2020-06-25 19:48 ` [PATCH v2 08/11] fast-export: add a "data" callback parameter to anonymize_str() Jeff King
@ 2020-06-25 19:48 ` Jeff King
2020-06-25 19:48 ` [PATCH v2 10/11] fast-export: anonymize "master" refname Jeff King
` (2 subsequent siblings)
11 siblings, 0 replies; 64+ messages in thread
From: Jeff King @ 2020-06-25 19:48 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Junio C Hamano, Johannes Schindelin, SZEDER Gábor
After you anonymize a repository, it can be hard to find which commits
correspond between the original and the result, and thus hard to
reproduce commands that triggered bugs in the original.
Let's make it possible to seed the anonymization map. This lets users
either:
- mark names to be retained as-is, if they don't consider them secret
(in which case their original commands would just work)
- map names to new values, which lets them adapt the reproduction
recipe to the new names without revealing the originals
The implementation is fairly straight-forward. We already store each
anonymized token in a hashmap (so that the same token appearing twice is
converted to the same result). We can just introduce a new "seed"
hashmap which is consulted first.
This does make a few more promises to the user about how we'll anonymize
things (e.g., token-splitting pathnames). But it's unlikely that we'd
want to change those rules, even if the actual anonymization of a single
token changes. And it makes things much easier for the user, who can
unblind only a directory name without having to specify each path within
it.
One alternative to this approach would be to anonymize as we see fit,
and then dump the whole refname and pathname mappings to a file. This
does work, but it's a bit awkward to use (you have to manually dig the
items you care about out of the mapping).
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/git-fast-export.txt | 29 ++++++++++++++++++
builtin/fast-export.c | 50 ++++++++++++++++++++++++++++++-
t/t9351-fast-export-anonymize.sh | 11 ++++++-
3 files changed, 88 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
index e8950de3ba..1978dbdc6a 100644
--- a/Documentation/git-fast-export.txt
+++ b/Documentation/git-fast-export.txt
@@ -119,6 +119,11 @@ by keeping the marks the same across runs.
the shape of the history and stored tree. See the section on
`ANONYMIZING` below.
+--anonymize-map=<from>[:<to>]::
+ Convert token `<from>` to `<to>` in the anonymized output. If
+ `<to>` is omitted, map `<from>` to itself (i.e., do not
+ anonymize it). See the section on `ANONYMIZING` below.
+
--reference-excluded-parents::
By default, running a command such as `git fast-export
master~5..master` will not include the commit master{tilde}5
@@ -238,6 +243,30 @@ collapse "User 0", "User 1", etc into "User X"). This produces a much
smaller output, and it is usually easy to quickly confirm that there is
no private data in the stream.
+Reproducing some bugs may require referencing particular commits or
+paths, which becomes challenging after refnames and paths have been
+anonymized. You can ask for a particular token to be left as-is or
+mapped to a new value. For example, if you have a bug which reproduces
+with `git rev-list sensitive -- secret.c`, you can run:
+
+---------------------------------------------------
+$ git fast-export --anonymize --all \
+ --anonymize-map=sensitive:foo \
+ --anonymize-map=secret.c:bar.c \
+ >stream
+---------------------------------------------------
+
+After importing the stream, you can then run `git rev-list foo -- bar.c`
+in the anonymized repository.
+
+Note that paths and refnames are split into tokens at slash boundaries.
+The command above would anonymize `subdir/secret.c` as something like
+`path123/bar.c`; you could then search for `bar.c` in the anonymized
+repository to determine the final pathname.
+
+To make referencing the final pathname simpler, you can map each path
+component; so if you also anonymize `subdir` to `publicdir`, then the
+final pathname would be `publicdir/bar.c`.
LIMITATIONS
-----------
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 1cbca5b4b4..b0b09bca30 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -45,6 +45,7 @@ static struct string_list extra_refs = STRING_LIST_INIT_NODUP;
static struct string_list tag_refs = STRING_LIST_INIT_NODUP;
static struct refspec refspecs = REFSPEC_INIT_FETCH;
static int anonymize;
+static struct hashmap anonymized_seeds;
static struct revision_sources revision_sources;
static int parse_opt_signed_tag_mode(const struct option *opt,
@@ -168,8 +169,18 @@ static const char *anonymize_str(struct hashmap *map,
hashmap_entry_init(&key.hash, memhash(orig, len));
key.orig = orig;
key.orig_len = len;
- ret = hashmap_get_entry(map, &key, hash, &key);
+ /* First check if it's a token the user configured manually... */
+ if (anonymized_seeds.cmpfn)
+ ret = hashmap_get_entry(&anonymized_seeds, &key, hash, &key);
+ else
+ ret = NULL;
+
+ /* ...otherwise check if we've already seen it in this context... */
+ if (!ret)
+ ret = hashmap_get_entry(map, &key, hash, &key);
+
+ /* ...and finally generate a new mapping if necessary */
if (!ret) {
FLEX_ALLOC_MEM(ret, orig, orig, len);
hashmap_entry_init(&ret->hash, key.hash.hash);
@@ -1147,6 +1158,37 @@ static void handle_deletes(void)
}
}
+static char *anonymize_seed(void *data)
+{
+ return xstrdup(data);
+}
+
+static int parse_opt_anonymize_map(const struct option *opt,
+ const char *arg, int unset)
+{
+ struct hashmap *map = opt->value;
+ const char *delim, *value;
+ size_t keylen;
+
+ BUG_ON_OPT_NEG(unset);
+
+ delim = strchr(arg, ':');
+ if (delim) {
+ keylen = delim - arg;
+ value = delim + 1;
+ } else {
+ keylen = strlen(arg);
+ value = arg;
+ }
+
+ if (!keylen || !*value)
+ return error(_("--anonymize-map token cannot be empty"));
+
+ anonymize_str(map, anonymize_seed, arg, keylen, (void *)value);
+
+ return 0;
+}
+
int cmd_fast_export(int argc, const char **argv, const char *prefix)
{
struct rev_info revs;
@@ -1188,6 +1230,9 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
OPT_STRING_LIST(0, "refspec", &refspecs_list, N_("refspec"),
N_("Apply refspec to exported refs")),
OPT_BOOL(0, "anonymize", &anonymize, N_("anonymize output")),
+ OPT_CALLBACK_F(0, "anonymize-map", &anonymized_seeds, N_("from:to"),
+ N_("convert <from> to <to> in anonymized output"),
+ PARSE_OPT_NONEG, parse_opt_anonymize_map),
OPT_BOOL(0, "reference-excluded-parents",
&reference_excluded_commits, N_("Reference parents which are not in fast-export stream by object id")),
OPT_BOOL(0, "show-original-ids", &show_original_ids,
@@ -1215,6 +1260,9 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
if (argc > 1)
usage_with_options (fast_export_usage, options);
+ if (anonymized_seeds.cmpfn && !anonymize)
+ die(_("--anonymize-map without --anonymize does not make sense"));
+
if (refspecs_list.nr) {
int i;
diff --git a/t/t9351-fast-export-anonymize.sh b/t/t9351-fast-export-anonymize.sh
index dc5d75cd19..5a21c71568 100755
--- a/t/t9351-fast-export-anonymize.sh
+++ b/t/t9351-fast-export-anonymize.sh
@@ -6,6 +6,7 @@ test_description='basic tests for fast-export --anonymize'
test_expect_success 'setup simple repo' '
test_commit base &&
test_commit foo &&
+ test_commit retain-me &&
git checkout -b other HEAD^ &&
mkdir subdir &&
test_commit subdir/bar &&
@@ -18,7 +19,10 @@ test_expect_success 'setup simple repo' '
'
test_expect_success 'export anonymized stream' '
- git fast-export --anonymize --all >stream
+ git fast-export --anonymize --all \
+ --anonymize-map=retain-me \
+ --anonymize-map=xyzzy:custom-name \
+ >stream
'
# this also covers commit messages
@@ -30,6 +34,11 @@ test_expect_success 'stream omits path names' '
! grep xyzzy stream
'
+test_expect_success 'stream contains user-specified names' '
+ grep retain-me stream &&
+ grep custom-name stream
+'
+
test_expect_success 'stream omits gitlink oids' '
# avoid relying on the whole oid to remain hash-agnostic; this is
# plenty to be unique within our test case
--
2.27.0.593.gb3082a2aaf
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH v2 10/11] fast-export: anonymize "master" refname
2020-06-25 19:48 ` [PATCH v2 0/11] " Jeff King
` (8 preceding siblings ...)
2020-06-25 19:48 ` [PATCH v2 09/11] fast-export: allow seeding the anonymized mapping Jeff King
@ 2020-06-25 19:48 ` Jeff King
2020-06-25 19:48 ` [PATCH v2 11/11] fast-export: use local array to store anonymized oid Jeff King
2020-06-25 21:22 ` [PATCH v2 0/11] fast-export: allow seeding the anonymized mapping Junio C Hamano
11 siblings, 0 replies; 64+ messages in thread
From: Jeff King @ 2020-06-25 19:48 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Junio C Hamano, Johannes Schindelin, SZEDER Gábor
Running "fast-export --anonymize" will leave "refs/heads/master"
untouched in the output, for two reasons:
- it helped to have some known reference point between the original
and anonymized repository
- since it's historically the default branch name, it doesn't leak any
information
Now that we can ask fast-export to retain particular tokens, we have a
much better tool for the first one (because it works for any ref, not
just master).
For the second, the notion of "default branch name" is likely to become
configurable soon, at which point the name _does_ leak information.
Let's drop this special case in preparation.
Note that we have to adjust the test a bit, since it relied on using the
name "master" in the anonymized repos. We could just use
--anonymize-map=master to keep the same output, but then we wouldn't
know if it works because of our hard-coded master or because of the
explicit map.
So let's flip the test a bit, and confirm that we anonymize "master",
but keep "other" in the output.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/fast-export.c | 7 -------
t/t9351-fast-export-anonymize.sh | 12 +++++++-----
2 files changed, 7 insertions(+), 12 deletions(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index b0b09bca30..c6ecf404d7 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -538,13 +538,6 @@ static const char *anonymize_refname(const char *refname)
static struct strbuf anon = STRBUF_INIT;
int i;
- /*
- * We also leave "master" as a special case, since it does not reveal
- * anything interesting.
- */
- if (!strcmp(refname, "refs/heads/master"))
- return refname;
-
strbuf_reset(&anon);
for (i = 0; i < ARRAY_SIZE(prefixes); i++) {
if (skip_prefix(refname, prefixes[i], &refname)) {
diff --git a/t/t9351-fast-export-anonymize.sh b/t/t9351-fast-export-anonymize.sh
index 5a21c71568..5ac2c3b5ee 100755
--- a/t/t9351-fast-export-anonymize.sh
+++ b/t/t9351-fast-export-anonymize.sh
@@ -22,6 +22,7 @@ test_expect_success 'export anonymized stream' '
git fast-export --anonymize --all \
--anonymize-map=retain-me \
--anonymize-map=xyzzy:custom-name \
+ --anonymize-map=other \
>stream
'
@@ -45,12 +46,12 @@ test_expect_success 'stream omits gitlink oids' '
! grep a000000000000000000 stream
'
-test_expect_success 'stream allows master as refname' '
- grep master stream
+test_expect_success 'stream retains other as refname' '
+ grep other stream
'
test_expect_success 'stream omits other refnames' '
- ! grep other stream &&
+ ! grep master stream &&
! grep mytag stream
'
@@ -76,15 +77,16 @@ test_expect_success 'import stream to new repository' '
test_expect_success 'result has two branches' '
git for-each-ref --format="%(refname)" refs/heads >branches &&
test_line_count = 2 branches &&
- other_branch=$(grep -v refs/heads/master branches)
+ other_branch=refs/heads/other &&
+ main_branch=$(grep -v $other_branch branches)
'
test_expect_success 'repo has original shape and timestamps' '
shape () {
git log --format="%m %ct" --left-right --boundary "$@"
} &&
(cd .. && shape master...other) >expect &&
- shape master...$other_branch >actual &&
+ shape $main_branch...$other_branch >actual &&
test_cmp expect actual
'
--
2.27.0.593.gb3082a2aaf
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH v2 11/11] fast-export: use local array to store anonymized oid
2020-06-25 19:48 ` [PATCH v2 0/11] " Jeff King
` (9 preceding siblings ...)
2020-06-25 19:48 ` [PATCH v2 10/11] fast-export: anonymize "master" refname Jeff King
@ 2020-06-25 19:48 ` Jeff King
2020-06-25 21:22 ` [PATCH v2 0/11] fast-export: allow seeding the anonymized mapping Junio C Hamano
11 siblings, 0 replies; 64+ messages in thread
From: Jeff King @ 2020-06-25 19:48 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Junio C Hamano, Johannes Schindelin, SZEDER Gábor
Some older versions of gcc complain about this line:
builtin/fast-export.c:412:2: error: dereferencing type-punned pointer
will break strict-aliasing rules [-Werror=strict-aliasing]
put_be32(oid.hash + hashsz - 4, counter++);
^
This seems to be a false positive, as there's no type-punning at all
here. oid.hash is an array of unsigned char; when we pass it to a
function it decays to a pointer to unsigned char. We do take a void
pointer in put_be32(), but it's immediately aliased with another pointer
to unsigned char (and clearly the compiler is looking inside the inlined
put_be32(), since the warning doesn't happen with -O0).
This happens on gcc 4.8 and 4.9, but not later versions (I tested gcc 6,
7, 8, and 9).
We can work around it by using a local array instead of an object_id
struct. This is a little more intimate with the details of object_id,
but for whatever reason doesn't seem to trigger the compiler warning.
We can revert this patch once we decide that those gcc versions are too
old to care about for a warning like this (gcc 4.8 is the default
compiler for Ubuntu Trusty, which is out-of-support but not fully
end-of-life'd until April 2022).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/fast-export.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index c6ecf404d7..9f37895d4c 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -405,12 +405,12 @@ static char *generate_fake_oid(void *data)
{
static uint32_t counter = 1; /* avoid null oid */
const unsigned hashsz = the_hash_algo->rawsz;
- struct object_id oid;
+ unsigned char out[GIT_MAX_RAWSZ];
char *hex = xmallocz(GIT_MAX_HEXSZ);
- oidclr(&oid);
- put_be32(oid.hash + hashsz - 4, counter++);
- return oid_to_hex_r(hex, &oid);
+ hashclr(out);
+ put_be32(out + hashsz - 4, counter++);
+ return hash_to_hex_algop_r(hex, out, the_hash_algo);
}
static const char *anonymize_oid(const char *oid_hex)
--
2.27.0.593.gb3082a2aaf
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [PATCH v2 0/11] fast-export: allow seeding the anonymized mapping
2020-06-25 19:48 ` [PATCH v2 0/11] " Jeff King
` (10 preceding siblings ...)
2020-06-25 19:48 ` [PATCH v2 11/11] fast-export: use local array to store anonymized oid Jeff King
@ 2020-06-25 21:22 ` Junio C Hamano
11 siblings, 0 replies; 64+ messages in thread
From: Junio C Hamano @ 2020-06-25 21:22 UTC (permalink / raw)
To: Jeff King; +Cc: git, Eric Sunshine, Johannes Schindelin, SZEDER Gábor
Jeff King <peff@peff.net> writes:
> [09/11]: fast-export: allow seeding the anonymized mapping
>
> - the option is now called "--anonymize-map", and the word "seed"
> isn't used in any user-facing documentation
>
> - incorporated Eric's documentation suggestions
>
> - --anonymize-map without --anonymize now triggers an error
The changes compared to the v1 here looked sensible.
> [10/11]: fast-export: anonymize "master" refname
>
> - minor adjustments to handle change of option name
>
> [11/11]: fast-export: use local array to store anonymized oid
>
> - new in this iteration; this address the gcc-4 warning that Gábor
> mentioned. I prepared it on top since I think we'd eventually want
> to revert it once we decide that compiler is too old (and I'd be
> perfectly fine to declare that it's so now, and just never apply
> it at all).
Sounds like a sensible way.
Let's merge it to 'next' soonish.
Thanks, all.
^ permalink raw reply [flat|nested] 64+ messages in thread