GeezerVison

Why is the name relative_fpath better than rela_file? Why was this chosen instead of relative_path? If the f in fpath is unimportant, then it should be removed. If it is important, then it should be spelled out. If explicitness is not required, then presumably rela_file is also okay, in which case it should not be changed just to make the spell checker happy.

This applies to most occurrences of relative_fpath, including in other files.

My guess is that this should be relative_path. The nonpublic _items_to_rela_paths method was renamed to _items_to_relative_paths. Assuming that change is good, which I think it is, it seems like relative_fpath should just be relative_path.

rela was marked as typo so I found easier to use full name without affecting API

I don't think relative_fpath is a full name. It looks like a typo that is meant to be relative_path without the f.

Maybe it is not a typo. Maybe f is an abbreviation for something that should be spelled out (if important) or omitted (if unimportant).

The key point is that I do not know what relative_fpath means in the places where this PR has introduced it, and I have not been able to figure that out. (I have been able to guess that the f stands for "file," but I am not certain of this, and without knowing the old variable name rela_file, I would likely not even have been able to guess this.) I expect that other current or future readers may also not know what it means.

I recommend changing it, probably to relative_path.

This change may seem at first glance to be obviously correct, but I think it actually may be wrong, and that if it is to be kept then it requires a specific technical justification.

I think uptodate is a specific technical term in Git. In the Git source code, it often appears capitalized, but it also appears lower-case in multiple places, which also seems to be intentional. As one example, in fetch.c:

		/* uptodate lines are only shown on high verbosity level */
		if (verbosity <= 0 && oideq(&ref->peer_ref->old_oid, &ref->old_oid))
			continue;

It seems like that specific technical meaning is the one relevant here. If this has been verified not to be the case, then the change here is okay. Otherwise, either the change should be undone and uptodate added as a correct spelling, or it should be investigated.

Although this feels minor, making technical terms harder to search for can accumulate and make a codebase difficult to work with. That is both potentially relevant to this specific change, and a potential risk of automated spell-checking.

I think in this case making it a command associate would be better

I don't think any of the files in fixtures that represent test input or expected output should be spell-checked. I think the *.py files in fixtures, which are actually run as code, should be spell-checked, and that other files should not.

It seems to me that the question to ask is, if code appearing inside a fixture were found to have a logic error, should that bug be fixed? A number of fixture files have Ruby code or diffs thereof, but these are just test data. If logic errors in that code (which isn't run) shouldn't be fixed, then either the same files should not be spell-checked, or the justification for spell-checking them should be made clear. The issues with these kinds of changes are:

1. Churn in test data may make it so that changes to test data that are actually done to improve the tests are hard to identify.
2. Changes in test data need to be reviewed to evaluate whether they could have any impact on the tests. It is possible, in general, for a change to test data to keep a test passing, while preventing it from catching regressions that it would have caught before the test data changed.

The first concern is minor and may well be overcome by the slight readability improvement of avoiding typos. The second concern is less minor and it seems to me that this is not worth the risk, even if small. Tests can assert things that are affected by the presence or absence of specific strings or that involve specific lengths.

This also applies to all changes in test/fixutres/diff_mode_only. I have not posted separate comments there.

Edit: See also #1920 (review).

yes all sounds reasonable to me, so how about splitting this into two PRs?

• add typos check with exclude fixtures
• revisit fixtures' typos and eventually remove ignoring this folder from typo's check

@EliahKagan Did you see this message?

This seems like a good approach to me.

this is probably linked to fixtures / test data