threads / discuss / 20621

"make quick-install-man" broke recently

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

## tl;dr

13 messages between Aug 17, 2009 and Aug 17, 2009.

replies: 12people: 5as markdown or json

Randal L. Schwartz· Aug 17, 2009, 01:16 UTC · lore

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
Junio C Hamano· Aug 17, 2009, 01:29 UTC · re: Randal L. Schwartz · lore

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

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.

Randal L. Schwartz· Aug 17, 2009, 01:33 UTC · re: Junio C Hamano · lore

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

>>>>> "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
Junio C Hamano· Aug 17, 2009, 05:17 UTC · re: Randal L. Schwartz · lore

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

merlyn@stonehenge.com (Randal L. Schwartz) writes:
Show 18 quoted lines
> % 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.
Jacob Helwig· Aug 17, 2009, 05:58 UTC · re: Junio C Hamano · lore

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

On Sun, Aug 16, 2009 at 22:17, Junio C Hamano<gitster@pobox.com> wrote:
Show 45 quoted lines
> 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?
Randal L. Schwartz· Aug 17, 2009, 06:01 UTC · re: Jacob Helwig · lore

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

>>>>> "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
Junio C Hamano· Aug 17, 2009, 06:53 UTC · re: Jacob Helwig · lore

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

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
Johannes Sixt· Aug 17, 2009, 08:00 UTC · re: Junio C Hamano · lore

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

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.
Show 7 quoted lines
> +	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

Junio C Hamano· Aug 17, 2009, 08:02 UTC · re: Johannes Sixt · lore

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

Johannes Sixt <j.sixt@viscovery.net> writes:
Show 14 quoted lines
> 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 ;-)
Randal L. Schwartz· Aug 17, 2009, 14:26 UTC · re: Junio C Hamano · lore

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

>>>>> "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
Linus Torvalds· Aug 17, 2009, 16:34 UTC · re: Junio C Hamano · lore

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

On Sun, 16 Aug 2009, Junio C Hamano wrote:
Show 29 quoted lines
>
> -/* "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
Junio C Hamano· Aug 17, 2009, 17:09 UTC · re: Linus Torvalds · lore

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

Linus Torvalds <torvalds@linux-foundation.org> writes:
Show 5 quoted lines
> 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.

← back to recent threads