Re: [PATCH v1 2/2] convert.c: stream and early out
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 10, 2016, 20:19 UTC
- Message-ID
- <xmqqd1j8dkjj.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20161009095654.1964-1-tboegi@web.de>
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?).
Show 38 quoted lines
> 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.
Show 6 quoted lines
> @@ -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.
Show 6 quoted lines
> } > > /* 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?
Show 13 quoted lines
> +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.
Show 15 quoted lines
> @@ -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?