Re: [PATCH v1 2/2] convert.c: stream and early out
From: Junio C Hamano <hidden>
Date: 2016-10-10 20:19:22
tboegi@web.de writes:
-static void gather_stats(const char *buf, unsigned long size, struct text_stat *stats)
+static void gather_stats_partly(const char *buf, unsigned long len,
+ struct text_stat *stats, unsigned earlyout)
{I think it is OK not to rename the function (you'd be passing earlyout=0 for callers that want exact stat, right?).
quoted hunk
unsigned long i; - memset(stats, 0, sizeof(*stats)); - - for (i = 0; i < size; i++) { + if (!buf || !len) + return; + for (i = 0; i < len; i++) { unsigned char c = buf[i]; if (c == '\r') { - if (i+1 < size && buf[i+1] == '\n') { + stats->stat_bits |= CONVERT_STAT_BITS_ANY_CR; + if (i+1 < len && buf[i+1] == '\n') { stats->crlf++; i++; - } else + stats->stat_bits |= CONVERT_STAT_BITS_TXT_CRLF; + } else { stats->lonecr++; + stats->stat_bits |= CONVERT_STAT_BITS_BIN; + } continue; } if (c == '\n') { stats->lonelf++; + stats->stat_bits |= CONVERT_STAT_BITS_TXT_LF; continue; } if (c == 127)@@ -67,7 +74,7 @@ static void gather_stats(const char *buf, unsigned long size, struct text_stat * stats->printable++; break; case 0: - stats->nul++; + stats->stat_bits |= CONVERT_STAT_BITS_BIN; /* fall through */ default: stats->nonprintable++;
So depending on the distribution of the bytes in the file, the bitfields in stats->stat_bits will be filled one bit at a time in random order.
quoted hunk
@@ -75,10 +82,12 @@ static void gather_stats(const char *buf, unsigned long size, struct text_stat * } else stats->printable++; + if (stats->stat_bits & earlyout) + break; /* We found what we have been searching for */
But an "earlyout" says that if "any" of the earlyout bit is seen, we can return. It somehow felt a bit too limited to me in my initial reading, but I guess I shouldn't be surprised to see that such a limited interface is sufficient for a file-local helper function ;-). The only caller that the semantics of this exit condition matters is the one that wants to know "do we have NUL or CR anywhere?", so I guess this should be sufficient.
} /* If file ends with EOF then don't count this EOF as non-printable. */ - if (size >= 1 && buf[size-1] == '\032') + if (len >= 1 && buf[len-1] == '\032') stats->nonprintable--;
This noise is somewhat irritating. Was there a reason why size was a bad name for the variable?
+static const char *convert_stats_ascii(unsigned convert_stats)
{
- unsigned int convert_stats = gather_convert_stats(data, size);
-
+ const unsigned mask = CONVERT_STAT_BITS_TXT_LF |
+ CONVERT_STAT_BITS_TXT_CRLF;
if (convert_stats & CONVERT_STAT_BITS_BIN)
return "-text";
- switch (convert_stats) {
+ switch (convert_stats & mask) {
case CONVERT_STAT_BITS_TXT_LF:
return "lf";
case CONVERT_STAT_BITS_TXT_CRLF:Subtle. The caller runs the stat colllection with early-out set to BITS_BIN, so that this can set "-text" early. It knows that without BITS_BIN, the stat was taken for the whole contents and the check lf or crlf can be reliable. I wonder if we can/need to do something to remove this subtleness out of this callchain, which could be a source of confusion.
quoted hunk
@@ -132,24 +162,45 @@ static const char *gather_convert_stats_ascii(const char *data, unsigned long si } } +static unsigned get_convert_stats_wt(const char *path) +{ + struct text_stat stats; + unsigned earlyout = CONVERT_STAT_BITS_BIN; + int fd; + memset(&stats, 0, sizeof(stats)); + fd = open(path, O_RDONLY); + if (fd < 0) + return 0; + for (;;) { + char buf[2*1024];
Where is this 2kB come from? Out of thin air?