| Message ID | 20190320114703.18659-11-pclouds@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nd/diff-parseopt the last part | expand |
On Wed, Mar 20 2019, Nguyễn Thái Ngọc Duy wrote: > [...]And the '40' change is self explanatory. Let me make an attempt at being dense anyway... > - else if (v > 40) > - v = 40; > + else if (v > the_hash_algo->hexsz) > + v = the_hash_algo->hexsz; > } This is obviously not a regression, it's a hardcoded 40 *now*. So we should take this patch. But in general, I wonder how this is going to work once we get a few steps further into the SHA-256 migration. I.e. here we're still parsing the command-line, and the_hash_algo might be initialized early to SHA-1. So if I set --abbrev=45 it'll be trimmed to --abbrev=40 by this code. But then shortly afterwards we pass my SHA-256 object down to some machinery, and will then want to abbreviate it. Isn't that part of the code something we're going to want to support looking up objects in either hash, even if we initially started out with SHA-1 in the_hash_algo? So we'll be over-abbreviating a SHA-256 object. Leaving aside the sillyness of wanting to abbreviate *anything* to 45 characters, I wonder how those sorts of chicken & egg hash scenarios will go involving the_hash_algo.
On Thu, Mar 21, 2019 at 6:00 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > > On Wed, Mar 20 2019, Nguyễn Thái Ngọc Duy wrote: > > > [...]And the '40' change is self explanatory. > > Let me make an attempt at being dense anyway... > > > - else if (v > 40) > > - v = 40; > > + else if (v > the_hash_algo->hexsz) > > + v = the_hash_algo->hexsz; > > } > > This is obviously not a regression, it's a hardcoded 40 *now*. So we > should take this patch. > > But in general, I wonder how this is going to work once we get a few > steps further into the SHA-256 migration. I.e. here we're still parsing > the command-line, and the_hash_algo might be initialized early to SHA-1. That would be wrong. the_hash_algo must be properly initialized by the time any command parsing is done (except maybe "git <options> <cmd>"). While parse_options() most of the time is just a dumb "set this variable, set that variable", it often can have callbacks to do more complicated stuff and we can't just go with "pre-initialized to SHA-1" assumption. That's as bad as "assume $CWD is worktree" until worktree is discovered. There is a corner case though. If some command takes hash algo as an option (e.g. git hash-object should work without a repo) then yes we might have a problem since the_hash_algo might not be initialized yet, depending on option order. > So if I set --abbrev=45 it'll be trimmed to --abbrev=40 by this code. > > But then shortly afterwards we pass my SHA-256 object down to some > machinery, and will then want to abbreviate it. > > Isn't that part of the code something we're going to want to support > looking up objects in either hash, even if we initially started out with > SHA-1 in the_hash_algo? So we'll be over-abbreviating a SHA-256 object. > > Leaving aside the sillyness of wanting to abbreviate *anything* to 45 > characters, I wonder how those sorts of chicken & egg hash scenarios > will go involving the_hash_algo.
diff --git a/diff.c b/diff.c index 6e84af1cce..30baf21021 100644 --- a/diff.c +++ b/diff.c @@ -5254,6 +5254,7 @@ static void prep_parse_options(struct diff_options *options) OPT_SET_INT('z', NULL, &options->line_termination, N_("do not munge pathnames and use NULs as output field terminators in --raw or --numstat"), 0), + OPT__ABBREV(&options->abbrev), OPT_CALLBACK_F(0, "output-indicator-new", &options->output_indicators[OUTPUT_INDICATOR_NEW], N_("<char>"), @@ -5448,17 +5449,6 @@ int diff_opt_parse(struct diff_options *options, } /* misc options */ - else if (!strcmp(arg, "--no-abbrev")) - options->abbrev = 0; - else if (!strcmp(arg, "--abbrev")) - options->abbrev = DEFAULT_ABBREV; - else if (skip_prefix(arg, "--abbrev=", &arg)) { - options->abbrev = strtoul(arg, NULL, 10); - if (options->abbrev < MINIMUM_ABBREV) - options->abbrev = MINIMUM_ABBREV; - else if (the_hash_algo->hexsz < options->abbrev) - options->abbrev = the_hash_algo->hexsz; - } else if ((argcount = parse_long_opt("src-prefix", av, &optarg))) { options->a_prefix = optarg; return argcount; diff --git a/parse-options-cb.c b/parse-options-cb.c index 2733393546..6e2e8d6273 100644 --- a/parse-options-cb.c +++ b/parse-options-cb.c @@ -22,8 +22,8 @@ int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset) opt->long_name); if (v && v < MINIMUM_ABBREV) v = MINIMUM_ABBREV; - else if (v > 40) - v = 40; + else if (v > the_hash_algo->hexsz) + v = the_hash_algo->hexsz; } *(int *)(opt->value) = v; return 0;
OPT__ABBREV() has the same behavior as the deleted code with one difference: it does check for valid number and error out if not. And the '40' change is self explanatory. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- diff.c | 12 +----------- parse-options-cb.c | 4 ++-- 2 files changed, 3 insertions(+), 13 deletions(-)