Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MDEV-12192 - Shell-style alias/unalias commands for MariaDB client #321

Closed
wants to merge 1 commit into from

Conversation

nirbhayc
Copy link

I submit this patch under the BSD-new license.

Best,
Nirbhay

@grooverdan
Copy link
Member

I think you are after http://mycli.net/favorites :-)

@svoj svoj added this to the 10.2 milestone Mar 1, 2017
@svoj svoj self-assigned this Mar 7, 2017
@svoj
Copy link

svoj commented Mar 7, 2017

Hi Nirbhay,

Thanks for your contribution. JIRA task has been created to track this pull request: https://jira.mariadb.org/browse/MDEV-12192

This task was added to 10.2 backlog, which is planned to be handled between 2017-03-09 and 2017-03-16.

Thanks,
Sergey

@svoj svoj changed the title Shell-style alias/unalias commands for MariaDB client MDEV-12192 - Shell-style alias/unalias commands for MariaDB client Mar 7, 2017
@@ -1289,6 +1302,10 @@ int main(int argc,char *argv[])
"Type 'help;' or '\\h' for help. Type '\\c' to clear the current input statement.\n");
put_info(buff,INFO_INFO);
status.exit_status= read_and_execute(!status.batch);

/* Free alias hash. */
deinit_alias();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should go into mysql_end().

@@ -1196,6 +1205,10 @@ int main(int argc,char *argv[])
my_end(0);
exit(1);
}

/* Init alias hash. */
init_alias();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't simply exit() in init_alias(). We should free_defaults(defaults_argv) and my_end(0) as well.

String aliased_buffer;
if (alias_element)
{
aliased_buffer.copy(alias_element->value, strlen(alias_element->value),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it makes sense to store value length in ALIAS?

@@ -3262,6 +3311,7 @@ com_go(String *buffer,char *line __attribute__((unused)))
#endif

buffer->length(0);
aliased_buffer.length(0);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why is this needed?

charset_info);
}

aliased_buffer.append(alias_end, buffer->length() - alias_len);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... how does it work if alias is not found? FWICS we end up with a string which has characters up to a space trimmed, which is not expected right?

I'd generally suggest to use String::set(const char* ...) for the case when alias is not found. In this case we don't have to memcopy/alloc.


/* Remove leading whitespaces. */
while (*ptr && my_isspace(charset_info, *ptr))
ptr ++;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ptr++, ptr+= here and everywhere else please.

Since '\0' is not a space, you don't need to explicitly check for it.

free(line);

if (file)
my_fclose(file, MYF(0));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to close file unconditionally here, because according to code it can never be NULL.

@@ -272,6 +279,7 @@ typedef struct {

static COMMANDS commands[] = {
{ "?", '?', com_help, 1, "Synonym for `help'." },
{ "alias", 'a', com_alias, 1, "Print or add aliases." },
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate syntax here or anywhere in a comment.

}
*error= false;
}
return pos;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In current implementation, I guess you need to free(name). It'd be nice if parse_alias_name wouldn't do string copy.

{
my_hash_delete(&aliases, record);
}
alias= (ALIAS *) my_malloc(sizeof(ALIAS), MYF(MY_WME));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please either allocate ALIAS + name + value at once or use MEM_ROOT for all allocations.

@svoj
Copy link

svoj commented Sep 6, 2017

No feedback was provided for this pull request for over a month, so it is being closed. Feel free to reopen if you are able to implement requested changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants