From: Brandon Williams <hidden> Date: 2016-12-06 21:53:22
The intent of this series is to cleanup some of the pathspec initialization
code as well as finally migrating the remaining users of the _raw field or
get_pathspec() to the pathspec struct interface. This way both the _raw field
and get_pathspec() can be removed from the codebase. This also removes the
functionality where parse_pathspec() modified the const char * argv array that
was passed in (which felt kind of odd to me as I wouldn't have expected the
passed in array to be modified).
I also noticed that there are memory leaks associated with the 'original' and
'match' strings. To fix this the pathspec struct needed to take ownership of
the memory for these fields so that they can be cleaned up when clearing the
pathspec struct.
Most of the work went to simplifying the prefix_pathspec function. This
consisted of factoring out long sections of code into their own helper
functions. The overall result is a much more readable function.
Brandon Williams (17):
mv: convert to using pathspec struct interface
dir: convert create_simplify to use the pathspec struct interface
dir: convert fill_directory to use the pathspec struct interface
ls-tree: convert show_recursive to use the pathspec struct interface
pathspec: remove the deprecated get_pathspec function
pathspec: copy and free owned memory
mv: small code cleanup
pathspec: remove unused variable from unsupported_magic
pathspec: always show mnemonic and name in unsupported_magic
pathspec: simpler logic to prefix original pathspec elements
pathspec: factor global magic into its own function
pathspec: create parse_short_magic function
pathspec: create parse_long_magic function
pathspec: create parse_element_magic helper
pathspec: create strip submodule slash helpers
pathspec: small readability changes
pathspec: remove outdated comment
Documentation/technical/api-setup.txt | 2 -
builtin/ls-tree.c | 12 +-
builtin/mv.c | 44 +++-
cache.h | 1 -
dir.c | 28 +--
pathspec.c | 449 +++++++++++++++++++---------------
pathspec.h | 5 +-
7 files changed, 301 insertions(+), 240 deletions(-)
--
2.8.0.rc3.226.g39d4020
From: Brandon Williams <hidden> Date: 2016-12-06 21:53:18
Convert the 'internal_copy_pathspec()' function to use the pathspec
struct interface from using the deprecated 'get_pathspec()' interface.
In addition to this, fix a memory leak caused by only duplicating some
of the pathspec elements. Instead always duplicate all of the the
pathspec elements as an intermediate step (with modificationed based on
the passed in flags). This way the intermediate strings can then be
freed prior to duplicating the result of parse_pathspec (which contains
each of the elements with the prefix prepended).
Signed-off-by: Brandon Williams <redacted>
---
builtin/mv.c | 45 ++++++++++++++++++++++++++++++++-------------
1 file changed, 32 insertions(+), 13 deletions(-)
@@ -25,25 +26,43 @@ static const char **internal_copy_pathspec(const char *prefix,{inti;constchar**result;+structpathspecps;ALLOC_ARRAY(result,count+1);-COPY_ARRAY(result,pathspec,count);-result[count]=NULL;++/* Create an intermediate copy of the pathspec based on the flags */for(i=0;i<count;i++){-intlength=strlen(result[i]);+intlength=strlen(pathspec[i]);intto_copy=length;+char*it;while(!(flags&KEEP_TRAILING_SLASH)&&-to_copy>0&&is_dir_sep(result[i][to_copy-1]))+to_copy>0&&is_dir_sep(pathspec[i][to_copy-1]))to_copy--;-if(to_copy!=length||flags&DUP_BASENAME){-char*it=xmemdupz(result[i],to_copy);-if(flags&DUP_BASENAME){-result[i]=xstrdup(basename(it));-free(it);-}else-result[i]=it;-}++it=xmemdupz(pathspec[i],to_copy);+if(flags&DUP_BASENAME){+result[i]=xstrdup(basename(it));+free(it);+}else+result[i]=it;+}+result[count]=NULL;++parse_pathspec(&ps,+PATHSPEC_ALL_MAGIC&+~(PATHSPEC_FROMTOP|PATHSPEC_LITERAL),+PATHSPEC_KEEP_ORDER|PATHSPEC_PREFER_CWD,+prefix,result);+assert(count==ps.nr);++/* Copy the pathspec and free the old intermediate strings */+for(i=0;i<count;i++){+constchar*match=xstrdup(ps.items[i].match);+free((char*)result[i]);+result[i]=match;}-returnget_pathspec(prefix,result);++clear_pathspec(&ps);+returnresult;}staticconstchar*add_slash(constchar*path)
From: Brandon Williams <hidden> Date: 2016-12-06 21:53:25
Convert 'create_simplify()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec.
Signed-off-by: Brandon Williams <redacted>
---
dir.c | 25 ++++++++++++-------------
1 file changed, 12 insertions(+), 13 deletions(-)
From: Brandon Williams <hidden> Date: 2016-12-06 21:53:28
Convert 'fill_directory()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec struct.
Signed-off-by: Brandon Williams <redacted>
---
dir.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
@@ -188,7 +188,8 @@ int fill_directory(struct dir_struct *dir, const struct pathspec *pathspec)len=common_prefix_len(pathspec);/* Read the directory and prune it */-read_directory(dir,pathspec->nr?pathspec->_raw[0]:"",len,pathspec);+read_directory(dir,pathspec->nr?pathspec->items[0].match:"",+len,pathspec);returnlen;}
From: Brandon Williams <hidden> Date: 2016-12-06 21:53:30
The logic used to prefix an original pathspec element with 'prefix'
magic is more general purpose and can be used for more than just short
magic. Remove the extra code paths and rename 'prefix_short_magic' to
'prefix_magic' to better indicate that it can be used in more general
situations.
Also, slightly change the logic which decides when to prefix the
original element in order to prevent a pathspec of "." from getting
converted to "" (empty string).
Signed-off-by: Brandon Williams <redacted>
---
pathspec.c | 33 +++++++++++++--------------------
1 file changed, 13 insertions(+), 20 deletions(-)
@@ -184,7 +183,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item,}if(*copyfrom!=')')die(_("Missing ')' at the end of pathspec magic in '%s'"),elt);-long_magic_end=copyfrom;copyfrom++;}else{/* shorthand */
@@ -243,18 +241,13 @@ static unsigned prefix_pathspec(struct pathspec_item *item,*Prefixthepathspec(keepallmagic)andassignto*original.Usefulforpassingtoanothercommand.*/-if(flags&PATHSPEC_PREFIX_ORIGIN){+if((flags&PATHSPEC_PREFIX_ORIGIN)&&+prefixlen&&!literal_global){structstrbufsb=STRBUF_INIT;-if(prefixlen&&!literal_global){-/* Preserve the actual prefix length of each pattern */-if(short_magic)-prefix_short_magic(&sb,prefixlen,short_magic);-elseif(long_magic_end){-strbuf_add(&sb,elt,long_magic_end-elt);-strbuf_addf(&sb,",prefix:%d)",prefixlen);-}else-strbuf_addf(&sb,":(prefix:%d)",prefixlen);-}++/* Preserve the actual prefix length of each pattern */+prefix_magic(&sb,prefixlen,element_magic);+strbuf_addstr(&sb,match);item->original=strbuf_detach(&sb,NULL);}else{
From: Brandon Williams <hidden> Date: 2016-12-06 21:53:34
For better clarity, always show the mnemonic and name of the unsupported
magic being used. This lets users have a more clear understanding of
what magic feature isn't supported. And if they supplied a mnemonic,
the user will be told what its corresponding name is which will allow
them to more easily search the man pages for that magic type.
This also avoids passing an extra parameter around the pathspec
initialization code.
Signed-off-by: Brandon Williams <redacted>
---
pathspec.c | 18 +++++++-----------
1 file changed, 7 insertions(+), 11 deletions(-)
From: Brandon Williams <hidden> Date: 2016-12-06 21:53:37
A few small changes to improve readability. This is done by grouping related
assignments, adding blank lines, ensuring lines are <80 characters, etc.
Signed-off-by: Brandon Williams <redacted>
---
pathspec.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
@@ -334,6 +334,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,if((magic&PATHSPEC_LITERAL)&&(magic&PATHSPEC_GLOB))die(_("%s: 'literal' and 'glob' are incompatible"),elt);+/* Create match string which will be used for pathspec matching */if(pathspec_prefix>=0){match=xstrdup(copyfrom);prefixlen=pathspec_prefix;
From: Brandon Williams <hidden> Date: 2016-12-06 21:53:41
Factor out the logic responsible for parsing long magic into its own
function. As well as hoist the prefix check logic outside of the inner
loop as there isn't anything that needs to be done after matching
"prefix:".
Signed-off-by: Brandon Williams <redacted>
---
pathspec.c | 92 ++++++++++++++++++++++++++++++++++++++------------------------
1 file changed, 57 insertions(+), 35 deletions(-)
@@ -156,6 +156,60 @@ static int get_global_magic(int element_magic)}/*+*Parsethepathspecelementlookingforlongmagic+*+*savesallmagicin'magic'+*ifprefixmagicisused,savetheprefixlengthin'prefix_len'+*returnsthepositionin'elem'afterallmagichasbeenparsed+*/+staticconstchar*parse_long_magic(unsigned*magic,int*prefix_len,+constchar*elem)+{+constchar*pos;+constchar*nextat;++for(pos=elem+2;*pos&&*pos!=')';pos=nextat){+size_tlen=strcspn(pos,",)");+inti;++if(pos[len]==',')+nextat=pos+len+1;/* handle ',' */+else+nextat=pos+len;/* handle ')' and '\0' */++if(!len)+continue;++if(starts_with(pos,"prefix:")){+char*endptr;+*prefix_len=strtol(pos+7,&endptr,10);+if(endptr-pos!=len)+die(_("invalid parameter for pathspec magic 'prefix'"));+continue;+}++for(i=0;i<ARRAY_SIZE(pathspec_magic);i++){+if(strlen(pathspec_magic[i].name)==len&&+!strncmp(pathspec_magic[i].name,pos,len)){+*magic|=pathspec_magic[i].bit;+break;+}+}++if(ARRAY_SIZE(pathspec_magic)<=i)+die(_("Invalid pathspec magic '%.*s' in '%s'"),+(int)len,pos,elem);+}++if(*pos!=')')+die(_("Missing ')' at the end of pathspec magic in '%s'"),+elem);+pos++;++returnpos;+}++/**Parsethepathspecelementlookingforshortmagic**savesallmagicin'magic'
@@ -218,41 +272,9 @@ static unsigned prefix_pathspec(struct pathspec_item *item,;/* nothing to do */}elseif(elt[1]=='('){/* longhand */-constchar*nextat;-for(copyfrom=elt+2;-*copyfrom&&*copyfrom!=')';-copyfrom=nextat){-size_tlen=strcspn(copyfrom,",)");-if(copyfrom[len]==',')-nextat=copyfrom+len+1;-else-/* handle ')' and '\0' */-nextat=copyfrom+len;-if(!len)-continue;-for(i=0;i<ARRAY_SIZE(pathspec_magic);i++){-if(strlen(pathspec_magic[i].name)==len&&-!strncmp(pathspec_magic[i].name,copyfrom,len)){-element_magic|=pathspec_magic[i].bit;-break;-}-if(starts_with(copyfrom,"prefix:")){-char*endptr;-pathspec_prefix=strtol(copyfrom+7,-&endptr,10);-if(endptr-copyfrom!=len)-die(_("invalid parameter for pathspec magic 'prefix'"));-/* "i" would be wrong, but it does not matter */-break;-}-}-if(ARRAY_SIZE(pathspec_magic)<=i)-die(_("Invalid pathspec magic '%.*s' in '%s'"),-(int)len,copyfrom,elt);-}-if(*copyfrom!=')')-die(_("Missing ')' at the end of pathspec magic in '%s'"),elt);-copyfrom++;+copyfrom=parse_long_magic(&element_magic,+&pathspec_prefix,+elt);}else{/* shorthand */copyfrom=parse_short_magic(&element_magic,elt);
From: Brandon Williams <hidden> Date: 2016-12-06 21:53:47
Factor out the logic responsible for the magic in a pathspec element
into its own function.
Also avoid calling into the parsing functions when
`PATHSPEC_LITERAL_PATH` is specified since it causes magic to be
ignored and all paths to be treated as literals.
Signed-off-by: Brandon Williams <redacted>
---
pathspec.c | 35 +++++++++++++++++++----------------
1 file changed, 19 insertions(+), 16 deletions(-)
From: Brandon Williams <hidden> Date: 2016-12-06 21:53:49
Create helper functions to read the global magic environment variables
in additon to factoring out the global magic gathering logic into its
own function.
Signed-off-by: Brandon Williams <redacted>
---
pathspec.c | 120 +++++++++++++++++++++++++++++++++++++------------------------
1 file changed, 74 insertions(+), 46 deletions(-)
@@ -87,6 +87,74 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)strbuf_addf(sb,",prefix:%d)",prefixlen);}+staticinlineintget_literal_global(void)+{+staticintliteral_global=-1;++if(literal_global<0)+literal_global=git_env_bool(GIT_LITERAL_PATHSPECS_ENVIRONMENT,+0);+returnliteral_global;+}++staticinlineintget_glob_global(void)+{+staticintglob_global=-1;++if(glob_global<0)+glob_global=git_env_bool(GIT_GLOB_PATHSPECS_ENVIRONMENT,0);+returnglob_global;+}++staticinlineintget_noglob_global(void)+{+staticintnoglob_global=-1;++if(noglob_global<0)+noglob_global=git_env_bool(GIT_NOGLOB_PATHSPECS_ENVIRONMENT,+0);+returnnoglob_global;+}++staticinlineintget_icase_global(void)+{+staticinticase_global=-1;++if(icase_global<0)+icase_global=git_env_bool(GIT_ICASE_PATHSPECS_ENVIRONMENT,0);++returnicase_global;+}++staticintget_global_magic(intelement_magic)+{+intglobal_magic=0;++if(get_literal_global())+global_magic|=PATHSPEC_LITERAL;++/* --glob-pathspec is overridden by :(literal) */+if(get_glob_global()&&!(element_magic&PATHSPEC_LITERAL))+global_magic|=PATHSPEC_GLOB;++if(get_glob_global()&&get_noglob_global())+die(_("global 'glob' and 'noglob' pathspec settings are incompatible"));++if(get_icase_global())+global_magic|=PATHSPEC_ICASE;++if((global_magic&PATHSPEC_LITERAL)&&+(global_magic&~PATHSPEC_LITERAL))+die(_("global 'literal' pathspec setting is incompatible "+"with all other global pathspec settings"));++/* --noglob-pathspec adds :(literal) _unless_ :(glob) is specified */+if(get_noglob_global()&&!(element_magic&PATHSPEC_GLOB))+global_magic|=PATHSPEC_LITERAL;++returnglobal_magic;+}+/**Takeanelementofapathspecandcheckformagicsignatures.*Appendtheresulttotheprefix.Returnthemagicbitmap.
@@ -105,46 +173,12 @@ static unsigned prefix_pathspec(struct pathspec_item *item,constchar*prefix,intprefixlen,constchar*elt){-staticintliteral_global=-1;-staticintglob_global=-1;-staticintnoglob_global=-1;-staticinticase_global=-1;-unsignedmagic=0,element_magic=0,global_magic=0;+unsignedmagic=0,element_magic=0;constchar*copyfrom=elt;char*match;inti,pathspec_prefix=-1;-if(literal_global<0)-literal_global=git_env_bool(GIT_LITERAL_PATHSPECS_ENVIRONMENT,0);-if(literal_global)-global_magic|=PATHSPEC_LITERAL;--if(glob_global<0)-glob_global=git_env_bool(GIT_GLOB_PATHSPECS_ENVIRONMENT,0);-if(glob_global)-global_magic|=PATHSPEC_GLOB;--if(noglob_global<0)-noglob_global=git_env_bool(GIT_NOGLOB_PATHSPECS_ENVIRONMENT,0);--if(glob_global&&noglob_global)-die(_("global 'glob' and 'noglob' pathspec settings are incompatible"));---if(icase_global<0)-icase_global=git_env_bool(GIT_ICASE_PATHSPECS_ENVIRONMENT,0);-if(icase_global)-global_magic|=PATHSPEC_ICASE;--if((global_magic&PATHSPEC_LITERAL)&&-(global_magic&~PATHSPEC_LITERAL))-die(_("global 'literal' pathspec setting is incompatible "-"with all other global pathspec settings"));--if(flags&PATHSPEC_LITERAL_PATH)-global_magic=0;--if(elt[0]!=':'||literal_global||+if(elt[0]!=':'||get_literal_global()||(flags&PATHSPEC_LITERAL_PATH)){;/* nothing to do */}elseif(elt[1]=='('){
@@ -208,15 +242,9 @@ static unsigned prefix_pathspec(struct pathspec_item *item,magic|=element_magic;-/* --noglob-pathspec adds :(literal) _unless_ :(glob) is specified */-if(noglob_global&&!(magic&PATHSPEC_GLOB))-global_magic|=PATHSPEC_LITERAL;--/* --glob-pathspec is overridden by :(literal) */-if((global_magic&PATHSPEC_GLOB)&&(magic&PATHSPEC_LITERAL))-global_magic&=~PATHSPEC_GLOB;--magic|=global_magic;+/* PATHSPEC_LITERAL_PATH ignores magic */+if(!(flags&PATHSPEC_LITERAL_PATH))+magic|=get_global_magic(element_magic);if(pathspec_prefix>=0&&(prefixlen||(prefix&&*prefix)))
@@ -242,7 +270,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,*original.Usefulforpassingtoanothercommand.*/if((flags&PATHSPEC_PREFIX_ORIGIN)&&-prefixlen&&!literal_global){+prefixlen&&!get_literal_global()){structstrbufsb=STRBUF_INIT;/* Preserve the actual prefix length of each pattern */
From: Brandon Williams <hidden> Date: 2016-12-06 21:53:50
Factor out the logic responsible for parsing short magic into its own
function.
Signed-off-by: Brandon Williams <redacted>
---
pathspec.c | 54 ++++++++++++++++++++++++++++++++++++------------------
1 file changed, 36 insertions(+), 18 deletions(-)
From: Brandon Williams <hidden> Date: 2016-12-06 21:53:52
Now that the call to 'parse_pathspec()' doesn't modify the passed in
const char **array there isn't a need to duplicate the pathspec element
prior to freeing the intermediate strings. This small cleanup just
makes the code a bit easier to read.
Signed-off-by: Brandon Williams <redacted>
---
builtin/mv.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
@@ -56,9 +56,8 @@ static const char **internal_copy_pathspec(const char *prefix,/* Copy the pathspec and free the old intermediate strings */for(i=0;i<count;i++){-constchar*match=xstrdup(ps.items[i].match);free((char*)result[i]);-result[i]=match;+result[i]=xstrdup(ps.items[i].match);}clear_pathspec(&ps);
From: Brandon Williams <hidden> Date: 2016-12-06 21:53:53
The 'original' string entry in a pathspec_item is only duplicated some
of the time, instead always make a copy of the original and take
ownership of the memory.
Since both 'match' and 'original' string entries in a pathspec_item are
owned by the pathspec struct, they need to be freed when clearing the
pathspec struct (in 'clear_pathspec()') and duplicated when copying the
pathspec struct (in 'copy_pathspec()').
Also change the type of 'match' and 'original' to 'char *' in order to
more explicitly show the ownership of the memory.
Signed-off-by: Brandon Williams <redacted>
---
pathspec.c | 22 ++++++++++++++++++----
pathspec.h | 4 ++--
2 files changed, 20 insertions(+), 6 deletions(-)
From: Brandon Williams <hidden> Date: 2016-12-06 21:53:56
Convert 'show_recursive()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec struct.
Signed-off-by: Brandon Williams <redacted>
---
builtin/ls-tree.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
From: Brandon Williams <hidden> Date: 2016-12-06 21:53:58
Now that all callers of the old 'get_pathspec' interface have been
migrated to use the new pathspec struct interface it can be removed
from the codebase.
Since there are no more users of the '_raw' field in the pathspec struct
it can also be removed. This patch also removes the old functionality
of modifying the const char **argv array that was passed into
parse_pathspec. Instead the constructed 'match' string (which is a
pathspec element with the prefix prepended) is only stored in its
corresponding pathspec_item entry.
Signed-off-by: Brandon Williams <redacted>
---
Documentation/technical/api-setup.txt | 2 --
cache.h | 1 -
pathspec.c | 42 +++--------------------------------
pathspec.h | 1 -
4 files changed, 3 insertions(+), 43 deletions(-)
@@ -27,8 +27,6 @@ parse_pathspec(). This function takes several arguments: - prefix and args come from cmd_* functions-get_pathspec() is obsolete and should never be used in new code.- parse_pathspec() helps catch unsupported features and reject them politely. At a lower level, different pathspec-related functions may not support the same set of features. Such pathspec-sensitive
From: Brandon Williams <hidden> Date: 2016-12-06 21:54:16
Remove part of the function header comment to prefix_pathspec as it is
no longer relevant.
Signed-off-by: Brandon Williams <redacted>
---
pathspec.c | 9 ---------
1 file changed, 9 deletions(-)
From: Brandon Williams <hidden> Date: 2016-12-06 21:54:18
Factor out the logic responsible for stripping the trailing slash on
pathspecs referencing submodules into its own function.
Signed-off-by: Brandon Williams <redacted>
---
pathspec.c | 68 ++++++++++++++++++++++++++++++++++++++------------------------
1 file changed, 42 insertions(+), 26 deletions(-)
From: Stefan Beller <hidden> Date: 2016-12-06 22:26:46
On Tue, Dec 6, 2016 at 1:51 PM, Brandon Williams [off-list ref] wrote:
struct strbuf sb = STRBUF_INIT;
- if (prefixlen && !literal_global) {
- /* Preserve the actual prefix length of each pattern */
- if (short_magic)
- prefix_short_magic(&sb, prefixlen, short_magic);
- else if (long_magic_end) {
- strbuf_add(&sb, elt, long_magic_end - elt);
- strbuf_addf(&sb, ",prefix:%d)", prefixlen);
- } else
- strbuf_addf(&sb, ":(prefix:%d)", prefixlen);
This fixes the issue with add -p . mentioned somewhere else on the mailing list.
- }
+
+ /* Preserve the actual prefix length of each pattern */
+ prefix_magic(&sb, prefixlen, element_magic);
+
Did you find a reason why we passed magic literally, i.e. short magic
was passed as short magic and long magic as long magic before?
I cannot think of any reason why that would have been the case,
but I assume there had to be a reason for that.
Another note: This collides with the attr system refactoring, which I
postpone redoing until the submodule checkout is done, so maybe
you want to pickup this patch:
https://public-inbox.org/git/20161110203428.30512-31-sbeller@google.com/
which only relies on one patch prior
https://public-inbox.org/git/20161110203428.30512-30-sbeller@google.com/
From: Brandon Williams <hidden> Date: 2016-12-06 22:38:13
On 12/06, Stefan Beller wrote:
On Tue, Dec 6, 2016 at 1:51 PM, Brandon Williams [off-list ref] wrote:
quoted
struct strbuf sb = STRBUF_INIT;
- if (prefixlen && !literal_global) {
- /* Preserve the actual prefix length of each pattern */
- if (short_magic)
- prefix_short_magic(&sb, prefixlen, short_magic);
- else if (long_magic_end) {
- strbuf_add(&sb, elt, long_magic_end - elt);
- strbuf_addf(&sb, ",prefix:%d)", prefixlen);
- } else
- strbuf_addf(&sb, ":(prefix:%d)", prefixlen);
This fixes the issue with add -p . mentioned somewhere else on the mailing list.
quoted
- }
+
+ /* Preserve the actual prefix length of each pattern */
+ prefix_magic(&sb, prefixlen, element_magic);
+
Did you find a reason why we passed magic literally, i.e. short magic
was passed as short magic and long magic as long magic before?
I cannot think of any reason why that would have been the case,
but I assume there had to be a reason for that.
nope, perhaps it was because we technically already have the long magic
string and the short magic needs to be converted to long magic (as you
can't mix short and long magic).
After looking at those patches I think I do something extremely similar
in a future patch in this series, the parse_long_magic patch.
--
Brandon Williams
On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams [off-list ref] wrote:
Convert the 'internal_copy_pathspec()' function to use the pathspec
struct interface from using the deprecated 'get_pathspec()' interface.
In addition to this, fix a memory leak caused by only duplicating some
of the pathspec elements. Instead always duplicate all of the the
pathspec elements as an intermediate step (with modificationed based on
the passed in flags). This way the intermediate strings can then be
freed prior to duplicating the result of parse_pathspec (which contains
each of the elements with the prefix prepended).
Signed-off-by: Brandon Williams <redacted>
---
builtin/mv.c | 45 ++++++++++++++++++++++++++++++++-------------
1 file changed, 32 insertions(+), 13 deletions(-)
@@ -25,25 +26,43 @@ static const char **internal_copy_pathspec(const char *prefix,{inti;constchar**result;+structpathspecps;ALLOC_ARRAY(result,count+1);-COPY_ARRAY(result,pathspec,count);-result[count]=NULL;++/* Create an intermediate copy of the pathspec based on the flags */for(i=0;i<count;i++){-intlength=strlen(result[i]);+intlength=strlen(pathspec[i]);intto_copy=length;+char*it;while(!(flags&KEEP_TRAILING_SLASH)&&-to_copy>0&&is_dir_sep(result[i][to_copy-1]))+to_copy>0&&is_dir_sep(pathspec[i][to_copy-1]))to_copy--;-if(to_copy!=length||flags&DUP_BASENAME){-char*it=xmemdupz(result[i],to_copy);-if(flags&DUP_BASENAME){-result[i]=xstrdup(basename(it));-free(it);-}else-result[i]=it;-}++it=xmemdupz(pathspec[i],to_copy);+if(flags&DUP_BASENAME){+result[i]=xstrdup(basename(it));+free(it);+}else+result[i]=it;+}+result[count]=NULL;++parse_pathspec(&ps,+PATHSPEC_ALL_MAGIC&+~(PATHSPEC_FROMTOP|PATHSPEC_LITERAL),+PATHSPEC_KEEP_ORDER|PATHSPEC_PREFER_CWD,+prefix,result);+assert(count==ps.nr);++/* Copy the pathspec and free the old intermediate strings */+for(i=0;i<count;i++){+constchar*match=xstrdup(ps.items[i].match);+free((char*)result[i]);+result[i]=match;
Sigh.. it looks so weird that we do all the parsing (in a _copy_
pathspec function) then remove struct pathspec and return the plain
string. I guess we can't do anything more until we rework cmd_mv code
to handle pathspec natively.
At the least I think we should rename this function to something else.
But if you have time I really wish we could kill this function. I
haven't stared at cmd_mv() long and hard, but it looks to me that we
combining two separate functionalities in the same function here.
If "mv" takes n arguments, then the first <n-1> arguments may be
pathspec, the last one is always a plain path. The "dest_path =
internal_copy_pathspec..." could be as simple as "dest_path =
prefix_path(argv[argc - 1])". the special treatment for this last
argument [1] can live here. Then, we can do parse_pathspec for the
<n-1> arguments in cmd_mv(). It's still far from perfect, because
cmd_mv can't handle pathspec properly, but it reduces the messy mess
in internal_copy_pathspec a bit, I hope.
[1] c57f628 (mv: let 'git mv file no-such-dir/' error out - 2013-12-03)
On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams [off-list ref] wrote:
Convert 'create_simplify()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec.
It would be even better to kill this create_simplify() and let
simplify_away() handle struct pathspec directly.
There is a bug in this code, that might have been found if we
simpify_away() handled pathspec directly: the memcmp() in
simplify_away() will not play well with :(icase) magic. My bad. If
:(icase) is used, the easiest/safe way is simplify nothing. Later on
maybe we can teach simplify_away() to do strncasecmp instead. We could
ignore exclude patterns there too (although not excluding is not a
bug).
--
Duy
On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams [off-list ref] wrote:
quoted hunk
Convert 'fill_directory()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec struct.
Signed-off-by: Brandon Williams <redacted>
---
dir.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
@@ -188,7 +188,8 @@ int fill_directory(struct dir_struct *dir, const struct pathspec *pathspec)len=common_prefix_len(pathspec);/* Read the directory and prune it */-read_directory(dir,pathspec->nr?pathspec->_raw[0]:"",len,pathspec);+read_directory(dir,pathspec->nr?pathspec->items[0].match:"",+len,pathspec);
Or even better, use common_prefix()'s return value here. I took me a
while to realize this code was not buggy. It is fine to just pick the
first item because the first <len> characters of _all_ pathspec items
must be the same. Something like this
prefix = common_prefix(..)
read_directory(..., prefix, strlen(prefix), pathspec);
expresses it much better. Yeah one extra mem allocation, no big deal
since fill_directory() is not called very often.
On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams [off-list ref] wrote:
Convert 'show_recursive()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec struct.
Slightly off-topic (sorry, but you made me look at this code! :D),
could you update the magic_mask argument of parse_pathspec() in this
file to PATHSPEC_ALL_MAGIC & ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL)?
It makes sure all future magic will be caught as unsupported (and I
think Stefan is adding one, but understandably he did not find this
code).
I think it's in the spirit of renaming _raw to match too. By limiting
magic to fromtop and literal, we are sure match can only be path and
nothing else, which is good because this show_recursive can't handle
anything else either.
On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams [off-list ref] wrote:
quoted hunk
@@ -413,10 +411,9 @@ void parse_pathspec(struct pathspec *pathspec, prefixlen = prefix ? strlen(prefix) : 0; for (i = 0; i < n; i++) {- unsigned short_magic; entry = argv[i];- item[i].magic = prefix_pathspec(item + i, &short_magic,+ item[i].magic = prefix_pathspec(item + i, flags, prefix, prefixlen, entry);
The final output looks a bit ...um.. strangely tall, with the first
two lines that have one argument each, then the last line comes with
three arguments. Maybe put 'flags' in the same line as 'item + i'?
Same here. Maybe put both arguments in the same line. It looks a bit
better. (sorry for two mails on the same patch, I'm reading the final
output first before going through individual patches that breaks this
function down)
if ((flags & PATHSPEC_SYMLINK_LEADING_PATH) &&
has_symlink_leading_path(item[i].match, item[i].len)) {
On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams [off-list ref] wrote:
quoted hunk
A few small changes to improve readability. This is done by grouping related
assignments, adding blank lines, ensuring lines are <80 characters, etc.
Signed-off-by: Brandon Williams <redacted>
---
pathspec.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
@@ -334,6 +334,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,if((magic&PATHSPEC_LITERAL)&&(magic&PATHSPEC_GLOB))die(_("%s: 'literal' and 'glob' are incompatible"),elt);+/* Create match string which will be used for pathspec matching */if(pathspec_prefix>=0){match=xstrdup(copyfrom);prefixlen=pathspec_prefix;
You probably can move this line up with the others too.
And since you have broken this function down so nicely, it made me see
that we could do
item->magic = magic instead of returning "magic" at the end, which is
assigned to item->magic anyway by the caller.
--
Duy
On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams [off-list ref] wrote:
quoted hunk
Create helper functions to read the global magic environment variables
in additon to factoring out the global magic gathering logic into its
own function.
Signed-off-by: Brandon Williams <redacted>
---
pathspec.c | 120 +++++++++++++++++++++++++++++++++++++------------------------
1 file changed, 74 insertions(+), 46 deletions(-)
These zeros look so lonely. I know it would exceed 80 columns if we
put it on the previous line. But I think it's ok for occasional
exceptions. Or you could rename noglob_global to noglob.
--
Duy
On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams [off-list ref] wrote:
quoted hunk
A few small changes to improve readability. This is done by grouping related
assignments, adding blank lines, ensuring lines are <80 characters, etc.
Signed-off-by: Brandon Williams <redacted>
---
pathspec.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
From: Brandon Williams <hidden> Date: 2016-12-07 22:37:05
On 12/07, Duy Nguyen wrote:
On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams [off-list ref] wrote:
quoted
A few small changes to improve readability. This is done by grouping related
assignments, adding blank lines, ensuring lines are <80 characters, etc.
Signed-off-by: Brandon Williams <redacted>
---
pathspec.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
From: Brandon Williams <hidden> Date: 2016-12-07 22:39:44
On 12/07, Duy Nguyen wrote:
On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams [off-list ref] wrote:
quoted
Create helper functions to read the global magic environment variables
in additon to factoring out the global magic gathering logic into its
own function.
Signed-off-by: Brandon Williams <redacted>
---
pathspec.c | 120 +++++++++++++++++++++++++++++++++++++------------------------
1 file changed, 74 insertions(+), 46 deletions(-)
These zeros look so lonely. I know it would exceed 80 columns if we
put it on the previous line. But I think it's ok for occasional
exceptions. Or you could rename noglob_global to noglob.
I was thinking the same thing but was so torn between the char limit. I
think it's probably ok to rename these vars by drooping the global since
the function name themselves indicate they are global.
--
Brandon Williams
Same here. Maybe put both arguments in the same line. It looks a bit
better. (sorry for two mails on the same patch, I'm reading the final
output first before going through individual patches that breaks this
function down)
All good. Sometimes its easier to parse comments if they are in
multiple small emails. I don't mind getting lots of mail :)
quoted
if ((flags & PATHSPEC_SYMLINK_LEADING_PATH) &&
has_symlink_leading_path(item[i].match, item[i].len)) {
From: Brandon Williams <hidden> Date: 2016-12-07 22:42:03
On 12/07, Duy Nguyen wrote:
On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams [off-list ref] wrote:
quoted
@@ -413,10 +411,9 @@ void parse_pathspec(struct pathspec *pathspec, prefixlen = prefix ? strlen(prefix) : 0; for (i = 0; i < n; i++) {- unsigned short_magic; entry = argv[i];- item[i].magic = prefix_pathspec(item + i, &short_magic,+ item[i].magic = prefix_pathspec(item + i, flags, prefix, prefixlen, entry);
The final output looks a bit ...um.. strangely tall, with the first
two lines that have one argument each, then the last line comes with
three arguments. Maybe put 'flags' in the same line as 'item + i'?
Yep you're right, it does look a bit funny.
--
Brandon Williams
From: Brandon Williams <hidden> Date: 2016-12-07 22:43:30
On 12/07, Duy Nguyen wrote:
On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams [off-list ref] wrote:
quoted
Convert 'show_recursive()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec struct.
Slightly off-topic (sorry, but you made me look at this code! :D),
could you update the magic_mask argument of parse_pathspec() in this
file to PATHSPEC_ALL_MAGIC & ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL)?
It makes sure all future magic will be caught as unsupported (and I
think Stefan is adding one, but understandably he did not find this
code).
I think it's in the spirit of renaming _raw to match too. By limiting
magic to fromtop and literal, we are sure match can only be path and
nothing else, which is good because this show_recursive can't handle
anything else either.
From: Brandon Williams <hidden> Date: 2016-12-07 22:46:39
On 12/07, Duy Nguyen wrote:
On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams [off-list ref] wrote:
quoted
Convert 'fill_directory()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec struct.
Signed-off-by: Brandon Williams <redacted>
---
dir.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
@@ -188,7 +188,8 @@ int fill_directory(struct dir_struct *dir, const struct pathspec *pathspec)len=common_prefix_len(pathspec);/* Read the directory and prune it */-read_directory(dir,pathspec->nr?pathspec->_raw[0]:"",len,pathspec);+read_directory(dir,pathspec->nr?pathspec->items[0].match:"",+len,pathspec);
Or even better, use common_prefix()'s return value here. I took me a
while to realize this code was not buggy. It is fine to just pick the
first item because the first <len> characters of _all_ pathspec items
must be the same. Something like this
prefix = common_prefix(..)
read_directory(..., prefix, strlen(prefix), pathspec);
expresses it much better. Yeah one extra mem allocation, no big deal
since fill_directory() is not called very often.
I didn't even notice that. Now looking at this you're right that its
not immediately obvious that what's there is correct. I'll change this.
From: Brandon Williams <hidden> Date: 2016-12-07 23:27:31
On 12/07, Duy Nguyen wrote:
On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams [off-list ref] wrote:
quoted
A few small changes to improve readability. This is done by grouping related
assignments, adding blank lines, ensuring lines are <80 characters, etc.
Signed-off-by: Brandon Williams <redacted>
---
pathspec.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
@@ -334,6 +334,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,if((magic&PATHSPEC_LITERAL)&&(magic&PATHSPEC_GLOB))die(_("%s: 'literal' and 'glob' are incompatible"),elt);+/* Create match string which will be used for pathspec matching */if(pathspec_prefix>=0){match=xstrdup(copyfrom);prefixlen=pathspec_prefix;
You probably can move this line up with the others too.
I didn't move the item->flags assignment up since the code immediately
following this assignment deal with setting item->flags. I made more
sense to keep them grouped.
--
Brandon Williams
From: Brandon Williams <hidden> Date: 2016-12-08 00:04:04
On 12/07, Duy Nguyen wrote:
On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams [off-list ref] wrote:
quoted
Convert 'create_simplify()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec.
It would be even better to kill this create_simplify() and let
simplify_away() handle struct pathspec directly.
There is a bug in this code, that might have been found if we
simpify_away() handled pathspec directly: the memcmp() in
simplify_away() will not play well with :(icase) magic. My bad. If
:(icase) is used, the easiest/safe way is simplify nothing. Later on
maybe we can teach simplify_away() to do strncasecmp instead. We could
ignore exclude patterns there too (although not excluding is not a
bug).
So are you implying that the simplify struct needs to be killed? That
way the pathspec struct itself is being passed around instead?
--
Brandon Williams
From: Brandon Williams <hidden> Date: 2016-12-08 00:36:12
On 12/07, Duy Nguyen wrote:
On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams [off-list ref] wrote:
quoted
Convert the 'internal_copy_pathspec()' function to use the pathspec
struct interface from using the deprecated 'get_pathspec()' interface.
In addition to this, fix a memory leak caused by only duplicating some
of the pathspec elements. Instead always duplicate all of the the
pathspec elements as an intermediate step (with modificationed based on
the passed in flags). This way the intermediate strings can then be
freed prior to duplicating the result of parse_pathspec (which contains
each of the elements with the prefix prepended).
Signed-off-by: Brandon Williams <redacted>
---
builtin/mv.c | 45 ++++++++++++++++++++++++++++++++-------------
1 file changed, 32 insertions(+), 13 deletions(-)
@@ -25,25 +26,43 @@ static const char **internal_copy_pathspec(const char *prefix,{inti;constchar**result;+structpathspecps;ALLOC_ARRAY(result,count+1);-COPY_ARRAY(result,pathspec,count);-result[count]=NULL;++/* Create an intermediate copy of the pathspec based on the flags */for(i=0;i<count;i++){-intlength=strlen(result[i]);+intlength=strlen(pathspec[i]);intto_copy=length;+char*it;while(!(flags&KEEP_TRAILING_SLASH)&&-to_copy>0&&is_dir_sep(result[i][to_copy-1]))+to_copy>0&&is_dir_sep(pathspec[i][to_copy-1]))to_copy--;-if(to_copy!=length||flags&DUP_BASENAME){-char*it=xmemdupz(result[i],to_copy);-if(flags&DUP_BASENAME){-result[i]=xstrdup(basename(it));-free(it);-}else-result[i]=it;-}++it=xmemdupz(pathspec[i],to_copy);+if(flags&DUP_BASENAME){+result[i]=xstrdup(basename(it));+free(it);+}else+result[i]=it;+}+result[count]=NULL;++parse_pathspec(&ps,+PATHSPEC_ALL_MAGIC&+~(PATHSPEC_FROMTOP|PATHSPEC_LITERAL),+PATHSPEC_KEEP_ORDER|PATHSPEC_PREFER_CWD,+prefix,result);+assert(count==ps.nr);++/* Copy the pathspec and free the old intermediate strings */+for(i=0;i<count;i++){+constchar*match=xstrdup(ps.items[i].match);+free((char*)result[i]);+result[i]=match;
Sigh.. it looks so weird that we do all the parsing (in a _copy_
pathspec function) then remove struct pathspec and return the plain
string. I guess we can't do anything more until we rework cmd_mv code
to handle pathspec natively.
At the least I think we should rename this function to something else.
But if you have time I really wish we could kill this function. I
haven't stared at cmd_mv() long and hard, but it looks to me that we
combining two separate functionalities in the same function here.
If "mv" takes n arguments, then the first <n-1> arguments may be
pathspec, the last one is always a plain path. The "dest_path =
internal_copy_pathspec..." could be as simple as "dest_path =
prefix_path(argv[argc - 1])". the special treatment for this last
argument [1] can live here. Then, we can do parse_pathspec for the
<n-1> arguments in cmd_mv(). It's still far from perfect, because
cmd_mv can't handle pathspec properly, but it reduces the messy mess
in internal_copy_pathspec a bit, I hope.
[1] c57f628 (mv: let 'git mv file no-such-dir/' error out - 2013-12-03)
Actually, after looking at this a bit more it seems like we could
technically use prefix_path for both source and dest (based on how the
current code is structured) since the source's provied must all exist (as
in no wildcards are allowed). We could drop using the pathspec struct
completely in addition to renaming the function (to what I'm still
unsure). I agree that this code should probably be rewritten and made a
bit cleaner, I don't know if that fits in the scope of this series or
should be done as a followup patch. If you think it fits here then I
can try and find some time to do the rework.
--
Brandon Williams
On Thu, Dec 8, 2016 at 5:39 AM, Brandon Williams [off-list ref] wrote:
On 12/07, Duy Nguyen wrote:
quoted
On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams [off-list ref] wrote:
quoted
Create helper functions to read the global magic environment variables
in additon to factoring out the global magic gathering logic into its
own function.
Signed-off-by: Brandon Williams <redacted>
---
pathspec.c | 120 +++++++++++++++++++++++++++++++++++++------------------------
1 file changed, 74 insertions(+), 46 deletions(-)
These zeros look so lonely. I know it would exceed 80 columns if we
put it on the previous line. But I think it's ok for occasional
exceptions. Or you could rename noglob_global to noglob.
I was thinking the same thing but was so torn between the char limit. I
think it's probably ok to rename these vars by drooping the global since
the function name themselves indicate they are global.
Exactly. I almost suggested just "ret" for that reason, but it was a
bit on the extreme side, relying entirely on the function's name for
context.
--
Duy
You probably can move this line up with the others too.
I didn't move the item->flags assignment up since the code immediately
following this assignment deal with setting item->flags. I made more
sense to keep them grouped.
It's probably why I put it there in the beginning :) Yes let's leave
it where it is then.
--
Duy
On Thu, Dec 8, 2016 at 7:36 AM, Brandon Williams [off-list ref] wrote:
quoted
quoted
@@ -25,25 +26,43 @@ static const char **internal_copy_pathspec(const char *prefix, { int i; const char **result;+ struct pathspec ps; ALLOC_ARRAY(result, count + 1);- COPY_ARRAY(result, pathspec, count);- result[count] = NULL;++ /* Create an intermediate copy of the pathspec based on the flags */ for (i = 0; i < count; i++) {- int length = strlen(result[i]);+ int length = strlen(pathspec[i]); int to_copy = length;+ char *it; while (!(flags & KEEP_TRAILING_SLASH) &&- to_copy > 0 && is_dir_sep(result[i][to_copy - 1]))+ to_copy > 0 && is_dir_sep(pathspec[i][to_copy - 1])) to_copy--;- if (to_copy != length || flags & DUP_BASENAME) {- char *it = xmemdupz(result[i], to_copy);- if (flags & DUP_BASENAME) {- result[i] = xstrdup(basename(it));- free(it);- } else- result[i] = it;- }++ it = xmemdupz(pathspec[i], to_copy);+ if (flags & DUP_BASENAME) {+ result[i] = xstrdup(basename(it));+ free(it);+ } else+ result[i] = it;+ }+ result[count] = NULL;++ parse_pathspec(&ps,+ PATHSPEC_ALL_MAGIC &+ ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),+ PATHSPEC_KEEP_ORDER | PATHSPEC_PREFER_CWD,+ prefix, result);+ assert(count == ps.nr);++ /* Copy the pathspec and free the old intermediate strings */+ for (i = 0; i < count; i++) {+ const char *match = xstrdup(ps.items[i].match);+ free((char *) result[i]);+ result[i] = match;
Sigh.. it looks so weird that we do all the parsing (in a _copy_
pathspec function) then remove struct pathspec and return the plain
string. I guess we can't do anything more until we rework cmd_mv code
to handle pathspec natively.
At the least I think we should rename this function to something else.
But if you have time I really wish we could kill this function. I
haven't stared at cmd_mv() long and hard, but it looks to me that we
combining two separate functionalities in the same function here.
If "mv" takes n arguments, then the first <n-1> arguments may be
pathspec, the last one is always a plain path. The "dest_path =
internal_copy_pathspec..." could be as simple as "dest_path =
prefix_path(argv[argc - 1])". the special treatment for this last
argument [1] can live here. Then, we can do parse_pathspec for the
<n-1> arguments in cmd_mv(). It's still far from perfect, because
cmd_mv can't handle pathspec properly, but it reduces the messy mess
in internal_copy_pathspec a bit, I hope.
Actually, after looking at this a bit more it seems like we could
technically use prefix_path for both source and dest (based on how the
current code is structured) since the source's provied must all exist (as
in no wildcards are allowed). We could drop using the pathspec struct
completely in addition to renaming the function (to what I'm still
unsure).
Yeah that sounds good too (with a caveat: I'm not a heavy user of
git-mv nor touching this code a lot, I might be missing something).
It'll take some looong time before somebody starts converting it to
use pathspec properly, I guess. prefix_path() would keep the code
clean meanwhile.
I agree that this code should probably be rewritten and made a
bit cleaner, I don't know if that fits in the scope of this series or
should be done as a followup patch. If you think it fits here then I
can try and find some time to do the rework.
On Thu, Dec 8, 2016 at 7:03 AM, Brandon Williams [off-list ref] wrote:
On 12/07, Duy Nguyen wrote:
quoted
On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams [off-list ref] wrote:
quoted
Convert 'create_simplify()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec.
It would be even better to kill this create_simplify() and let
simplify_away() handle struct pathspec directly.
There is a bug in this code, that might have been found if we
simpify_away() handled pathspec directly: the memcmp() in
simplify_away() will not play well with :(icase) magic. My bad. If
:(icase) is used, the easiest/safe way is simplify nothing. Later on
maybe we can teach simplify_away() to do strncasecmp instead. We could
ignore exclude patterns there too (although not excluding is not a
bug).
So are you implying that the simplify struct needs to be killed? That
way the pathspec struct itself is being passed around instead?
Yes. simplify struct was a thing when pathspec was an array of char *.
At this point I think it can retire (when we have time to retire it)
--
Duy
From: Brandon Williams <hidden> Date: 2016-12-08 18:06:22
On 12/08, Duy Nguyen wrote:
On Thu, Dec 8, 2016 at 7:36 AM, Brandon Williams [off-list ref] wrote:
quoted
quoted
quoted
@@ -25,25 +26,43 @@ static const char **internal_copy_pathspec(const char *prefix, { int i; const char **result;+ struct pathspec ps; ALLOC_ARRAY(result, count + 1);- COPY_ARRAY(result, pathspec, count);- result[count] = NULL;++ /* Create an intermediate copy of the pathspec based on the flags */ for (i = 0; i < count; i++) {- int length = strlen(result[i]);+ int length = strlen(pathspec[i]); int to_copy = length;+ char *it; while (!(flags & KEEP_TRAILING_SLASH) &&- to_copy > 0 && is_dir_sep(result[i][to_copy - 1]))+ to_copy > 0 && is_dir_sep(pathspec[i][to_copy - 1])) to_copy--;- if (to_copy != length || flags & DUP_BASENAME) {- char *it = xmemdupz(result[i], to_copy);- if (flags & DUP_BASENAME) {- result[i] = xstrdup(basename(it));- free(it);- } else- result[i] = it;- }++ it = xmemdupz(pathspec[i], to_copy);+ if (flags & DUP_BASENAME) {+ result[i] = xstrdup(basename(it));+ free(it);+ } else+ result[i] = it;+ }+ result[count] = NULL;++ parse_pathspec(&ps,+ PATHSPEC_ALL_MAGIC &+ ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),+ PATHSPEC_KEEP_ORDER | PATHSPEC_PREFER_CWD,+ prefix, result);+ assert(count == ps.nr);++ /* Copy the pathspec and free the old intermediate strings */+ for (i = 0; i < count; i++) {+ const char *match = xstrdup(ps.items[i].match);+ free((char *) result[i]);+ result[i] = match;
Sigh.. it looks so weird that we do all the parsing (in a _copy_
pathspec function) then remove struct pathspec and return the plain
string. I guess we can't do anything more until we rework cmd_mv code
to handle pathspec natively.
At the least I think we should rename this function to something else.
But if you have time I really wish we could kill this function. I
haven't stared at cmd_mv() long and hard, but it looks to me that we
combining two separate functionalities in the same function here.
If "mv" takes n arguments, then the first <n-1> arguments may be
pathspec, the last one is always a plain path. The "dest_path =
internal_copy_pathspec..." could be as simple as "dest_path =
prefix_path(argv[argc - 1])". the special treatment for this last
argument [1] can live here. Then, we can do parse_pathspec for the
<n-1> arguments in cmd_mv(). It's still far from perfect, because
cmd_mv can't handle pathspec properly, but it reduces the messy mess
in internal_copy_pathspec a bit, I hope.
Actually, after looking at this a bit more it seems like we could
technically use prefix_path for both source and dest (based on how the
current code is structured) since the source's provied must all exist (as
in no wildcards are allowed). We could drop using the pathspec struct
completely in addition to renaming the function (to what I'm still
unsure).
Yeah that sounds good too (with a caveat: I'm not a heavy user of
git-mv nor touching this code a lot, I might be missing something).
It'll take some looong time before somebody starts converting it to
use pathspec properly, I guess. prefix_path() would keep the code
clean meanwhile.
K for now I'll switch to using prefix_path() and rename the function
`internal_prefix_pathspec()` as that is a bit more descriptive.
--
Brandon Williams
From: Brandon Williams <hidden> Date: 2016-12-08 18:20:05
On 12/08, Duy Nguyen wrote:
On Thu, Dec 8, 2016 at 7:03 AM, Brandon Williams [off-list ref] wrote:
quoted
On 12/07, Duy Nguyen wrote:
quoted
On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams [off-list ref] wrote:
quoted
Convert 'create_simplify()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec.
It would be even better to kill this create_simplify() and let
simplify_away() handle struct pathspec directly.
There is a bug in this code, that might have been found if we
simpify_away() handled pathspec directly: the memcmp() in
simplify_away() will not play well with :(icase) magic. My bad. If
:(icase) is used, the easiest/safe way is simplify nothing. Later on
maybe we can teach simplify_away() to do strncasecmp instead. We could
ignore exclude patterns there too (although not excluding is not a
bug).
So are you implying that the simplify struct needs to be killed? That
way the pathspec struct itself is being passed around instead?
Yes. simplify struct was a thing when pathspec was an array of char *.
At this point I think it can retire (when we have time to retire it)
Alright, then for now I can leave this change as is and have a follow up
series that kills the simplify struct.
--
Brandon Williams
From: Brandon Williams <hidden> Date: 2016-12-08 18:59:31
v2 of this series addresses the comments brought up in v1, most of which were
small cosmetic changes (since this is mostly a cosmetic series to begin with).
Brandon Williams (16):
mv: remove use of deprecated 'get_pathspec()'
dir: convert create_simplify to use the pathspec struct interface
dir: convert fill_directory to use the pathspec struct interface
ls-tree: convert show_recursive to use the pathspec struct interface
pathspec: remove the deprecated get_pathspec function
pathspec: copy and free owned memory
pathspec: remove unused variable from unsupported_magic
pathspec: always show mnemonic and name in unsupported_magic
pathspec: simpler logic to prefix original pathspec elements
pathspec: factor global magic into its own function
pathspec: create parse_short_magic function
pathspec: create parse_long_magic function
pathspec: create parse_element_magic helper
pathspec: create strip submodule slash helpers
pathspec: small readability changes
pathspec: rename prefix_pathspec to init_pathspec_item
Documentation/technical/api-setup.txt | 2 -
builtin/ls-tree.c | 16 +-
builtin/mv.c | 50 ++--
cache.h | 1 -
dir.c | 37 +--
pathspec.c | 468 +++++++++++++++++++---------------
pathspec.h | 5 +-
7 files changed, 317 insertions(+), 262 deletions(-)
@@ -20,13 +20,13 @@ static const char * const builtin_mv_usage[] = {#define DUP_BASENAME 1#define KEEP_TRAILING_SLASH 2-staticconstchar**internal_copy_pathspec(constchar*prefix,-constchar**pathspec,-intcount,unsignedflags)+staticconstchar**internal_prefix_pathspec(constchar*prefix,+constchar**pathspec,+intcount,unsignedflags){inti;constchar**result;-structpathspecps;+intprefixlen=prefix?strlen(prefix):0;ALLOC_ARRAY(result,count+1);/* Create an intermediate copy of the pathspec based on the flags */
@@ -42,25 +42,19 @@ static const char **internal_copy_pathspec(const char *prefix,if(flags&DUP_BASENAME){result[i]=xstrdup(basename(it));free(it);-}else+}else{result[i]=it;+}}result[count]=NULL;-parse_pathspec(&ps,-PATHSPEC_ALL_MAGIC&-~(PATHSPEC_FROMTOP|PATHSPEC_LITERAL),-PATHSPEC_KEEP_ORDER|PATHSPEC_PREFER_CWD,-prefix,result);-assert(count==ps.nr);--/* Copy the pathspec and free the old intermediate strings */+/* Prefix the pathspec and free the old intermediate strings */for(i=0;i<count;i++){+constchar*match=prefix_path(prefix,prefixlen,result[i]);free((char*)result[i]);-result[i]=xstrdup(ps.items[i].match);+result[i]=match;}-clear_pathspec(&ps);returnresult;}
@@ -158,16 +152,16 @@ int cmd_mv(int argc, const char **argv, const char *prefix)flags=KEEP_TRAILING_SLASH;if(argc==1&&is_directory(argv[0])&&!is_directory(argv[1]))flags=0;-dest_path=internal_copy_pathspec(prefix,argv+argc,1,flags);+dest_path=internal_prefix_pathspec(prefix,argv+argc,1,flags);submodule_gitfile=xcalloc(argc,sizeof(char*));if(dest_path[0][0]=='\0')/* special case: "." was normalized to "" */-destination=internal_copy_pathspec(dest_path[0],argv,argc,DUP_BASENAME);+destination=internal_prefix_pathspec(dest_path[0],argv,argc,DUP_BASENAME);elseif(!lstat(dest_path[0],&st)&&S_ISDIR(st.st_mode)){dest_path[0]=add_slash(dest_path[0]);-destination=internal_copy_pathspec(dest_path[0],argv,argc,DUP_BASENAME);+destination=internal_prefix_pathspec(dest_path[0],argv,argc,DUP_BASENAME);}else{if(argc!=1)die(_("destination '%s' is not a directory"),dest_path[0]);
@@ -318,6 +319,8 @@ static unsigned prefix_pathspec(struct pathspec_item *item,magic|=get_global_magic(element_magic);}+item->magic=magic;+if(pathspec_prefix>=0&&(prefixlen||(prefix&&*prefix)))die("BUG: 'prefix' magic is supposed to be used at worktree's root");
@@ -390,7 +393,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item,/* sanity checks, pathspec matchers assume these are sane */assert(item->nowildcard_len<=item->len&&item->prefix<=item->len);-returnmagic;}staticintpathspec_item_cmp(constvoid*a_,constvoid*b_)
From: Brandon Williams <hidden> Date: 2016-12-08 18:59:34
Convert the 'internal_copy_pathspec()' function to 'prefix_path()'
instead of using the deprecated 'get_pathspec()' interface. Also,
rename 'internal_copy_pathspec()' to 'internal_prefix_pathspec()' to be
more descriptive of what the funciton is actually doing.
In addition to this, fix a memory leak caused by only duplicating some
of the pathspec elements. Instead always duplicate all of the the
pathspec elements as an intermediate step (with modificationed based on
the passed in flags). This way the intermediate strings can then be
freed after getting the result from 'prefix_path()'.
Signed-off-by: Brandon Williams <redacted>
---
builtin/mv.c | 50 +++++++++++++++++++++++++++++++-------------------
1 file changed, 31 insertions(+), 19 deletions(-)
@@ -19,31 +20,42 @@ static const char * const builtin_mv_usage[] = {#define DUP_BASENAME 1#define KEEP_TRAILING_SLASH 2-staticconstchar**internal_copy_pathspec(constchar*prefix,-constchar**pathspec,-intcount,unsignedflags)+staticconstchar**internal_prefix_pathspec(constchar*prefix,+constchar**pathspec,+intcount,unsignedflags){inti;constchar**result;+intprefixlen=prefix?strlen(prefix):0;ALLOC_ARRAY(result,count+1);-COPY_ARRAY(result,pathspec,count);-result[count]=NULL;++/* Create an intermediate copy of the pathspec based on the flags */for(i=0;i<count;i++){-intlength=strlen(result[i]);+intlength=strlen(pathspec[i]);intto_copy=length;+char*it;while(!(flags&KEEP_TRAILING_SLASH)&&-to_copy>0&&is_dir_sep(result[i][to_copy-1]))+to_copy>0&&is_dir_sep(pathspec[i][to_copy-1]))to_copy--;-if(to_copy!=length||flags&DUP_BASENAME){-char*it=xmemdupz(result[i],to_copy);-if(flags&DUP_BASENAME){-result[i]=xstrdup(basename(it));-free(it);-}else-result[i]=it;++it=xmemdupz(pathspec[i],to_copy);+if(flags&DUP_BASENAME){+result[i]=xstrdup(basename(it));+free(it);+}else{+result[i]=it;}}-returnget_pathspec(prefix,result);+result[count]=NULL;++/* Prefix the pathspec and free the old intermediate strings */+for(i=0;i<count;i++){+constchar*match=prefix_path(prefix,prefixlen,result[i]);+free((char*)result[i]);+result[i]=match;+}++returnresult;}staticconstchar*add_slash(constchar*path)
@@ -140,16 +152,16 @@ int cmd_mv(int argc, const char **argv, const char *prefix)flags=KEEP_TRAILING_SLASH;if(argc==1&&is_directory(argv[0])&&!is_directory(argv[1]))flags=0;-dest_path=internal_copy_pathspec(prefix,argv+argc,1,flags);+dest_path=internal_prefix_pathspec(prefix,argv+argc,1,flags);submodule_gitfile=xcalloc(argc,sizeof(char*));if(dest_path[0][0]=='\0')/* special case: "." was normalized to "" */-destination=internal_copy_pathspec(dest_path[0],argv,argc,DUP_BASENAME);+destination=internal_prefix_pathspec(dest_path[0],argv,argc,DUP_BASENAME);elseif(!lstat(dest_path[0],&st)&&S_ISDIR(st.st_mode)){dest_path[0]=add_slash(dest_path[0]);-destination=internal_copy_pathspec(dest_path[0],argv,argc,DUP_BASENAME);+destination=internal_prefix_pathspec(dest_path[0],argv,argc,DUP_BASENAME);}else{if(argc!=1)die(_("destination '%s' is not a directory"),dest_path[0]);
From: Brandon Williams <hidden> Date: 2016-12-08 18:59:38
Convert 'create_simplify()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec.
Signed-off-by: Brandon Williams <redacted>
---
dir.c | 25 ++++++++++++-------------
1 file changed, 12 insertions(+), 13 deletions(-)
From: Brandon Williams <hidden> Date: 2016-12-08 18:59:40
Convert 'fill_directory()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec struct.
Signed-off-by: Brandon Williams <redacted>
---
dir.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
From: Brandon Williams <hidden> Date: 2016-12-08 18:59:46
The 'original' string entry in a pathspec_item is only duplicated some
of the time, instead always make a copy of the original and take
ownership of the memory.
Since both 'match' and 'original' string entries in a pathspec_item are
owned by the pathspec struct, they need to be freed when clearing the
pathspec struct (in 'clear_pathspec()') and duplicated when copying the
pathspec struct (in 'copy_pathspec()').
Also change the type of 'match' and 'original' to 'char *' in order to
more explicitly show the ownership of the memory.
Signed-off-by: Brandon Williams <redacted>
---
pathspec.c | 22 ++++++++++++++++++----
pathspec.h | 4 ++--
2 files changed, 20 insertions(+), 6 deletions(-)
From: Brandon Williams <hidden> Date: 2016-12-08 18:59:49
Factor out the logic responsible for parsing short magic into its own
function.
Signed-off-by: Brandon Williams <redacted>
---
pathspec.c | 54 ++++++++++++++++++++++++++++++++++++------------------
1 file changed, 36 insertions(+), 18 deletions(-)
From: Brandon Williams <hidden> Date: 2016-12-08 18:59:50
Create helper functions to read the global magic environment variables
in additon to factoring out the global magic gathering logic into its
own function.
Signed-off-by: Brandon Williams <redacted>
---
pathspec.c | 127 +++++++++++++++++++++++++++++++++++++------------------------
1 file changed, 78 insertions(+), 49 deletions(-)
@@ -87,6 +87,75 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)strbuf_addf(sb,",prefix:%d)",prefixlen);}+staticinlineintget_literal_global(void)+{+staticintliteral=-1;++if(literal<0)+literal=git_env_bool(GIT_LITERAL_PATHSPECS_ENVIRONMENT,0);++returnliteral;+}++staticinlineintget_glob_global(void)+{+staticintglob=-1;++if(glob<0)+glob=git_env_bool(GIT_GLOB_PATHSPECS_ENVIRONMENT,0);++returnglob;+}++staticinlineintget_noglob_global(void)+{+staticintnoglob=-1;++if(noglob<0)+noglob=git_env_bool(GIT_NOGLOB_PATHSPECS_ENVIRONMENT,0);++returnnoglob;+}++staticinlineintget_icase_global(void)+{+staticinticase=-1;++if(icase<0)+icase=git_env_bool(GIT_ICASE_PATHSPECS_ENVIRONMENT,0);++returnicase;+}++staticintget_global_magic(intelement_magic)+{+intglobal_magic=0;++if(get_literal_global())+global_magic|=PATHSPEC_LITERAL;++/* --glob-pathspec is overridden by :(literal) */+if(get_glob_global()&&!(element_magic&PATHSPEC_LITERAL))+global_magic|=PATHSPEC_GLOB;++if(get_glob_global()&&get_noglob_global())+die(_("global 'glob' and 'noglob' pathspec settings are incompatible"));++if(get_icase_global())+global_magic|=PATHSPEC_ICASE;++if((global_magic&PATHSPEC_LITERAL)&&+(global_magic&~PATHSPEC_LITERAL))+die(_("global 'literal' pathspec setting is incompatible "+"with all other global pathspec settings"));++/* --noglob-pathspec adds :(literal) _unless_ :(glob) is specified */+if(get_noglob_global()&&!(element_magic&PATHSPEC_GLOB))+global_magic|=PATHSPEC_LITERAL;++returnglobal_magic;+}+/**Takeanelementofapathspecandcheckformagicsignatures.*Appendtheresulttotheprefix.Returnthemagicbitmap.
@@ -104,46 +173,12 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,constchar*prefix,intprefixlen,constchar*elt){-staticintliteral_global=-1;-staticintglob_global=-1;-staticintnoglob_global=-1;-staticinticase_global=-1;-unsignedmagic=0,element_magic=0,global_magic=0;+unsignedmagic=0,element_magic=0;constchar*copyfrom=elt;char*match;inti,pathspec_prefix=-1;-if(literal_global<0)-literal_global=git_env_bool(GIT_LITERAL_PATHSPECS_ENVIRONMENT,0);-if(literal_global)-global_magic|=PATHSPEC_LITERAL;--if(glob_global<0)-glob_global=git_env_bool(GIT_GLOB_PATHSPECS_ENVIRONMENT,0);-if(glob_global)-global_magic|=PATHSPEC_GLOB;--if(noglob_global<0)-noglob_global=git_env_bool(GIT_NOGLOB_PATHSPECS_ENVIRONMENT,0);--if(glob_global&&noglob_global)-die(_("global 'glob' and 'noglob' pathspec settings are incompatible"));---if(icase_global<0)-icase_global=git_env_bool(GIT_ICASE_PATHSPECS_ENVIRONMENT,0);-if(icase_global)-global_magic|=PATHSPEC_ICASE;--if((global_magic&PATHSPEC_LITERAL)&&-(global_magic&~PATHSPEC_LITERAL))-die(_("global 'literal' pathspec setting is incompatible "-"with all other global pathspec settings"));--if(flags&PATHSPEC_LITERAL_PATH)-global_magic=0;--if(elt[0]!=':'||literal_global||+if(elt[0]!=':'||get_literal_global()||(flags&PATHSPEC_LITERAL_PATH)){;/* nothing to do */}elseif(elt[1]=='('){
@@ -207,15 +242,11 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,magic|=element_magic;-/* --noglob-pathspec adds :(literal) _unless_ :(glob) is specified */-if(noglob_global&&!(magic&PATHSPEC_GLOB))-global_magic|=PATHSPEC_LITERAL;--/* --glob-pathspec is overridden by :(literal) */-if((global_magic&PATHSPEC_GLOB)&&(magic&PATHSPEC_LITERAL))-global_magic&=~PATHSPEC_GLOB;--magic|=global_magic;+/* PATHSPEC_LITERAL_PATH ignores magic */+if(flags&PATHSPEC_LITERAL_PATH)+magic=PATHSPEC_LITERAL;+else+magic|=get_global_magic(element_magic);if(pathspec_prefix>=0&&(prefixlen||(prefix&&*prefix)))
@@ -241,7 +272,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,*original.Usefulforpassingtoanothercommand.*/if((flags&PATHSPEC_PREFIX_ORIGIN)&&-prefixlen&&!literal_global){+prefixlen&&!get_literal_global()){structstrbufsb=STRBUF_INIT;/* Preserve the actual prefix length of each pattern */
From: Brandon Williams <hidden> Date: 2016-12-08 18:59:51
The logic used to prefix an original pathspec element with 'prefix'
magic is more general purpose and can be used for more than just short
magic. Remove the extra code paths and rename 'prefix_short_magic' to
'prefix_magic' to better indicate that it can be used in more general
situations.
Also, slightly change the logic which decides when to prefix the
original element in order to prevent a pathspec of "." from getting
converted to "" (empty string).
Signed-off-by: Brandon Williams <redacted>
---
pathspec.c | 33 +++++++++++++--------------------
1 file changed, 13 insertions(+), 20 deletions(-)
@@ -183,7 +182,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,}if(*copyfrom!=')')die(_("Missing ')' at the end of pathspec magic in '%s'"),elt);-long_magic_end=copyfrom;copyfrom++;}else{/* shorthand */
@@ -242,18 +240,13 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,*Prefixthepathspec(keepallmagic)andassignto*original.Usefulforpassingtoanothercommand.*/-if(flags&PATHSPEC_PREFIX_ORIGIN){+if((flags&PATHSPEC_PREFIX_ORIGIN)&&+prefixlen&&!literal_global){structstrbufsb=STRBUF_INIT;-if(prefixlen&&!literal_global){-/* Preserve the actual prefix length of each pattern */-if(short_magic)-prefix_short_magic(&sb,prefixlen,short_magic);-elseif(long_magic_end){-strbuf_add(&sb,elt,long_magic_end-elt);-strbuf_addf(&sb,",prefix:%d)",prefixlen);-}else-strbuf_addf(&sb,":(prefix:%d)",prefixlen);-}++/* Preserve the actual prefix length of each pattern */+prefix_magic(&sb,prefixlen,element_magic);+strbuf_addstr(&sb,match);item->original=strbuf_detach(&sb,NULL);}else{
From: Brandon Williams <hidden> Date: 2016-12-08 18:59:56
Factor out the logic responsible for parsing long magic into its own
function. As well as hoist the prefix check logic outside of the inner
loop as there isn't anything that needs to be done after matching
"prefix:".
Signed-off-by: Brandon Williams <redacted>
---
pathspec.c | 92 ++++++++++++++++++++++++++++++++++++++------------------------
1 file changed, 57 insertions(+), 35 deletions(-)
@@ -157,6 +157,60 @@ static int get_global_magic(int element_magic)}/*+*Parsethepathspecelementlookingforlongmagic+*+*savesallmagicin'magic'+*ifprefixmagicisused,savetheprefixlengthin'prefix_len'+*returnsthepositionin'elem'afterallmagichasbeenparsed+*/+staticconstchar*parse_long_magic(unsigned*magic,int*prefix_len,+constchar*elem)+{+constchar*pos;+constchar*nextat;++for(pos=elem+2;*pos&&*pos!=')';pos=nextat){+size_tlen=strcspn(pos,",)");+inti;++if(pos[len]==',')+nextat=pos+len+1;/* handle ',' */+else+nextat=pos+len;/* handle ')' and '\0' */++if(!len)+continue;++if(starts_with(pos,"prefix:")){+char*endptr;+*prefix_len=strtol(pos+7,&endptr,10);+if(endptr-pos!=len)+die(_("invalid parameter for pathspec magic 'prefix'"));+continue;+}++for(i=0;i<ARRAY_SIZE(pathspec_magic);i++){+if(strlen(pathspec_magic[i].name)==len&&+!strncmp(pathspec_magic[i].name,pos,len)){+*magic|=pathspec_magic[i].bit;+break;+}+}++if(ARRAY_SIZE(pathspec_magic)<=i)+die(_("Invalid pathspec magic '%.*s' in '%s'"),+(int)len,pos,elem);+}++if(*pos!=')')+die(_("Missing ')' at the end of pathspec magic in '%s'"),+elem);+pos++;++returnpos;+}++/**Parsethepathspecelementlookingforshortmagic**savesallmagicin'magic'
@@ -218,41 +272,9 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,;/* nothing to do */}elseif(elt[1]=='('){/* longhand */-constchar*nextat;-for(copyfrom=elt+2;-*copyfrom&&*copyfrom!=')';-copyfrom=nextat){-size_tlen=strcspn(copyfrom,",)");-if(copyfrom[len]==',')-nextat=copyfrom+len+1;-else-/* handle ')' and '\0' */-nextat=copyfrom+len;-if(!len)-continue;-for(i=0;i<ARRAY_SIZE(pathspec_magic);i++){-if(strlen(pathspec_magic[i].name)==len&&-!strncmp(pathspec_magic[i].name,copyfrom,len)){-element_magic|=pathspec_magic[i].bit;-break;-}-if(starts_with(copyfrom,"prefix:")){-char*endptr;-pathspec_prefix=strtol(copyfrom+7,-&endptr,10);-if(endptr-copyfrom!=len)-die(_("invalid parameter for pathspec magic 'prefix'"));-/* "i" would be wrong, but it does not matter */-break;-}-}-if(ARRAY_SIZE(pathspec_magic)<=i)-die(_("Invalid pathspec magic '%.*s' in '%s'"),-(int)len,copyfrom,elt);-}-if(*copyfrom!=')')-die(_("Missing ')' at the end of pathspec magic in '%s'"),elt);-copyfrom++;+copyfrom=parse_long_magic(&element_magic,+&pathspec_prefix,+elt);}else{/* shorthand */copyfrom=parse_short_magic(&element_magic,elt);
From: Brandon Williams <hidden> Date: 2016-12-08 18:59:59
Give a more relevant name to the prefix_pathspec function as it does
more than just prefix a pathspec element.
Signed-off-by: Brandon Williams <redacted>
---
pathspec.c | 24 +++++++-----------------
1 file changed, 7 insertions(+), 17 deletions(-)
@@ -329,6 +319,8 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,magic|=get_global_magic(element_magic);}+item->magic=magic;+if(pathspec_prefix>=0&&(prefixlen||(prefix&&*prefix)))die("BUG: 'prefix' magic is supposed to be used at worktree's root");
@@ -401,7 +393,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,/* sanity checks, pathspec matchers assume these are sane */assert(item->nowildcard_len<=item->len&&item->prefix<=item->len);-returnmagic;}staticintpathspec_item_cmp(constvoid*a_,constvoid*b_)
From: Brandon Williams <hidden> Date: 2016-12-08 19:01:59
Factor out the logic responsible for the magic in a pathspec element
into its own function.
Also avoid calling into the parsing functions when
`PATHSPEC_LITERAL_PATH` is specified since it causes magic to be
ignored and all paths to be treated as literals.
Signed-off-by: Brandon Williams <redacted>
---
pathspec.c | 37 ++++++++++++++++++++-----------------
1 file changed, 20 insertions(+), 17 deletions(-)
From: Brandon Williams <hidden> Date: 2016-12-08 19:02:04
A few small changes to improve readability. This is done by grouping related
assignments, adding blank lines, ensuring lines are <80 characters, and
adding additional comments.
Signed-off-by: Brandon Williams <redacted>
---
pathspec.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
@@ -336,6 +336,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,if((magic&PATHSPEC_LITERAL)&&(magic&PATHSPEC_GLOB))die(_("%s: 'literal' and 'glob' are incompatible"),elt);+/* Create match string which will be used for pathspec matching */if(pathspec_prefix>=0){match=xstrdup(copyfrom);prefixlen=pathspec_prefix;
From: Brandon Williams <hidden> Date: 2016-12-08 19:02:07
Factor out the logic responsible for stripping the trailing slash on
pathspecs referencing submodules into its own function.
Signed-off-by: Brandon Williams <redacted>
---
pathspec.c | 68 ++++++++++++++++++++++++++++++++++++++------------------------
1 file changed, 42 insertions(+), 26 deletions(-)
From: Brandon Williams <hidden> Date: 2016-12-08 19:02:10
For better clarity, always show the mnemonic and name of the unsupported
magic being used. This lets users have a more clear understanding of
what magic feature isn't supported. And if they supplied a mnemonic,
the user will be told what its corresponding name is which will allow
them to more easily search the man pages for that magic type.
This also avoids passing an extra parameter around the pathspec
initialization code.
Signed-off-by: Brandon Williams <redacted>
---
pathspec.c | 23 ++++++++---------------
1 file changed, 8 insertions(+), 15 deletions(-)
From: Brandon Williams <hidden> Date: 2016-12-08 19:02:15
Convert 'show_recursive()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec struct.
Signed-off-by: Brandon Williams <redacted>
---
builtin/ls-tree.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
From: Brandon Williams <hidden> Date: 2016-12-08 19:02:37
Now that all callers of the old 'get_pathspec' interface have been
migrated to use the new pathspec struct interface it can be removed
from the codebase.
Since there are no more users of the '_raw' field in the pathspec struct
it can also be removed. This patch also removes the old functionality
of modifying the const char **argv array that was passed into
parse_pathspec. Instead the constructed 'match' string (which is a
pathspec element with the prefix prepended) is only stored in its
corresponding pathspec_item entry.
Signed-off-by: Brandon Williams <redacted>
---
Documentation/technical/api-setup.txt | 2 --
cache.h | 1 -
pathspec.c | 42 +++--------------------------------
pathspec.h | 1 -
4 files changed, 3 insertions(+), 43 deletions(-)
@@ -27,8 +27,6 @@ parse_pathspec(). This function takes several arguments: - prefix and args come from cmd_* functions-get_pathspec() is obsolete and should never be used in new code.- parse_pathspec() helps catch unsupported features and reject them politely. At a lower level, different pathspec-related functions may not support the same set of features. Such pathspec-sensitive
On Fri, Dec 9, 2016 at 1:19 AM, Brandon Williams [off-list ref] wrote:
On 12/08, Duy Nguyen wrote:
quoted
On Thu, Dec 8, 2016 at 7:03 AM, Brandon Williams [off-list ref] wrote:
quoted
On 12/07, Duy Nguyen wrote:
quoted
On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams [off-list ref] wrote:
quoted
Convert 'create_simplify()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec.
It would be even better to kill this create_simplify() and let
simplify_away() handle struct pathspec directly.
There is a bug in this code, that might have been found if we
simpify_away() handled pathspec directly: the memcmp() in
simplify_away() will not play well with :(icase) magic. My bad. If
:(icase) is used, the easiest/safe way is simplify nothing. Later on
maybe we can teach simplify_away() to do strncasecmp instead. We could
ignore exclude patterns there too (although not excluding is not a
bug).
So are you implying that the simplify struct needs to be killed? That
way the pathspec struct itself is being passed around instead?
Yes. simplify struct was a thing when pathspec was an array of char *.
At this point I think it can retire (when we have time to retire it)
Alright, then for now I can leave this change as is and have a follow up
series that kills the simplify struct.
Do let me know if you decide to drop it, so I can put it back in my backlog.
--
Duy
From: Brandon Williams <hidden> Date: 2016-12-09 19:23:34
On 12/09, Duy Nguyen wrote:
On Fri, Dec 9, 2016 at 1:19 AM, Brandon Williams [off-list ref] wrote:
quoted
On 12/08, Duy Nguyen wrote:
quoted
On Thu, Dec 8, 2016 at 7:03 AM, Brandon Williams [off-list ref] wrote:
quoted
On 12/07, Duy Nguyen wrote:
quoted
On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams [off-list ref] wrote:
quoted
Convert 'create_simplify()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec.
It would be even better to kill this create_simplify() and let
simplify_away() handle struct pathspec directly.
There is a bug in this code, that might have been found if we
simpify_away() handled pathspec directly: the memcmp() in
simplify_away() will not play well with :(icase) magic. My bad. If
:(icase) is used, the easiest/safe way is simplify nothing. Later on
maybe we can teach simplify_away() to do strncasecmp instead. We could
ignore exclude patterns there too (although not excluding is not a
bug).
So are you implying that the simplify struct needs to be killed? That
way the pathspec struct itself is being passed around instead?
Yes. simplify struct was a thing when pathspec was an array of char *.
At this point I think it can retire (when we have time to retire it)
Alright, then for now I can leave this change as is and have a follow up
series that kills the simplify struct.
Do let me know if you decide to drop it, so I can put it back in my backlog.
From: Brandon Williams <hidden> Date: 2016-12-13 22:49:29
On 12/09, Brandon Williams wrote:
On 12/09, Duy Nguyen wrote:
quoted
On Fri, Dec 9, 2016 at 1:19 AM, Brandon Williams [off-list ref] wrote:
quoted
On 12/08, Duy Nguyen wrote:
quoted
On Thu, Dec 8, 2016 at 7:03 AM, Brandon Williams [off-list ref] wrote:
quoted
On 12/07, Duy Nguyen wrote:
quoted
On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams [off-list ref] wrote:
quoted
Convert 'create_simplify()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec.
It would be even better to kill this create_simplify() and let
simplify_away() handle struct pathspec directly.
There is a bug in this code, that might have been found if we
simpify_away() handled pathspec directly: the memcmp() in
simplify_away() will not play well with :(icase) magic. My bad. If
:(icase) is used, the easiest/safe way is simplify nothing. Later on
maybe we can teach simplify_away() to do strncasecmp instead. We could
ignore exclude patterns there too (although not excluding is not a
bug).
So are you implying that the simplify struct needs to be killed? That
way the pathspec struct itself is being passed around instead?
Yes. simplify struct was a thing when pathspec was an array of char *.
At this point I think it can retire (when we have time to retire it)
Alright, then for now I can leave this change as is and have a follow up
series that kills the simplify struct.
Do let me know if you decide to drop it, so I can put it back in my backlog.
K will do
This actually turned out to be more straight forward than I thought.
I'll reroll this series again (with a few other changes) and include
killing the simplify struct.
--
Brandon Williams
From: Brandon Williams <hidden> Date: 2016-12-13 23:15:52
Differences in v3:
* more readable strip submodule slash helper function which conforms to git's
style guide. [14/16]
* instead of having create_simply() use struct pathspec directly, remove the
struct path_simplify entirely and use struct pathspec directly in both
simplify_away() and exclude_matches_pathspec(). [02/16]
* small style issues corrected from v2. [15/16]
Brandon Williams (16):
mv: remove use of deprecated 'get_pathspec()'
dir: remove struct path_simplify
dir: convert fill_directory to use the pathspec struct interface
ls-tree: convert show_recursive to use the pathspec struct interface
pathspec: remove the deprecated get_pathspec function
pathspec: copy and free owned memory
pathspec: remove unused variable from unsupported_magic
pathspec: always show mnemonic and name in unsupported_magic
pathspec: simpler logic to prefix original pathspec elements
pathspec: factor global magic into its own function
pathspec: create parse_short_magic function
pathspec: create parse_long_magic function
pathspec: create parse_element_magic helper
pathspec: create strip submodule slash helpers
pathspec: small readability changes
pathspec: rename prefix_pathspec to init_pathspec_item
Documentation/technical/api-setup.txt | 2 -
builtin/ls-tree.c | 16 +-
builtin/mv.c | 50 ++--
cache.h | 1 -
dir.c | 166 +++++-------
pathspec.c | 476 +++++++++++++++++++---------------
pathspec.h | 5 +-
7 files changed, 369 insertions(+), 347 deletions(-)
@@ -1316,7 +1311,7 @@ static enum exist_status directory_exists_in_index(const char *dirname, int len)staticenumpath_treatmenttreat_directory(structdir_struct*dir,structuntracked_cache_dir*untracked,constchar*dirname,intlen,intbaselen,intexclude,-conststructpath_simplify*simplify)+conststructpathspec*pathspec){/* The "len-1" is to strip the final '/' */switch(directory_exists_in_index(dirname,len-1)){
@@ -1723,7 +1725,8 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,while(!read_cached_dir(&cdir)){/* check how the file or directory should be treated */-state=treat_path(dir,untracked,&cdir,&path,baselen,simplify);+state=treat_path(dir,untracked,&cdir,&path,+baselen,pathspec);if(state>dir_state)dir_state=state;
@@ -1843,9 +1821,9 @@ static int treat_leading_path(struct dir_struct *dir,strbuf_add(&sb,path,baselen);if(!is_directory(sb.buf))break;-if(simplify_away(sb.buf,sb.len,simplify))+if(simplify_away(sb.buf,sb.len,pathspec))break;-if(treat_one_path(dir,NULL,&sb,baselen,simplify,+if(treat_one_path(dir,NULL,&sb,baselen,pathspec,DT_DIR,NULL)==path_none)break;/* do not recurse into it */if(len<=baselen){
From: Brandon Williams <hidden> Date: 2016-12-13 23:15:57
Teach simplify_away() and exclude_matches_pathspec() to handle struct
pathspec directly, eliminating the need for the struct path_simplify.
Also renamed the len parameter to pathlen in exclude_matches_pathspec()
to match the parameter names used in simplify_away().
Signed-off-by: Brandon Williams <redacted>
---
dir.c | 154 ++++++++++++++++++++++++++----------------------------------------
1 file changed, 60 insertions(+), 94 deletions(-)
@@ -1312,7 +1307,7 @@ static enum exist_status directory_exists_in_index(const char *dirname, int len)staticenumpath_treatmenttreat_directory(structdir_struct*dir,structuntracked_cache_dir*untracked,constchar*dirname,intlen,intbaselen,intexclude,-conststructpath_simplify*simplify)+conststructpathspec*pathspec){/* The "len-1" is to strip the final '/' */switch(directory_exists_in_index(dirname,len-1)){
@@ -1719,7 +1721,8 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,while(!read_cached_dir(&cdir)){/* check how the file or directory should be treated */-state=treat_path(dir,untracked,&cdir,&path,baselen,simplify);+state=treat_path(dir,untracked,&cdir,&path,+baselen,pathspec);if(state>dir_state)dir_state=state;
@@ -1840,9 +1817,9 @@ static int treat_leading_path(struct dir_struct *dir,strbuf_add(&sb,path,baselen);if(!is_directory(sb.buf))break;-if(simplify_away(sb.buf,sb.len,simplify))+if(simplify_away(sb.buf,sb.len,pathspec))break;-if(treat_one_path(dir,NULL,&sb,baselen,simplify,+if(treat_one_path(dir,NULL,&sb,baselen,pathspec,DT_DIR,NULL)==path_none)break;/* do not recurse into it */if(len<=baselen){
From: Brandon Williams <hidden> Date: 2016-12-13 23:16:36
Convert 'show_recursive()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec struct.
Signed-off-by: Brandon Williams <redacted>
---
builtin/ls-tree.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
From: Brandon Williams <hidden> Date: 2016-12-13 23:16:40
Convert 'fill_directory()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec struct.
Signed-off-by: Brandon Williams <redacted>
---
dir.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
From: Brandon Williams <hidden> Date: 2016-12-13 23:16:50
For better clarity, always show the mnemonic and name of the unsupported
magic being used. This lets users have a more clear understanding of
what magic feature isn't supported. And if they supplied a mnemonic,
the user will be told what its corresponding name is which will allow
them to more easily search the man pages for that magic type.
This also avoids passing an extra parameter around the pathspec
initialization code.
Signed-off-by: Brandon Williams <redacted>
---
pathspec.c | 21 +++++++--------------
1 file changed, 7 insertions(+), 14 deletions(-)
From: Brandon Williams <hidden> Date: 2016-12-13 23:16:53
The 'original' string entry in a pathspec_item is only duplicated some
of the time, instead always make a copy of the original and take
ownership of the memory.
Since both 'match' and 'original' string entries in a pathspec_item are
owned by the pathspec struct, they need to be freed when clearing the
pathspec struct (in 'clear_pathspec()') and duplicated when copying the
pathspec struct (in 'copy_pathspec()').
Also change the type of 'match' and 'original' to 'char *' in order to
more explicitly show the ownership of the memory.
Signed-off-by: Brandon Williams <redacted>
---
pathspec.c | 22 ++++++++++++++++++----
pathspec.h | 4 ++--
2 files changed, 20 insertions(+), 6 deletions(-)
From: Brandon Williams <hidden> Date: 2016-12-13 23:16:54
Convert the 'internal_copy_pathspec()' function to 'prefix_path()'
instead of using the deprecated 'get_pathspec()' interface. Also,
rename 'internal_copy_pathspec()' to 'internal_prefix_pathspec()' to be
more descriptive of what the funciton is actually doing.
In addition to this, fix a memory leak caused by only duplicating some
of the pathspec elements. Instead always duplicate all of the the
pathspec elements as an intermediate step (with modificationed based on
the passed in flags). This way the intermediate strings can then be
freed after getting the result from 'prefix_path()'.
Signed-off-by: Brandon Williams <redacted>
---
builtin/mv.c | 50 +++++++++++++++++++++++++++++++-------------------
1 file changed, 31 insertions(+), 19 deletions(-)
@@ -19,31 +20,42 @@ static const char * const builtin_mv_usage[] = {#define DUP_BASENAME 1#define KEEP_TRAILING_SLASH 2-staticconstchar**internal_copy_pathspec(constchar*prefix,-constchar**pathspec,-intcount,unsignedflags)+staticconstchar**internal_prefix_pathspec(constchar*prefix,+constchar**pathspec,+intcount,unsignedflags){inti;constchar**result;+intprefixlen=prefix?strlen(prefix):0;ALLOC_ARRAY(result,count+1);-COPY_ARRAY(result,pathspec,count);-result[count]=NULL;++/* Create an intermediate copy of the pathspec based on the flags */for(i=0;i<count;i++){-intlength=strlen(result[i]);+intlength=strlen(pathspec[i]);intto_copy=length;+char*it;while(!(flags&KEEP_TRAILING_SLASH)&&-to_copy>0&&is_dir_sep(result[i][to_copy-1]))+to_copy>0&&is_dir_sep(pathspec[i][to_copy-1]))to_copy--;-if(to_copy!=length||flags&DUP_BASENAME){-char*it=xmemdupz(result[i],to_copy);-if(flags&DUP_BASENAME){-result[i]=xstrdup(basename(it));-free(it);-}else-result[i]=it;++it=xmemdupz(pathspec[i],to_copy);+if(flags&DUP_BASENAME){+result[i]=xstrdup(basename(it));+free(it);+}else{+result[i]=it;}}-returnget_pathspec(prefix,result);+result[count]=NULL;++/* Prefix the pathspec and free the old intermediate strings */+for(i=0;i<count;i++){+constchar*match=prefix_path(prefix,prefixlen,result[i]);+free((char*)result[i]);+result[i]=match;+}++returnresult;}staticconstchar*add_slash(constchar*path)
@@ -140,16 +152,16 @@ int cmd_mv(int argc, const char **argv, const char *prefix)flags=KEEP_TRAILING_SLASH;if(argc==1&&is_directory(argv[0])&&!is_directory(argv[1]))flags=0;-dest_path=internal_copy_pathspec(prefix,argv+argc,1,flags);+dest_path=internal_prefix_pathspec(prefix,argv+argc,1,flags);submodule_gitfile=xcalloc(argc,sizeof(char*));if(dest_path[0][0]=='\0')/* special case: "." was normalized to "" */-destination=internal_copy_pathspec(dest_path[0],argv,argc,DUP_BASENAME);+destination=internal_prefix_pathspec(dest_path[0],argv,argc,DUP_BASENAME);elseif(!lstat(dest_path[0],&st)&&S_ISDIR(st.st_mode)){dest_path[0]=add_slash(dest_path[0]);-destination=internal_copy_pathspec(dest_path[0],argv,argc,DUP_BASENAME);+destination=internal_prefix_pathspec(dest_path[0],argv,argc,DUP_BASENAME);}else{if(argc!=1)die(_("destination '%s' is not a directory"),dest_path[0]);
From: Brandon Williams <hidden> Date: 2016-12-13 23:17:11
The logic used to prefix an original pathspec element with 'prefix'
magic is more general purpose and can be used for more than just short
magic. Remove the extra code paths and rename 'prefix_short_magic' to
'prefix_magic' to better indicate that it can be used in more general
situations.
Also, slightly change the logic which decides when to prefix the
original element in order to prevent a pathspec of "." from getting
converted to "" (empty string).
Signed-off-by: Brandon Williams <redacted>
---
pathspec.c | 33 +++++++++++++--------------------
1 file changed, 13 insertions(+), 20 deletions(-)
@@ -183,7 +182,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,}if(*copyfrom!=')')die(_("Missing ')' at the end of pathspec magic in '%s'"),elt);-long_magic_end=copyfrom;copyfrom++;}else{/* shorthand */
@@ -242,18 +240,13 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,*Prefixthepathspec(keepallmagic)andassignto*original.Usefulforpassingtoanothercommand.*/-if(flags&PATHSPEC_PREFIX_ORIGIN){+if((flags&PATHSPEC_PREFIX_ORIGIN)&&+prefixlen&&!literal_global){structstrbufsb=STRBUF_INIT;-if(prefixlen&&!literal_global){-/* Preserve the actual prefix length of each pattern */-if(short_magic)-prefix_short_magic(&sb,prefixlen,short_magic);-elseif(long_magic_end){-strbuf_add(&sb,elt,long_magic_end-elt);-strbuf_addf(&sb,",prefix:%d)",prefixlen);-}else-strbuf_addf(&sb,":(prefix:%d)",prefixlen);-}++/* Preserve the actual prefix length of each pattern */+prefix_magic(&sb,prefixlen,element_magic);+strbuf_addstr(&sb,match);item->original=strbuf_detach(&sb,NULL);}else{
From: Brandon Williams <hidden> Date: 2016-12-13 23:17:16
Now that all callers of the old 'get_pathspec' interface have been
migrated to use the new pathspec struct interface it can be removed
from the codebase.
Since there are no more users of the '_raw' field in the pathspec struct
it can also be removed. This patch also removes the old functionality
of modifying the const char **argv array that was passed into
parse_pathspec. Instead the constructed 'match' string (which is a
pathspec element with the prefix prepended) is only stored in its
corresponding pathspec_item entry.
Signed-off-by: Brandon Williams <redacted>
---
Documentation/technical/api-setup.txt | 2 --
cache.h | 1 -
pathspec.c | 42 +++--------------------------------
pathspec.h | 1 -
4 files changed, 3 insertions(+), 43 deletions(-)
@@ -27,8 +27,6 @@ parse_pathspec(). This function takes several arguments: - prefix and args come from cmd_* functions-get_pathspec() is obsolete and should never be used in new code.- parse_pathspec() helps catch unsupported features and reject them politely. At a lower level, different pathspec-related functions may not support the same set of features. Such pathspec-sensitive