[v7,26/30] t/helper/hexdump: add helper to print hexdump of stdin

Commit Message

Jeff Hostetler May 23, 2022, 8:12 p.m. UTC

From: Jeff Hostetler <[e-mail]>

Co-authored-by: Johannes Schindelin <[e-mail]>
Signed-off-by: Johannes Schindelin <[e-mail]>
Signed-off-by: Jeff Hostetler <[e-mail]>
---
 Makefile                |  1 +
 t/helper/test-hexdump.c | 24 ++++++++++++++++++++++++
 t/helper/test-tool.c    |  1 +
 t/helper/test-tool.h    |  1 +
 4 files changed, 27 insertions(+)
 create mode 100644 t/helper/test-hexdump.c

Comments

Junio C Hamano May 23, 2022, 9:19 p.m. UTC | #1

"Jeff Hostetler via GitGitGadget" <[e-mail]> writes:

> +int cmd__hexdump(int argc, const char **argv)
> +{
> +	char buf[1024];
> +	ssize_t i, len;
> +
> +	for (;;) {
> +		len = xread(0, buf, sizeof(buf));
> +		if (len < 0)
> +			die_errno("failure reading stdin");
> +		if (!len)
> +			break;
> +
> +		for (i = 0; i < len; i++)
> +			printf("%02x ", (unsigned char)buf[i]);
> +	}
> +
> +	return 0;
> +}

It is meant to be consumed by machine, so I do not think we would
mind too much about a single long line, but given that consumers
include "grep", it would probably be better to avoid emitting an
incomplete line, especially since addition of this tool is all about
portability across platforms.

An extra putchar('\n'); after the loop would fix it easily.

Thanks.

> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
> index 0424f7adf5d..88c4b28cdfa 100644
> --- a/t/helper/test-tool.c
> +++ b/t/helper/test-tool.c
> @@ -38,6 +38,7 @@ static struct test_cmd cmds[] = {
>  	{ "getcwd", cmd__getcwd },
>  	{ "hashmap", cmd__hashmap },
>  	{ "hash-speed", cmd__hash_speed },
> +	{ "hexdump", cmd__hexdump },
>  	{ "index-version", cmd__index_version },
>  	{ "json-writer", cmd__json_writer },
>  	{ "lazy-init-name-hash", cmd__lazy_init_name_hash },
> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
> index c876e8246fb..511f6251bf5 100644
> --- a/t/helper/test-tool.h
> +++ b/t/helper/test-tool.h
> @@ -29,6 +29,7 @@ int cmd__genzeros(int argc, const char **argv);
>  int cmd__getcwd(int argc, const char **argv);
>  int cmd__hashmap(int argc, const char **argv);
>  int cmd__hash_speed(int argc, const char **argv);
> +int cmd__hexdump(int argc, const char **argv);
>  int cmd__index_version(int argc, const char **argv);
>  int cmd__json_writer(int argc, const char **argv);
>  int cmd__lazy_init_name_hash(int argc, const char **argv);

Johannes Schindelin May 24, 2022, 12:16 p.m. UTC | #2

Hi Junio,

On Mon, 23 May 2022, Junio C Hamano wrote:

> "Jeff Hostetler via GitGitGadget" <[e-mail]> writes:
>
> > +int cmd__hexdump(int argc, const char **argv)
> > +{
> > +	char buf[1024];
> > +	ssize_t i, len;
> > +
> > +	for (;;) {
> > +		len = xread(0, buf, sizeof(buf));
> > +		if (len < 0)
> > +			die_errno("failure reading stdin");
> > +		if (!len)
> > +			break;
> > +
> > +		for (i = 0; i < len; i++)
> > +			printf("%02x ", (unsigned char)buf[i]);
> > +	}
> > +
> > +	return 0;
> > +}
>
> It is meant to be consumed by machine, so I do not think we would
> mind too much about a single long line, but given that consumers
> include "grep", it would probably be better to avoid emitting an
> incomplete line, especially since addition of this tool is all about
> portability across platforms.

Do you know of any `grep` implementation that has problems with text
missing the usual trailing newlines?

Ciao,
Dscho

Jeff Hostetler May 24, 2022, 2:44 p.m. UTC | #3

On 5/23/22 5:19 PM, Junio C Hamano wrote:
> "Jeff Hostetler via GitGitGadget" <[e-mail]> writes:
> 
>> +int cmd__hexdump(int argc, const char **argv)
>> +{
>> +	char buf[1024];
>> +	ssize_t i, len;
>> +
>> +	for (;;) {
>> +		len = xread(0, buf, sizeof(buf));
>> +		if (len < 0)
>> +			die_errno("failure reading stdin");
>> +		if (!len)
>> +			break;
>> +
>> +		for (i = 0; i < len; i++)
>> +			printf("%02x ", (unsigned char)buf[i]);
>> +	}
>> +
>> +	return 0;
>> +}
> 
> It is meant to be consumed by machine, so I do not think we would
> mind too much about a single long line, but given that consumers
> include "grep", it would probably be better to avoid emitting an
> incomplete line, especially since addition of this tool is all about
> portability across platforms.
> 
> An extra putchar('\n'); after the loop would fix it easily.

Yes, I should have added a final LF.  I was more focused
on cleaning up the test cases.

Would you prefer a send a V8 or would you be willing
to push a fixup commit on top?

Jeff

Junio C Hamano May 24, 2022, 7:52 p.m. UTC | #4

Johannes Schindelin <[e-mail]> writes:

> Do you know of any `grep` implementation that has problems with text
> missing the usual trailing newlines?

I recall that we had reports on BSD variants, but please do not
quote me on that.  Perhaps all the BSD variants are good now, or
perhaps some aren't.

In any case, I do not take the "if it works on Windows and Linux, we
do not care about the rest of the world" world view, so finding the
answer to that question unfortunately does not give much input to
the issue in either way.

And in this particular case, it is much simpler to mak sure that the
file does not end in an incomplete line than us exchanging e-mails
back and forth, so that would be the most economical solution I
would prefer.

Thanks.

Junio C Hamano May 24, 2022, 7:54 p.m. UTC | #5

Jeff Hostetler <[e-mail]> writes:

> Would you prefer a send a V8 or would you be willing
> to push a fixup commit on top?

If "test-tool hexgrep" does not turn out to be a better solution,
and if there is no other changes needed, then I do not mind locally
amending, but I'd rather avoid touching a middle step in multi dozen
patch series myself if I can.

Thanks.

Johannes Schindelin May 25, 2022, 10:21 a.m. UTC | #6

Hi Junio,

On Tue, 24 May 2022, Junio C Hamano wrote:

> Johannes Schindelin <[e-mail]> writes:
>
> > Do you know of any `grep` implementation that has problems with text
> > missing the usual trailing newlines?
>
> I recall that we had reports on BSD variants, but please do not
> quote me on that.  Perhaps all the BSD variants are good now, or
> perhaps some aren't.

I did not recall any such reports, but take your word for it.

> In any case, I do not take the "if it works on Windows and Linux, we
> do not care about the rest of the world" world view,

Just in case it was unclear to you: we're on the same page here.

> so finding the answer to that question unfortunately does not give much
> input to the issue in either way.
>
> And in this particular case, it is much simpler to mak sure that the
> file does not end in an incomplete line than us exchanging e-mails
> back and forth, so that would be the most economical solution I
> would prefer.

Indeed, the trailing newline is easily added and fixes a known issue, so
I'm all for it.

Ciao,
Dscho

Jeff Hostetler May 25, 2022, 1:45 p.m. UTC | #7

On 5/24/22 3:54 PM, Junio C Hamano wrote:
> Jeff Hostetler <[e-mail]> writes:
> 
>> Would you prefer a send a V8 or would you be willing
>> to push a fixup commit on top?
> 
> If "test-tool hexgrep" does not turn out to be a better solution,
> and if there is no other changes needed, then I do not mind locally
> amending, but I'd rather avoid touching a middle step in multi dozen
> patch series myself if I can.
> 
> Thanks.
> 

That's fine. I'll fix it and send V8.
Thanks
Jeff

Patch

diff --git a/Makefile b/Makefile
index 5f1623baadd..5afa194aac6 100644
--- a/Makefile
+++ b/Makefile
@@ -729,6 +729,7 @@  TEST_BUILTINS_OBJS += test-getcwd.o
 TEST_BUILTINS_OBJS += test-hash-speed.o
 TEST_BUILTINS_OBJS += test-hash.o
 TEST_BUILTINS_OBJS += test-hashmap.o
+TEST_BUILTINS_OBJS += test-hexdump.o
 TEST_BUILTINS_OBJS += test-index-version.o
 TEST_BUILTINS_OBJS += test-json-writer.o
 TEST_BUILTINS_OBJS += test-lazy-init-name-hash.o
diff --git a/t/helper/test-hexdump.c b/t/helper/test-hexdump.c
new file mode 100644
index 00000000000..f1e0a0fabf3
--- /dev/null
+++ b/t/helper/test-hexdump.c
@@ -0,0 +1,24 @@ 
+#include "test-tool.h"
+#include "git-compat-util.h"
+
+/*
+ * Read stdin and print a hexdump to stdout.
+ */
+int cmd__hexdump(int argc, const char **argv)
+{
+	char buf[1024];
+	ssize_t i, len;
+
+	for (;;) {
+		len = xread(0, buf, sizeof(buf));
+		if (len < 0)
+			die_errno("failure reading stdin");
+		if (!len)
+			break;
+
+		for (i = 0; i < len; i++)
+			printf("%02x ", (unsigned char)buf[i]);
+	}
+
+	return 0;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 0424f7adf5d..88c4b28cdfa 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -38,6 +38,7 @@  static struct test_cmd cmds[] = {
 	{ "getcwd", cmd__getcwd },
 	{ "hashmap", cmd__hashmap },
 	{ "hash-speed", cmd__hash_speed },
+	{ "hexdump", cmd__hexdump },
 	{ "index-version", cmd__index_version },
 	{ "json-writer", cmd__json_writer },
 	{ "lazy-init-name-hash", cmd__lazy_init_name_hash },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index c876e8246fb..511f6251bf5 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -29,6 +29,7 @@  int cmd__genzeros(int argc, const char **argv);
 int cmd__getcwd(int argc, const char **argv);
 int cmd__hashmap(int argc, const char **argv);
 int cmd__hash_speed(int argc, const char **argv);
+int cmd__hexdump(int argc, const char **argv);
 int cmd__index_version(int argc, const char **argv);
 int cmd__json_writer(int argc, const char **argv);
 int cmd__lazy_init_name_hash(int argc, const char **argv);