On 2020-05-18 at 16:20:22, Junio C Hamano wrote:
> "brian m. carlson" <sandals@xxxxxxxxxxxxxxxxxxxx> 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:
signature.asc
Description: PGP signature
