Re: [PATCH 27/44] builtin/show-index: provide options to determine hash algo

["brian m. carlson" <sandals@crustytoothpaste.net>]

[-- Attachment #1: Type: text/plain, Size: 3344 bytes --]

On 2020-05-18 at 16:20:22, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > It's possible to use a variety of index formats with show-index, and we
> > need a way to indicate the hash algorithm which is in use for a
> > particular index we'd like to show.  Default to using the value for the
> > repository we're in by calling setup_git_directory_gently, and allow
> > overriding it by using a --hash argument.
> 
> I think you meant to say that "show-index" does not autodetect what
> hash algorithm is used from its input, and the new argument is a way
> for the user to help the command when the hash algorithm is
> different from what is used in the current repository?

Correct.

> I ask because I found that your version can be read to say that
> "show-index" can show the contents of a given pack index using any
> hash algorithm we support, and the user can specify --hash=SHA-256
> when running the command on a pack .idx that uses SHA-1 object names
> to auto-convert it, and readers wouldn't be able to guess which was
> meant with only the above five lines.

No, that's definitely not what I meant.  I'll adjust the commit message
to make this clearer.

> Do we say --hash=SHA-1 etc. or --hash-algo=SHA-256 in other places?
> Would the word "hash" alone clear enough that it does not refer to
> a specific "hash" value but the name of an algorithm?
> 
> The generating side seems to use "index-pack --object-format=<algo>"
> and the transport seems to use a capability "object-format=<algo>",
> neither of which is directly visible to the end users, but I think
> they follow "git init --object-format=<algo>", so we are consistent
> there.
> 
> Perhaps we should follow suit here, too?

Yeah, as I mentioned to Martin elsewhere in the thread, I'm going to
make this consistent and use --object-formta.

> > diff --git a/git.c b/git.c
> > index 2e4efb4ff0..e53e8159a2 100644
> > --- a/git.c
> > +++ b/git.c
> > @@ -573,7 +573,7 @@ static struct cmd_struct commands[] = {
> >  	{ "shortlog", cmd_shortlog, RUN_SETUP_GENTLY | USE_PAGER },
> >  	{ "show", cmd_show, RUN_SETUP },
> >  	{ "show-branch", cmd_show_branch, RUN_SETUP },
> > -	{ "show-index", cmd_show_index },
> > +	{ "show-index", cmd_show_index, RUN_SETUP_GENTLY },
> 
> Hmph, this is not necessary to support peeking an .idx file in
> another repository that uses a different hash algorithm than ours
> (we do need the --hash=<algo> override to tell that the algo is
> different from what we read from our repository settings).  Is this
> absolutely necessary?
> 
> Ah, I am misreading the patch.  We didn't even do setup but we now
> optionally do, in order to see if we are in a repository and what
> object format it uses to give the default value to --hash=<algo>
> when the argument is not given.  The need for RUN_SETUP_GENTLY
> is understandable.

Yes, this is designed to make us do the right thing when we're in a
repository (e.g., with --stdin) by autodetecting the algorithm in use
but not fail when we're outside of a repository.  I'll update the commit
message to make this a lot clearer, since I clearly omitted a lot of
things that were in my head when writing this.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]