| Message ID | 20210712223139.24409-3-randall.becker@nexbridge.ca (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | includeIf series for worktrees | expand |
randall.becker@nexbridge.ca writes: > From: "Randall S. Becker" <rsbecker@nexbridge.com> > > Documentation of the worktree and worktree/i conditionals is add based on > gitdir rules except that the trailing / form of the path is not supported. > > Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com> > --- > Documentation/config.txt | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/Documentation/config.txt b/Documentation/config.txt > index bf82766a6a..7e951937ae 100644 > --- a/Documentation/config.txt > +++ b/Documentation/config.txt > @@ -143,7 +143,16 @@ refer to linkgit:gitignore[5] for details. For convenience: > > `gitdir/i`:: > This is the same as `gitdir` except that matching is done > - case-insensitively (e.g. on case-insensitive file systems) > + case-insensitively (e.g. on case-insensitive file systems). > + > +`worktree`:: > + This is similar to `gitdir` except that matching is done with > + the path of a worktree instead of the main repository. Unlike > + `gitdir`, the trailing / form of the worktree path is not supported. It is not immediately obvious what "the trailing / form" means. Does it refer to the 4th item in the 4-bullet list in the description just above the patch context (I am trying to make a guess here)? The problem I perceive in this description is that there is no phrase "trailing" in the vicinity of what readers have read so far; readers who are not exactly familiar with the system may need a bit more assurance that they guessed correctly. Unlike `gitdir`, `**` will not be automatically added to a pattern that ends with `/` would be easier to give that assurance, albeit more verbosely. Assuming that I guessed correctly, is this a deliberate design decision not to "automatically add ** after a pattern that ends with a slash", and if so why? I would have thought that "in the worktrees that I create inside /var/tmp/, please enable these configuration variables" would be a fairly natural thing to ask, and I do not immediately see a reason why we want to apply different syntax rules between "gitdir" and "worktree". Thanks.
On July 13, 2021 9:05 PM. Junio C Hamano wrote: >randall.becker@nexbridge.ca writes: > >> From: "Randall S. Becker" <rsbecker@nexbridge.com> >> >> Documentation of the worktree and worktree/i conditionals is add based >> on gitdir rules except that the trailing / form of the path is not supported. >> >> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com> >> --- >> Documentation/config.txt | 11 ++++++++++- >> 1 file changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/config.txt b/Documentation/config.txt index >> bf82766a6a..7e951937ae 100644 >> --- a/Documentation/config.txt >> +++ b/Documentation/config.txt >> @@ -143,7 +143,16 @@ refer to linkgit:gitignore[5] for details. For convenience: >> >> `gitdir/i`:: >> This is the same as `gitdir` except that matching is done >> - case-insensitively (e.g. on case-insensitive file systems) >> + case-insensitively (e.g. on case-insensitive file systems). >> + >> +`worktree`:: >> + This is similar to `gitdir` except that matching is done with >> + the path of a worktree instead of the main repository. Unlike >> + `gitdir`, the trailing / form of the worktree path is not supported. > >It is not immediately obvious what "the trailing / form" means. > >Does it refer to the 4th item in the 4-bullet list in the description just above the patch context (I am trying to make a guess here)? > >The problem I perceive in this description is that there is no phrase "trailing" in the vicinity of what readers have read so far; readers who >are not exactly familiar with the system may need a bit more assurance that they guessed correctly. > > Unlike `gitdir`, `**` will not be automatically added to a > pattern that ends with `/` > >would be easier to give that assurance, albeit more verbosely. > >Assuming that I guessed correctly, is this a deliberate design decision not to "automatically add ** after a pattern that ends with a slash", >and if so why? I would have thought that "in the worktrees that I create inside /var/tmp/, please enable these configuration variables" >would be a fairly natural thing to ask, and I do not immediately see a reason why we want to apply different syntax rules between "gitdir" >and "worktree". The reason for this comes down to what is in *the_repository. Essentially, the_repository->gitdir always has a /path/to/.git directory with full qualification. the_repository->worktree does not have /.git added for obvious reasons, so the /path/to is bare of the trailing /. This causes a trailing pattern /to/path/** match to fail. I could copy the value into a working buffer but that seemed a bit clunky. So using the available information, the syntax rules need to be different between the two, unless the value of worktree is augmented. I was unsure which way the team wanted to go on this.
"Randall S. Becker" <rsbecker@nexbridge.com> writes: >>Assuming that I guessed correctly, is this a deliberate design >>decision not to "automatically add ** after a pattern that ends >>with a slash", and if so why? I would have thought that "in the >>worktrees that I create inside /var/tmp/, please enable these >>configuration variables" would be a fairly natural thing to ask, >>and I do not immediately see a reason why we want to apply >>different syntax rules between "gitdir" and "worktree". > The reason for this comes down to what is in >*the_repository. Sorry, but I still do not understand. > Essentially, the_repository->gitdir always has a /path/to/.git > directory with full qualification. Yes. > the_repository->worktree does not have /.git added > for obvious reasons, so the /path/to is bare of the trailing >/. It may be the case, but /path/to/.git does not have trailing slash, either, so I do not see the relevance. When you say [includeIf "gitdir:/path/"], the "behave as if ** is added after the slash at the end" rule kicks in, and the pattern "/path/**" is used to see if it matches "/path/to/.git" and it does, right? When you say [includeIf "worktree:/path/"], wouldn't the resulting "/path/**" match "/path/to"? By the way, I think [PATCH 1/3] should turn the body of include_by_gitdir() to a common helper function that - accepts a path to a directory and a pattern - turns it into a relpath - prepares the pattern with prepare_include_condition_pattern() - do the match include_by_gitdir() does. and make include_by_gitdir() a very thin wrapper that passes opts->git_dir to that common helper. Then you do not have to copy the entire function to create your new include_by_worktree(); it can be another very thin wrapper that passes the_repository->worktree instead of opts->git_dir to the common helper, as there is no other difference in these two functions. Thanks.
On July 14, 2021 1:10 PM, Junio C Hamano >"Randall S. Becker" <rsbecker@nexbridge.com> writes: > >>>Assuming that I guessed correctly, is this a deliberate design >>>decision not to "automatically add ** after a pattern that ends with a >>>slash", and if so why? I would have thought that "in the worktrees >>>that I create inside /var/tmp/, please enable these configuration >>>variables" would be a fairly natural thing to ask, and I do not >>>immediately see a reason why we want to apply different syntax rules >>>between "gitdir" and "worktree". > >> The reason for this comes down to what is in *the_repository. > >Sorry, but I still do not understand. > >> Essentially, the_repository->gitdir always has a /path/to/.git >> directory with full qualification. > >Yes. > >> the_repository->worktree does not have /.git added for obvious >>reasons, so the /path/to is bare of the trailing /. > >It may be the case, but /path/to/.git does not have trailing slash, either, so I do not see the relevance. > >When you say [includeIf "gitdir:/path/"], the "behave as if ** is added after the slash at the end" rule kicks in, and the pattern "/path/**" is >used to see if it matches "/path/to/.git" and it does, right? When you say [includeIf "worktree:/path/"], wouldn't the resulting "/path/**" >match "/path/to"? I think I over-complicated the first test case and got myself into a mess. Will fix that. >By the way, I think [PATCH 1/3] should turn the body of >include_by_gitdir() to a common helper function that > > - accepts a path to a directory and a pattern > - turns it into a relpath > - prepares the pattern with prepare_include_condition_pattern() > - do the match include_by_gitdir() does. > >and make include_by_gitdir() a very thin wrapper that passes >opts->git_dir to that common helper. Then you do not have to copy >the entire function to create your new include_by_worktree(); it can be another very thin wrapper that passes the_repository->worktree >instead of opts->git_dir to the common helper, as there is no other difference in these two functions. That sounds like a plan. Will go for it in V2. -Randall
diff --git a/Documentation/config.txt b/Documentation/config.txt index bf82766a6a..7e951937ae 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -143,7 +143,16 @@ refer to linkgit:gitignore[5] for details. For convenience: `gitdir/i`:: This is the same as `gitdir` except that matching is done - case-insensitively (e.g. on case-insensitive file systems) + case-insensitively (e.g. on case-insensitive file systems). + +`worktree`:: + This is similar to `gitdir` except that matching is done with + the path of a worktree instead of the main repository. Unlike + `gitdir`, the trailing / form of the worktree path is not supported. + +`worktree/i`:: + This is the same as `worktree` except that matching is done + case-insensitively (e.g. on case-insensitive file systems). `onbranch`:: The data that follows the keyword `onbranch:` is taken to be a