Skip to content

builtin/blame.c: bit field constants into bit shift format #382

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

builtin/blame.c: bit field constants into bit shift format #382

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 12 additions & 12 deletions builtin/blame.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,18 +319,18 @@ static const char *format_time(timestamp_t time, const char *tz_str,
return time_buf.buf;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Pratyush Yadav wrote (reply to this):

On 16/10/19 06:30PM, Hariom Verma via GitGitGadget wrote:
> From: Hariom Verma <[email protected]>
> 
> We are looking at bitfield constants, and elsewhere in the Git source
> code, such cases are handled via bit shift operators rather than octal
> numbers, which also makes it easier to spot holes in the range
> (if, say, 1<<5 was missing, it is easier to spot it between 1<<4
> and 1<<6 than it is to spot a missing 040 between a 020 and a 0100).
> 
> Signed-off-by: Hariom Verma <[email protected]>
> ---
>  builtin/blame.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/builtin/blame.c b/builtin/blame.c
> index e946ba6cd9..a57020acf9 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -319,18 +319,18 @@ static const char *format_time(timestamp_t time, const char *tz_str,
>  	return time_buf.buf;
>  }
>  
> -#define OUTPUT_ANNOTATE_COMPAT	001
> -#define OUTPUT_LONG_OBJECT_NAME	002
> -#define OUTPUT_RAW_TIMESTAMP	004
> -#define OUTPUT_PORCELAIN	010
> -#define OUTPUT_SHOW_NAME	020
> -#define OUTPUT_SHOW_NUMBER	040
> -#define OUTPUT_SHOW_SCORE	0100
> -#define OUTPUT_NO_AUTHOR	0200
> -#define OUTPUT_SHOW_EMAIL	0400
> -#define OUTPUT_LINE_PORCELAIN	01000
> -#define OUTPUT_COLOR_LINE	02000
> -#define OUTPUT_SHOW_AGE_WITH_COLOR	04000
> +#define OUTPUT_ANNOTATE_COMPAT      (1<<0)
> +#define OUTPUT_LONG_OBJECT_NAME     (1<<1)
> +#define OUTPUT_RAW_TIMESTAMP        (1<<2)
> +#define OUTPUT_PORCELAIN            (1<<3)
> +#define OUTPUT_SHOW_NAME            (1<<4)
> +#define OUTPUT_SHOW_NUMBER          (1<<5)
> +#define OUTPUT_SHOW_SCORE           (1<<6)
> +#define OUTPUT_NO_AUTHOR            (1<<7)
> +#define OUTPUT_SHOW_EMAIL           (1<<8)
> +#define OUTPUT_LINE_PORCELAIN       (1<<9)
> +#define OUTPUT_COLOR_LINE           (1<<10)
> +#define OUTPUT_SHOW_AGE_WITH_COLOR  (1<<11)

Nitpick: In the code you remove, tabs were used for alignment. Here, you 
use spaces. Unless there is any specific reason to do it this way, might 
as well keep the older style.

There was some discussion recently about converting these related 
#defines to enums [0]. We might consider doing that here.

If you read through that entire thread, you'd see that there were some 
disagreements about whether using enums for sets of bits is a good idea 
([1] and [2]), but it is at least something worth considering while we 
are on this topic.

FWIW, I think it is a good idea to use an enum here.

>  
>  static void emit_porcelain_details(struct blame_origin *suspect, int repeat)
>  {

[0] https://public-inbox.org/git/[email protected]/
[1] https://public-inbox.org/git/[email protected]/
[2] https://public-inbox.org/git/[email protected]/

-- 
Regards,
Pratyush Yadav

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Jonathan Tan wrote (reply to this):

> There was some discussion recently about converting these related 
> #defines to enums [0]. We might consider doing that here.
> 
> If you read through that entire thread, you'd see that there were some 
> disagreements about whether using enums for sets of bits is a good idea 
> ([1] and [2]), but it is at least something worth considering while we 
> are on this topic.
> 
> FWIW, I think it is a good idea to use an enum here.

[snip]

> [0] https://public-inbox.org/git/[email protected]/
> [1] https://public-inbox.org/git/[email protected]/
> [2] https://public-inbox.org/git/[email protected]/

Thanks for the handy references. You know my opinion on bitflags as
enums from reading them, but I think that we have already had that
discussion and came to a conclusion. So don't use an enum here.

The patch itself looks good, and I also prefer the bit shift format over
octal.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Pratyush Yadav wrote (reply to this):

On 16/10/19 12:37PM, Jonathan Tan wrote:
> > There was some discussion recently about converting these related 
> > #defines to enums [0]. We might consider doing that here.
> > 
> > If you read through that entire thread, you'd see that there were some 
> > disagreements about whether using enums for sets of bits is a good idea 
> > ([1] and [2]), but it is at least something worth considering while we 
> > are on this topic.
> > 
> > FWIW, I think it is a good idea to use an enum here.
> 
> [snip]
> 
> > [0] https://public-inbox.org/git/[email protected]/
> > [1] https://public-inbox.org/git/[email protected]/
> > [2] https://public-inbox.org/git/[email protected]/
> 
> Thanks for the handy references. You know my opinion on bitflags as
> enums from reading them, but I think that we have already had that
> discussion and came to a conclusion. So don't use an enum here.

Ah! I missed your last email in that thread that finally settled on 
avoiding bitsets, and thought the discussion was still ongoing. My bad 
:)
 
> The patch itself looks good, and I also prefer the bit shift format over
> octal.

-- 
Regards,
Pratyush Yadav

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Hariom Verma via GitGitGadget" <[email protected]> writes:

> -#define OUTPUT_SHOW_AGE_WITH_COLOR	04000
> +#define OUTPUT_ANNOTATE_COMPAT      (1<<0)
> +#define OUTPUT_LONG_OBJECT_NAME     (1<<1)
> +#define OUTPUT_RAW_TIMESTAMP        (1<<2)
> +#define OUTPUT_PORCELAIN            (1<<3)
> +#define OUTPUT_SHOW_NAME            (1<<4)
> +#define OUTPUT_SHOW_NUMBER          (1<<5)
> +#define OUTPUT_SHOW_SCORE           (1<<6)
> +#define OUTPUT_NO_AUTHOR            (1<<7)
> +#define OUTPUT_SHOW_EMAIL           (1<<8)
> +#define OUTPUT_LINE_PORCELAIN       (1<<9)
> +#define OUTPUT_COLOR_LINE           (1<<10)
> +#define OUTPUT_SHOW_AGE_WITH_COLOR  (1<<11)

For these small shift counts it probably would not matter, but it
may be a good discipline to make sure they are treated as constants
of an unsigned type (i.e. write them as (1U<<0) etc.).  It probably
starts to matter when you reach 1<<31 if these are bits stuffed into
"unsigned int" on 32-bit arch.

One advantage of octal and hexadecimal notations have is that
0x80000000 is automatically unsigned, IIRC, on such an archtecture.

}

#define OUTPUT_ANNOTATE_COMPAT 001
#define OUTPUT_LONG_OBJECT_NAME 002
#define OUTPUT_RAW_TIMESTAMP 004
#define OUTPUT_PORCELAIN 010
#define OUTPUT_SHOW_NAME 020
#define OUTPUT_SHOW_NUMBER 040
#define OUTPUT_SHOW_SCORE 0100
#define OUTPUT_NO_AUTHOR 0200
#define OUTPUT_SHOW_EMAIL 0400
#define OUTPUT_LINE_PORCELAIN 01000
#define OUTPUT_COLOR_LINE 02000
#define OUTPUT_SHOW_AGE_WITH_COLOR 04000
#define OUTPUT_ANNOTATE_COMPAT (1U<<0)
#define OUTPUT_LONG_OBJECT_NAME (1U<<1)
#define OUTPUT_RAW_TIMESTAMP (1U<<2)
#define OUTPUT_PORCELAIN (1U<<3)
#define OUTPUT_SHOW_NAME (1U<<4)
#define OUTPUT_SHOW_NUMBER (1U<<5)
#define OUTPUT_SHOW_SCORE (1U<<6)
#define OUTPUT_NO_AUTHOR (1U<<7)
#define OUTPUT_SHOW_EMAIL (1U<<8)
#define OUTPUT_LINE_PORCELAIN (1U<<9)
#define OUTPUT_COLOR_LINE (1U<<10)
#define OUTPUT_SHOW_AGE_WITH_COLOR (1U<<11)

static void emit_porcelain_details(struct blame_origin *suspect, int repeat)
{
Expand Down