Re: [PATCH v9 3/5] dir_iterator: add helpers to dir_iterator_advance

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 18, 2017, 04:23 UTC
Message-ID: <xmqq8tmyfio6.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1492462296-4990-4-git-send-email-bnmvco@gmail.com>

Daniel Ferreira <bnmvco@gmail.com> writes:

Show 5 quoted lines

> Create helpers to dir_iterator_advance(). Make dir_iterator_advance()'s
> code more legible and allow some behavior to be reusable.
>
> Signed-off-by: Daniel Ferreira <bnmvco@gmail.com>
> ---

This is the kind of change we typically call "refactoring" --- factoring out reusable helpers out of existing code, without changing the behaviour of the code at all.

> +static int set_iterator_data(struct dir_iterator_int *iter, struct dir_iterator_level *level)
> +{

If you are going to fold line in a patch that immediately follows this one, do so from the get-go here.

I am not happy with the name of the function in that "set data" does not convey as much information as it spends words.

Conceptually, this is called once per each new directory entry readdir() returns, and the most important "iterator data" that is set to iter structure per loop is to update iter->base.path with de->d_name; everything else is secondary and derived data from that. And from that point of view, I have trouble with this function not getting "de" passed to it, but the most important setting is instead done by its caller.

If the function were called "adjust_iterator_data()", I wouldn't find this function so troubling, I guess; after the caller sets the iter.base.path, all the secondary data that can be derived from it that are used by the caller of "advance" are set by this function.

Show 21 quoted lines

> +	if (lstat(iter->base.path.buf, &iter->base.st) < 0) {
> +		if (errno != ENOENT)
> +			warning("error reading path '%s': %s",
> +				iter->base.path.buf,
> +				strerror(errno));
> +		return -1;
> +	}
> +
> +	/*
> +	 * We have to set these each time because
> +	 * the path strbuf might have been realloc()ed.
> +	 */
> +	iter->base.relative_path =
> +		iter->base.path.buf + iter->levels[0].prefix_len;
> +	iter->base.basename =
> +		iter->base.path.buf + level->prefix_len;
> +	level->dir_state = DIR_STATE_ITER;
> +
> +	return 0;
> +}
> +