Re: [PATCH 05/11] has_dir_name(): make code more obvious
- From
Jeff King <peff@peff.net>
- Date
- May 15, 2025, 20:04 UTC
- Message-ID
- <20250515200435.GF3320240@coredump.intra.peff.net>
- In-Reply-To
- <80422a5770ded04993c73c657b363ddad45e2f4a.1747314709.git.gitgitgadget@gmail.com>
On Thu, May 15, 2025 at 01:11:43PM +0000, Johannes Schindelin via GitGitGadget wrote:
Show 7 quoted lines
> One thing that might be non-obvious to readers (or to analyzers like > CodeQL) is that the function essentially does nothing when the Git index > is empty, and in particular that it does not look at the value of > `len_eq_last` (which would be uninitialized at that point). > > Let's make this much easier to understand, by returning early if the Git > index is empty, and by avoiding empty `else` blocks.
OK, so we return early, skipping not only what's in the conditional that you're touching here, but also the "for(;;)" loop below.
And in that one, we'll look for the next slash (and break if none). We'll check the name up to that stage via index_name_stage_pos(). And obviously that will not find a match if there are no index entries. So we'd do nothing and loop again, looking for the next slash, until we eventually hit the end.
So yeah, I agree if there are no index entries we can bail immediately.
> This commit changes indentation and is hence best viewed using > `--ignore-space-change`.
Yeah. I was puzzled at first by the amount of dropped code, but they are all comments that say "fall through to the code below".
So I think the change here is correct. We are losing some comments that could be helpful, but I'm not familiar enough with those code to say whether they would be. Just reading what you've left makes sense to me on its own.
-Peff