Re: [PATCH v5 2/5] stash: convert apply to builtin

[Johannes Schindelin]

Hi Joel,

completely forgot to reiterate this (my reply to Christian was probably
buried in the thread):

On Wed, 4 Apr 2018, Joel Teichroeb wrote:

> +int cmd_stash__helper(int argc, const char **argv, const char *prefix)
> +{
> + int result = 0;
> + pid_t pid = getpid();
> + const char *index_file;
> +
> + struct option options[] = {
> + OPT_END()
> + };
> +
> + git_config(git_default_config, NULL);
> +
> + argc = parse_options(argc, argv, prefix, options, 
> git_stash_helper_usage,
> + PARSE_OPT_KEEP_UNKNOWN|PARSE_OPT_KEEP_DASHDASH);
> +
> + index_file = get_index_file();
> + strbuf_addf(_index_path, "%s.stash.%" PRIuMAX, index_file, 
> (uintmax_t)pid);
> +
> + if (argc < 1)
> + usage_with_options(git_stash_helper_usage, options);
> + else if (!strcmp(argv[0], "apply"))
> + result = apply_stash(argc, argv, prefix);
> + else {
> + error(_("unknown subcommand: %s"), argv[0]);
> + usage_with_options(git_stash_helper_usage, options);
> + result = 1;
> + }
> +
> + return result;
> +}

The `result` variable can contain negative values, while the exit status
really should be either 0 or 1 (and the return value of
`cmd_stash__helper()` is what `run_builtin()` hands to `exit()` as exit
status). So please use

return !!result;

here (which returns 0 if result is 0, and 1 otherwise).

Ciao,
Dscho

Re: [PATCH v5 2/5] stash: convert apply to builtin

[Johannes Schindelin]

Hi Joel,

On Wed, 4 Apr 2018, Joel Teichroeb wrote:

> Add a bulitin helper for performing stash commands. Converting
> all at once proved hard to review, so starting with just apply
> let conversion get started without the other command being

s/let/lets the/ s/command/commands/

> finished.
> 
> The helper is being implemented as a drop in replacement for
> stash so that when it is complete it can simply be renamed and
> the shell script deleted.
> 
> Delete the contents of the apply_stash shell function and replace
> it with a call to stash--helper apply until pop is also
> converted.
> 
> [...]
>
> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> new file mode 100644
> index 00..9d00a9547d
> --- /dev/null
> +++ b/builtin/stash--helper.c
> @@ -0,0 +1,431 @@
> +#include "builtin.h"
> +#include "config.h"
> +#include "parse-options.h"
> +#include "refs.h"
> +#include "lockfile.h"
> +#include "cache-tree.h"
> +#include "unpack-trees.h"
> +#include "merge-recursive.h"
> +#include "argv-array.h"
> +#include "run-command.h"
> +#include "dir.h"
> +#include "rerere.h"
> +
> +static const char * const git_stash_helper_usage[] = {

Note to other reviewers: while it would appear that our convention is
usually to cuddle the `*` with the identifier afterwards (i.e. `char
*const` instead of `char * const`), it would *also* appear that we somehow
prefer spaces on both sides when it comes to the _usage:

$ git grep 'usage\[' builtin/*.c | grep -c ' \* '
88

$ git grep 'usage\[' builtin/*.c | grep -c ' \*[^ ]'
18

So much to clean up and make more consistent... (not your problem, Joel)

> + N_("git stash--helper apply [--index] [-q|--quiet] []"),
> + NULL
> +};
> +
> +static const char * const git_stash_helper_apply_usage[] = {
> + N_("git stash--helper apply [--index] [-q|--quiet] []"),
> + NULL
> +};
> +
> +static const char *ref_stash = "refs/stash";
> +static int quiet;
> +static struct strbuf stash_index_path = STRBUF_INIT;
> +
> +struct stash_info {
> + struct object_id w_commit;
> + struct object_id b_commit;
> + struct object_id i_commit;
> + struct object_id u_commit;
> + struct object_id w_tree;
> + struct object_id b_tree;
> + struct object_id i_tree;
> + struct object_id u_tree;
> + struct strbuf revision;
> + int is_stash_ref;
> + int has_u;
> +};
> +
> +static int grab_oid(struct object_id *oid, const char *fmt, const char *rev)
> +{
> + struct strbuf buf = STRBUF_INIT;
> + int ret;
> +
> + strbuf_addf(, fmt, rev);
> + ret = get_oid(buf.buf, oid);
> + strbuf_release();
> + return ret;
> +}

It could be even more reusable, by using

int get_oidf(struct object_id *oid, const char *fmt, ...)
{
va_list ap;
char *p;
int ret;

va_start(ap, fmt);
p = xstrvfmt(fmt, ap);
va_end(ap);
ret = get_oid(p, oid);
free(p);

return ret;
}

Feel free to do this as a preparatory patch (it should probably live next
to get_oid()), otherwise there is always an opportunity for a
micro-project (or not so micro- one).

> +static void free_stash_info(struct stash_info *info)
> +{
> + strbuf_release(>revision);
> +}
> +
> +static int get_stash_info(struct stash_info *info, int argc, const char 
> **argv)
> +{
> + struct strbuf symbolic = STRBUF_INIT;
> + int ret;
> + const char *revision;
> + const char *commit = NULL;
> + char *end_of_rev;
> + char *expanded_ref;
> + struct object_id discard;

I think elsewhere we prefer the name "dummy" for variables that are
assigned without really needing the values.

> +
> + if (argc > 1) {
> + int i;
> + struct strbuf refs_msg = STRBUF_INIT;
> + for (i = 0; i < argc; ++i)
> + strbuf_addf(_msg, " '%s'", argv[i]);
> +
> + fprintf_ln(stderr, _("Too many revisions specified:%s"), 
> refs_msg.buf);

Note to interested parties: while it may have been okay for a mere Unix
shell script to output such a message directly to stderr, in C, we need to
use `error()` and start the error message with lower-case. This should
*not* be done as part of the conversion to C, which is kind of bug-for-bug.

> + strbuf_release(_msg);
> +
> + return -1;
> + }
> +
> + if (argc == 1)
> + commit = argv[0];

It is probably a bit too magical to assign `commit = argv[0]` right
away... but it would work, as `argv` is guaranteed to be NULL-terminated.

> +
> + strbuf_init(>revision, 0);
> + if (commit == NULL) {

We usually use `!commit` instead of `commit == NULL`. It's just the style
we use, not a judgement.

> + if (!ref_exists(ref_stash)) {
> + free_stash_info(info);
> + return error(_("No stash entries found."));

Technically, the shell version used `die`. We could use the same here,
too, but `error()` seems to be fine, too.

However, if you 

Re: [PATCH v5 2/5] stash: convert apply to builtin

[Johannes Schindelin]

Hi Christian,

[please cull a *lot* more of the quoted mail when you do not reference any
of it... Thanks]

On Thu, 5 Apr 2018, Christian Couder wrote:

> On Thu, Apr 5, 2018 at 4:28 AM, Joel Teichroeb  wrote:
> >
> > [...]
> > +
> > +   revision = info->revision.buf;
> > +
> > +   if (get_oid(revision, >w_commit)) {
> > +   error(_("%s is not a valid reference"), revision);
> > +   free_stash_info(info);
> > +   return -1;
> 
> Maybe:
> 
>free_stash_info(info);
>return error(_("%s is not a valid reference"), revision);
> 
> to save one line and be more consistent with above.

No. The parameter `revision` of the `error()` call is assigned just above
the `if()` block and clearly points into the `info` structure. So you must
not release that `info` before printing the error. The order of statements
is correct.

> > +   if (grab_oid(>b_commit, "%s^1", revision) ||
> > +   grab_oid(>w_tree, "%s:", revision) ||
> > +   grab_oid(>b_tree, "%s^1:", revision) ||
> > +   grab_oid(>i_tree, "%s^2:", revision)) {
> > +
> > +   error(_("'%s' is not a stash-like commit"), revision);
> > +   free_stash_info(info);
> > +   return -1;
> 
> Here also.

Yes, here, too, `revision` points at a field inside `info`, so we must not
release it before using it.

> > +   if (info->has_u) {
> > +   if (restore_untracked(>u_tree))
> > +   return error(_("Could not restore untracked files 
> > from stash"));
> > +   }
> 
> Maybe:
> 
>if (info->has_u && restore_untracked(>u_tree))
>return error(_("Could not restore untracked files from 
> stash"));

I agree with this, as it avoids an unncessary indentation level.

> So maybe we can get rid of `result` and have something like:
> 
>if (argc < 1) {
>error(_("at least one argument is required"));
>usage_with_options(git_stash_helper_usage, options);
>}
> 
>if (!strcmp(argv[0], "apply"))
>return apply_stash(argc, argv, prefix);

... except we have to use !!apply_stash() here: apply_stash() probably
returns -1 in case of error (at least that would be consistent with our
coding conventions), and the return value from cmd_*() is handed to exit()
as exit status. The `!!` trick turns any non-zero value into a 1, also
consistent with our coding conventions where we set exit code 1 upon error
in the "business logic".

> 
>error(_("unknown subcommand: %s"), argv[0]);
>usage_with_options(git_stash_helper_usage, options);
> }

Ciao,
Dscho

Re: [PATCH v5 2/5] stash: convert apply to builtin

[Christian Couder]

On Thu, Apr 5, 2018 at 9:59 AM, Christian Couder
 wrote:
> On Thu, Apr 5, 2018 at 9:50 AM, Christian Couder
>  wrote:
>>
>> So maybe we can get rid of `result` and have something like:
>>
>>if (argc < 1) {
>>error(_("at least one argument is required"));
>>usage_with_options(git_stash_helper_usage, options);
>
> Maybe we could also simplify these 2 lines by using usage_msg_opt().
>
>>}
>>
>>if (!strcmp(argv[0], "apply"))
>>return apply_stash(argc, argv, prefix);
>>
>>error(_("unknown subcommand: %s"), argv[0]);
>>usage_with_options(git_stash_helper_usage, options);

And here actually we could improve the above 2 lines using something like:

usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
  git_stash_helper_usage, options);

It's better than using `error()` because the printed message will
start with "fatal" instead of "error".

Re: [PATCH v5 2/5] stash: convert apply to builtin

[Christian Couder]

On Thu, Apr 5, 2018 at 9:50 AM, Christian Couder
 wrote:
>
> So maybe we can get rid of `result` and have something like:
>
>if (argc < 1) {
>error(_("at least one argument is required"));
>usage_with_options(git_stash_helper_usage, options);

Maybe we could also simplify these 2 lines by using usage_msg_opt().

>}
>
>if (!strcmp(argv[0], "apply"))
>return apply_stash(argc, argv, prefix);
>
>error(_("unknown subcommand: %s"), argv[0]);
>usage_with_options(git_stash_helper_usage, options);
> }

Re: [PATCH v5 2/5] stash: convert apply to builtin

[Christian Couder]

On Thu, Apr 5, 2018 at 4:28 AM, Joel Teichroeb  wrote:
> Add a bulitin helper for performing stash commands. Converting
> all at once proved hard to review, so starting with just apply
> let conversion get started without the other command being
> finished.
>
> The helper is being implemented as a drop in replacement for
> stash so that when it is complete it can simply be renamed and
> the shell script deleted.
>
> Delete the contents of the apply_stash shell function and replace
> it with a call to stash--helper apply until pop is also
> converted.
>
> Signed-off-by: Joel Teichroeb 
> ---
>  .gitignore  |   1 +
>  Makefile|   1 +
>  builtin.h   |   1 +
>  builtin/stash--helper.c | 431 
> 
>  git-stash.sh|  75 +
>  git.c   |   1 +
>  6 files changed, 440 insertions(+), 70 deletions(-)
>  create mode 100644 builtin/stash--helper.c
>
> diff --git a/.gitignore b/.gitignore
> index 833ef3b0b7..296d5f376d 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -152,6 +152,7 @@
>  /git-show-ref
>  /git-stage
>  /git-stash
> +/git-stash--helper
>  /git-status
>  /git-stripspace
>  /git-submodule
> diff --git a/Makefile b/Makefile
> index 96f6138f63..6cfdbe9a32 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1028,6 +1028,7 @@ BUILTIN_OBJS += builtin/send-pack.o
>  BUILTIN_OBJS += builtin/shortlog.o
>  BUILTIN_OBJS += builtin/show-branch.o
>  BUILTIN_OBJS += builtin/show-ref.o
> +BUILTIN_OBJS += builtin/stash--helper.o
>  BUILTIN_OBJS += builtin/stripspace.o
>  BUILTIN_OBJS += builtin/submodule--helper.o
>  BUILTIN_OBJS += builtin/symbolic-ref.o
> diff --git a/builtin.h b/builtin.h
> index 42378f3aa4..a14fd85b0e 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -219,6 +219,7 @@ extern int cmd_shortlog(int argc, const char **argv, 
> const char *prefix);
>  extern int cmd_show(int argc, const char **argv, const char *prefix);
>  extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
>  extern int cmd_status(int argc, const char **argv, const char *prefix);
> +extern int cmd_stash__helper(int argc, const char **argv, const char 
> *prefix);
>  extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
>  extern int cmd_submodule__helper(int argc, const char **argv, const char 
> *prefix);
>  extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> new file mode 100644
> index 00..9d00a9547d
> --- /dev/null
> +++ b/builtin/stash--helper.c
> @@ -0,0 +1,431 @@
> +#include "builtin.h"
> +#include "config.h"
> +#include "parse-options.h"
> +#include "refs.h"
> +#include "lockfile.h"
> +#include "cache-tree.h"
> +#include "unpack-trees.h"
> +#include "merge-recursive.h"
> +#include "argv-array.h"
> +#include "run-command.h"
> +#include "dir.h"
> +#include "rerere.h"
> +
> +static const char * const git_stash_helper_usage[] = {
> +   N_("git stash--helper apply [--index] [-q|--quiet] []"),
> +   NULL
> +};
> +
> +static const char * const git_stash_helper_apply_usage[] = {
> +   N_("git stash--helper apply [--index] [-q|--quiet] []"),
> +   NULL
> +};
> +
> +static const char *ref_stash = "refs/stash";
> +static int quiet;
> +static struct strbuf stash_index_path = STRBUF_INIT;
> +
> +struct stash_info {
> +   struct object_id w_commit;
> +   struct object_id b_commit;
> +   struct object_id i_commit;
> +   struct object_id u_commit;
> +   struct object_id w_tree;
> +   struct object_id b_tree;
> +   struct object_id i_tree;
> +   struct object_id u_tree;
> +   struct strbuf revision;
> +   int is_stash_ref;
> +   int has_u;
> +};
> +
> +static int grab_oid(struct object_id *oid, const char *fmt, const char *rev)
> +{
> +   struct strbuf buf = STRBUF_INIT;
> +   int ret;
> +
> +   strbuf_addf(, fmt, rev);
> +   ret = get_oid(buf.buf, oid);
> +   strbuf_release();
> +   return ret;
> +}
> +
> +static void free_stash_info(struct stash_info *info)
> +{
> +   strbuf_release(>revision);
> +}
> +
> +static int get_stash_info(struct stash_info *info, int argc, const char 
> **argv)
> +{
> +   struct strbuf symbolic = STRBUF_INIT;
> +   int ret;
> +   const char *revision;
> +   const char *commit = NULL;
> +   char *end_of_rev;
> +   char *expanded_ref;
> +   struct object_id discard;
> +
> +   if (argc > 1) {
> +   int i;
> +   struct strbuf refs_msg = STRBUF_INIT;
> +   for (i = 0; i < argc; ++i)
> +   strbuf_addf(_msg, " '%s'", argv[i]);
> +
> +   fprintf_ln(stderr, _("Too many revisions specified:%s"), 
> refs_msg.buf);
> +   strbuf_release(_msg);
> +
> +   return -1;
> +   }
> +
> +   if (argc == 1)
> +   commit