Re: [PATCH v4 1/2] add: add --ignore-submodules[=<when>] parameter

[Ronald Weiss <weiss.ronald@gmail.com>]

On 18. 4. 2014 13:53, Jens Lehmann wrote:
> Am 13.04.2014 00:45, schrieb Ronald Weiss:
>> Allow ignoring submodules (or not) by command line switch, like diff
>> and status do.
>>
>> Git add currently doesn't honor ignore from .gitmodules or .git/config,
>> which is related functionality, however I'd like to change that in
>> another patch, coming soon.
> 
> I think we should drop this paragraph from this commit message. Though
> I believe it's helpful to add this information after the "---" below
> to inform readers of the list of your future plans.
> 
>> This commit is also a prerequisite for the next one in series, which
>> adds the --ignore-submodules switch to git commit.
> 
> But this information definitely belongs here.
> 
>> That's why signature
>> of function add_files_to_cache was changed.
> 
> But that's necessary for this patch too, no?

No, for this patch alone, we could just use the global variable, which
is set up by parse_options(), without changing the public function and
breaking compilation in other files.

> 
>> That also requires compilo fixes in checkout.c and commit.c
> 
> Sorry, but I don't know what a "compilo fix" is ;-) .. I suspect you
> mean that add_files_to_cache() needs a new NULL argument in some
> places. What about dropping the last two sentences and adding
> something like "Add a new argument to add_files_to_cache() to pass
> the command line option and update all other call sites to pass
> NULL instead." to the first paragraph?
> 
> Apart from that and the flags of the test (see below) this patch
> is looking good to me.

OK, I'll update the commit message, and fix the file mode for the added
test script. Will repost once we sort out the problems (failing tests)
You have with the second patch (for commit).

>> diff --git a/t/t3704-add-ignore-submodules.sh b/t/t3704-add-ignore-submodules.sh
>> new file mode 100644
> 
> The flags should be 100755 here, currently "make test" fails because
> of this.

I'm sorry for this, I was testing it on Windows, where the file mode
doesn't matter, that's why I missed it.