Re: [PATCH v2 1/4] sha1_name: add get_sha1_with_context()

From: Jeff King <peff@peff.net>
Date: Jun 9, 2010, 06:13 UTC
Message-ID: <20100609061337.GA14007@coredump.intra.peff.net>
In-Reply-To: <AANLkTinI_ghLE5U3tQ0JFmvuU8DySLFtdl03sv0uW-Ab@mail.gmail.com>

On Wed, Jun 09, 2010 at 12:30:31AM +0200, Clément Poulain wrote:

Show 14 quoted lines

> Le 8 juin 2010 19:57, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> a écrit :
> > This patch produces uncompilable code for me:
> >
> > cc1: warnings being treated as errors
> > In file included from builtin.h:6,
> >                 from fast-import.c:147:
> > cache.h: In function ‘get_sha1_with_context’:
> > cache.h:748: error: implicit declaration of function ‘get_sha1_with_context_1’
> >
> > Forgot to add get_sha1_with_context_1 to cache.h?
> 
> Uh, we compiled it almost ten times on both our pc and ensibm (our
> school server), whithout any problems. Seems that we need to check our
> compilation configurations.

Note the "warnings being treated as errors". Matthieu is compiling with -Werror (and presumably -Wall). We strive to be warning-free in git, and I think many of the developers compile with "-Wall -Werror".

> Right. But the aim was to skip one function call (see the call-stack below)
> _with_mode => _with_mode_1 => _with_context_1
> whereas:
>  _with_context => _with_context_1

Perhaps that was your goal, but my goal when I suggested it was to give us a cleaner codebase. We don't want a proliferation of get_sha1_with_* functions. Introducing _with_context instead of _with_tree or _with_path was meant not to make things worse. But collapsing _with_mode into _with_context actively makes things better.

Show 5 quoted lines

> >> +                     orc->path[sizeof(orc->path)] = '\0';
> >
> > Same here.
> 
> That's true. Stupid error, we copied this line without checking it.

Oops, that's my fault for introducing the bug in the first place (I had originally had an snprintf and changed it to strncpy at the last minute). :)

-Peff