Re: [PATCH v4 1/2] bundle: lost objects when removing duplicate pendings

From: Junio C Hamano <gitster@pobox.com>
To: Jiang Xin <worldhello.net@gmail.com>
Cc: "Git List" <git@vger.kernel.org>,
	"Đoàn Trần Công Danh" <congdanhqx@gmail.com>,
	"Jiang Xin" <zhiyou.jx@alibaba-inc.com>
Subject: Re: [PATCH v4 1/2] bundle: lost objects when removing duplicate pendings
Date: Sat, 09 Jan 2021 14:02:09 -0800	[thread overview]
Message-ID: <xmqqbldxg466.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <CANYiYbGcXORT-kryEngy17_J1g3FDKbty9wVXj1U5OWHu8yM8g@mail.gmail.com> (Jiang Xin's message of "Sat, 9 Jan 2021 21:32:18 +0800")

Jiang Xin <worldhello.net@gmail.com> writes:

>> > +# Create a commit or tag and set the variable with the object ID.
>> > +test_commit_setvar () {
>> > +     notick= &&
>> > +     signoff= &&
>> > +     indir= &&
>> > +     merge= &&
>> > +     tag= &&
>> > +     var= &&
>> > +     while test $# != 0
>> > +     do
>> > +             case "$1" in
>> > ...
>> > +             -*)
>> > +                     echo >&2 "error: unknown option $1"
>> > +                     return 1
>> > +                     ;;
>> > +             *)
>> > +                     test -n "$var" && break
>> > +                     var=$1
>
> The loop ends only if $var has been assigned a value, or no other
> args.  Will report error if no other args later.

"We see an arg that is not an dashed option.  We already saw an arg
and taken it as the name of the variable, so break" was a misleading
way to structure this loop.

It looked as if it was just refusing to parse the remainder of the
command line, so that a check after the loop would complain if there
is still remaining arguments (as if to warn "var given twice").  But
that is not what the post-loop check does; it expects there still
are some argument left to be processed in mode-specific code that
follows, so it happens to work as intended.

That is brittle, though.  The current code may always consume one or
more extra arguments in $merge/$tag/other specific mode in the code
after the loop, but a new mode that will get added in the future to
sit next to --merge and --tag may learn all necessary info in the
command line parsing loop above, without any need for extra args to
be processed after the loop.  And $#=0 may not always be an error at
that point.

I forgot to notice / mention it, but now you made me to look at the
loop again, I see this part

	-C)
		indir="$2"
		shift
		;;

does not ensure we are getting something sensible in -C; that
potential bug by the caller also happens to be covered by the
post-loop "we require at least one argument that we can use as an
arg" check, but as I said already, that feels rather brittle.

Anyway, let's queue the patches as-is and see what others think.

Thanks.