On 24/04/17 23:17, Stefan Beller wrote: > On Mon, Apr 24, 2017 at 3:11 PM, Ramsay Jones > <ram...@ramsayjones.plus.com> wrote: >> >> >> On 24/04/17 21:03, Stefan Beller wrote: >> [snip] >> >>> + >>> + argv_array_pushf(&cp.env_array, "name=%s", sub->name); >>> + argv_array_pushf(&cp.env_array, "path=%s", displaypath); >>> + argv_array_pushf(&cp.env_array, "sm_path=%s", displaypath); >>> >>> You mention keeping 'sm_path' in the notes after the commit message. I would >>> add that part to the commit message, to explain why we have multiple >>> variables >>> that have the same value. Maybe even a comment in the code: >>> >>> /* Keep sm_path for historic reasons, see tests in 091a6eb0fee. */ >>> .. sm_path .. >> >> Hmm, you need to be a bit careful with putting 'path' in the >> environment (if you then export it to sub-processes) on windows >> (cygwin, MinGW, GfW). See commit 64394e3ae9. I would have liked >> to remove $path altogether from the 'submodule-foreach api' in >> that commit, but users and their scripts were already using it >> (so I couldn't just drop it, without some deprecation period). >> So long as whatever was being 'eval'-ed in the script didn't >> export $path, ... >> > > Oh, I misread the comment > > # we make $path available to scripts ... > path=$sm_path > > as it was such a casual friendly thing to say in that context. > So the *real* historic baggage is > argv_array_pushf(&cp.env_array, "path=%s", displaypath); > whereas > argv_array_pushf(&cp.env_array, "sm_path=%s", displaypath); > is considered the correct way to go.
I have to admit that I didn't actually read the code in this patch. I just saw the subject line and the ass-backward comment about $sm_path. ;-) My intention was simply to warn: 'there be dragons'. ATB, Ramsay Jones
