From: Danh Doan <congdanhqx@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 1/1] notes: copy notes to HEAD by default
Date: Wed, 16 Oct 2019 12:17:18 +0700 [thread overview]
Message-ID: <20191016051718.GA2034@danh.dev> (raw)
In-Reply-To: <xmqqpnixa1oh.fsf@gitster-ct.c.googlers.com>
On 2019-10-16 11:01:34 +0900, Junio C Hamano wrote:
> Doan Tran Cong Danh <congdanhqx@gmail.com> writes:
>
> > The target objects for copying notes was defaulted to HEAD from very
> > early stage of git-notes.
> >
> > However, that default was limited by commit bbb1b8a35a, ("notes: check
> > number of parameters to "git notes copy"", 2010-06-28).
>
> Sorry, I don't quite get the above. The said commit made sure 'git
> notes copy' gets the right number of arguments, saying """Otherwise
> we may segfault with too few parameters.""" I take that as a sign
> that before that commit it was not defaulting to HEAD but attempting
> to access the missing argv[2] (or whatever the index the <to-object>
> should be at) and dereferencing a NULL?
>
> ... goes and digs ...
>
> I think v1.6.6.1-458-g74884b524e is the commit that made the command
> line parsing into the current shape, i.e. one parse_options() call
> in each of the subcommand that gets dispatched, and you are right
> that with that version a single argument given on the command line
> is taken as the <from-object> and <to-object> defaults to HEAD.
>
> So... what happend between that vesrion and v1.7.1-200-gbbb1b8a35a?
>
> ... goes and looks at bbb1b8a35a again ...
>
> Ah, I think there is an off-by-one. When not from-stdin and not
> using rewrite-cmd, before that patch, we did not even check if
> from-obj exists, so in that sense, the commit had a right idea that
> it must check for "too few parameters", but it shouldn't have
> insisted that we have at least two. It is OK to have just one,
> i.e. only the from-obj, for our purpose.
Yes, this is my intention.
> > diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> > index d3fa298c6a..a8f9a0f36c 100755
> > --- a/t/t3301-notes.sh
> > +++ b/t/t3301-notes.sh
> > @@ -908,6 +908,10 @@ test_expect_success 'allow overwrite with "git notes copy -f"' '
> > git notes copy -f HEAD~2 HEAD &&
> > git log -1 >actual &&
> > test_cmp expect actual &&
> > + test "$(git notes list HEAD)" = "$(git notes list HEAD~2)" &&
> > + git notes copy -f HEAD~2 &&
> > + git log -1 >actual &&
> > + test_cmp expect actual &&
> > test "$(git notes list HEAD)" = "$(git notes list HEAD~2)"
> > '
>
> This I am not sure is a good test to add to, especially as a fix to
I was writing this patch just before my bed time, just to get some
comments on the directions, e.g:
- loosen the argc requirements; or
- do the code cleanup
> bbb1b8a, which added this test:
>
> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> index 64f32ad94d..2d67a40fc1 100755
> --- a/t/t3301-notes.sh
> +++ b/t/t3301-notes.sh
> @@ -1044,4 +1044,10 @@ test_expect_success 'GIT_NOTES_REWRITE_REF overrides config' '
> git log -1 > output &&
> test_cmp expect output
> '
> +
> +test_expect_success 'git notes copy diagnoses too many or too few parameters' '
> + test_must_fail git notes copy &&
> + test_must_fail git notes copy one two three
> +'
> +
> test_done
>
> The lack of testing that "git notes copy <from-obj>" succeeding is
> why the off-by-one bug was not noticed, so I think that test (which
> still exists to this day) is the right place to add a test to
> protect this fix.
I don't think this is a good place to add this test either,
since the test description specificaly said it diagnoses too many or
too few parameters.
Anyway, the test `git notes copy one two three` still fails if we
accidentally allow 3 arguments since one two three isn't valid ref.
I'm gonna add a statement to assert the diagnose message.
Since I don't want to update the commit id (e.g. 10th, 11th, etc..)
of other test cases, I think it'd be better to modify the current test:
- for the test case without '-f' flag, remove HEAD if it's the target,
and add a test-case to copy to somewhere else. Well, all of our current
test cases only test with HEAD as target-object.
- for the test case with '-f' flag, I think I'll keep my current
approach.
>
> As to the log message, here is how I would explain/justify the
> change, if I were writing it.
I'll update the commit message.
--
Danh
next prev parent reply other threads:[~2019-10-16 5:17 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-15 16:36 [PATCH 0/1] Allow default value for target of git-notes copy Doan Tran Cong Danh
2019-10-15 16:36 ` [PATCH 1/1] notes: copy notes to HEAD by default Doan Tran Cong Danh
2019-10-16 2:01 ` Junio C Hamano
2019-10-16 5:17 ` Danh Doan [this message]
2019-10-16 5:18 ` [PATCH 1/2] t3301: test diagnose messages for too few/many paramters Doan Tran Cong Danh
2019-10-16 5:18 ` [PATCH 2/2] notes: fix minimum number of parameters to "copy" subcommand Doan Tran Cong Danh
2019-10-18 0:44 ` Junio C Hamano
2019-10-18 0:38 ` [PATCH 1/2] t3301: test diagnose messages for too few/many paramters Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191016051718.GA2034@danh.dev \
--to=congdanhqx@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).