Re: [PATCH v2 4/5] diff.c: initialize hash algo when running in --no-index mode

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Feb 24, 2018, 22:29 UTC
Message-ID: <20180224222900.GA4620@genre.crustytoothpaste.net>
In-Reply-To: <CACsJy8AUXf4uA-zymrhWUagW84bUzfB6JBQLyaHDJtudTcNy=A@mail.gmail.com>

On Sat, Feb 24, 2018 at 09:36:03PM +0700, Duy Nguyen wrote:

Show 25 quoted lines

> On Sat, Feb 24, 2018 at 10:34 AM, Nguyễn Thái Ngọc Duy
> <pclouds@gmail.com> wrote:
> > @@ -3995,6 +3995,18 @@ static void run_diff(struct diff_filepair *p, struct diff_options *o)
> >                 return;
> >         }
> >
> > +       /*
> > +        * NEEDSWORK: When running in no-index mode (and no repo is
> > +        * found, thus no hash algo conifugred), fall back to SHA-1
> > +        * hashing (which is used by diff_fill_oid_info below) to
> > +        * avoid regression in diff output.
> > +        *
> > +        * In future, perhaps we can allow the user to specify their
> > +        * hash algorithm from command line in this mode.
> > +        */
> > +       if (o->flags.no_index && !the_hash_algo)
> > +               the_hash_algo = &hash_algos[GIT_HASH_SHA1];
> 
> Brian, are we supposed to use the_hash_algo this way (i.e. as a
> writable var)? Or should I stick to something like
> 
>     repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
> 
> which allows us to notify other parts inside struct repository about
> the hash algorithm change, if we ever need to?

I would definitely recommend using the function. As you pointed out, it makes our code future-proof against needing to more work when setting the value.

> If the_hash_algo is supposed to be read-only, maybe I should convert
> that macro to an inline function to prevent people from accidentally
> reassigning it?

You could if you want to, although I don't really see a need to, since we can just catch it in review. If you wanted to, I'd make it an inline function for performance reasons.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204