Subject: Re: [GSoC][PATCH v8 00/20] Convert "git stash" to C builtin

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Fri, 31 Aug 2018 00:19:50 +0200

On Thu, Aug 30 2018, Paul-Sebastian Ungureanu wrote:

> Hello,
>
> This a new iteration of `stash.c`. What is new?
>
>  * Some commits got squashed. The commit related to replacing
>  `git apply` child process was dropped since it wasn't the best
>  idea.
>
>  * In v7, there was a bug [1] related to config `git stash show`
>  The bug was fixed and a test file was added for this.
>
>  * Fixed `git stash [push]` [2]. In v7, `git stash -q drop` would
>  act like `git stash push -q drop`.
>
>  * Fixed coding-style nits. Verified that messages are marked
>  for translation and are going to the correct output stream.
>
>  * Fixed one memory leak (related to `strbuf_detach`).
>
>  * Simplified the code a little bit.

This looks good, I read this and a previous version. I'e tested this on
Linux, FreeBSD & AIX. All tests pass on all of them.

One style nit: These patches consistently indent wrapped lines not to
align with the opening "(" (as is our usual style), but just with
"\n\t". I.e. this patch on top would make it like what we usually do
(but should be squashed as appropriate):

    diff --git a/builtin/stash.c b/builtin/stash.c
    index dd1084afd4..6d849ed811 100644
    --- a/builtin/stash.c
    +++ b/builtin/stash.c
    @@ -389,3 +389,3 @@ static int restore_untracked(struct object_id *u_tree)
     static int do_apply_stash(const char *prefix, struct stash_info *info,
    -	int index)
    +			  int index)
     {
    @@ -408,3 +408,3 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,
     		if (!oidcmp(&info->b_tree, &info->i_tree) || !oidcmp(&c_tree,
    -			&info->i_tree)) {
    +								     &info->i_tree)) {
     			has_index = 0;
    @@ -514,3 +514,3 @@ static int apply_stash(int argc, const char **argv, const char *prefix)
     		OPT_BOOL(0, "index", &index,
    -			N_("attempt to recreate the index")),
    +			 N_("attempt to recreate the index")),
     		OPT_END()
    @@ -610,3 +610,3 @@ static int pop_stash(int argc, const char **argv, const char *prefix)
     		OPT_BOOL(0, "index", &index,
    -			N_("attempt to recreate the index")),
    +			 N_("attempt to recreate the index")),
     		OPT_END()
    @@ -971,3 +971,3 @@ static int stash_patch(struct stash_info *info, struct pathspec ps, int quiet)
     	argv_array_pushl(&cp_add_i.args, "add--interactive", "--patch=stash",
    -			"--", NULL);
    +			 "--", NULL);
     	for (i = 0; i < ps.nr; ++i)
    @@ -1279,3 +1279,3 @@ static int do_push_stash(struct pathspec ps, const char *stash_msg, int quiet,
     		printf_ln(_("Saved working directory and index state %s"),
    -			stash_msg);
    +			  stash_msg);

    @@ -1413,5 +1413,5 @@ static int push_stash(int argc, const char **argv, const char *prefix)
     		OPT_SET_INT('k', "keep-index", &keep_index,
    -			N_("keep index"), 1),
    +			    N_("keep index"), 1),
     		OPT_BOOL('p', "patch", &patch_mode,
    -			N_("stash in patch mode")),
    +			 N_("stash in patch mode")),
     		OPT__QUIET(&quiet, N_("quiet mode")),
    @@ -1422,3 +1422,3 @@ static int push_stash(int argc, const char **argv, const char *prefix)
     		OPT_STRING('m', "message", &stash_msg, N_("message"),
    -			 N_("stash message")),
    +			   N_("stash message")),
     		OPT_END()
    @@ -1450,5 +1450,5 @@ static int save_stash(int argc, const char **argv, const char *prefix)
     		OPT_SET_INT('k', "keep-index", &keep_index,
    -			N_("keep index"), 1),
    +			    N_("keep index"), 1),
     		OPT_BOOL('p', "patch", &patch_mode,
    -			N_("stash in patch mode")),
    +			 N_("stash in patch mode")),
     		OPT__QUIET(&quiet, N_("quiet mode")),

Tip: It's also better to get feedback by CC-ing people who've had
feedback on previous versions.