git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "William Baker via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, williamtbakeremail@gmail.com,
	stolee@gmail.com, Johannes.Schindelin@gmx.de,
	jeffhost@microsoft.com,
	William Baker <William.Baker@microsoft.com>
Subject: Re: [PATCH 1/1] fsmonitor: don't fill bitmap with entries to be removed
Date: Fri, 04 Oct 2019 08:36:34 +0900
Message-ID: <xmqq7e5l9zb1.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <ce9bf4237e69fcaf2b3e8b50bb88ff61c3b0f710.1570132194.git.gitgitgadget@gmail.com> (William Baker via GitGitGadget's message of "Thu, 03 Oct 2019 12:49:56 -0700 (PDT)")

"William Baker via GitGitGadget" <gitgitgadget@gmail.com> writes:

>  create mode 100755 t/t7519/fsmonitor-env
> ...
> +	if (pos >= istate->cache_nr)
> +		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" >= %"PRIuMAX")",
> +		    (uintmax_t)pos, (uintmax_t)istate->cache_nr);

This is how we show size_t values without using "%z" that we avoid,
but are "pos" and 'cache_nr" size_t or ssize_t?  I thought they are
plain boring unsigned, so shouldn't we use the plain boring "%u"
without casting?

The same comment applies to other uses of uintmax_t cast in this
patch.

>  void fill_fsmonitor_bitmap(struct index_state *istate)
>  {
> -	unsigned int i;
> +	unsigned int i, skipped = 0;
>  	istate->fsmonitor_dirty = ewah_new();
> -	for (i = 0; i < istate->cache_nr; i++)
> -		if (!(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID))
> -			ewah_set(istate->fsmonitor_dirty, i);
> +	for (i = 0; i < istate->cache_nr; i++) {
> +		if (istate->cache[i]->ce_flags & CE_REMOVE)
> +			skipped++;
> +		else if (!(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID))
> +			ewah_set(istate->fsmonitor_dirty, i - skipped);
> +	}
>  }

Matches the explanation in the proposed log message pretty well.
Good job.

> @@ -354,4 +354,16 @@ test_expect_success 'discard_index() also discards fsmonitor info' '
>  	test_cmp expect actual
>  '
>  
> +# Use test files that start with 'z' so that the entries being added
> +# and removed appear at the end of the index.

In other words, future developers are warned against adding entries
to and leaving them in the index that sort later than z100 in new
tests they add before this point.  Is the above wording clear enough
to tell them that, I wonder?

> +test_expect_success 'status succeeds after staging/unstaging ' '
> +	test_commit initial &&
> +	removed=$(test_seq 1 100 | sed "s/^/z/") &&

Thanks.

  reply index

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-03 19:49 [PATCH 0/1] " William Baker via GitGitGadget
2019-10-03 19:49 ` [PATCH 1/1] " William Baker via GitGitGadget
2019-10-03 23:36   ` Junio C Hamano [this message]
2019-10-07 18:10     ` William Baker
2019-10-03 21:08 ` [PATCH 0/1] " Johannes Schindelin
2019-10-03 23:17   ` Junio C Hamano
2019-10-09 21:00 ` [PATCH v2 " William Baker via GitGitGadget
2019-10-09 21:00   ` [PATCH v2 1/1] " William Baker via GitGitGadget
2019-10-10 11:07     ` SZEDER Gábor
2019-10-10 11:22       ` SZEDER Gábor
2019-10-11 16:38         ` William Baker
2019-10-10  1:56   ` [PATCH v2 0/1] " Junio C Hamano
2019-10-10 12:02     ` Johannes Schindelin
2019-10-11 20:11   ` [PATCH v3 " William Baker via GitGitGadget
2019-10-11 20:11     ` [PATCH v3 1/1] " William Baker via GitGitGadget
2019-10-12  1:26       ` Junio C Hamano
2019-10-15 19:07         ` William Baker

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq7e5l9zb1.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=William.Baker@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=jeffhost@microsoft.com \
    --cc=stolee@gmail.com \
    --cc=williamtbakeremail@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git