From: Junio C Hamano <gitster@pobox•com>
To: Martin Erik Werner <martinerikwerner@gmail•com>
Cc: git@vger•kernel.org, richih@debian•org, tboegi@web•de,
pclouds@gmail•com, dak@gnu•org
Subject: Re: [PATCH v5 4/5] setup: Add 'abspath_part_inside_repo' function
Date: Mon, 03 Feb 2014 13:00:48 -0800 [thread overview]
Message-ID: <xmqq1tzjewsf.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: 1391358940-17373-5-git-send-email-martinerikwerner@gmail.com
Martin Erik Werner <martinerikwerner@gmail•com> writes:
> The path being exactly equal to the work tree is handled separately,
> since then there is no directory separator between the work tree and
> in-repo part.
What is an "in-repo part"? Whatever it is, I am not sure if I
follow that logic. After the while (*path) loop checks each level,
you check the whole path---wouldn't that code handle that special
case already?
Because it is probably the normal case not to have any symbolic link
in the leading part (hence not having to dereference them), I think
checking "is work_tree[] a prefix of path[]" early is justified from
performance point of view, though.
> /*
> + * No checking if the path is in fact an absolute path is done, and it must
> + * already be normalized.
This is not quite parsable to me.
> + * Find the part of an absolute path that lies inside the work tree by
> + * dereferencing symlinks outside the work tree, for example:
> + * /dir1/repo/dir2/file (work tree is /dir1/repo) -> dir2/file
> + * /dir/file (work tree is /) -> dir/file
> + * /dir/symlink1/symlink2 (symlink1 points to work tree) -> symlink2
> + * /dir/repolink/file (repolink points to /dir/repo) -> file
> + * /dir/repo (exactly equal to work tree) -> (empty string)
> + */
> +static inline int abspath_part_inside_repo(char *path)
It looks a bit too big to be marked "inline"; better leave it to the
compiler to notice that there is only a single callsite and decide
to (or not to) inline it.
> +{
> + size_t len;
> + size_t wtlen;
> + char *path0;
> + int off;
> +
> + const char* work_tree = get_git_work_tree();
> + if (!work_tree)
> + return -1;
> + wtlen = strlen(work_tree);
> + len = strlen(path);
I expect that this is called from a callsite that _knows_ how long
path[] is. Shouldn't len be a parameter to this function instead?
> + off = 0;
> +
> + /* check if work tree is already the prefix */
> + if (strncmp(path, work_tree, wtlen) == 0) {
I wonder if we want to be explicit and compare wtlen and len before
even attempting strncmp(). If work_tree is longer than path, it
cannot be a prefix of path, right?
In other words, also with a style-fix to prefer !XXcmp() over
XXcmp() == 0:
if (wtlen <= len && !strncmp(path, work_tree, wtlen)) {
perhaps? That would make it clear why referring to path[wtlen] on
the next line is permissible (it is obvious that it won't access
past the end of path[]):
> + if (path[wtlen] == '/') {
> + memmove(path, path + wtlen + 1, len - wtlen);
> + return 0;
> + } else if (path[wtlen - 1] == '/' || path[wtlen] == '\0') {
> + /* work tree is the root, or the whole path */
> + memmove(path, path + wtlen, len - wtlen + 1);
If work_tree[] == "/", path[] == "/a", then len == 2, wtlen == 1,
path[wtlen - 1] == '/' and this shifts path[] to the left by one,
leaving path[] = "a", which is what we want. OK.
If work_tree[] == path[] == "/a", then len == wtlen == 2,
path[wtlen] == '\0', and this becomes equivalent to path[0] = '\0'.
Hmph.... How do our callers treat an empty path? In other words,
should these three be equivalent?
cd /a && git ls-files /a
cd /a && git ls-files ""
cd /a && git ls-files .
> + return 0;
> + }
> + /* work tree might match beginning of a symlink to work tree */
> + off = wtlen;
> + }
> + path0 = path;
> + path += offset_1st_component(path) + off;
> +
> + /* check each level */
> + while (*path) {
> + path++;
> + if (*path == '/') {
> + *path = '\0';
> + if (strcmp(real_path(path0), work_tree) == 0) {
> + memmove(path0, path + 1, len - (path - path0));
> + return 0;
> + }
> + *path = '/';
> + }
> + }
> +
> + /* check whole path */
> + if (strcmp(real_path(path0), work_tree) == 0) {
> + *path0 = '\0';
> + return 0;
> + }
> +
> + return -1;
> +}
> +
> +/*
> * Normalize "path", prepending the "prefix" for relative paths. If
> * remaining_prefix is not NULL, return the actual prefix still
> * remains in the path. For example, prefix = sub1/sub2/ and path is
next prev parent reply other threads:[~2014-02-03 21:01 UTC|newest]
Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-01-15 12:48 git-mv with absolute path derefereces symlinks Martin Erik Werner
2014-01-26 14:22 ` [PATCH 0/2] in-tree symlink handling with absolute paths Martin Erik Werner
2014-01-26 14:22 ` [PATCH 1/2] t0060: Add test for manipulating symlinks via " Martin Erik Werner
2014-01-26 14:22 ` [PATCH 2/2] setup: Don't dereference in-tree symlinks for " Martin Erik Werner
2014-01-26 17:19 ` Torsten Bögershausen
2014-01-27 0:07 ` Martin Erik Werner
2014-01-27 0:07 ` [PATCH v2 " Martin Erik Werner
2014-01-27 0:49 ` Duy Nguyen
2014-01-27 16:31 ` Junio C Hamano
2014-01-31 20:21 ` [PATCH v3 0/4] " Martin Erik Werner
2014-02-02 1:59 ` [PATCH v4 0/4] Handling of " Martin Erik Werner
2014-02-02 1:59 ` [PATCH v4 1/4] t0060: Add test for manipulating symlinks via " Martin Erik Werner
2014-02-02 1:59 ` [PATCH v4 2/4] t0060: Add test for prefix_path when path == work tree Martin Erik Werner
2014-02-02 1:59 ` [PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function Martin Erik Werner
2014-02-02 2:19 ` Duy Nguyen
2014-02-02 2:23 ` Duy Nguyen
2014-02-02 11:13 ` Martin Erik Werner
2014-02-02 11:21 ` David Kastrup
2014-02-02 11:37 ` Torsten Bögershausen
2014-02-02 12:09 ` Martin Erik Werner
2014-02-02 12:27 ` Torsten Bögershausen
2014-02-02 12:15 ` Duy Nguyen
2014-02-02 1:59 ` [PATCH v4 4/4] setup: Don't dereference in-tree symlinks for absolute paths Martin Erik Werner
2014-02-02 16:35 ` [PATCH v5 0/5] Handling of " Martin Erik Werner
2014-02-02 16:35 ` [PATCH v5 1/5] t0060: Add test for manipulating symlinks via " Martin Erik Werner
2014-02-03 18:50 ` Junio C Hamano
2014-02-03 19:52 ` Junio C Hamano
2014-02-03 20:12 ` Martin Erik Werner
2014-02-02 16:35 ` [PATCH v5 2/5] t0060: Add test for prefix_path when path == work tree Martin Erik Werner
2014-02-02 16:35 ` [PATCH v5 3/5] t0060: Add tests for prefix_path when path begins with " Martin Erik Werner
2014-02-02 16:35 ` [PATCH v5 4/5] setup: Add 'abspath_part_inside_repo' function Martin Erik Werner
2014-02-03 21:00 ` Junio C Hamano [this message]
2014-02-03 23:16 ` Martin Erik Werner
2014-02-04 18:09 ` Junio C Hamano
2014-02-04 18:32 ` Martin Erik Werner
2014-02-02 16:35 ` [PATCH v5 5/5] setup: Don't dereference in-tree symlinks for absolute paths Martin Erik Werner
2014-02-03 4:15 ` Duy Nguyen
2014-02-03 13:17 ` Martin Erik Werner
2014-02-04 0:05 ` Junio C Hamano
2014-02-04 14:25 ` [PATCH v6 0/6] Handling of " Martin Erik Werner
2014-02-04 14:25 ` [PATCH v6 1/6] t3004: Add test for ls-files on symlinks via " Martin Erik Werner
2014-02-04 14:25 ` [PATCH v6 2/6] t0060: Add test for prefix_path " Martin Erik Werner
2014-02-04 14:25 ` [PATCH v6 3/6] t0060: Add test for prefix_path when path == work tree Martin Erik Werner
2014-02-04 14:25 ` [PATCH v6 4/6] t0060: Add tests for prefix_path when path begins with " Martin Erik Werner
2014-02-04 20:00 ` Torsten Bögershausen
2014-02-04 20:07 ` Junio C Hamano
2014-02-04 14:25 ` [PATCH v6 5/6] setup: Add 'abspath_part_inside_repo' function Martin Erik Werner
2014-02-04 19:18 ` Junio C Hamano
2014-02-04 14:25 ` [PATCH v6 6/6] setup: Don't dereference in-tree symlinks for absolute paths Martin Erik Werner
2014-01-31 20:22 ` [PATCH v3 1/4] t0060: Add test for manipulating symlinks via " Martin Erik Werner
2014-01-31 20:22 ` [PATCH v3 2/4] t0060: Add test for prefix_path when path == work tree Martin Erik Werner
2014-01-31 20:22 ` [PATCH v3 3/4] setup: Add 'abspath_part_inside_repo' function Martin Erik Werner
2014-01-31 22:37 ` Torsten Bögershausen
2014-02-01 1:31 ` Martin Erik Werner
2014-02-01 2:31 ` Duy Nguyen
2014-01-31 20:23 ` [PATCH v3 4/4] setup: Don't dereference in-tree symlinks for absolute paths Martin Erik Werner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq1tzjewsf.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=dak@gnu$(echo .)org \
--cc=git@vger$(echo .)kernel.org \
--cc=martinerikwerner@gmail$(echo .)com \
--cc=pclouds@gmail$(echo .)com \
--cc=richih@debian$(echo .)org \
--cc=tboegi@web$(echo .)de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox