On Wed, Oct 03, 2018 at 08:24:14AM +0200, Rasmus Villemoes wrote: > > The comment probably needs to spell out that exclude_guides > > is the same as your "we were invoked as...". > > Will do. That will also make the string --exclude-guides (i.e., with a > dash) appear in the comment, making it more likely to be found should > anyone change when and how --exclude-guides is implied.
OK. I can live with that. > > So we split only to find argv[0] here. But then we don't return it. That > > works because the split is done in place, meaning we must have inserted > > a NUL in alias. That's sufficiently subtle that it might be worth > > spelling it out in a comment. > > OK, I actually had precisely > > + /* > + * We use split_cmdline() to get the first word of the > + * alias, to ensure that we use the same rules as when > + * the alias is actually used. split_cmdline() > + * modifies alias in-place. > + */ > > in v1, but thought it might be overly verbose. I'll put it back in. :) That's perfect. > > We don't need to free alias here as we do above, because we're passing > > it back. We should free argv, though, I think (not its elements, just > > the array itself). > > Yeah, I thought about this, and removing free(argv) was the last thing I > did before sending v1 - because we were going to leak alias anyway. I'm > happy to put it back in, along with... Thanks. I think this is different than "alias" because we really do leak it _here_, whereas alias lives on and can be UNLEAKed later. > > Unfortunately the caller is going to leak our returned "alias", [...] I > > think it may be OK to overlook > > that and just UNLEAK() it in cmd_help(). > > ...this. Except I'd rather do the UNLEAK in check_git_cmd (the > documentation does say "only from cmd_* functions or their direct > helpers") to make it a more targeted annotation. Yeah, I think that's fine. Thanks! -Peff
