Re: [PATCH v2 14/16] pathspec: create strip submodule slash helpers

[Duy Nguyen]

On Fri, Dec 9, 2016 at 7:28 AM, Junio C Hamano  wrote:
> Brandon Williams  writes:
>
>> +static void strip_submodule_slash_cheap(struct pathspec_item *item)
>> +{
>> + int i;
>> +
>> + if ((item->len >= 1 && item->match[item->len - 1] == '/') &&
>> + (i = cache_name_pos(item->match, item->len - 1)) >= 0 &&
>> + S_ISGITLINK(active_cache[i]->ce_mode)) {
>> + item->len--;
>> + item->match[item->len] = '\0';
>> + }
>> +}
>
> I know that this is merely a moved code, but while I was reading
> this, it triggered "Do not make an assignment inside if condition"
> check.

Yeah with a function of its own, it's probably better to separate that
assignment.

> But more importantly, is the code even correct?  If the path
> for the submodule is unmerged, we would get a negative i that points
> at the conflicting entry; don't we want to do something about it, at
> least when we have a submodule entry at stage #2 (i.e. ours)?

In my defense I was simply moving (again!) the code from
strip_trailing_slash_from_submodules() in b69bb3f:builtin/ls-files.c.
Could be an improvement point for submodule people, I guess.
-- 
Duy

Re: [PATCH v2 14/16] pathspec: create strip submodule slash helpers

[Junio C Hamano]

Brandon Williams  writes:

> +static void strip_submodule_slash_cheap(struct pathspec_item *item)
> +{
> + int i;
> +
> + if ((item->len >= 1 && item->match[item->len - 1] == '/') &&
> + (i = cache_name_pos(item->match, item->len - 1)) >= 0 &&
> + S_ISGITLINK(active_cache[i]->ce_mode)) {
> + item->len--;
> + item->match[item->len] = '\0';
> + }
> +}

I know that this is merely a moved code, but while I was reading
this, it triggered "Do not make an assignment inside if condition"
check.  But more importantly, is the code even correct?  If the path
for the submodule is unmerged, we would get a negative i that points
at the conflicting entry; don't we want to do something about it, at
least when we have a submodule entry at stage #2 (i.e. ours)?

[PATCH v2 14/16] pathspec: create strip submodule slash helpers

[Brandon Williams]

Factor out the logic responsible for stripping the trailing slash on
pathspecs referencing submodules into its own function.

Signed-off-by: Brandon Williams 
---
 pathspec.c | 68 ++
 1 file changed, 42 insertions(+), 26 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index f37f887..cf88390 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -258,6 +258,44 @@ static const char *parse_element_magic(unsigned *magic, 
int *prefix_len,
return parse_short_magic(magic, elem);
 }
 
+static void strip_submodule_slash_cheap(struct pathspec_item *item)
+{
+   int i;
+
+   if ((item->len >= 1 && item->match[item->len - 1] == '/') &&
+   (i = cache_name_pos(item->match, item->len - 1)) >= 0 &&
+   S_ISGITLINK(active_cache[i]->ce_mode)) {
+   item->len--;
+   item->match[item->len] = '\0';
+   }
+}
+
+static void strip_submodule_slash_expensive(struct pathspec_item *item)
+{
+   int i;
+
+   for (i = 0; i < active_nr; i++) {
+   struct cache_entry *ce = active_cache[i];
+   int ce_len = ce_namelen(ce);
+
+   if (!S_ISGITLINK(ce->ce_mode))
+   continue;
+
+   if (item->len <= ce_len || item->match[ce_len] != '/' ||
+   memcmp(ce->name, item->match, ce_len))
+   continue;
+
+   if (item->len == ce_len + 1) {
+   /* strip trailing slash */
+   item->len--;
+   item->match[item->len] = '\0';
+   } else {
+   die (_("Pathspec '%s' is in submodule '%.*s'"),
+item->original, ce_len, ce->name);
+   }
+   }
+}
+
 /*
  * Take an element of a pathspec and check for magic signatures.
  * Append the result to the prefix. Return the magic bitmap.
@@ -278,7 +316,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item, 
unsigned flags,
unsigned magic = 0, element_magic = 0;
const char *copyfrom = elt;
char *match;
-   int i, pathspec_prefix = -1;
+   int pathspec_prefix = -1;
 
/* PATHSPEC_LITERAL_PATH ignores magic */
if (flags & PATHSPEC_LITERAL_PATH) {
@@ -329,33 +367,11 @@ static unsigned prefix_pathspec(struct pathspec_item 
*item, unsigned flags,
item->len = strlen(item->match);
item->prefix = prefixlen;
 
-   if ((flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) &&
-   (item->len >= 1 && item->match[item->len - 1] == '/') &&
-   (i = cache_name_pos(item->match, item->len - 1)) >= 0 &&
-   S_ISGITLINK(active_cache[i]->ce_mode)) {
-   item->len--;
-   match[item->len] = '\0';
-   }
+   if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
+   strip_submodule_slash_cheap(item);
 
if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
-   for (i = 0; i < active_nr; i++) {
-   struct cache_entry *ce = active_cache[i];
-   int ce_len = ce_namelen(ce);
-
-   if (!S_ISGITLINK(ce->ce_mode))
-   continue;
-
-   if (item->len <= ce_len || match[ce_len] != '/' ||
-   memcmp(ce->name, match, ce_len))
-   continue;
-   if (item->len == ce_len + 1) {
-   /* strip trailing slash */
-   item->len--;
-   match[item->len] = '\0';
-   } else
-   die (_("Pathspec '%s' is in submodule '%.*s'"),
-elt, ce_len, ce->name);
-   }
+   strip_submodule_slash_expensive(item);
 
if (magic & PATHSPEC_LITERAL)
item->nowildcard_len = item->len;
-- 
2.8.0.rc3.226.g39d4020