Re: [PATCH v3 4/4] stash: implement builtin stash

[Jeff King]

On Wed, May 31, 2017 at 08:29:43PM -0700, Joel Teichroeb wrote:

> I'm running into a lot of trouble using argv_array_clear. It seems
> that some of the builtin git cmd functions move the parameters around,
> and write new pointers to argv. There's three options I have now, and
> I'm not sure which is the best one.

Hrm. It's normal for parsing to reorder the parameters (e.g., shifting
non-options to the front), but that should still allow a clear at the
end. New pointers would definitely cause a problem, though. I don't know
of any cases where we do that, but on the other hand I wouldn't be too
surprised to find that the revision.c options parser does some nasty
tricks.

Do you have a specific example? I'd be curious to see if we can just fix
the parser to be less surprising (i.e., your (1) below).

> 1. Fix all the builtin cmd functions that I use to not mess around with argv

If it's just one or two spots, this might be viable.

> 2. Stop using the builtin cmd functions, and use child processes exclusively

That might not be the worst thing in the world for a first cut at a
shell to C transition, because it eliminates a whole class of possible
problems. But it really just side-steps the problem, as we'd want to
eventually deal with it and reduce the process count.

> 3. Don't worry about clearing the memory used for these function calls.

That might be do-able, as long as the leaks are O(1) for a program run
(and not say, a leak per commit). At the very least we should mark
those spots with a "NEEDSWORK" comment and an explanation of the issue
so that your work in finding them isn't wasted.

> It looks like the rest of the code generally does #3.

It looks like we don't actually pass argv arrays to setup_revisions()
all that often. The three I see are:

  - bisect_rev_setup(), which is a known leak. This is trickier, though,
because we actually pass the initialized rev_info out of the
function, and the memory needs to last until we're done with the
traversal

  - http-push, which does seem to free the memory

  - stat_tracking_info(), which does seem to free

I could well believe there are places where we leak, though, especially
for top-level functions that exit the program when they're done.

A fourth option is to massage the argv array into something that can be
massaged by the callee, and retain the original array for freeing. I.e.,
something like:

  struct argv_array argv = ARGV_ARRAY_INIT;
  const char **massaged;

  argv_array_pushl(, ...whatever...);

  ALLOC_ARRAY(massaged, argc);
  COPY_ARRAY(massaged, argv, argc);

  setup_revisions(argv.argc, massaged, , NULL);

  /*
   * No clue what's in "massaged" now, as setup_revisions() may have
   * reordered things, added new elements, deleted some, etc. But we
   * don't have to care because any pointers we need to free are still
   * in the original argv struct, and we should be safe to free the
   * massaged array itself.
   */
  free(massaged);
  argv_array_clear();

That's pretty horrible, though. If setup_revisions() is requiring us to
do that, I'd really prefer to look into fixing it.

-Peff

Re: [PATCH v3 4/4] stash: implement builtin stash

[Joel Teichroeb]

I'm running into a lot of trouble using argv_array_clear. It seems
that some of the builtin git cmd functions move the parameters around,
and write new pointers to argv. There's three options I have now, and
I'm not sure which is the best one.

1. Fix all the builtin cmd functions that I use to not mess around with argv
2. Stop using the builtin cmd functions, and use child processes exclusively
3. Don't worry about clearing the memory used for these function calls.

It looks like the rest of the code generally does #3.

Any advice here would be appreciated.

Re: [PATCH v3 4/4] stash: implement builtin stash

[Ævar Arnfjörð Bjarmason]

On Mon, May 29, 2017 at 8:18 PM, Joel Teichroeb  wrote:
> Once I have all those leaks fixed, is there a way to make sure I'm not
> missing any? I tried using valgrind with leak-check enabled, but there
> are too many leaks from other git commands.

I just used:

valgrind --leak-check=full ./git-stash list

And then skimmed things that mentioned stash.c in the pager. There
might be some better way to do this (e.g. instrument the test suite to
run valgrind for all commands and summarize that somehow), but I don't
know how to do that offhand...

Re: [PATCH v3 4/4] stash: implement builtin stash

[Joel Teichroeb]

Once I have all those leaks fixed, is there a way to make sure I'm not
missing any? I tried using valgrind with leak-check enabled, but there
are too many leaks from other git commands.

Joel

Re: [PATCH v3 4/4] stash: implement builtin stash

[Junio C Hamano]

Joel Teichroeb  writes:

> +int untracked_files(struct strbuf *out, int include_untracked,
> + const char **argv)

Does this need to be public?  

For a caller that wants to learn if there is _any_ untracked file,
having a strbuf that holds all output is overkill.  For a caller
that wants to learn what are the untracked paths, having a strbuf
that it needs to parse is not all that helpful.  Only for a caller
that does an unusual thing, namely, just grab the output and throw
it at another command as input, without checking what the output was
itself at all, would benefit.

So the interface to this function doesn't look like a very good one
if this wants to be a public helper.  Perhaps mark it as "static"?

> +{
> + struct child_process cp = CHILD_PROCESS_INIT;
> + cp.git_cmd = 1;
> + argv_array_push(, "ls-files");
> + argv_array_push(, "-o");
> + argv_array_push(, "-z");
> + if (include_untracked != 2)
> + argv_array_push(, "--exclude-standard");

This magic number "2" will be hard to grok by future readers.

> + argv_array_push(, "--");
> + if (argv)
> + argv_array_pushv(, argv);
> + return pipe_command(, NULL, 0, out, 0, NULL, 0);
> +}
> +
> +static int check_no_changes(const char *prefix, int include_untracked,
> + const char **argv)
> +{
> + struct argv_array args1;
> + struct argv_array args2;
> + struct strbuf out = STRBUF_INIT;
> +
> + argv_array_init();
> + argv_array_push(, "diff-index");
> + argv_array_push(, "--quiet");
> + argv_array_push(, "--cached");
> + argv_array_push(, "HEAD");
> + argv_array_push(, "--ignore-submodules");
> + argv_array_push(, "--");
> + if (argv)
> + argv_array_pushv(, argv);
> + argv_array_init();
> + argv_array_push(, "diff-files");
> + argv_array_push(, "--quiet");
> + argv_array_push(, "--ignore-submodules");
> + argv_array_push(, "--");
> + if (argv)
> + argv_array_pushv(, argv);

> + if (include_untracked)
> + untracked_files(, include_untracked, argv);
> + return cmd_diff_index(args1.argc, args1.argv, prefix) == 0 &&
> + cmd_diff_files(args2.argc, args2.argv, prefix) == 0 &&
> + (!include_untracked || out.len == 0);
> +}

Possible leak of out.buf here.

> +
> +static int get_stash_info(struct stash_info *info, const char *commit)
> +{

Judging from the callers of get_stash_info(), nobody passes a
"commit" as parameter; as far as they are concerned, they are
dealing with the name of one item in the stash (stash-id?).  They do
not care the fact that the item happens to be implemented as a
commit object.

Perhaps rename "commit" (parameter) to "stash_id" or something.

> + struct strbuf w_commit_rev = STRBUF_INIT;
> + struct strbuf b_commit_rev = STRBUF_INIT;
> + struct strbuf i_commit_rev = STRBUF_INIT;
> + struct strbuf u_commit_rev = STRBUF_INIT;
> + struct strbuf w_tree_rev = STRBUF_INIT;
> + struct strbuf b_tree_rev = STRBUF_INIT;
> + struct strbuf i_tree_rev = STRBUF_INIT;
> + struct strbuf u_tree_rev = STRBUF_INIT;
> + struct strbuf commit_buf = STRBUF_INIT;
> + struct strbuf symbolic = STRBUF_INIT;
> + struct strbuf out = STRBUF_INIT;
> + struct object_context unused;
> + char *str;
> + int ret;
> + const char *REV = commit;

Variables and field names that are all caps become misleading.
Avoid them.

> + struct child_process cp = CHILD_PROCESS_INIT;
> + info->is_stash_ref = 0;
> +
> + if (commit == NULL) {
> + strbuf_addf(_buf, "%s@{0}", ref_stash);
> + REV = commit_buf.buf;
> + } else if (strlen(commit) < 3) {
> + strbuf_addf(_buf, "%s@{%s}", ref_stash, commit);
> + REV = commit_buf.buf;
> + }
> + info->REV = REV;
> +
> + strbuf_addf(_commit_rev, "%s", REV);
> + strbuf_addf(_commit_rev, "%s^1", REV);
> + strbuf_addf(_commit_rev, "%s^2", REV);
> + strbuf_addf(_commit_rev, "%s^3", REV);
> + strbuf_addf(_tree_rev, "%s:", REV);
> + strbuf_addf(_tree_rev, "%s^1:", REV);
> + strbuf_addf(_tree_rev, "%s^2:", REV);
> + strbuf_addf(_tree_rev, "%s^3:", REV);
> +
> +
> + ret = (
> + get_sha1_with_context(w_commit_rev.buf, 0, info->w_commit.hash, 
> ) == 0 &&
> + get_sha1_with_context(b_commit_rev.buf, 0, info->b_commit.hash, 
> ) == 0 &&
> + get_sha1_with_context(i_commit_rev.buf, 0, info->i_commit.hash, 
> ) == 0 &&
> + get_sha1_with_context(w_tree_rev.buf, 0, info->w_tree.hash, 
> ) == 0 &&
> + get_sha1_with_context(b_tree_rev.buf, 0, info->b_tree.hash, 
> ) == 0 &&
> + get_sha1_with_context(i_tree_rev.buf, 0, info->i_tree.hash, 
> ) == 0);

Hmph.  What's the reason to use get_sha1_with_context() if you
declare the context is unused?  Wouldn't plain-vanilla get_sha1()
work better?

Re: [PATCH v3 4/4] stash: implement builtin stash

[Jeff King]

On Sun, May 28, 2017 at 11:31:48AM -0700, Joel Teichroeb wrote:

> >> +   /* TODO: Improve this logic */
> >> +   strbuf_addf(, "%s", REV);
> >> +   str = strstr(symbolic.buf, "@");
> >
> > Could you elaborate on how this should be improved?
> 
> I just figured there would be a builtin function that could help here,
> but hadn't had the chance to look into it. It's something easy to do
> in bash, but more complicated in C.

There's no strbuf function for "truncate at this character". But:

  - you can use strchr for a single-character match, which is more
efficient; i.e.:

  str = strchr(symbolic.buf, '@');

  - instead of inserting a '\0' into the strbuf, use strbuf_setlen(),
which also updates the symbolic.len; i.e.:

  strbuf_setlen(, str - symbolic.buf);

  - it looks like you copy into the strbuf just to truncate, so you
could actually get the final size before inserting into the strbuf
using strchrnul. Like:

  end_of_rev = strchrnul(REV, '@');
  strbuf_add(, REV, end_of_rev - REV);

-Peff

Re: [PATCH v3 4/4] stash: implement builtin stash

[Jeff King]

On Sun, May 28, 2017 at 08:51:07PM +0200, Ævar Arnfjörð Bjarmason wrote:

> On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb  wrote:
> > Implement all git stash functionality as a builtin command
> >
> > Signed-off-by: Joel Teichroeb 
> > ---
> 
> General note on this that I missed in my first E-Mail, you have ~20
> calls to argv_array_init() but none to argv_array_clear(). So you're
> leaking memory, and it obscures potential other issues with valgrind.

I haven't looked carefully at the patches, but note that the argv array
in a child_process is handled automatically by start/finish_command.

So:

> @@ -1107,9 +,9 @@ static int list_stash(int argc, const char
> **argv, const char *prefix)
> argv_array_pushv(, argv);
> argv_array_push(, ref_stash);
> if (cmd_log(args.argc, args.argv, prefix))
> -   return 1;
> -
> -   return 0;
> +   ret = 1;
> +   argv_array_clear();
> +   return ret;
>  }

This one does need a clear, because it's calling an internal function.
But...

> @@ -741,13 +740,18 @@ static int do_push_stash(const char *prefix,
> const char *message,
> argv_array_push(, "--");
> argv_array_pushv(, argv);
> pipe_command(, NULL, 0, , 0, NULL, 0);
> +   argv_array_clear();

This one does not, because the array will have been cleared by calling
pipe_command (because it does the start/finish block itself; and yes,
before you go checking, start_command() clears it even when it returns
error).

> +   child_process_clear();

This should not be necessary for the same reason.

> -   cp2.git_cmd = 1;
> -   argv_array_push(, "checkout-index");
> -   argv_array_push(, "-z");
> -   argv_array_push(, "--force");
> -   argv_array_push(, "--stdin");
> -   pipe_command(, out.buf, out.len, NULL, 0, NULL, 
> 0);
> +   child_process_init();
> +   cp.git_cmd = 1;
> +   argv_array_push(, "checkout-index");
> +   argv_array_push(, "-z");
> +   argv_array_push(, "--force");
> +   argv_array_push(, "--stdin");
> +   pipe_command(, out.buf, out.len, NULL, 0, NULL, 0);
> +   argv_array_clear();
> +   child_process_clear();

And ditto here.

I'd also encourage using CHILD_PROCESS_INIT and ARGV_ARRAY_INIT constant
initializers instead of their function-call counterparts, as that
matches our usual style.

-Peff

Re: [PATCH v3 4/4] stash: implement builtin stash

[Ævar Arnfjörð Bjarmason]

On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb  wrote:
> Implement all git stash functionality as a builtin command
>
> Signed-off-by: Joel Teichroeb 
> ---

General note on this that I missed in my first E-Mail, you have ~20
calls to argv_array_init() but none to argv_array_clear(). So you're
leaking memory, and it obscures potential other issues with valgrind.

A lot of that's easy to solve, but sometimes requires a temporary
variable since the code is now returning directly, e.g:

@@ -1091,6 +1094,7 @@ static int list_stash(int argc, const char
**argv, const char *prefix)
struct object_id obj;
struct object_context unused;
struct argv_array args;
+   int ret = 0;

argc = parse_options(argc, argv, prefix, options,
 git_stash_list_usage, PARSE_OPT_KEEP_UNKNOWN);
@@ -1107,9 +,9 @@ static int list_stash(int argc, const char
**argv, const char *prefix)
argv_array_pushv(, argv);
argv_array_push(, ref_stash);
if (cmd_log(args.argc, args.argv, prefix))
-   return 1;
-
-   return 0;
+   ret = 1;
+   argv_array_clear();
+   return ret;
 }

But more generally this goes a long way to resolving the issue where
you have variables like out1, out2 or cp1, cp2 etc. which Christian
pointed out. I.e. you're not freeing/clearing strbufs either, instead
just creating new ones that also aren't freed, or not clearing
child_process structs, e.g. this on top allows you to re-use the same
variable and stops leaking memory:

diff --git a/builtin/stash.c b/builtin/stash.c
index bf36ff8f9b..4e7344501a 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -729,7 +729,6 @@ static int do_push_stash(const char *prefix, const
char *message,

if (keep_index) {
struct child_process cp = CHILD_PROCESS_INIT;
-   struct child_process cp2 = CHILD_PROCESS_INIT;
struct strbuf out = STRBUF_INIT;

reset_tree(info.i_tree, 0, 1);
@@ -741,13 +740,18 @@ static int do_push_stash(const char *prefix,
const char *message,
argv_array_push(, "--");
argv_array_pushv(, argv);
pipe_command(, NULL, 0, , 0, NULL, 0);
+   argv_array_clear();
+   child_process_clear();

-   cp2.git_cmd = 1;
-   argv_array_push(, "checkout-index");
-   argv_array_push(, "-z");
-   argv_array_push(, "--force");
-   argv_array_push(, "--stdin");
-   pipe_command(, out.buf, out.len, NULL, 0, NULL, 0);
+   child_process_init();
+   cp.git_cmd = 1;
+   argv_array_push(, "checkout-index");
+   argv_array_push(, "-z");
+   argv_array_push(, "--force");
+   argv_array_push(, "--stdin");
+   pipe_command(, out.buf, out.len, NULL, 0, NULL, 0);
+   argv_array_clear();
+   child_process_clear();
}
} else {
struct child_process cp2 = CHILD_PROCESS_INIT;

Re: [PATCH v3 4/4] stash: implement builtin stash

[Joel Teichroeb]

On Sun, May 28, 2017 at 11:26 AM, Ævar Arnfjörð Bjarmason
 wrote:
> On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb  wrote:
>> Implement all git stash functionality as a builtin command
>
> First thanks for working on this, it's great. Applied it locally,
> passes all tests for me. A couple of comments Christian didn't cover
>
>> +   info->has_u = get_sha1_with_context(u_commit_rev.buf, 0, 
>> info->u_commit.hash, ) == 0 &&
>> +   get_sha1_with_context(u_tree_rev.buf, 0, info->u_tree.hash, 
>> ) == 0;
>> +
>> +
>> +   /* TODO: Improve this logic */
>> +   strbuf_addf(, "%s", REV);
>> +   str = strstr(symbolic.buf, "@");
>
> Could you elaborate on how this should be improved?
>

I just figured there would be a builtin function that could help here,
but hadn't had the chance to look into it. It's something easy to do
in bash, but more complicated in C.

>
>> +static int patch_working_tree(struct stash_info *info, const char *prefix,
>> +   const char **argv)
>> +{
>> +   const char *stash_index_path = ".git/foocache2";
>
> This foocache path isn't created by the shell code, if it's a new
> thing that's needed (and I haven't followed this code in detail, don'n
> know what it's for) shouldn't we give it a more descriptive name so
> that if git crashes it's obvious what it is?
>

Opps, I had cleaned that part up locally, but I forgot to push it when
switching computers. It'll be better in the next patch set.

>> +   const char *message = NULL;
>> +   const char *commit = NULL;
>> +   struct object_id obj;
>> +   struct option options[] = {
>> +   OPT_STRING('m', "message", , N_("message"),
>> +N_("stash commit message")),
>> +   OPT__QUIET(, N_("be quiet, only report errors")),
>> +   OPT_END()
>> +   };
>> +   argc = parse_options(argc, argv, prefix, options,
>> +git_stash_store_usage, 0);
>
> Nit: In general in this patch the 2nd line of parse_options doesn't
> align with a tabwidth of 8. Ditto for indenting function arguments
> (e.g. for untracked_files).

I'll fix my tab width. Forgot that long lines would change, haha.

Re: [PATCH v3 4/4] stash: implement builtin stash

[Ævar Arnfjörð Bjarmason]

On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb  wrote:
> Implement all git stash functionality as a builtin command

First thanks for working on this, it's great. Applied it locally,
passes all tests for me. A couple of comments Christian didn't cover

> +   info->has_u = get_sha1_with_context(u_commit_rev.buf, 0, 
> info->u_commit.hash, ) == 0 &&
> +   get_sha1_with_context(u_tree_rev.buf, 0, info->u_tree.hash, 
> ) == 0;
> +
> +
> +   /* TODO: Improve this logic */
> +   strbuf_addf(, "%s", REV);
> +   str = strstr(symbolic.buf, "@");

Could you elaborate on how this should be improved?


> +static int patch_working_tree(struct stash_info *info, const char *prefix,
> +   const char **argv)
> +{
> +   const char *stash_index_path = ".git/foocache2";

This foocache path isn't created by the shell code, if it's a new
thing that's needed (and I haven't followed this code in detail, don'n
know what it's for) shouldn't we give it a more descriptive name so
that if git crashes it's obvious what it is?

> +   const char *message = NULL;
> +   const char *commit = NULL;
> +   struct object_id obj;
> +   struct option options[] = {
> +   OPT_STRING('m', "message", , N_("message"),
> +N_("stash commit message")),
> +   OPT__QUIET(, N_("be quiet, only report errors")),
> +   OPT_END()
> +   };
> +   argc = parse_options(argc, argv, prefix, options,
> +git_stash_store_usage, 0);

Nit: In general in this patch the 2nd line of parse_options doesn't
align with a tabwidth of 8. Ditto for indenting function arguments
(e.g. for untracked_files).

Re: [PATCH v3 4/4] stash: implement builtin stash

[Christian Couder]

On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb  wrote:

[...]

> +int untracked_files(struct strbuf *out, int include_untracked,
> +   const char **argv)
> +{
> +   struct child_process cp = CHILD_PROCESS_INIT;
> +   cp.git_cmd = 1;
> +   argv_array_push(, "ls-files");
> +   argv_array_push(, "-o");
> +   argv_array_push(, "-z");

You might want to use argv_array_pushl(), for example:

   argv_array_pushl(, "ls-files", "-o", "-z", NULL);

> +   if (include_untracked != 2)
> +   argv_array_push(, "--exclude-standard");
> +   argv_array_push(, "--");
> +   if (argv)
> +   argv_array_pushv(, argv);
> +   return pipe_command(, NULL, 0, out, 0, NULL, 0);
> +}
> +
> +static int check_no_changes(const char *prefix, int include_untracked,
> +   const char **argv)
> +{
> +   struct argv_array args1;
> +   struct argv_array args2;
> +   struct strbuf out = STRBUF_INIT;
> +
> +   argv_array_init();
> +   argv_array_push(, "diff-index");
> +   argv_array_push(, "--quiet");
> +   argv_array_push(, "--cached");
> +   argv_array_push(, "HEAD");
> +   argv_array_push(, "--ignore-submodules");
> +   argv_array_push(, "--");

Here and in other places also you could use argv_array_pushl().

> +   if (argv)
> +   argv_array_pushv(, argv);
> +   argv_array_init();
> +   argv_array_push(, "diff-files");
> +   argv_array_push(, "--quiet");
> +   argv_array_push(, "--ignore-submodules");
> +   argv_array_push(, "--");
> +   if (argv)
> +   argv_array_pushv(, argv);
> +   if (include_untracked)
> +   untracked_files(, include_untracked, argv);
> +   return cmd_diff_index(args1.argc, args1.argv, prefix) == 0 &&
> +   cmd_diff_files(args2.argc, args2.argv, prefix) == 0 &&
> +   (!include_untracked || out.len == 0);
> +}
> +
> +static int get_stash_info(struct stash_info *info, const char *commit)
> +{
> +   struct strbuf w_commit_rev = STRBUF_INIT;
> +   struct strbuf b_commit_rev = STRBUF_INIT;
> +   struct strbuf i_commit_rev = STRBUF_INIT;
> +   struct strbuf u_commit_rev = STRBUF_INIT;
> +   struct strbuf w_tree_rev = STRBUF_INIT;
> +   struct strbuf b_tree_rev = STRBUF_INIT;
> +   struct strbuf i_tree_rev = STRBUF_INIT;
> +   struct strbuf u_tree_rev = STRBUF_INIT;
> +   struct strbuf commit_buf = STRBUF_INIT;
> +   struct strbuf symbolic = STRBUF_INIT;
> +   struct strbuf out = STRBUF_INIT;
> +   struct object_context unused;
> +   char *str;
> +   int ret;
> +   const char *REV = commit;

We use lower case variable names.

> +   struct child_process cp = CHILD_PROCESS_INIT;
> +   info->is_stash_ref = 0;
> +
> +   if (commit == NULL) {
> +   strbuf_addf(_buf, "%s@{0}", ref_stash);
> +   REV = commit_buf.buf;
> +   } else if (strlen(commit) < 3) {
> +   strbuf_addf(_buf, "%s@{%s}", ref_stash, commit);
> +   REV = commit_buf.buf;
> +   }
> +   info->REV = REV;

Also the "REV" member of struct stash_info could be lower cased.

> +   strbuf_addf(_commit_rev, "%s", REV);
> +   strbuf_addf(_commit_rev, "%s^1", REV);
> +   strbuf_addf(_commit_rev, "%s^2", REV);
> +   strbuf_addf(_commit_rev, "%s^3", REV);
> +   strbuf_addf(_tree_rev, "%s:", REV);
> +   strbuf_addf(_tree_rev, "%s^1:", REV);
> +   strbuf_addf(_tree_rev, "%s^2:", REV);
> +   strbuf_addf(_tree_rev, "%s^3:", REV);
> +
> +

Spurious new line above.

> +   ret = (
> +   get_sha1_with_context(w_commit_rev.buf, 0, 
> info->w_commit.hash, ) == 0 &&
> +   get_sha1_with_context(b_commit_rev.buf, 0, 
> info->b_commit.hash, ) == 0 &&
> +   get_sha1_with_context(i_commit_rev.buf, 0, 
> info->i_commit.hash, ) == 0 &&
> +   get_sha1_with_context(w_tree_rev.buf, 0, info->w_tree.hash, 
> ) == 0 &&
> +   get_sha1_with_context(b_tree_rev.buf, 0, info->b_tree.hash, 
> ) == 0 &&
> +   get_sha1_with_context(i_tree_rev.buf, 0, info->i_tree.hash, 
> ) == 0);
> +
> +   if (!ret) {
> +   fprintf_ln(stderr, _("%s is not a valid reference"), REV);
> +   return 1;

Maybe use "return error(_("%s is not a valid reference"), REV);"

> +   }
> +
> +

Spurious new lines above.

> +
> +static void stash_create_callback(struct diff_queue_struct *q,
> +   struct diff_options *opt, void *cbdata)
> +{
> +   int i;
> +
> +   for (i = 0; i < q->nr; i++) {
> +   struct diff_filepair *p = q->queue[i];
> +   const char *path = p->one->path;
> +   struct stat st;
> +   remove_file_from_index(_index, path);
> +   if (!lstat(path, ))
> +   add_to_index(_index, path, , 0);
> +