* [PATCH] convert: mark a file-local symbol static @ 2016-10-11 23:46 Ramsay Jones 2016-10-15 15:05 ` Lars Schneider 0 siblings, 1 reply; 14+ messages in thread From: Ramsay Jones @ 2016-10-11 23:46 UTC (permalink / raw) To: Lars Schneider; +Cc: Junio C Hamano, GIT Mailing-list Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com> --- Hi Lars, If you need to re-roll your 'ls/filter-process' branch, could you please squash this into commit 85290197 ("convert: add filter.<driver>.process option", 08-10-2016). Thanks. ATB, Ramsay Jones convert.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/convert.c b/convert.c index fe68316..cf30380 100644 --- a/convert.c +++ b/convert.c @@ -568,7 +568,7 @@ static void kill_multi_file_filter(struct hashmap *hashmap, struct cmd2process * free(entry); } -void stop_multi_file_filter(struct child_process *process) +static void stop_multi_file_filter(struct child_process *process) { sigchain_push(SIGPIPE, SIG_IGN); /* Closing the pipe signals the filter to initiate a shutdown. */ -- 2.10.0 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] convert: mark a file-local symbol static 2016-10-11 23:46 [PATCH] convert: mark a file-local symbol static Ramsay Jones @ 2016-10-15 15:05 ` Lars Schneider 2016-10-15 21:01 ` Ramsay Jones 0 siblings, 1 reply; 14+ messages in thread From: Lars Schneider @ 2016-10-15 15:05 UTC (permalink / raw) To: Ramsay Jones; +Cc: Junio C Hamano, GIT Mailing-list > On 11 Oct 2016, at 16:46, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote: > > If you need to re-roll your 'ls/filter-process' branch, could you > please squash this into commit 85290197 > ("convert: add filter.<driver>.process option", 08-10-2016). > > > -void stop_multi_file_filter(struct child_process *process) > +static void stop_multi_file_filter(struct child_process *process) Done! Do you have some kind of script to detect these things automatically or do you read the code that carefully? Thanks, Lars ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] convert: mark a file-local symbol static 2016-10-15 15:05 ` Lars Schneider @ 2016-10-15 21:01 ` Ramsay Jones 2016-10-16 0:15 ` Lars Schneider 0 siblings, 1 reply; 14+ messages in thread From: Ramsay Jones @ 2016-10-15 21:01 UTC (permalink / raw) To: Lars Schneider; +Cc: Junio C Hamano, GIT Mailing-list On 15/10/16 16:05, Lars Schneider wrote: >> On 11 Oct 2016, at 16:46, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote: [snip] >> -void stop_multi_file_filter(struct child_process *process) >> +static void stop_multi_file_filter(struct child_process *process) > > Done! Do you have some kind of script to detect these things > automatically or do you read the code that carefully? Heh, I'm _far_ too lazy to read the code that carefully. :-D A combination of 'make sparse' and a perl script (originally posted to the list by Junio) find all of these for me. ATB, Ramsay Jones ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] convert: mark a file-local symbol static 2016-10-15 21:01 ` Ramsay Jones @ 2016-10-16 0:15 ` Lars Schneider 2016-10-17 1:37 ` Ramsay Jones 0 siblings, 1 reply; 14+ messages in thread From: Lars Schneider @ 2016-10-16 0:15 UTC (permalink / raw) To: Ramsay Jones; +Cc: Junio C Hamano, GIT Mailing-list > On 15 Oct 2016, at 14:01, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote: > > > > On 15/10/16 16:05, Lars Schneider wrote: >>> On 11 Oct 2016, at 16:46, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote: > [snip] >>> -void stop_multi_file_filter(struct child_process *process) >>> +static void stop_multi_file_filter(struct child_process *process) >> >> Done! Do you have some kind of script to detect these things >> automatically or do you read the code that carefully? > > Heh, I'm _far_ too lazy to read the code that carefully. :-D > > A combination of 'make sparse' and a perl script (originally > posted to the list by Junio) find all of these for me. Interesting. Do you have a link to this script? Does it generate false positives? Maybe I can add `make sparse` to the TravisCI build? Cheers, Lars ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] convert: mark a file-local symbol static 2016-10-16 0:15 ` Lars Schneider @ 2016-10-17 1:37 ` Ramsay Jones 2016-10-17 2:18 ` Jeff King 0 siblings, 1 reply; 14+ messages in thread From: Ramsay Jones @ 2016-10-17 1:37 UTC (permalink / raw) To: Lars Schneider; +Cc: Junio C Hamano, GIT Mailing-list [-- Attachment #1: Type: text/plain, Size: 4128 bytes --] On 16/10/16 01:15, Lars Schneider wrote: >> On 15 Oct 2016, at 14:01, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote: >> On 15/10/16 16:05, Lars Schneider wrote: >>>> On 11 Oct 2016, at 16:46, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote: >> [snip] >>>> -void stop_multi_file_filter(struct child_process *process) >>>> +static void stop_multi_file_filter(struct child_process *process) >>> >>> Done! Do you have some kind of script to detect these things >>> automatically or do you read the code that carefully? >> >> Heh, I'm _far_ too lazy to read the code that carefully. :-D >> >> A combination of 'make sparse' and a perl script (originally >> posted to the list by Junio) find all of these for me. > > Interesting. Do you have a link to this script? I'm attaching _a_ version of the script (you would think that I would have it under version control; but no, I copy versions around the several machines/git.git repos I have ... :-P ). > Does it generate false positives? Hmm, well, you have to remember that 'make clean' sometimes doesn't make clean. Ever since the Makefile was changed to only remove $(OBJECTS), rather than *.o xdiff/*.o etc., you have to remember to 'make clean' _before_ you switch branches. Otherwise, you risk leaving some objects laying around. Since the script runs 'nm' on all objects it finds, any stale ones can cause problems. (Of course, I almost always forget, so I frequently have to manually check for and remove stale objects!) Also, you have to interpret the results of the script. Just because it shows you an unused external, that doesn't make it an error. The way I use it, BTW, is to run it on a freshly built branch (saving to a file) and then compare the files from master->next->pu. I don't bother looking at the output on master (the 'stop list' in the script is _way_ out of date), just the diffs master->next and next->pu. The current next->pu diff looks like: $ diff nsc psc 0a1,2 > attr.o - attr_name_valid > attr.o - invalid_attr_name_message 17a20 > convert.o - stop_multi_file_filter 34d36 < quote.o - sq_quotef 43a46 > sequencer.o - append_new_todo 50a54 > trailer.o - conf_head 55a60,61 > wt-status.o - has_uncommitted_changes > wt-status.o - has_unstaged_changes $ I have already sent mails about 'stop_multi_file_filter', 'append_new_todo' and 'conf_head'. The symbols in attr.o are part of the revamp of the attribute system that Junio and Stefan are working on, which is very much in flux. If they are still there when it looks to have firmed up, then I will take a look. The 'sq_quotef' symbol is an example of a useful function that was written without having an immediate user (actually, if memory serves correctly, it was originally used in the series which introduced it, but later modifications removed the caller). It now has a user. The wt-status.o symbols have been introduced by Johannes in his 'rebase -i' series (of series) and I am assuming that these symbols will find additional external callers in up-coming series. > Maybe I can add `make sparse` to the TravisCI build? I don't know how feasible that would be. Apparently, the version of sparse in most distribution repositories is so old that it spews so many spurious errors as to make it unusable. See, for example: https://public-inbox.org/git/56CA5753.9030308@ramsayjones.plus.com/ So, you would need to build from source to get a usable version (I am currently running a version built with some local patches). If you were to build from the sparse repo's master branch, then it would work great on Linux, at least. I have no idea if it would work on mac OS X. However, even then, a single sparse warning is issued on Linux (for the master branch, several others on pu): SP pack-revindex.c pack-revindex.c:65:23: warning: memset with byte count of 262144 This is a problem with sparse (the byte count limit used is hardcoded into sparse), but I haven't sent a patch upstream to fix it yet. (I've been looking at that warning for _many_ years, so don't hold your breath!) ATB, Ramsay Jones [-- Attachment #2: static-check.pl --] [-- Type: application/x-perl, Size: 1333 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] convert: mark a file-local symbol static 2016-10-17 1:37 ` Ramsay Jones @ 2016-10-17 2:18 ` Jeff King 2016-10-17 9:04 ` Johannes Schindelin 2016-10-17 17:15 ` Ramsay Jones 0 siblings, 2 replies; 14+ messages in thread From: Jeff King @ 2016-10-17 2:18 UTC (permalink / raw) To: Ramsay Jones; +Cc: Lars Schneider, Junio C Hamano, GIT Mailing-list On Mon, Oct 17, 2016 at 02:37:58AM +0100, Ramsay Jones wrote: > Hmm, well, you have to remember that 'make clean' sometimes > doesn't make clean. Ever since the Makefile was changed to only > remove $(OBJECTS), rather than *.o xdiff/*.o etc., you have to > remember to 'make clean' _before_ you switch branches. Otherwise, > you risk leaving some objects laying around. Since the script > runs 'nm' on all objects it finds, any stale ones can cause problems. > (Of course, I almost always forget, so I frequently have to manually > check for and remove stale objects!) Gross. I would not be opposed to a Makefile rule that outputs the correct set of OBJECTS so this (or other) scripts could build on it. IIRC, BSD make has an option to do this "make -V OBJECTS" or something, but I don't thnk there's an easy way to do so. Or, since it seems to find useful results quite frequently, maybe it would be worth including the script inside git (and triggering it with an optional Makefile rule). It sounds like we'd need a way to annotate known false positives, but if it were in common use, it would be easier to get people to keep that list up to date. -Peff ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] convert: mark a file-local symbol static 2016-10-17 2:18 ` Jeff King @ 2016-10-17 9:04 ` Johannes Schindelin 2016-10-17 9:37 ` Jeff King 2016-10-17 17:15 ` Ramsay Jones 1 sibling, 1 reply; 14+ messages in thread From: Johannes Schindelin @ 2016-10-17 9:04 UTC (permalink / raw) To: Jeff King; +Cc: Ramsay Jones, Lars Schneider, Junio C Hamano, GIT Mailing-list Hi Ramsay & Peff, On Sun, 16 Oct 2016, Jeff King wrote: > On Mon, Oct 17, 2016 at 02:37:58AM +0100, Ramsay Jones wrote: > > > Hmm, well, you have to remember that 'make clean' sometimes doesn't > > make clean. Ever since the Makefile was changed to only remove > > $(OBJECTS), rather than *.o xdiff/*.o etc., you have to remember to > > 'make clean' _before_ you switch branches. Otherwise, you risk leaving > > some objects laying around. Since the script runs 'nm' on all objects > > it finds, any stale ones can cause problems. (Of course, I almost > > always forget, so I frequently have to manually check for and remove > > stale objects!) > > Gross. I would not be opposed to a Makefile rule that outputs the > correct set of OBJECTS so this (or other) scripts could build on it. You could also use the method I use in Git for Windows to "extend" the Makefile: -- snipsnap -- cat >dummy.mak <<EOF include Makefile blub: $(OBJECTS) do-something-with $^ EOF make -f dummy.mak blub ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] convert: mark a file-local symbol static 2016-10-17 9:04 ` Johannes Schindelin @ 2016-10-17 9:37 ` Jeff King 2016-10-17 17:21 ` Ramsay Jones 0 siblings, 1 reply; 14+ messages in thread From: Jeff King @ 2016-10-17 9:37 UTC (permalink / raw) To: Johannes Schindelin Cc: Ramsay Jones, Lars Schneider, Junio C Hamano, GIT Mailing-list On Mon, Oct 17, 2016 at 11:04:19AM +0200, Johannes Schindelin wrote: > > Gross. I would not be opposed to a Makefile rule that outputs the > > correct set of OBJECTS so this (or other) scripts could build on it. > > You could also use the method I use in Git for Windows to "extend" the > Makefile: > > -- snipsnap -- > cat >dummy.mak <<EOF > include Makefile > > blub: $(OBJECTS) > do-something-with $^ > EOF > > make -f dummy.mak blub Hacky but clever. I like it. In the particular case of git, I think I've cheated similarly before by putting things in config.mak, though of course an arbitrary script can't assume it can overwrite that file. -Peff ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] convert: mark a file-local symbol static 2016-10-17 9:37 ` Jeff King @ 2016-10-17 17:21 ` Ramsay Jones 2016-10-17 20:07 ` Junio C Hamano 0 siblings, 1 reply; 14+ messages in thread From: Ramsay Jones @ 2016-10-17 17:21 UTC (permalink / raw) To: Jeff King, Johannes Schindelin Cc: Lars Schneider, Junio C Hamano, GIT Mailing-list On 17/10/16 10:37, Jeff King wrote: > On Mon, Oct 17, 2016 at 11:04:19AM +0200, Johannes Schindelin wrote: > >>> Gross. I would not be opposed to a Makefile rule that outputs the >>> correct set of OBJECTS so this (or other) scripts could build on it. >> >> You could also use the method I use in Git for Windows to "extend" the >> Makefile: >> >> -- snipsnap -- >> cat >dummy.mak <<EOF >> include Makefile >> >> blub: $(OBJECTS) >> do-something-with $^ >> EOF >> >> make -f dummy.mak blub > > Hacky but clever. I like it. > > In the particular case of git, I think I've cheated similarly before by > putting things in config.mak, though of course an arbitrary script can't > assume it can overwrite that file. Heh, I actually have the following in my config.mak already: extra-clean: clean find . -iname '*.o' -exec rm {} \; But for some reason I _always_ type 'make clean' and then, to top it off, I _always_ type the 'find' command by hand (I have no idea why) :-D ATB, Ramsay Jones ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] convert: mark a file-local symbol static 2016-10-17 17:21 ` Ramsay Jones @ 2016-10-17 20:07 ` Junio C Hamano 2016-10-17 20:48 ` René Scharfe ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Junio C Hamano @ 2016-10-17 20:07 UTC (permalink / raw) To: Ramsay Jones Cc: Jeff King, Johannes Schindelin, Lars Schneider, GIT Mailing-list Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > Heh, I actually have the following in my config.mak already: > > extra-clean: clean > find . -iname '*.o' -exec rm {} \; > > But for some reason I _always_ type 'make clean' and then, to top > it off, I _always_ type the 'find' command by hand (I have no idea > why) :-D "git clean -x" anybody? ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] convert: mark a file-local symbol static 2016-10-17 20:07 ` Junio C Hamano @ 2016-10-17 20:48 ` René Scharfe 2016-10-17 20:48 ` Stefan Beller 2016-10-17 21:48 ` Ramsay Jones 2 siblings, 0 replies; 14+ messages in thread From: René Scharfe @ 2016-10-17 20:48 UTC (permalink / raw) To: Junio C Hamano, Ramsay Jones Cc: Jeff King, Johannes Schindelin, Lars Schneider, GIT Mailing-list Am 17.10.2016 um 22:07 schrieb Junio C Hamano: > Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > >> Heh, I actually have the following in my config.mak already: >> >> extra-clean: clean >> find . -iname '*.o' -exec rm {} \; >> >> But for some reason I _always_ type 'make clean' and then, to top >> it off, I _always_ type the 'find' command by hand (I have no idea >> why) :-D > > "git clean -x" anybody? This removes config.mak as well. René ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] convert: mark a file-local symbol static 2016-10-17 20:07 ` Junio C Hamano 2016-10-17 20:48 ` René Scharfe @ 2016-10-17 20:48 ` Stefan Beller 2016-10-17 21:48 ` Ramsay Jones 2 siblings, 0 replies; 14+ messages in thread From: Stefan Beller @ 2016-10-17 20:48 UTC (permalink / raw) To: Junio C Hamano Cc: Ramsay Jones, Jeff King, Johannes Schindelin, Lars Schneider, GIT Mailing-list On Mon, Oct 17, 2016 at 1:07 PM, Junio C Hamano <gitster@pobox.com> wrote: > Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > >> Heh, I actually have the following in my config.mak already: >> >> extra-clean: clean >> find . -iname '*.o' -exec rm {} \; >> >> But for some reason I _always_ type 'make clean' and then, to top >> it off, I _always_ type the 'find' command by hand (I have no idea >> why) :-D > > "git clean -x" anybody? I only want to cleanup compiled stuff and such, not the precious unversioned text files I have laying around here. So I guess git clean -x -e:(attr:!binary) would suffice, though. ;) ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] convert: mark a file-local symbol static 2016-10-17 20:07 ` Junio C Hamano 2016-10-17 20:48 ` René Scharfe 2016-10-17 20:48 ` Stefan Beller @ 2016-10-17 21:48 ` Ramsay Jones 2 siblings, 0 replies; 14+ messages in thread From: Ramsay Jones @ 2016-10-17 21:48 UTC (permalink / raw) To: Junio C Hamano Cc: Jeff King, Johannes Schindelin, Lars Schneider, GIT Mailing-list On 17/10/16 21:07, Junio C Hamano wrote: > Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > >> Heh, I actually have the following in my config.mak already: >> >> extra-clean: clean >> find . -iname '*.o' -exec rm {} \; >> >> But for some reason I _always_ type 'make clean' and then, to top >> it off, I _always_ type the 'find' command by hand (I have no idea >> why) :-D > > "git clean -x" anybody? I don't use 'git clean' because, on the very few occasions that I have tried to use it, it always deletes _far_ more than I thought it would or should. (particularly config.mak). Hmm, "git clean -X -- '*.o'" _might_ do what I want, but 'find' is so much easier ... :-D ATB, Ramsay Jones ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] convert: mark a file-local symbol static 2016-10-17 2:18 ` Jeff King 2016-10-17 9:04 ` Johannes Schindelin @ 2016-10-17 17:15 ` Ramsay Jones 1 sibling, 0 replies; 14+ messages in thread From: Ramsay Jones @ 2016-10-17 17:15 UTC (permalink / raw) To: Jeff King; +Cc: Lars Schneider, Junio C Hamano, GIT Mailing-list On 17/10/16 03:18, Jeff King wrote: > On Mon, Oct 17, 2016 at 02:37:58AM +0100, Ramsay Jones wrote: > >> Hmm, well, you have to remember that 'make clean' sometimes >> doesn't make clean. Ever since the Makefile was changed to only >> remove $(OBJECTS), rather than *.o xdiff/*.o etc., you have to >> remember to 'make clean' _before_ you switch branches. Otherwise, >> you risk leaving some objects laying around. Since the script >> runs 'nm' on all objects it finds, any stale ones can cause problems. >> (Of course, I almost always forget, so I frequently have to manually >> check for and remove stale objects!) > > Gross. I would not be opposed to a Makefile rule that outputs the > correct set of OBJECTS so this (or other) scripts could build on it. > > IIRC, BSD make has an option to do this "make -V OBJECTS" or something, > but I don't thnk there's an easy way to do so. Hmm, I would go in the opposite direction and take a leaf out of Ævar's book (see commit bc548efe) and this one-liner: diff --git a/Makefile b/Makefile index ee89c06..c08c25e 100644 --- a/Makefile +++ b/Makefile @@ -2506,7 +2506,7 @@ profile-clean: clean: profile-clean coverage-clean $(RM) *.res - $(RM) $(OBJECTS) + $(RM) $(addsuffix *.o,$(object_dirs)) $(RM) $(LIB_FILE) $(XDIFF_LIB) $(VCSSVN_LIB) $(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X $(RM) $(TEST_PROGRAMS) $(NO_INSTALL) This would actually solve my problem, but it actually isn't a _complete_ solution. (Hint: think about what isn't in $(OBJECTS), depending on the configuration). ;-) > Or, since it seems to find useful results quite frequently, maybe it > would be worth including the script inside git (and triggering it with > an optional Makefile rule). It sounds like we'd need a way to annotate > known false positives, but if it were in common use, it would be easier > to get people to keep that list up to date. Hmm, I suspect that wouldn't happen, which would reduce it usefulness and ultimately lead to it not being used. (Updating the 'stop list' would fast become a burden.) I find it useful to flag these issues automatically, but I still need to look at each symbol and decide what to do (you may not agree with some of my choices either - take a look at the output on the master branch!). The way I use it, I effectively ignore the 'stop list' maintenance issues. ATB, Ramsay Jones ^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2016-10-17 21:48 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-10-11 23:46 [PATCH] convert: mark a file-local symbol static Ramsay Jones 2016-10-15 15:05 ` Lars Schneider 2016-10-15 21:01 ` Ramsay Jones 2016-10-16 0:15 ` Lars Schneider 2016-10-17 1:37 ` Ramsay Jones 2016-10-17 2:18 ` Jeff King 2016-10-17 9:04 ` Johannes Schindelin 2016-10-17 9:37 ` Jeff King 2016-10-17 17:21 ` Ramsay Jones 2016-10-17 20:07 ` Junio C Hamano 2016-10-17 20:48 ` René Scharfe 2016-10-17 20:48 ` Stefan Beller 2016-10-17 21:48 ` Ramsay Jones 2016-10-17 17:15 ` Ramsay Jones
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.