> On May 16, 2014, 7:32 a.m., Niklas Nielsen wrote: > > Hey Chengwei - sorry for the tardy turnaround time on this review request. > > > > To me, it still seems like we are treating the symptoms of the real issue: > > PATH is appended multiple times and the subsequent globbing adds the > > available commands to same number of times. > > The reason I am saying this is because the fix is difficult to understand > > (it is not immediate that this is the problem it solves) and seems very > > specialized for the "mesos help --help" and "mesos help help" case. > > > > Two things we could do: > > 1) Don't add the new path unconditionally to the PATH variable i.e. check > > if it is already there. > > 2) In usage(), don't add duplicates to the commands from the globbed list > > of candidates. This can be done pretty easy and local by using a set > > instead of a list. Try to take a look at: > > https://github.com/nqn/mesos/commit/01f77a1ab6d96f48765cc3539da1a1ebd4ba1d56 > > > > Thoughts? > > Adam B wrote: > +1 Love the 'set'. Calling "mesos help foo" will still recurse into main > and dirname will still be prepended to PATH multiple times, but the commands > will not be printed multiple times. > "mesos help help" will give a weird error ("Not expecting --help before > command") instead of calling usage, but I think that's a pretty contrived > case. > > Chengwei Yang wrote: > Hi Niklas, > > Sorry for late reply, so since the 2) improvement landed into usage(), so > anyway we can't get duplicated commands in usage now though the 1) thing is > still left to take. Do you like the first version of this patch? Which just > do the small fix, add directory to PATH in the first through. > > Niklas Nielsen wrote: > Can you point me review there 2) landed? If that's is in, why bother with > 1)? > I am still _not_ in favor of a notion of firstThrough with a static > variable - if anything, it should be firstPass and I already enumerated other > ways of doing it. > If you want to push for that fix still, I suggest we find another > shepherd for this RR. > > > Chengwei Yang wrote: > Sorry, I didn't aware that patch is still in your branch, not the > official mesos repo. Do you submit that patch recently? Even we have your > patch merged, the issue (directory added into $PATH more than once) is still > here. I'll align this patch with your vision soon. > > Niklas Nielsen wrote: > Hi Chengwei - do you still want this to go in? > > Chengwei Yang wrote: > I'll revisite this patch this week, just have one week holiday last week. > > Niklas Nielsen wrote: > Chengwei, do you still want this in?
Sorry, Niklas, please take it over and go ahead. - Chengwei ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19180/#review43181 ----------------------------------------------------------- On May 16, 2014, 6:48 a.m., Chengwei Yang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19180/ > ----------------------------------------------------------- > > (Updated May 16, 2014, 6:48 a.m.) > > > Review request for mesos and Niklas Nielsen. > > > Bugs: MESOS-1093 > https://issues.apache.org/jira/browse/MESOS-1093 > > > Repository: mesos > > > Description > ------- > > Fix mesos command parsing help > > Without this patch, "mesos help --help" will output below > > Not expecting '--help' before command > Usage: mesos <command> [OPTIONS] > > Available commands: > help > resolve > cat > scp > log > tail > execute > ps > local > resolve > cat > scp > log > tail > execute > ps > local > > Apparently all available commands printed twice, the "mesos help help" > will print available commands 3 times. > > The root cause is the directory contains available mesos commands are > added more than one times when recursive to main() again. > > Idea comes from Adam B. > > Review: https://reviews.apache.org/r/19180 > > > Diffs > ----- > > src/cli/mesos.cpp 171a707cd2ba2348898e7fbe8fe9f0634edd6d86 > > Diff: https://reviews.apache.org/r/19180/diff/ > > > Testing > ------- > > done? > > > Thanks, > > Chengwei Yang > >
