Re: [PATCH v2 04/12] worktree.c: mark current worktree

[Junio C Hamano]

Jeff King  writes:

> To me, the benefit is that you don't have to care about ignore_case. You
> have asked to compare two paths, and any system-appropriate magic should
> be applied. That may be icase, or it may be weird unicode normalization.
>
> I think the key thing missing is that this is only about _filesystem_
> paths. You would not want to use it for tree-to-tree pathname
> comparisons. So maybe "fspath" or something would be more descriptive.

Yup, I would be very happy with "fs" in the name.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 04/12] worktree.c: mark current worktree

[Jeff King]

On Thu, Apr 21, 2016 at 08:37:32AM -0700, Junio C Hamano wrote:

> >> > While we're at it, how about renaming it to pathcmp (and its friend
> >> > strncmp_icase to pathncmp)?
> >> 
> >> Yes, that seems like a good idea. For anyone familiar with
> >> strcasecmp() or stricmp(), having "icase" in the name makes it seem as
> >> though it's unconditionally case-insensitive, so dropping it from the
> >> name would likely be beneficial.
> >
> > Seconded (thirded?). I have been caught by this confusion in the past,
> > too.
> 
> I agree that strcmp_icase() gives a false impression that it always
> ignores case differences, but a new name that does not at all hint
> that it may do icase comparison as necessary will catch me by an
> opposite confusion in the future.

To me, the benefit is that you don't have to care about ignore_case. You
have asked to compare two paths, and any system-appropriate magic should
be applied. That may be icase, or it may be weird unicode normalization.

I think the key thing missing is that this is only about _filesystem_
paths. You would not want to use it for tree-to-tree pathname
comparisons. So maybe "fspath" or something would be more descriptive.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 04/12] worktree.c: mark current worktree

[Junio C Hamano]

Jeff King  writes:

> On Thu, Apr 21, 2016 at 10:23:09AM -0400, Eric Sunshine wrote:
>
>> > While we're at it, how about renaming it to pathcmp (and its friend
>> > strncmp_icase to pathncmp)?
>> 
>> Yes, that seems like a good idea. For anyone familiar with
>> strcasecmp() or stricmp(), having "icase" in the name makes it seem as
>> though it's unconditionally case-insensitive, so dropping it from the
>> name would likely be beneficial.
>
> Seconded (thirded?). I have been caught by this confusion in the past,
> too.

I agree that strcmp_icase() gives a false impression that it always
ignores case differences, but a new name that does not at all hint
that it may do icase comparison as necessary will catch me by an
opposite confusion in the future.

I have not yet formed a firm opinion if pathcmp() conveys enough
hint.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 04/12] worktree.c: mark current worktree

[Jeff King]

On Thu, Apr 21, 2016 at 10:23:09AM -0400, Eric Sunshine wrote:

> > While we're at it, how about renaming it to pathcmp (and its friend
> > strncmp_icase to pathncmp)?
> 
> Yes, that seems like a good idea. For anyone familiar with
> strcasecmp() or stricmp(), having "icase" in the name makes it seem as
> though it's unconditionally case-insensitive, so dropping it from the
> name would likely be beneficial.

Seconded (thirded?). I have been caught by this confusion in the past,
too.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 04/12] worktree.c: mark current worktree

[Eric Sunshine]

On Thu, Apr 21, 2016 at 5:33 AM, Duy Nguyen  wrote:
> On Thu, Apr 21, 2016 at 3:19 PM, Duy Nguyen  wrote:
>> On Thu, Apr 21, 2016 at 2:20 PM, Eric Sunshine  
>> wrote:
>>> On Wed, Apr 20, 2016 at 9:24 AM, Nguyễn Thái Ngọc Duy  
>>> wrote:
 Signed-off-by: Nguyễn Thái Ngọc Duy 
 +   strbuf_addstr(_dir, absolute_path(get_git_dir()));
 +   for (i = 0; i < counter; i++) {
 +   struct worktree *wt = list[i];
 +   strbuf_addstr(, 
 absolute_path(get_worktree_git_dir(wt)));
 +   wt->is_current = !strcmp_icase(git_dir.buf, path.buf);
>>>
>>> Can you talk a bit about why this uses 'icase'? Should it be
>>> respecting cache.h:ignore_case?
>>
>> It does.That function (in dir.c) is just one-liner
>>
>> return ignore_case ? strcasecmp(a, b) : strcmp(a, b);
>>
>> I admit though, the naming does not make that clear.

Ugh, this is only the fourth patch, yet the second stupid review
mistake I've made thus far in this series. For some reason, I kept
reading this as a call to strcasecmp() or stricmp() rather than
strcmp_icase(). Worse, I had even consulted path.c:strcmp_icase() to
see how the issue was handled there.

> While we're at it, how about renaming it to pathcmp (and its friend
> strncmp_icase to pathncmp)?

Yes, that seems like a good idea. For anyone familiar with
strcasecmp() or stricmp(), having "icase" in the name makes it seem as
though it's unconditionally case-insensitive, so dropping it from the
name would likely be beneficial.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 04/12] worktree.c: mark current worktree

[Duy Nguyen]

On Thu, Apr 21, 2016 at 3:19 PM, Duy Nguyen  wrote:
> On Thu, Apr 21, 2016 at 2:20 PM, Eric Sunshine  
> wrote:
>> On Wed, Apr 20, 2016 at 9:24 AM, Nguyễn Thái Ngọc Duy  
>> wrote:
>>> Signed-off-by: Nguyễn Thái Ngọc Duy 
>>> ---
>>> diff --git a/worktree.c b/worktree.c
>>> @@ -178,6 +182,18 @@ struct worktree **get_worktrees(void)
>>> }
>>> ALLOC_GROW(list, counter + 1, alloc);
>>> list[counter] = NULL;
>>> +
>>> +   strbuf_addstr(_dir, absolute_path(get_git_dir()));
>>> +   for (i = 0; i < counter; i++) {
>>> +   struct worktree *wt = list[i];
>>> +   strbuf_addstr(, 
>>> absolute_path(get_worktree_git_dir(wt)));
>>> +   wt->is_current = !strcmp_icase(git_dir.buf, path.buf);
>>
>> Can you talk a bit about why this uses 'icase'? Should it be
>> respecting cache.h:ignore_case?
>
> It does.That function (in dir.c) is just one-liner
>
> return ignore_case ? strcasecmp(a, b) : strcmp(a, b);
>
> I admit though, the naming does not make that clear.

While we're at it, how about renaming it to pathcmp (and its friend
strncmp_icase to pathncmp)?
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 04/12] worktree.c: mark current worktree

[Duy Nguyen]

On Thu, Apr 21, 2016 at 2:20 PM, Eric Sunshine  wrote:
> On Wed, Apr 20, 2016 at 9:24 AM, Nguyễn Thái Ngọc Duy  
> wrote:
>> Signed-off-by: Nguyễn Thái Ngọc Duy 
>> ---
>> diff --git a/worktree.c b/worktree.c
>> @@ -178,6 +182,18 @@ struct worktree **get_worktrees(void)
>> }
>> ALLOC_GROW(list, counter + 1, alloc);
>> list[counter] = NULL;
>> +
>> +   strbuf_addstr(_dir, absolute_path(get_git_dir()));
>> +   for (i = 0; i < counter; i++) {
>> +   struct worktree *wt = list[i];
>> +   strbuf_addstr(, 
>> absolute_path(get_worktree_git_dir(wt)));
>> +   wt->is_current = !strcmp_icase(git_dir.buf, path.buf);
>
> Can you talk a bit about why this uses 'icase'? Should it be
> respecting cache.h:ignore_case?

It does.That function (in dir.c) is just one-liner

return ignore_case ? strcasecmp(a, b) : strcmp(a, b);

I admit though, the naming does not make that clear.

>> +   strbuf_reset();
>> +   if (wt->is_current)
>> +   break;
>> +   }
>> +   strbuf_release(_dir);
>> +   strbuf_release();
>
> Minor: Would it make sense to place this new code in its own function
> -- say, mark_current_worktree() -- to keep get_worktrees() from
> becoming overlong?

Good idea. Will do.
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 04/12] worktree.c: mark current worktree

[Eric Sunshine]

On Wed, Apr 20, 2016 at 9:24 AM, Nguyễn Thái Ngọc Duy  wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/worktree.c b/worktree.c
> @@ -178,6 +182,18 @@ struct worktree **get_worktrees(void)
> }
> ALLOC_GROW(list, counter + 1, alloc);
> list[counter] = NULL;
> +
> +   strbuf_addstr(_dir, absolute_path(get_git_dir()));
> +   for (i = 0; i < counter; i++) {
> +   struct worktree *wt = list[i];
> +   strbuf_addstr(, absolute_path(get_worktree_git_dir(wt)));
> +   wt->is_current = !strcmp_icase(git_dir.buf, path.buf);

Can you talk a bit about why this uses 'icase'? Should it be
respecting cache.h:ignore_case?

> +   strbuf_reset();
> +   if (wt->is_current)
> +   break;
> +   }
> +   strbuf_release(_dir);
> +   strbuf_release();

Minor: Would it make sense to place this new code in its own function
-- say, mark_current_worktree() -- to keep get_worktrees() from
becoming overlong?

> return list;
>  }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 04/12] worktree.c: mark current worktree

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 worktree.c | 18 +-
 worktree.h |  1 +
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/worktree.c b/worktree.c
index 360ba41..452f64a 100644
--- a/worktree.c
+++ b/worktree.c
@@ -2,6 +2,7 @@
 #include "refs.h"
 #include "strbuf.h"
 #include "worktree.h"
+#include "dir.h"
 
 void free_worktrees(struct worktree **worktrees)
 {
@@ -94,6 +95,7 @@ static struct worktree *get_main_worktree(void)
worktree->is_bare = is_bare;
worktree->head_ref = NULL;
worktree->is_detached = is_detached;
+   worktree->is_current = 0;
add_head_info(_ref, worktree);
 
 done:
@@ -138,6 +140,7 @@ static struct worktree *get_linked_worktree(const char *id)
worktree->is_bare = 0;
worktree->head_ref = NULL;
worktree->is_detached = is_detached;
+   worktree->is_current = 0;
add_head_info(_ref, worktree);
 
 done:
@@ -150,10 +153,11 @@ done:
 struct worktree **get_worktrees(void)
 {
struct worktree **list = NULL;
+   struct strbuf git_dir = STRBUF_INIT;
struct strbuf path = STRBUF_INIT;
DIR *dir;
struct dirent *d;
-   int counter = 0, alloc = 2;
+   int i, counter = 0, alloc = 2;
 
list = xmalloc(alloc * sizeof(struct worktree *));
 
@@ -178,6 +182,18 @@ struct worktree **get_worktrees(void)
}
ALLOC_GROW(list, counter + 1, alloc);
list[counter] = NULL;
+
+   strbuf_addstr(_dir, absolute_path(get_git_dir()));
+   for (i = 0; i < counter; i++) {
+   struct worktree *wt = list[i];
+   strbuf_addstr(, absolute_path(get_worktree_git_dir(wt)));
+   wt->is_current = !strcmp_icase(git_dir.buf, path.buf);
+   strbuf_reset();
+   if (wt->is_current)
+   break;
+   }
+   strbuf_release(_dir);
+   strbuf_release();
return list;
 }
 
diff --git a/worktree.h b/worktree.h
index d71d7ec..625fb8d 100644
--- a/worktree.h
+++ b/worktree.h
@@ -8,6 +8,7 @@ struct worktree {
unsigned char head_sha1[20];
int is_detached;
int is_bare;
+   int is_current;
 };
 
 /* Functions for acting on the information about worktrees. */
-- 
2.8.0.rc0.210.gd302cd2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html