Re: [PATCH] index-pack: teach --promisor to require --stdin
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 20, 2024, 01:34 UTC
- Message-ID
- <xmqqcyiqadtf.fsf@gitster.g>
- In-Reply-To
- <20241119185345.GB15723@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 22 quoted lines
> But I think that makes the --stdin check redundant. I.e., here:
>
>> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
>> index 08b340552f..c46b6e4061 100644
>> --- a/builtin/index-pack.c
>> +++ b/builtin/index-pack.c
>> @@ -1970,6 +1970,10 @@ int cmd_index_pack(int argc,
>> usage(index_pack_usage);
>> if (fix_thin_pack && !from_stdin)
>> die(_("the option '%s' requires '%s'"), "--fix-thin", "--stdin");
>> + if (promisor_msg && !from_stdin)
>> + die(_("the option '%s' requires '%s'"), "--promisor", "--stdin");
>> + if (promisor_msg && pack_name)
>> + die(_("--promisor cannot be used with a pack name"));
>
> ...just the second one would be sufficient, because the context just
> above this has:
>
> if (!pack_name && !from_stdin)
> usage(index_pack_usage);
>
> So if there isn't a pack name then from_stdin must be set anyway.Nice findings that leads to ...
Show 15 quoted lines
> What you've written won't behave incorrectly, but I wonder if this means > we can explain the rule in a more simple way: > > - the --promisor option requires that we be indexing a pack in the > object database > > - when not given a pack name on the command line, we know this is true > (because we generate the name ourselves internally) > > - when given a pack name on the command line, we _could_ check that it > is inside the object directory, but we don't currently do so and > just bail. That could be changed in the future. > > And then there is no mention of --stdin at all (though of course it is > an implication of the second point, since we have to get input somehow).
... a good simplification. Not of the implementation---as it is already simple enough---but of the concept, and simplification of the latter counts a lot more ;-)
Thanks, both, for working on this.