On Thu, Sep 20, 2012 at 10:43 PM, Junio C Hamano <gits...@pobox.com> wrote: > Junio C Hamano <gits...@pobox.com> writes: >> Adam Spiers <g...@adamspiers.org> writes: >>> Adam Spiers (14): >>> Update directory listing API doc to match code >>> Improve documentation and comments regarding directory traversal API >>> Rename cryptic 'which' variable to more consistent name >>> Rename path_excluded() to is_path_excluded() >>> Rename excluded_from_list() to is_excluded_from_list() >>> Rename excluded() to is_excluded() >>> Refactor is_excluded_from_list() >>> Refactor is_excluded() >>> Refactor is_path_excluded() >>> For each exclude pattern, store information about where it came from >>> Refactor treat_gitlinks() >>> Extract some useful pathspec handling code from builtin/add.c into a >>> library >>> Provide free_directory() for reclaiming dir_struct memory >>> Add git-check-ignore sub-command >> >> Please retitle these to have a short "prefix: " that names a >> specific area the series intends to touch. I retitled your other >> series to share "test :" as their common prefix. > > Just to clarify, I think most of them can say "dir.c: ".
Sure, I can do that, but shouldn't this convention be documented in SubmittingPatches? > I saw quite a few decl-after-statement in new code. Please fix > them. Again, I can do that no problem, but again this convention is undocumented and I am not psychic ;-) I see that a patch was provided 5 years ago to document this, but was apparently never pulled in: http://thread.gmane.org/gmane.comp.version-control.git/47843/focus=48015 It would save everybody's time if these things are documented, since then they wouldn't create noise during the review process. I also see in the same thread that a patch to add -Wdeclaration-after-statement to CFLAGS was also offered but never pulled in, presumably on the stated grounds that that option was relatively recent 5 years ago. But wouldn't it be trivial to do gcc -v --help 2>&1 | grep declaration-after-statement at build-time? I'm also curious to know why this convention exists. Are people really still compiling git with compilers which don't support C99? > As to the "who owns x->src and when is it freed" question, it may > make sense to give el a "filename" field (command line and other > special cases would get whatever value you deem appropriate, like > NULL or "<command line>"), have x->src point at that field when you > queue many x's to the el in add_exc_from_file_to_list(). Then when > you pop an element in the exclude_stack, you can free el->filename > to plug a potential leak. > > Also I do not see why you need to have the strdup() in the caller of > add_excludes_from_file_to_list(). If you need to keep it stable > because you are copying it away in exclude or excludde_list, > wouldn't it make more sense to do that at the beginning of the > callee, i.e. add_excludes_from_file_to_list() function? Thanks, I'll take a look at these suggestions tomorrow. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html