Re: [PATCH v2 1/2] revision: differentiate if --no-abbrev asked explicitly

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 11, 2020, 18:54 UTC
Message-ID: <xmqqwo259fp2.fsf@gitster.c.googlers.com>
In-Reply-To: <9daef7445c09d355d4801f64e427c27dd6c44afb.1597146478.git.congdanhqx@gmail.com>

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

> When we see `--no-abbrev' in command's arguments, we reset `abbrev' of
> `diff_options` to 0, thus, on a later stage, the object id won't
> be shortened (by not set object_id[abbrev] to '\0').

s/by not set/&ting/ probably?

Please do not write pseudo-code that does not exist anywhere in the codebase that pretends to be a quote from some real code. It wasts reviewer's time looking for non-existing code to see what really is going on.

I think you meant this line in diff_abbrev_oid():

	if (abbrev)
		hex[abbrev] = '\0';

so you can say that more explicitly, perhaps:

  ... we reset the 'abbrev' field in diff-options to 0 and this
  value is looked at diff_abbrev_oid() to decide not to truncate
  the object name.

or somesuch.

> While not doing anything is very effective way to show full object id,
> we couldn't differentiate if --no-abbrev or not.

You mean you want to tell if --no-abbrev and/or --full-index was given from the command line and act differently? You need to justify why you need to be able to do so (which you attempted in the remainder of the proposed log message), and also you need to justify why changing revs->abbrev's meaning is the best way to do so (which you did not). In fill_metainfo(), which you are going to touch in [2/2], you can peek o->flags.full_index to see if --full-index was given, and the fact that revs->abbrev is not DEFAULT_ABBREV (which is how the field is initialized in repo_init_revisions()) but is 0 tells us that "--no-abbrev" was given. --abbrev=0 would have busted minimum abbrev and set the field to MINIMUM_ABBREV, --abbrev=99 would have busted hash size and set the field to hexsz. So 0 is the sign that --no-abbrev was given. No?

> In a later change, we want to extend --abbrev support to diff-patch
> format.

And it is unclear why this planned change requires it.

> Let's ask for full object id if we see --no-abbrev instead.

Hence, this "let's ask" is not justified very well.

Show 18 quoted lines

> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> ---
>  revision.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/revision.c b/revision.c
> index 3dcf689341..24027b1af3 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2430,7 +2430,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>  	} else if (!strcmp(arg, "--always")) {
>  		revs->always_show_header = 1;
>  	} else if (!strcmp(arg, "--no-abbrev")) {
> -		revs->abbrev = 0;
> +		revs->abbrev = hexsz;
>  	} else if (!strcmp(arg, "--abbrev")) {
>  		revs->abbrev = DEFAULT_ABBREV;
>  	} else if (skip_prefix(arg, "--abbrev=", &optarg)) {