From: Junio C Hamano <gitster@pobox•com>
To: Robert Stanca <robert.stanca7@gmail•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH 1/2] [GSOC] Convert signed flags to unsigned
Date: Sat, 01 Apr 2017 12:12:55 -0700 [thread overview]
Message-ID: <xmqqy3vkt008.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20170401153049.21400-1-robert.stanca7@gmail.com> (Robert Stanca's message of "Sat, 1 Apr 2017 18:30:48 +0300")
Robert Stanca <robert.stanca7@gmail•com> writes:
> Subject: Re: [PATCH 1/2] [GSOC] Convert signed flags to unsigned
Try
git shortlog --no-merges -40
to learn how commits are typically titled. And then imagine this
patch makes into our codebase and appear in the output.
Can you tell what this commit is about from that single line title?
No. You wouldn't even know that it is only touching bisect.h and
nothing else.
What do your readers want "shortlog" output to tell them about your
commit? What are the most important thing (other than giving you an
excuse to say "I have completed a microproject and now I am eligible
to apply to GSoC" ;-)? Your proposed commit log message, especially
its title, must be written to help future readers of the project
history.
Perhaps
bisect.h: make flags field in rev_list_info unsigned
would help them better.
> Unsigned int is a closer representation of bitflags rather than signed int that uses 1 special bit for sign.This shouldn't make much difference because rev_list_info.flags uses only 2 bits(BISECT_SHOW_ALL and REV_LIST_QUIET)
Overlong lines, without space after full-stop before the second
sentence, without full-stop at the end of the sentence.
>
> Signed-off-by: Robert Stanca <robert.stanca7@gmail•com>
> ---
> bisect.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/bisect.h b/bisect.h
> index acd12ef80..a979a7f11 100644
> --- a/bisect.h
> +++ b/bisect.h
> @@ -16,7 +16,7 @@ extern struct commit_list *filter_skipped(struct commit_list *list,
>
> struct rev_list_info {
> struct rev_info *revs;
> - int flags;
> + unsigned int flags;
Have you checked how this field is used? For example, there is this
line somewhere in rev-list.c
int cnt, flags = info->flags;
and the reason why the code copies the value to a local variable
"flags" must be because it is going to use it, just like it and
other codepaths use info->flags, no? It makes the code inconsistent
by leaving the local variable a signed int, I suspect.
next prev parent reply other threads:[~2017-04-01 19:13 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-01 15:30 [PATCH 1/2] [GSOC] Convert signed flags to unsigned Robert Stanca
2017-04-01 15:30 ` [PATCH 2/2] [GSOC] show_bisect_vars(): Use unsigned int instead of signed int for flags Robert Stanca
2017-04-01 19:13 ` Junio C Hamano
2017-04-01 19:19 ` Robert Stanca
2017-04-02 3:30 ` Junio C Hamano
2017-04-02 13:18 ` Robert Stanca
2017-04-02 17:14 ` Junio C Hamano
2017-04-01 19:12 ` Junio C Hamano [this message]
2017-04-01 19:30 ` [PATCH 1/2] [GSOC] Convert signed flags to unsigned Robert Stanca
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqy3vkt008.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=robert.stanca7@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox