mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Niklas Nielsen" <...@qni.dk>
Subject Re: Review Request 19180: Fix mesos command parsing help
Date Wed, 11 Feb 2015 00:51:25 GMT


> On May 15, 2014, 4:32 p.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.

Chengwei, do you still want this in?


- Niklas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19180/#review43181
-----------------------------------------------------------


On May 15, 2014, 3:48 p.m., Chengwei Yang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19180/
> -----------------------------------------------------------
> 
> (Updated May 15, 2014, 3:48 p.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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message