[PATCH] Makefile: add clean-obsolete-scripts target

[PATCH] Makefile: add clean-obsolete-scripts target

[Johannes Schindelin]

On Cygwin, newly builtins are not recognized, because there exist both
the executable binaries (with .exe extension) _and_ the now-obsolete
scripts (without extension), but the script is executed.

"make clean-obsolete-scripts" removes these ambiguities by removing the
older of these file pairs.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
 Makefile |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index 007ceb2..a46683a 100644
--- a/Makefile
+++ b/Makefile
@@ -905,6 +905,25 @@ dist-doc:
 
 ### Cleaning rules
 
+clean-obsolete-scripts:
+	@if test -n "$X"; \
+	then \
+		ls *$X '$(DESTDIR_SQ)$(gitexecdir_SQ)'/*$X | while read f; do \
+			script="`echo "$$f" | sed 's/$X\$$/./'`"; \
+			if test -f "$$script"; \
+			then \
+				if test "$$script" -ot "$$f"; \
+				then \
+					echo removing "$$script"; \
+					rm "$$script"; \
+				else \
+					echo removing "$$f"; \
+					rm "$$f"; \
+				fi; \
+			fi; \
+		done; \
+	fi
+
 clean:
 	rm -f *.o mozilla-sha1/*.o arm/*.o ppc/*.o compat/*.o xdiff/*.o \
 		$(LIB_FILE) $(XDIFF_LIB)
-- 
1.4.4.4.g774d-dirty

Re: [PATCH] Makefile: add clean-obsolete-scripts target

[Alex Riesen]

On 1/10/07, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>
> On Cygwin, newly builtins are not recognized, because there exist both
> the executable binaries (with .exe extension) _and_ the now-obsolete
> scripts (without extension), but the script is executed.
>
> "make clean-obsolete-scripts" removes these ambiguities by removing the
> older of these file pairs.

Not enough. You have to cleanup the build directory too,
otherwise tests fail, and git-hacking on git itself is ... complicated.

Re: [PATCH] Makefile: add clean-obsolete-scripts target

[Johannes Schindelin]

Hi,

On Wed, 10 Jan 2007, Alex Riesen wrote:

> On 1/10/07, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > 
> > On Cygwin, newly builtins are not recognized, because there exist both
> > the executable binaries (with .exe extension) _and_ the now-obsolete
> > scripts (without extension), but the script is executed.
> > 
> > "make clean-obsolete-scripts" removes these ambiguities by removing the
> > older of these file pairs.
> 
> Not enough. You have to cleanup the build directory too,
> otherwise tests fail, and git-hacking on git itself is ... complicated.

Doesn't it handle that?

	ls *$X '$(DESTDIR_SQ)$(gitexecdir_SQ)'/*$X

Note the first "*$X". It really should pick up on all executables in the 
build directory. Or do I miss something?

Ciao,
Dscho

Re: [PATCH] Makefile: add clean-obsolete-scripts target

[Alex Riesen]

On 1/10/07, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > Not enough. You have to cleanup the build directory too,
> > otherwise tests fail, and git-hacking on git itself is ... complicated.
>
> Doesn't it handle that?
>
>         ls *$X '$(DESTDIR_SQ)$(gitexecdir_SQ)'/*$X
>
> Note the first "*$X". It really should pick up on all executables in the
> build directory. Or do I miss something?

No-no, sorry. I didn't noticed the first *$X, which will of course catch the
execs in the current directory.

Re: [PATCH] Makefile: add clean-obsolete-scripts target

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Cygwin, newly builtins are not recognized, because there exist both
> the executable binaries (with .exe extension) _and_ the now-obsolete
> scripts (without extension), but the script is executed.
>
> "make clean-obsolete-scripts" removes these ambiguities by removing the
> older of these file pairs.

I would prefer it to be rather automatic -- how about this instead?

-- >8 --
diff --git a/Makefile b/Makefile
index e4f520a..9077941 100644
--- a/Makefile
+++ b/Makefile
@@ -1,5 +1,5 @@
 # The default target of this Makefile is...
-all:
+all::
 
 # Define NO_OPENSSL environment variable if you do not have OpenSSL.
 # This also implies MOZILLA_SHA1.
@@ -605,9 +605,12 @@ export prefix TAR INSTALL DESTDIR SHELL_PATH template_dir
 
 ### Build rules
 
-all: $(ALL_PROGRAMS) $(BUILT_INS) git$X gitk gitweb/gitweb.cgi
+all:: $(ALL_PROGRAMS) $(BUILT_INS) git$X gitk gitweb/gitweb.cgi
+ifneq (,$X)
+	$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_PROGRAMS) $(BUILT_INS) git$X)), echo rm -f '$p';) 
+endif
 
-all:
+all::
 	$(MAKE) -C perl PERL_PATH='$(PERL_PATH_SQ)' prefix='$(prefix_SQ)' all
 	$(MAKE) -C templates
 
@@ -849,6 +852,9 @@ install: all
 			'$(DESTDIR_SQ)$(gitexecdir_SQ)/git$X'; \
 	fi
 	$(foreach p,$(BUILT_INS), rm -f '$(DESTDIR_SQ)$(gitexecdir_SQ)/$p' && ln '$(DESTDIR_SQ)$(gitexecdir_SQ)/git$X' '$(DESTDIR_SQ)$(gitexecdir_SQ)/$p' ;)
+ifneq (,$X)
+	$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_PROGRAMS) $(BUILT_INS) git$X)), echo rm -f '$(DESTDIR_SQ)$(gitexecdir_SQ)/$p';) 
+endif
 
 install-doc:
 	$(MAKE) -C Documentation install

Re: [PATCH] Makefile: add clean-obsolete-scripts target

[Alex Riesen]

On 1/10/07, Junio C Hamano <junkio@cox.net> wrote:
>
> I would prefer it to be rather automatic -- how about this instead?
>
...
> +ifneq (,$X)

Perfect! :)

Re: [PATCH] Makefile: add clean-obsolete-scripts target

[Junio C Hamano]

"Alex Riesen" <raa.lkml@gmail.com> writes:

> On 1/10/07, Junio C Hamano <junkio@cox.net> wrote:
>>
>> I would prefer it to be rather automatic -- how about this instead?
>>
> ...
>> +ifneq (,$X)
>
> Perfect! :)

Heh, you couldn't have tested -- I still had "echo rm -f" in the
patch ;-).

Re: [PATCH] Makefile: add clean-obsolete-scripts target

[Johannes Schindelin]

Hi,

On Wed, 10 Jan 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Cygwin, newly builtins are not recognized, because there exist both
> > the executable binaries (with .exe extension) _and_ the now-obsolete
> > scripts (without extension), but the script is executed.
> >
> > "make clean-obsolete-scripts" removes these ambiguities by removing the
> > older of these file pairs.
> 
> I would prefer it to be rather automatic -- how about this instead?

I like your method better. However, does it execute an rm -f for _every_ 
builtin, even if it does not exist? This is a huge performance eater on 
cygwin. Every fork() is _expensive_.

Even worse: are you sure that it does not remove the _builtins_? Maybe you 
noticed that I asked for "git-bla." instead of "git-bla", since the latter 
will happily access "git-bla.exe" (at least when you do an "ls git-bla" 
and "git-bla.exe" exists, but not "git-bla", it _will_ show 
the metadata of "git-bla.exe", but with the name "git-bla").

Will test.

Ciao,
Dscho

Re: [PATCH] Makefile: add clean-obsolete-scripts target

[Johannes Schindelin]

Hi,

On Thu, 11 Jan 2007, Johannes Schindelin wrote:

> On Wed, 10 Jan 2007, Junio C Hamano wrote:
> 
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > 
> > > On Cygwin, newly builtins are not recognized, because there exist both
> > > the executable binaries (with .exe extension) _and_ the now-obsolete
> > > scripts (without extension), but the script is executed.
> > >
> > > "make clean-obsolete-scripts" removes these ambiguities by removing the
> > > older of these file pairs.
> > 
> > I would prefer it to be rather automatic -- how about this instead?
> 
> I like your method better. However, does it execute an rm -f for _every_ 
> builtin, even if it does not exist? This is a huge performance eater on 
> cygwin. Every fork() is _expensive_.
> 
> Even worse: are you sure that it does not remove the _builtins_? Maybe you 
> noticed that I asked for "git-bla." instead of "git-bla", since the latter 
> will happily access "git-bla.exe" (at least when you do an "ls git-bla" 
> and "git-bla.exe" exists, but not "git-bla", it _will_ show 
> the metadata of "git-bla.exe", but with the name "git-bla").
> 
> Will test.

Okay, I tested. And it works, indeed. No idea why, though. And while it 
looks a bit strange [*1*], the performance is not as bad as I expected.

Ciao,
Dscho

[*1*]

$ make
[...]
rm -f git-pack-refs.exe && ln git.exe git-pack-refs.exe
rm -f 'git-convert-objects';  rm -f 'git-fetch-pack';  rm -f 
'git-fsck-objects';  rm -f 'git-hash-object';  rm -f 'git-index-pack';  rm 
-f 'git-local-fetch';  rm -f 'git-daemon';  rm -f 'git-merge-index';  rm 
-f 'git-mktag';  rm -f 'git-mktree';  rm -f 'git-patch-id';  rm -f 
'git-peek-remote';  rm -f 'git-receive-pack';  rm -f 'git-send-pack';  rm 
-f 'git-shell';  rm -f 'git-show-index';  rm -f 'git-ssh-fetch';  rm -f 
'git-ssh-upload';  rm -f 'git-unpack-file';  rm -f 
'git-update-server-info';  rm -f 'git-upload-pack';  rm -f 
'git-verify-pack';  rm -f 'git-pack-redundant';  rm -f 'git-var';  rm -f 
'git-merge-tree';  rm -f 'git-imap-send';  rm -f 'git-merge-recursive';  
rm -f 'git-ssh-pull';  rm -f 'git-ssh-push';  rm -f 'git-http-fetch';  rm 
-f 'git-http-push';  rm -f 'git-merge-recur';  rm -f 'git-format-patch';  
rm -f 'git-show';  rm -f 'git-whatchanged';  rm -f 'git-cherry';  rm -f 
'git-get-tar-commit-id';  rm -f 'git-init';  rm -f 'git-add';  rm -f 
'git-annotate';  rm -f 'git-apply';  rm -f 'git-archive';  rm -f 
'git-blame';  rm -f 'git-branch';  rm -f 'git-cat-file';  rm -f 
'git-checkout-index';  rm -f 'git-check-ref-format';  rm -f 
'git-commit-tree';  rm -f 'git-count-objects';  rm -f 'git-describe';  rm 
-f 'git-diff';  rm -f 'git-diff-files';  rm -f 'git-diff-index';  rm -f 
'git-diff-stages';  rm -f 'git-diff-tree';  rm -f 'git-fmt-merge-msg';  rm 
-f 'git-for-each-ref';  rm -f 'git-grep';  rm -f 'git-init-db';  rm -f 
'git-log';  rm -f 'git-ls-files';  rm -f 'git-ls-tree';  rm -f 
'git-mailinfo';  rm -f 'git-mailsplit';  rm -f 'git-merge-base';  rm -f 
'git-merge-file';  rm -f 'git-mv';  rm -f 'git-name-rev';  rm -f 
'git-pack-objects';  rm -f 'git-prune';  rm -f 'git-prune-packed';  rm -f 
'git-push';  rm -f 'git-read-tree';  rm -f 'git-reflog';  rm -f 
'git-repo-config';  rm -f 'git-rerere';  rm -f 'git-rev-list';  rm -f 
'git-rev-parse';  rm -f 'git-rm';  rm -f 'git-runstatus';  rm -f 
'git-shortlog';  rm -f 'git-show-branch';  rm -f 'git-stripspace';  rm -f 
'git-symbolic-ref';  rm -f 'git-tar-tree';  rm -f 'git-unpack-objects';  
rm -f 'git-update-index';  rm -f 'git-update-ref';  rm -f 
'git-upload-archive';  rm -f 'git-verify-pack';  rm -f 'git-write-tree';  
rm -f 'git-show-ref';  rm -f 'git-pack-refs';  rm -f 'git';
[...]

Re: [PATCH] Makefile: add clean-obsolete-scripts target

[Alex Riesen]

Junio C Hamano, Thu, Jan 11, 2007 01:34:41 +0100:
> >> I would prefer it to be rather automatic -- how about this instead?
> > ...
> >> +ifneq (,$X)
> >
> > Perfect! :)
> 
> Heh, you couldn't have tested -- I still had "echo rm -f" in the
> patch ;-).

Didn't. Just liked the approach :)