Re: [PATCH 27/44] builtin/show-index: provide options to determine hash algo
- From
brian m. carlson <sandals@crustytoothpaste.net>
- Date
- May 19, 2020, 00:31 UTC
- Message-ID
- <20200519003100.GN6362@camp.crustytoothpaste.net>
- In-Reply-To
- <xmqqimgt5i61.fsf@gitster.c.googlers.com>
On 2020-05-18 at 16:20:22, Junio C Hamano wrote:
Show 12 quoted lines
> "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.
Show 6 quoted lines
> 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.
Show 11 quoted lines
> 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.
Show 22 quoted lines
> > 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