On Sat, May 14, 2011 at 3:01 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
> Why two options? ÂIt would be more conventional to provide just
> log.abbrevCommit. ÂThe existing "[add] ignore-errors" is explained in
> the manual to be a mistake
Ah, okay.
>> config option as
>> a convenience for users who often use --abbrev-commit with git log and
>> git show (but not git whatchanged, as its output is more likely to be
>> parsed even though it is not technically plumbing).
>
> Hm, that wouldn't have been my hunch. ÂAre you aware of any scripts
> that parse "git whatchanged" output?
Yes, I've seen it in post-receive hooks, however, that's typically in
bare repos where I suppose it's unlikely for the user to set
log.abbrevCommit, so maybe I was just being overly cautious.
> More worrying is "git log --format=raw". ÂI think as long as we're
> cautious about rolling this out slowly and noticing breakage early it
> should be okay. ÂIt might even be nice to find out if there are
> scripts or tests that care deeply about "git log"'s output format
> (which would be more reliable if they had been written to use
> "git rev-list | git diff-tree -s --stdin").
>
>> Allow the option to be overridden via --no-abbrev-commit.
>
> Good idea anyway. ÂOnce parse_revision_opt learns to use parse_options
> these negated options would be automatic (though that's a long way
> away).
>
> Unfortunately this wouldn't help scripts much until the option has
> been around for a while. ÂMaybe it would be safer to have two patches
> --- one to add --no-abbrev-commit which could be included in "maint"
> and widely deployed, and one to add the new configuration only after
> --no-abbrev-commit can be relied on? ÂBut on the other hand, scripts
> can be updated today to use rev-list | diff-tree, so maybe that's not
> worth the trouble.
I think that's overkill. Thinking through it more, this would only
break any scripts:
a) that are parsing whatchanged output; and
b) the user sets log.abbrevCommit w/o updating his script
> People using git by hand would certainly appreciate
> --no-abbrev-commit, I suspect.
That's why I added it.
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -1314,6 +1314,12 @@ interactive.singlekey::
>> Â Â Â linkgit:git-checkout[1]. Note that this setting is silently
>> Â Â Â ignored if portable keystroke input is not available.
>>
>> +log.abbrev-commit::
>> +log.abbrevCommit::
>> + Â Â If true, act as if --abbrev-commit were specified on the command
>> + Â Â line. May be overridden with --no-abbrev-commit. Note that this setting
>> + Â Â is ignored by rev-list.
>
> Style: most of that page is written from the point of view of the
> user
>
> [...]
>
> So maybe something like:
>
> Â Â Â ÂWhether to abbreviate hexadecimal commit object names in
> Â Â Â Âoutput from the 'log' family of commands. ÂThe number of
> Â Â Â Âdigits shown is determined by the `--abbrev` command-line
> Â Â Â Âoption and `core.abbrev` configuration variable. ÂCan be
> Â Â Â Âoverridden on the command line by --abbrev-commit /
> Â Â Â Â--no-abbrev-commit. ÂThe default is false.
> Â+
> ÂThis does not affect the 'git diff-tree' and 'git rev-list'
> Âcommands.
I thought I phrased it like the nearby options in config.txt. and that
sounds overly verbose to me, but I'll take another look.
>> --- a/Documentation/git-log.txt
>> +++ b/Documentation/git-log.txt
>> @@ -38,6 +38,9 @@ OPTIONS
>> Â Â Â Continue listing the history of a file beyond renames
>> Â Â Â (works only for a single file).
>>
>> +--no-abbrev-commit::
>> + Â Â Don't abbreviate commit name. Useful for overridding log.abbrevCommit.
>
> Also useful for overriding --abbrev-commit from aliases. :)
>
> Shouldn't it be documented next to --abbrev-commit?
As I implemented it, --no-abbrev-commit is only honored from within
log.c, so I didn't want it to show up in the rev-list man page.
But, see below where I address your question about why I didn't
implement --no-abbrev-commit in revision.c
>> --- a/builtin/log.c
>> +++ b/builtin/log.c
>> @@ -23,6 +23,7 @@
>> Â/* Set a default date-time format for git log ("log.date" config variable) */
>> Âstatic const char *default_date_mode = NULL;
>>
>> +static int default_abbrev_commit = 0;
>> Âstatic int default_show_root = 1;
>> Âstatic int decoration_style;
>
> Style: we try to avoid unnecessary zero initializers for variables in
> the BSS section.
Okay.
> [...]
>> @@ -89,11 +91,13 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
>> Â Â Â Â Â Â Â Â Â Â Â Âstruct rev_info *rev, struct setup_revision_opt *opt)
>> Â{
>> Â Â Â struct userformat_want w;
>> - Â Â int quiet = 0, source = 0;
>> + Â Â int quiet = 0, source = 0, no_abbrev_commit = 0;
>>
>> Â Â Â const struct option builtin_log_options[] = {
>> - Â Â Â Â Â Â OPT_BOOLEAN(0, "quiet", &quiet, "supress diff output"),
>> + Â Â Â Â Â Â OPT_BOOLEAN(0, "quiet", &quiet, "suppress diff output"),
>> Â Â Â Â Â Â Â OPT_BOOLEAN(0, "source", &source, "show source"),
>> + Â Â Â Â Â Â OPT_BOOLEAN(0, "no-abbrev-commit", &no_abbrev_commit,
>> + Â Â Â Â Â Â Â Â Â Â Â Â "don't abbreviate commit name"),
>
> What happens if I do
>
> Â Â Â Âgit log --no-abbrev-commit --abbrev-commit
>
> ? ÂHow about
>
> Â Â Â Âgit log --no-abbrev-commit --no-no-abbrev-commit --abbrev-commit
>
> ? :) ÂThe behavior should be nicer if this is implemented in revision.c.
It was a thinko to put it in log.c. I added --no-abbrev-commit
thinking of its primary use case to override log.abbrevCommit. But
obviously it's more generally useful in revision.c (even though that
doesn't honor log.abbrevCommit), since it can still be used to
override earlier CLI options that might enable abbreviation.
I'll move it. Then I can document the option next to --abbrev-commit
where it makes sense.
> [...]
>> @@ -323,6 +330,11 @@ static int git_log_config(const char *var, const char *value, void *cb)
>> Â Â Â Â Â Â Â return git_config_string(&fmt_pretty, var, value);
>> Â Â Â if (!strcmp(var, "format.subjectprefix"))
>> Â Â Â Â Â Â Â return git_config_string(&fmt_patch_subject_prefix, var, value);
>> + Â Â if (!strcasecmp(var, "log.abbrevcommit") ||
>> + Â Â Â Â !strcasecmp(var, "log.abbrev-commit")) {
>
> No need to use strcasecmp --- the vars passed to config functions
> already have the section and variable names in lowercase.
Okay, it was a cut-and-paste from ignore-errors I think.
>> @@ -347,6 +359,7 @@ int cmd_whatchanged(int argc, const char **argv, const char *prefix)
>> Â Â Â struct setup_revision_opt opt;
>>
>> Â Â Â git_config(git_log_config, NULL);
>> + Â Â default_abbrev_commit = 0;
>
> Puzzling as mentioned above.
Will drop.
>> --- a/t/t4202-log.sh
>> +++ b/t/t4202-log.sh
>> @@ -450,6 +450,14 @@ test_expect_success 'log.decorate configuration' '
>>
>> Â'
>>
>> +test_expect_success 'log.abbrev-commit configuration' '
>> + Â Â test_might_fail git config --remove-section log &&
>> + Â Â git log --abbrev-commit >expect &&
>> + Â Â git config log.abbrev-commit true &&
>> + Â Â git log >actual &&
>> + Â Â test_cmp expect actual
>> +'
>
> To avoid polluting the configuration, it would be nicest to do:
>
> Â Â Â Âgit config log.abbrev-commit true &&
> Â Â Â Âtest_when_finished "git config --unset log.abbrev-commit" &&
> Â Â Â Âgit log >actual &&
>
> though it looks like some tests already protect themselves.
Other tests in t4202 protect themselves at the start, I emulated that behavior.
> Just because I'm curious: what happens if you do
>
> Â Â Â Âgit config log.abbrev-commit true
>
> in test_create_repo in test-lib.sh? Â(I.e., are there many tests that
> would be confused by this?) ÂTests tend to be more picky than user
> scripts about the output of git but it might still be an ok way to
> get a vague sense of the impact.
I'll let you know. :-)
> Hope that helps, and thanks for a pleasant read.
Thanks for the quick review.
j.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html