Re: [PATCH v2] add, rm, mv: fix bug that prevents the update of non-sparse dirs
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Oct 26, 2021, 12:53 UTC
- Message-ID
- <47aec8ed-5e54-6d13-8154-0202ef0fd747@gmail.com>
- In-Reply-To
- <5e99c039db0b9644fb21f2ea72a464c67a74ff64.1635191000.git.matheus.bernardino@usp.br>
On 10/25/2021 5:07 PM, Matheus Tavares wrote:
Show 5 quoted lines
> Changes since RFC/v1 [1]: > > - Inverted the loop direction to start from the full path and go backwards in > the parent dirs. This way we can stop early when we find the first > non-UNDECIDED match result.
This loop direction change is a good idea.
Show 5 quoted lines
> - Simplified the implementation by unifing the code path for cone mode and > full pattern mode. Since path_matches_pattern_list() never returns UNDECIDED > for cone mode, it will always execute only one iteration of the loop and then > find the final answer. There is no need to handle this case in a separate > block.
This was unexpected, but makes sense. While your commit message hints at the fact that cone mode never returns UNDECIDED, it doesn't explicitly mention that cone mode will exit the loop after a single iteration. It might be nice to make that explicit either in the commit message or the block comment before the loop.
> - Inside the loop, made sure to change dtype to DT_DIR when going to parent > directories. Without this, the pattern match would fail if we had a path > like "a/b/c" and the pattern "b/" (with trailing slash).
Very good. We typically need to detect the type for the first path given, but we know that all parents are directories. I've used this trick elsewhere.
I see in the code that the first path is used as DT_REG. It's my fault, but perhaps it should be made more clear that path_in_sparse_checkout() will consider the given path as a file, not a directory. The current users of the method are using it properly, but I'm suddenly worried about another caller misinterpreting the generality of the problem.
Would a comment be sufficient? Or should we rename it to something like file_path_patches_pattern_list()? (A rename can be done separately.)
> - Changed the tests to use trailing slash to make sure they cover the corner > case described above.
Good.
Show 6 quoted lines
> - Improved commit message. > > [1]: https://lore.kernel.org/git/80b5ba61861193daf7132aa64b65fc7dde90dacb.1634866698.git.matheus.bernardino@usp.br > (The RFC was deep down another thread, so I separated v2 to help > readers. Please, let me know if that is not a good approach and I will > avoid it in the future.)
I appreciate that you split this out into its own thread!
Show 6 quoted lines
> @@ -1504,8 +1504,9 @@ static int path_in_sparse_checkout_1(const char *path,
> struct index_state *istate,
> int require_cone_mode)
> {
> - const char *base;
> int dtype = DT_REG;Here is where we assume a file to start.
Show 19 quoted lines
> + enum pattern_match_result match = UNDECIDED;
> + const char *end, *slash;
>
> /*
> * We default to accepting a path if there are no patterns or
> @@ -1516,11 +1517,23 @@ static int path_in_sparse_checkout_1(const char *path,
> !istate->sparse_checkout_patterns->use_cone_patterns))
> return 1;
>
> - base = strrchr(path, '/');
> - return path_matches_pattern_list(path, strlen(path), base ? base + 1 : path,
> - &dtype,
> - istate->sparse_checkout_patterns,
> - istate) > 0;
> + /*
> + * If UNDECIDED, use the match from the parent dir (recursively),
> + * or fall back to NOT_MATCHED at the topmost level.
> + */
> + for (end = path + strlen(path); end > path && match == UNDECIDED; end = slash) {nit: since this line is long and the sentinel is complicated, it might be worth splitting the different parts into their own lines:
for (end = path + strlen(path);
end > path && match == UNDECIDED;
end = slash) {Show 13 quoted lines
> + > + for (slash = end - 1; slash >= path && *slash != '/'; slash--) > + ; /* do nothing */ > + > + match = path_matches_pattern_list(path, end - path, > + slash >= path ? slash + 1 : path, &dtype, > + istate->sparse_checkout_patterns, istate); > + > + /* We are going to match the parent dir now */ > + dtype = DT_DIR; > + } > + return match > 0; > }
This implementation looks good.
And I appreciate the robust tests you added.
Thanks, -Stolee