Re: [PATCH v1 11/45] parse_pathspec: support stripping submodule trailing slashes

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

I see that having cheap and expensive variant at these steps is a
way to achieve a bug-for-bug compatible rewrite of the existing
code, but in the longer term should we lose one of them?  After all,
a path that points at the working tree of another repository that is
beyond a symlink cannot be added to us as a submodule, so CHEAP can
be used only when we know that any intermediate component on the
path is not a symlink pointing elsewhere, and the user in the
codepath of ls-files may know it.

To put it differently, shouldn't CHEAP and EXPENSIVE be accompanied
by in-code comment and updates to Documentation/technical/api-*.txt
to help users of the API to decide which one to use?

On Mon, Mar 18, 2013 at 4:55 AM, Junio C Hamano [off-list ref] wrote:

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

quoted

This flag is equivalent to builtin/ls-files.c:strip_trailing_slashes()
and is intended to replace that function when ls-files is converted to
use parse_pathspec.

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 pathspec.c | 9 +++++++++
 pathspec.h | 1 +
 2 files changed, 10 insertions(+)

diff --git a/pathspec.c b/pathspec.c
index b2446c3..2da8bc9 100644
--- a/pathspec.c
+++ b/pathspec.c

@@ -204,6 +204,15 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
      *raw = item->match = match;
      item->original = elt;
      item->len = strlen(item->match);
+
+     if ((flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) &&

I see that having cheap and expensive variant at these steps is a
way to achieve a bug-for-bug compatible rewrite of the existing
code, but in the longer term should we lose one of them?  After all,
a path that points at the working tree of another repository that is
beyond a symlink cannot be added to us as a submodule, so CHEAP can
be used only when we know that any intermediate component on the
path is not a symlink pointing elsewhere, and the user in the
codepath of ls-files may know it.

I did not concentrate on the future part. But yes only one should
remain in long term. Cheap vs expensive does not say much.

To put it differently, shouldn't CHEAP and EXPENSIVE be accompanied
by in-code comment and updates to Documentation/technical/api-*.txt
to help users of the API to decide which one to use?

Will do (also for the comment on 14/45)
-- 
Duy