git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: William Baker <williamtbakeremail@gmail.com>
To: "SZEDER Gábor" <szeder.dev@gmail.com>,
	"Junio C Hamano" <gitster@pobox.com>
Cc: William Baker via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, stolee@gmail.com, jeffhost@microsoft.com,
	William Baker <William.Baker@microsoft.com>
Subject: Re: [PATCH v2 1/6] midx: add MIDX_PROGRESS flag <snip>
Date: Tue, 15 Oct 2019 13:00:45 -0700	[thread overview]
Message-ID: <04342d12-fffc-afb6-fa4e-c2e2bf88d1b6@gmail.com> (raw)
In-Reply-To: <20191009013231.GF29845@szeder.dev>

On 10/8/19 6:32 PM, SZEDER Gábor wrote:
>>> No, they tend to show (PROGRESS | REGRESS), at least both gdb and lldb
>>> do.
> 
> I was wrong here, gdb does this, but lldb, unfortunately, doesn't; see
> my other reply in this thread.
>
>> What I was worried about is that the constants that are used to
>> represent something that are *NOT* set of bits (hence "PROGRESS * 3"
>> may be perfectly a reasonable thing for such an application)
> 
> I don't really see how that could be reasonable, it's prone to break
> when changing the values of the enum constants.
> 
>> may be
>> mistaken by an overly clever debugger and "3" may end up getting
>> shown as "PROGRESS | REGRESS".  When there are only two constants
>> (PROGRESS=1 and REGRESS=2), we humans nor debuggers can tell if that
>> is to represent two bits that can be or'ed together, or it is an
>> enumeration.
>>
>> Until we gain the third constant, that is, at which time the third
>> one may likely be 3 (if enumeration) or 4 (if bits).
> 
> Humans benefit from context: they understand the name of the enum type
> (e.g. does it end with "_flags"?), the name of the enum constants, and
> the comment above the enum's definition (if any), and can then infer
> whether those constants represent OR-able bits or not.  If they can't
> find this out, then that enum is poorly named and/or documented, which
> should be fixed.  As for the patch that I originally commented on, I
> would expect the enum to be called e.g. 'midx_flags', and thus already
> with that single constant in there it'll be clear that it is intended
> as a collection of related OR-able bits.
> 
> As for the debugger, if it sees a variable of an enum type whose value
> doesn't match any of the enum constants, then there are basically
> three possibilities:
> 
>   - All constants in that enum have power-of-two values.  In this case
>     it's reasonable from the debugger to assume that those constants
>     are OR'ed together, and is extremely helpful to display the value
>     that way.
> 
>   - The constants are just a set of values (1, 2, 3, 42, etc).  In
>     this case the variable shouldn't have a value that doesn't match
>     one of the constants in the first place, and I would first suspect
>     that the program might be buggy.
> 
>   - A "mostly" power-of-two enum might contain shorthand constants for
>     combinations of a set of other constants, e.g.:
> 
>       enum flags {
>               BIT0 = (1 << 0),
>               BIT1 = (1 << 1),
>               BIT2 = (1 << 2),
> 
>               FIRST_TWO = (BIT0 | BIT1),
>       };
>       enum flags f0 = BIT0;
>       enum flags f1 = BIT0 | BIT1;
>       enum flags f2 = BIT0 | BIT2;
>       enum flags f3 = BIT0 | BIT1 | BIT2;
> 
>     In this case, sadly, gdb shows only matching constants:
> 
>       (gdb) p f0
>       $1 = BIT0
>       (gdb) p f1
>       $2 = FIRST_TWO
>       (gdb) p f2
>       $3 = 5
>       (gdb) p f3
>       $4 = 7
> 

I believe this is the last open thread/discussion on the "midx: add MIDX_PROGRESS
flag" patch series.  

A quick summary of where the discussion stands:

- The patch series currently uses #define for the new MIDX_PROGRESS flag.
- The git codebase uses both #defines and enums for flags.
- The debugger always shows the enum value's name if there is a match, if values
  are OR-ed together there a few possibilities (see discussion above and earlier
  in the thread).
- There are concerns regarding misinterpreting constants that are not a set of
  bits (e.g. "PROGRESS * 3").

Are there any other details I can provide that would help in reaching a
conclusion as to how to proceed?

At this time there are no other MIDX_* flags and so there's always the option
to revisit the best way to handle multiple MIDX_* flags if/when additional
flags are added.

Thanks,
William


  reply	other threads:[~2019-10-15 20:00 UTC|newest]

Thread overview: 58+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-11 15:37 [PATCH 0/1] multi-pack-index: add --no-progress William Baker via GitGitGadget
2019-09-11 15:37 ` [PATCH 1/1] multi-pack-index: add --no-progress Add --no-progress option to git multi-pack-index. The progress feature was added in 144d703 ("multi-pack-index: report progress during 'verify'", 2018-09-13) but the ability to opt-out was overlooked William Baker via GitGitGadget
2019-09-12 20:17   ` Junio C Hamano
2019-09-13 18:45     ` William Baker
2019-09-13 20:26       ` Junio C Hamano
2019-09-16 19:49         ` William Baker
2019-09-11 20:30 ` [PATCH 0/1] multi-pack-index: add --no-progress Derrick Stolee
2019-09-20 16:53 ` [PATCH v2 0/6] " William Baker via GitGitGadget
2019-09-20 16:53   ` [PATCH v2 1/6] midx: add MIDX_PROGRESS flag Add the MIDX_PROGRESS flag and update the write|verify|expire|repack functions in midx.h to accept a flags parameter. The MIDX_PROGRESS flag indicates whether the caller of the function would like progress information to be displayed. This patch only changes the method prototypes and does not change the functionality. The functionality change will be handled by a later patch William Baker via GitGitGadget
2019-09-20 20:01     ` Junio C Hamano
2019-09-20 20:16       ` Junio C Hamano
2019-09-21 12:11     ` [PATCH v2 1/6] midx: add MIDX_PROGRESS flag <snip> SZEDER Gábor
2019-09-23 21:55       ` William Baker
2019-09-28  3:40       ` Junio C Hamano
2019-09-30 17:01         ` William Baker
2019-10-02  5:38           ` Junio C Hamano
2019-10-02  5:43           ` Junio C Hamano
2019-10-02  7:04             ` Junio C Hamano
2019-10-02 15:56               ` William Baker
2019-10-07 17:12             ` SZEDER Gábor
2019-10-07 17:29         ` SZEDER Gábor
2019-10-08  4:30           ` Junio C Hamano
2019-10-08 16:24             ` William Baker
2019-10-09  0:16               ` SZEDER Gábor
2019-10-09  1:32             ` SZEDER Gábor
2019-10-15 20:00               ` William Baker [this message]
2019-10-16  2:09                 ` Junio C Hamano
2019-10-16 19:48                   ` William Baker
2019-10-18 21:35                     ` William Baker
2019-09-20 16:53   ` [PATCH v2 2/6] midx: add progress to write_midx_file Add progress to write_midx_file. Progress is displayed when the MIDX_PROGRESS flag is set William Baker via GitGitGadget
2019-09-20 20:10     ` Junio C Hamano
2019-09-23 21:12       ` William Baker
2019-09-28  3:49         ` Junio C Hamano
2019-09-30 16:36           ` William Baker
2019-09-20 16:53   ` [PATCH v2 3/6] midx: add progress to expire_midx_packs Add progress to expire_midx_packs. " William Baker via GitGitGadget
2019-09-20 16:53   ` [PATCH v2 5/6] midx: honor the MIDX_PROGRESS flag in midx_repack Update midx_repack to only display progress " William Baker via GitGitGadget
2019-09-20 20:12     ` Junio C Hamano
2019-09-20 16:53   ` [PATCH v2 4/6] midx: honor the MIDX_PROGRESS flag in verify_midx_file Update verify_midx_file " William Baker via GitGitGadget
2019-09-20 16:53   ` [PATCH v2 6/6] multi-pack-index: add [--[no-]progress] option. Add the --[no-]progress option to git multi-pack-index. Pass the MIDX_PROGRESS flag to the subcommand functions when progress should be displayed by multi-pack-index. The progress feature was added to 'verify' in 144d703 ("multi-pack-index: report progress during 'verify'", 2018-09-13) but some subcommands were not updated to display progress, and the ability to opt-out was overlooked William Baker via GitGitGadget
2019-09-20 19:54   ` [PATCH v2 0/6] multi-pack-index: add --no-progress Junio C Hamano
2019-09-20 20:33     ` William Baker
2019-09-20 20:23   ` Philip Oakley
2019-09-20 20:39     ` William Baker
2019-10-03 17:53   ` [PATCH v3 " William Baker via GitGitGadget
2019-10-03 17:53     ` [PATCH v3 1/6] midx: add MIDX_PROGRESS flag William Baker via GitGitGadget
2019-10-03 17:53     ` [PATCH v3 2/6] midx: add progress to write_midx_file William Baker via GitGitGadget
2019-10-03 17:53     ` [PATCH v3 3/6] midx: add progress to expire_midx_packs William Baker via GitGitGadget
2019-10-03 17:53     ` [PATCH v3 4/6] midx: honor the MIDX_PROGRESS flag in verify_midx_file William Baker via GitGitGadget
2019-10-03 17:53     ` [PATCH v3 5/6] midx: honor the MIDX_PROGRESS flag in midx_repack William Baker via GitGitGadget
2019-10-03 17:53     ` [PATCH v3 6/6] multi-pack-index: add [--[no-]progress] option William Baker via GitGitGadget
2019-10-03 22:46     ` [PATCH v3 0/6] multi-pack-index: add --no-progress Junio C Hamano
2019-10-21 18:39     ` [PATCH v4 " William Baker via GitGitGadget
2019-10-21 18:39       ` [PATCH v4 1/6] midx: add MIDX_PROGRESS flag William Baker via GitGitGadget
2019-10-21 18:39       ` [PATCH v4 2/6] midx: add progress to write_midx_file William Baker via GitGitGadget
2019-10-21 18:40       ` [PATCH v4 3/6] midx: add progress to expire_midx_packs William Baker via GitGitGadget
2019-10-21 18:40       ` [PATCH v4 4/6] midx: honor the MIDX_PROGRESS flag in verify_midx_file William Baker via GitGitGadget
2019-10-21 18:40       ` [PATCH v4 5/6] midx: honor the MIDX_PROGRESS flag in midx_repack William Baker via GitGitGadget
2019-10-21 18:40       ` [PATCH v4 6/6] multi-pack-index: add [--[no-]progress] option William Baker via GitGitGadget

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=04342d12-fffc-afb6-fa4e-c2e2bf88d1b6@gmail.com \
    --to=williamtbakeremail@gmail.com \
    --cc=William.Baker@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=jeffhost@microsoft.com \
    --cc=stolee@gmail.com \
    --cc=szeder.dev@gmail.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).