Stefan Sperling <stsp@stsp.name> wrote:
> Make 'got status' compare the file size recorded in the file index to
> the size of the on-disk file, before running a full content comparison.
> Skip the full content comparison loop if the sizes differ.
> 
> Speeds up 'got status' in work trees that contain large modified files.
> This speed-up is not very significant because we still compute diffs in
> order to detect conflicts (by looking for conflict markers on newly added
> lines). Which means large files will still be read anyway in order to
> compare them to their base blob. But at least this avoids reading such
> files twice in a row.
> 
> More interestingly, this change exposes bugs in 'got revert' which manifest
> as test failures.
> One bug where the size recorded in the file index is not updated during
> revert, and another where a wrong base commit is recorded in the file
> index entry during revert. Fix those bugs, too.
> To see the relevant test failures run 'make regress' without the changes
> to the revert_file() function below.

Nice find! I observed the breakage in the test_update_binary_file and
test_update_file_skipped_due_to_conflict regression tests with the
revert_file() hunks omitted; and applying those does indeed fix it.

It'd be good if we could elide the conflict marker detection, too, but
bypassing the full content comparison when we know the file size is
different is a nice little tweak :)

> ok?

ok

> M  lib/worktree.c  |  25+  9-
> 
> 1 file changed, 25 insertions(+), 9 deletions(-)
> 
> commit - 34b2702155c3b2f22d92d76d2f807e818ad5789f
> commit + f6dff504a5361205afd9d92e015dd00dd8b8da60
> blob - df89f493b283c3c8957846b5f60994c98756d8ae
> blob + 71a215359b98caf08ed6fafa8841fdd8dbd4caac
> --- lib/worktree.c
> +++ lib/worktree.c
> @@ -1839,8 +1839,22 @@ get_file_status(unsigned char *status, struct stat *sb
>  		goto done;
>  	}
>  	fd = -1;
> +
> +	if (staged_status == GOT_STATUS_NO_CHANGE &&
> +	    S_ISREG(sb->st_mode) &&
> +	    got_fileindex_entry_filetype_get(ie) ==
> +	    GOT_FILEIDX_MODE_REGULAR_FILE &&
> +	    ie->size != (sb->st_size & 0xffffffff)) {
> +		/*
> +		 * The size of regular files differs. We can skip the full
> +		 * content comparison loop below but still need to check
> +		 * for conflict markers.
> +		 */
> +		*status = GOT_STATUS_MODIFY;
> +	}
> +
>  	hdrlen = got_object_blob_get_hdrlen(blob);
> -	for (;;) {
> +	while (*status == GOT_STATUS_NO_CHANGE) {
>  		const uint8_t *bbuf = got_object_blob_get_read_buf(blob);
>  		err = got_object_blob_read_block(&blen, blob);
>  		if (err)
> @@ -5355,6 +5369,11 @@ revert_file(void *arg, unsigned char status, unsigned 
>  					goto done;
>  				}
>  			}
> +			err = got_fileindex_entry_update(ie,
> +			    a->worktree->root_fd, relpath,
> +			    &blob->id, &ie->commit, 0);
> +			if (err)
> +				goto done;
>  		} else {
>  			int is_bad_symlink = 0;
>  			if (te && S_ISLNK(te->mode)) {
> @@ -5372,14 +5391,11 @@ revert_file(void *arg, unsigned char status, unsigned 
>  			}
>  			if (err)
>  				goto done;
> -			if (status == GOT_STATUS_DELETE ||
> -			    status == GOT_STATUS_MODE_CHANGE) {
> -				err = got_fileindex_entry_update(ie,
> -				    a->worktree->root_fd, relpath,
> -				    &blob->id, a->worktree->base_commit_id, 1);
> -				if (err)
> -					goto done;
> -			}
> +			err = got_fileindex_entry_update(ie,
> +			    a->worktree->root_fd, relpath,
> +			    &blob->id, &ie->commit, 0);
> +			if (err)
> +				goto done;
>  			if (is_bad_symlink) {
>  				got_fileindex_entry_filetype_set(ie,
>  				    GOT_FILEIDX_MODE_BAD_SYMLINK);


-- 
Mark Jamsek <https://bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68