"make quick-install-man" broke recently

"make quick-install-man" broke recently

[Randal L. Schwartz]

Something broke recently in "make quick-install-man".  It works
fine if my mandir is empty, but not if a previous installation
is there.

Perhaps missing something that deletes the previous values?

Or maybe a change in the way "git checkout-index" works?

-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Smalltalk/Perl/Unix consulting, Technical writing, Comedy, etc. etc.
See http://methodsandmessages.vox.com/ for Smalltalk and Seaside discussion

Re: "make quick-install-man" broke recently

[Junio C Hamano]

merlyn@stonehenge.com (Randal L. Schwartz) writes:

> Something broke recently in "make quick-install-man".  It works
> fine if my mandir is empty, but not if a previous installation
> is there.

Broken how?

Sorry, but I obviously do not use the target myself, because I am the one
who builds manpages for real and stuff them in the 'man' branch for you to
check out after all.

"Something broke" is bit too vague a problem description if you expect me
to look into it.

Re: "make quick-install-man" broke recently

[Randal L. Schwartz]

>>>>> "Junio" == Junio C Hamano <gitster@pobox.com> writes:

Junio> "Something broke" is bit too vague a problem description if you expect
Junio> me to look into it.

Very sorry.  Let me include some text.

% rm -rf /opt/git/share/man
% make prefix=/opt/git quick-install-man
make -C Documentation quick-install-man
    SUBDIR ../
make[2]: `GIT-VERSION-FILE' is up to date.
'/bin/sh' ./install-doc-quick.sh origin/man /opt/git/share/man
% make prefix=/opt/git quick-install-man
make -C Documentation quick-install-man
    SUBDIR ../
make[2]: `GIT-VERSION-FILE' is up to date.
'/bin/sh' ./install-doc-quick.sh origin/man /opt/git/share/man
error: git checkout-index: unable to create file /opt/git/share/man/man1/git-add.1 (File exists)
error: git checkout-index: unable to create file /opt/git/share/man/man1/git-am.1 (File exists)
error: git checkout-index: unable to create file /opt/git/share/man/man1/git-annotate.1 (File exists)
error: git checkout-index: unable to create file /opt/git/share/man/man1/git-apply.1 (File exists)
[...]

So it fails the second time.  This is new behavior.

-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Smalltalk/Perl/Unix consulting, Technical writing, Comedy, etc. etc.
See http://methodsandmessages.vox.com/ for Smalltalk and Seaside discussion

Re: "make quick-install-man" broke recently

[Junio C Hamano]

merlyn@stonehenge.com (Randal L. Schwartz) writes:

> % rm -rf /opt/git/share/man
> % make prefix=/opt/git quick-install-man
> make -C Documentation quick-install-man
>     SUBDIR ../
> make[2]: `GIT-VERSION-FILE' is up to date.
> '/bin/sh' ./install-doc-quick.sh origin/man /opt/git/share/man
> % make prefix=/opt/git quick-install-man
> make -C Documentation quick-install-man
>     SUBDIR ../
> make[2]: `GIT-VERSION-FILE' is up to date.
> '/bin/sh' ./install-doc-quick.sh origin/man /opt/git/share/man
> error: git checkout-index: unable to create file /opt/git/share/man/man1/git-add.1 (File exists)
> error: git checkout-index: unable to create file /opt/git/share/man/man1/git-am.1 (File exists)
> error: git checkout-index: unable to create file /opt/git/share/man/man1/git-annotate.1 (File exists)
> error: git checkout-index: unable to create file /opt/git/share/man/man1/git-apply.1 (File exists)
> [...]
>
> So it fails the second time.  This is new behavior.

Interesting, and this does not reproduce for me.  

I've tried "run twice back to back" like you did above, "run once, then
'find -type f -print | xargs touch' the target, and then run again", and also
"run once, then 'find -type f -print | xargs chmod -w' the target, and then run again".

None of these fail.

One way I can think of to break it deliberately would be:

	make prefix=/var/tmp/gomikuzu quick-install-man
        find /var/tmp/gomikuzu -type d | xargs chmod -w
	make prefix=/var/tmp/gomikuzu quick-install-man

But then the failure is different from what you showed above:

    error: unable to unlink old '/var/tmp/gomikuzu/share/...' (Permission denied)

Puzzled.

Re: "make quick-install-man" broke recently

[Jacob Helwig]

On Sun, Aug 16, 2009 at 22:17, Junio C Hamano<gitster@pobox.com> wrote:
> merlyn@stonehenge.com (Randal L. Schwartz) writes:
>
>> % rm -rf /opt/git/share/man
>> % make prefix=/opt/git quick-install-man
>> make -C Documentation quick-install-man
>>     SUBDIR ../
>> make[2]: `GIT-VERSION-FILE' is up to date.
>> '/bin/sh' ./install-doc-quick.sh origin/man /opt/git/share/man
>> % make prefix=/opt/git quick-install-man
>> make -C Documentation quick-install-man
>>     SUBDIR ../
>> make[2]: `GIT-VERSION-FILE' is up to date.
>> '/bin/sh' ./install-doc-quick.sh origin/man /opt/git/share/man
>> error: git checkout-index: unable to create file /opt/git/share/man/man1/git-add.1 (File exists)
>> error: git checkout-index: unable to create file /opt/git/share/man/man1/git-am.1 (File exists)
>> error: git checkout-index: unable to create file /opt/git/share/man/man1/git-annotate.1 (File exists)
>> error: git checkout-index: unable to create file /opt/git/share/man/man1/git-apply.1 (File exists)
>> [...]
>>
>> So it fails the second time.  This is new behavior.
>
> Interesting, and this does not reproduce for me.
>
> I've tried "run twice back to back" like you did above, "run once, then
> 'find -type f -print | xargs touch' the target, and then run again", and also
> "run once, then 'find -type f -print | xargs chmod -w' the target, and then run again".
>
> None of these fail.
>
> One way I can think of to break it deliberately would be:
>
>        make prefix=/var/tmp/gomikuzu quick-install-man
>        find /var/tmp/gomikuzu -type d | xargs chmod -w
>        make prefix=/var/tmp/gomikuzu quick-install-man
>
> But then the failure is different from what you showed above:
>
>    error: unable to unlink old '/var/tmp/gomikuzu/share/...' (Permission denied)
>
> Puzzled.
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

I was able to reproduce this one one of the machines I have access to,
and bisecting shows that it was introduced by b6986d8 (git-checkout: be
careful about untracked symlinks).  This makes sense, since home-dirs
are symlinks on the machine I was able to reproduce it on.

Hopefully this helps someone with a little stronger fu, to point them in
the right direction.

Randal, does your machine have a similar symlinked $HOME setup?

Re: "make quick-install-man" broke recently

[Randal L. Schwartz]

>>>>> "Jacob" == Jacob Helwig <jacob.helwig@gmail.com> writes:

Jacob> Randal, does your machine have a similar symlinked $HOME setup?

/opt/git is through two symlinks, yes.

-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Smalltalk/Perl/Unix consulting, Technical writing, Comedy, etc. etc.
See http://methodsandmessages.vox.com/ for Smalltalk and Seaside discussion

Re: "make quick-install-man" broke recently

[Junio C Hamano]

Jacob Helwig <jacob.helwig@gmail.com> writes:

> I was able to reproduce this one one of the machines I have access to,
> and bisecting shows that it was introduced by b6986d8 (git-checkout: be
> careful about untracked symlinks).

Ahh..

-- >8 --
check_path(): allow symlinked directories to checkout-index --prefix

Merlyn noticed that Documentation/install-doc-quick.sh no longer correctly
removes old installed documents when the target directory has a leading
path that is a symlink.  It turns out that "checkout-index --prefix" was
broken by recent b6986d8 (git-checkout: be careful about untracked
symlinks, 2009-07-29).

I suspect has_symlink_leading_path() could learn the third parameter
(prefix that is allowed to be symlinked directories) to allow us to retire
a similar function has_dirs_only_path().

Another avenue of fixing this I considered was to get rid of base_dir and
base_dir_len from "struct checkout", and instead make "git checkout-index"
when run with --prefix mkdir the leading path and chdir in there.  It
might be the best longer term solution to this issue, as the base_dir
feature is used only by that rather obscure codepath as far as I know.

But at least this patch should fix this breakage.

SIgned-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h                         |    3 ---
 entry.c                         |   15 ++++++++++++---
 t/t2000-checkout-cache-clash.sh |    9 +++++++++
 3 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/cache.h b/cache.h
index 9222774..e6c7f33 100644
--- a/cache.h
+++ b/cache.h
@@ -468,9 +468,6 @@ extern int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_obje
 extern int index_path(unsigned char *sha1, const char *path, struct stat *st, int write_object);
 extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);
 
-/* "careful lstat()" */
-extern int check_path(const char *path, int len, struct stat *st);
-
 #define REFRESH_REALLY		0x0001	/* ignore_valid */
 #define REFRESH_UNMERGED	0x0002	/* allow unmerged */
 #define REFRESH_QUIET		0x0004	/* be quiet about it */
diff --git a/entry.c b/entry.c
index f276cf3..6813f8a 100644
--- a/entry.c
+++ b/entry.c
@@ -179,9 +179,18 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
  * This is like 'lstat()', except it refuses to follow symlinks
  * in the path.
  */
-int check_path(const char *path, int len, struct stat *st)
+static int check_path(const char *path, int len, struct stat *st,
+		      const struct checkout *co)
 {
-	if (has_symlink_leading_path(path, len)) {
+	if (co->base_dir_len) {
+		const char *slash = path + len;
+		while (path < slash && *slash != '/')
+			slash--;
+		if (!has_dirs_only_path(path, slash-path, co->base_dir_len)) {
+			errno = ENOENT;
+			return -1;
+		}
+	} else if (has_symlink_leading_path(path, len)) {
 		errno = ENOENT;
 		return -1;
 	}
@@ -201,7 +210,7 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *t
 	strcpy(path + len, ce->name);
 	len += ce_namelen(ce);
 
-	if (!check_path(path, len, &st)) {
+	if (!check_path(path, len, &st, state)) {
 		unsigned changed = ce_match_stat(ce, &st, CE_MATCH_IGNORE_VALID);
 		if (!changed)
 			return 0;
diff --git a/t/t2000-checkout-cache-clash.sh b/t/t2000-checkout-cache-clash.sh
index f7e1a73..3edade0 100755
--- a/t/t2000-checkout-cache-clash.sh
+++ b/t/t2000-checkout-cache-clash.sh
@@ -48,4 +48,13 @@ test_expect_success \
     'git checkout-index conflicting paths.' \
     'test -f path0 && test -d path1 && test -f path1/file1'
 
+test_expect_success 'checkout-index -f twice with --prefix' '
+	mkdir -p tar/get &&
+	ln -s tar/get there &&
+	echo first &&
+	git checkout-index -a -f --prefix=there/ &&
+	echo second &&
+	git checkout-index -a -f --prefix=there/
+'
+
 test_done

Re: "make quick-install-man" broke recently

[Johannes Sixt]

Junio C Hamano schrieb:
> check_path(): allow symlinked directories to checkout-index --prefix

> +test_expect_success 'checkout-index -f twice with --prefix' '

Please add SYMLINKS prerequisite before you publish this test case.

> +	mkdir -p tar/get &&
> +	ln -s tar/get there &&
> +	echo first &&
> +	git checkout-index -a -f --prefix=there/ &&
> +	echo second &&
> +	git checkout-index -a -f --prefix=there/
> +'

Thanks,
-- Hannes

Re: "make quick-install-man" broke recently

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Junio C Hamano schrieb:
>> check_path(): allow symlinked directories to checkout-index --prefix
>
>> +test_expect_success 'checkout-index -f twice with --prefix' '
>
> Please add SYMLINKS prerequisite before you publish this test case.
>
>> +	mkdir -p tar/get &&
>> +	ln -s tar/get there &&
>> +	echo first &&
>> +	git checkout-index -a -f --prefix=there/ &&
>> +	echo second &&
>> +	git checkout-index -a -f --prefix=there/
>> +'

Heh, I am not sure if the fix is the best approach to begin with yet ;-)

Re: "make quick-install-man" broke recently

[Randal L. Schwartz]

>>>>> "Junio" == Junio C Hamano <gitster@pobox.com> writes:

Junio> Merlyn noticed that Documentation/install-doc-quick.sh no longer correctly
Junio> removes old installed documents when the target directory has a leading
Junio> path that is a symlink.  It turns out that "checkout-index --prefix" was
Junio> broken by recent b6986d8 (git-checkout: be careful about untracked
Junio> symlinks, 2009-07-29).

It's good to know I wasn't hallucinating, in any case. :)

-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Smalltalk/Perl/Unix consulting, Technical writing, Comedy, etc. etc.
See http://methodsandmessages.vox.com/ for Smalltalk and Seaside discussion

Re: "make quick-install-man" broke recently

[Junio C Hamano]

merlyn@stonehenge.com (Randal L. Schwartz) writes:

> It's good to know I wasn't hallucinating, in any case. :)

Thanks.

Re: "make quick-install-man" broke recently

[Linus Torvalds]

On Sun, 16 Aug 2009, Junio C Hamano wrote:
>
> -/* "careful lstat()" */
> -extern int check_path(const char *path, int len, struct stat *st);
> -
>  #define REFRESH_REALLY		0x0001	/* ignore_valid */
>  #define REFRESH_UNMERGED	0x0002	/* allow unmerged */
>  #define REFRESH_QUIET		0x0004	/* be quiet about it */
> diff --git a/entry.c b/entry.c
> index f276cf3..6813f8a 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -179,9 +179,18 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
>   * This is like 'lstat()', except it refuses to follow symlinks
>   * in the path.
>   */
> -int check_path(const char *path, int len, struct stat *st)
> +static int check_path(const char *path, int len, struct stat *st,
> +		      const struct checkout *co)
>  {
> -	if (has_symlink_leading_path(path, len)) {
> +	if (co->base_dir_len) {
> +		const char *slash = path + len;
> +		while (path < slash && *slash != '/')
> +			slash--;
> +		if (!has_dirs_only_path(path, slash-path, co->base_dir_len)) {
> +			errno = ENOENT;
> +			return -1;
> +		}
> +	} else if (has_symlink_leading_path(path, len)) {

Grr. Now 'check_path()' is no longer something generically useful.

Could you perhaps instead only change 'checkout_entry()' to do this hack, 
and leave 'check_path()' as a generic replacement for "lstat()" that 
doesn't follow symlinks?

		Linus

Re: "make quick-install-man" broke recently

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> Grr. Now 'check_path()' is no longer something generically useful.
>
> Could you perhaps instead only change 'checkout_entry()' to do this hack, 
> and leave 'check_path()' as a generic replacement for "lstat()" that 
> doesn't follow symlinks?

Given that non-empty base_dir is only used for "checkout-index --prefix",
iow, the "path" internally used by git and fed to our symlink.c cache are
supposed to be always relative to the work tree, I think that may be a
good thing to do in the short-term.

But only if we won't add any more like "checkout-index --prefix".  If you
want to implement "git checkout --prefix=over-there/" and if you want to
call check_path() directly (iow not as a part of callchain from
checkout_entry()) while doing so, for example, you would regret keeping
the check_path() function unaware of base_dir, as you would reintroduce
the same bug.

I thought about getting rid of base_dir from struct checkout by running
create_directories() in checkout-index and chdir(2) there, because this is
a very special case codepath anyway.  I actually haven't tried it, but it
probably will have bad interactions with the way we find $GIT_DIR.