Re:* [Resend PATCH] Fix segmentation fault when user doesn't have access permission to the repository.

[Junio C Hamano <gitster@pobox.com>]

"André Goddard Rosa" <andre.goddard@gmail.com> writes:

> On Nov 22, 2007 2:09 PM, Alex Riesen <raa.lkml@gmail.com> wrote:
> ...
> I tested it here before posting but luckly (or not, as I didn't catch
> this when compiling) it worked,
> as a pointer have the sizeof(int) in my x86 platform. >:|

Most of the changes look trivially correct.  Thanks.

> diff --git a/builtin-fetch.c b/builtin-fetch.c
> index be9e3ea..84c8ed4 100644
> --- a/builtin-fetch.c
> +++ b/builtin-fetch.c
> @@ -263,8 +263,13 @@ static void store_updated_refs(const char *url,
> struct ref *ref_map)
>         char note[1024];
>         const char *what, *kind;
>         struct ref *rm;
> +       char *filename = git_path("FETCH_HEAD");
>
> -       fp = fopen(git_path("FETCH_HEAD"), "a");
> +       fp = fopen(filename, "a");
> +       if (!fp) {
> +               error("cannot open %s: %s\n", filename, strerror(errno));
> +               return 1;
> +       }
>         for (rm = ref_map; rm; rm = rm->next) {
>                 struct ref *ref = NULL;

I started to wonder if there was a particular reason you chose to return
1, not -1 (or just say 'return error("cannot open...", ...)')?

> @@ -404,7 +410,7 @@ static int fetch_refs(struct transport *transport,
> struct ref *ref_map)
>         if (ret)
>                 ret = transport_fetch_refs(transport, ref_map);
>         if (!ret)
> -               store_updated_refs(transport->url, ref_map);
> +               ret |= store_updated_refs(transport->url, ref_map);
>         transport_unlock_pack(transport);
>         return ret;
>  }

I think the callers of fetch_refs() are interested in seeing only 0 or
non-zero, so it seems more consistent to signal error with negative
return.  I modified the hunk that starts at line 263 to return the
return value from error() and applied.

That made me follow the transport code, fetch_refs_via_pack().  This is
not your code, so Shawn and Daniel are CC'ed.

The code calls fetch_pack() to get the list of refs it fetched, and
discards refs and always returns 0 to signal success.

But builtin-fetch-pack.c::fetch_pack() has error cases.  The function
returns NULL if error is detected (shallow-support side seems to choose
to die but I suspect that is easily fixable to error out as well).

Shouldn't fetch_refs_via_pack() propagate that error to the caller?

---
 transport.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/transport.c b/transport.c
index 50db980..048df1f 100644
--- a/transport.c
+++ b/transport.c
@@ -655,7 +655,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 	free(heads);
 	free_refs(refs);
 	free(dest);
-	return 0;
+	return (refs ? 0 : -1);
 }
 
 static int git_transport_push(struct transport *transport, int refspec_nr, const char **refspec, int flags)