git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [RFC PATCH] rev-list: clarify --abbrev and --abbrev-commit usage
@ 2019-06-13 22:15 Emily Shaffer
  2019-06-14 16:09 ` Junio C Hamano
  2019-06-14 16:18 ` Jeff King
  0 siblings, 2 replies; 8+ messages in thread
From: Emily Shaffer @ 2019-06-13 22:15 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

Indicate that --abbrev only works with --abbrev-commit also specified.
It seems that simply running `git rev-list --abbrev=5` doesn't
abbreviate commit OIDs. But the combination of `git rev-list
--abbrev-commit --abbrev=5` works as expected. Clarify in the
documentation by indicating that --abbrev is an optional addition to the
--abbrev-commit option. --no-abbrev remains on a separate line as it can
still be used to disable OID abbreviation even if --abbrev-commit has
been specified.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Change-Id: If9b1198938e1a3515ae6740241f7b791fb7a88bd
---
I thought this was odd when I was working on the other rev-list changes
- --abbrev doesn't do anything on its own. It looks like it does work by
itself in other commands, but apparently not in rev-list.

Listed this patch as RFC because maybe instead it's better to fix
something so --abbrev can be used alone, or teach --abbrev-commit=<n>.
It looks like `git log --abbrev=5` also doesn't work the way one might
expect, which makes sense to me, as they use the same internals for
option parsing (parse_revisions()).

The manpages for log and rev-list both correctly indicate that
--abbrev=<n> is an optional addition to --abbrev-commit. `git log -h` is
generated by parse-options tooling and doesn't cover --abbrev-commit at
all, but `git rev-list` doesn't use an option parser on its own and the
usage is hardcoded.

 - Emily

 builtin/rev-list.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 9f31837d30..6ae0087b01 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -49,8 +49,8 @@ static const char rev_list_usage[] =
 "    --objects | --objects-edge\n"
 "    --unpacked\n"
 "    --header | --pretty\n"
-"    --abbrev=<n> | --no-abbrev\n"
-"    --abbrev-commit\n"
+"    --abbrev-commit [--abbrev=<n>]\n"
+"    --no-abbrev\n"
 "    --left-right\n"
 "    --count\n"
 "  special purpose:\n"
-- 
2.22.0.rc2.383.gf4fbbf30c2-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH] rev-list: clarify --abbrev and --abbrev-commit usage
  2019-06-13 22:15 [RFC PATCH] rev-list: clarify --abbrev and --abbrev-commit usage Emily Shaffer
@ 2019-06-14 16:09 ` Junio C Hamano
  2019-06-14 16:18 ` Jeff King
  1 sibling, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2019-06-14 16:09 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: git

Emily Shaffer <emilyshaffer@google.com> writes:

> It looks like `git log --abbrev=5` also doesn't work the way one might
> expect, which makes sense to me, as they use the same internals for
> option parsing (parse_revisions()).

I suspect that was primarily because --abbrev-commit was only to
abbreviate the commit object names on the log header, and --abbrev
was to abbreviate the object names seen in the --raw output.  These
days, even --raw output seems to abbreviate by default, so --abbrev
alone lost its usefulness quite a bit.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH] rev-list: clarify --abbrev and --abbrev-commit usage
  2019-06-13 22:15 [RFC PATCH] rev-list: clarify --abbrev and --abbrev-commit usage Emily Shaffer
  2019-06-14 16:09 ` Junio C Hamano
@ 2019-06-14 16:18 ` Jeff King
  2019-06-14 20:59   ` Emily Shaffer
  1 sibling, 1 reply; 8+ messages in thread
From: Jeff King @ 2019-06-14 16:18 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: git

On Thu, Jun 13, 2019 at 03:15:41PM -0700, Emily Shaffer wrote:

> I thought this was odd when I was working on the other rev-list changes
> - --abbrev doesn't do anything on its own. It looks like it does work by
> itself in other commands, but apparently not in rev-list.
> 
> Listed this patch as RFC because maybe instead it's better to fix
> something so --abbrev can be used alone, or teach --abbrev-commit=<n>.
> It looks like `git log --abbrev=5` also doesn't work the way one might
> expect, which makes sense to me, as they use the same internals for
> option parsing (parse_revisions()).
> 
> The manpages for log and rev-list both correctly indicate that
> --abbrev=<n> is an optional addition to --abbrev-commit. `git log -h` is
> generated by parse-options tooling and doesn't cover --abbrev-commit at
> all, but `git rev-list` doesn't use an option parser on its own and the
> usage is hardcoded.

Yeah, "--abbrev" is a bit tricky here. It is really "when you abbrev, do
it to this level". In "log", that means that "git log --abbrev=5 --raw"
_does_ do something useful (it abbreviates the blob hashes). And then
you may add "--abbrev-commit" on top to ask to abbreviate the commit
ids.

And rev-list follows that same pattern, except that rev-list _never_
shows diff output. You'd traditionally do (and this is how log was
implemented once upon a time):

  git rev-list HEAD | git diff-tree --stdin --abbrev=5 --raw

But even there, we are not seeing the commit id output by rev-list. It
goes to diff-tree, which then formats it separately. So if you wanted
abbreviated commits there, you'd add "--abbrev-commit" to the diff-tree
invocation, not rev-list!

So no, I cannot see a way in which "rev-list --abbrev" is useful without
"--abbrev-commit". Which means that perhaps the former should imply the
latter.

And as you noticed in your other patch, there is no way to abbreviate
"--objects" output at all. I am not sure I have ever seen a good use for
that. Though to be honest, I am not sure that "--abbrev" is really all
that useful in the first place. Machine-readable output should never
abbreviate, and human-readable ones generally already do.

But at any rate, before making any behavior changes it may make sense to
think about how they'd interact with "rev-list --objects" abbreviation,
if it were to be added.

As for the patch itself:

> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index 9f31837d30..6ae0087b01 100644
> --- a/builtin/rev-list.c
> +++ b/builtin/rev-list.c
> @@ -49,8 +49,8 @@ static const char rev_list_usage[] =
>  "    --objects | --objects-edge\n"
>  "    --unpacked\n"
>  "    --header | --pretty\n"
> -"    --abbrev=<n> | --no-abbrev\n"
> -"    --abbrev-commit\n"
> +"    --abbrev-commit [--abbrev=<n>]\n"
> +"    --no-abbrev\n"

So --no-abbrev clears both --abbrev and --abbrev-commit. That sort of
makes sense, though I did not expect it. But it also means that:

  --abbrev-commit [--abbrev=<n> | --no-abbrev]

is not right. Possibly:

  --abbrev-commit [--abbrev=<n>] | --no-abbrev

would show the interaction more clearly, but I don't have a strong
opinion.

-Peff

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH] rev-list: clarify --abbrev and --abbrev-commit usage
  2019-06-14 16:18 ` Jeff King
@ 2019-06-14 20:59   ` Emily Shaffer
  2019-06-14 21:27     ` Jeff King
  0 siblings, 1 reply; 8+ messages in thread
From: Emily Shaffer @ 2019-06-14 20:59 UTC (permalink / raw)
  To: Jeff King; +Cc: git

On Fri, Jun 14, 2019 at 12:18:41PM -0400, Jeff King wrote:
> On Thu, Jun 13, 2019 at 03:15:41PM -0700, Emily Shaffer wrote:
> 
> > I thought this was odd when I was working on the other rev-list changes
> > - --abbrev doesn't do anything on its own. It looks like it does work by
> > itself in other commands, but apparently not in rev-list.
> > 
> > Listed this patch as RFC because maybe instead it's better to fix
> > something so --abbrev can be used alone, or teach --abbrev-commit=<n>.
> > It looks like `git log --abbrev=5` also doesn't work the way one might
> > expect, which makes sense to me, as they use the same internals for
> > option parsing (parse_revisions()).
> > 
> > The manpages for log and rev-list both correctly indicate that
> > --abbrev=<n> is an optional addition to --abbrev-commit. `git log -h` is
> > generated by parse-options tooling and doesn't cover --abbrev-commit at
> > all, but `git rev-list` doesn't use an option parser on its own and the
> > usage is hardcoded.
> 
> Yeah, "--abbrev" is a bit tricky here. It is really "when you abbrev, do
> it to this level". In "log", that means that "git log --abbrev=5 --raw"
> _does_ do something useful (it abbreviates the blob hashes). And then
> you may add "--abbrev-commit" on top to ask to abbreviate the commit
> ids.
> 
> And rev-list follows that same pattern, except that rev-list _never_
> shows diff output. You'd traditionally do (and this is how log was
> implemented once upon a time):
> 
>   git rev-list HEAD | git diff-tree --stdin --abbrev=5 --raw
> 
> But even there, we are not seeing the commit id output by rev-list. It
> goes to diff-tree, which then formats it separately. So if you wanted
> abbreviated commits there, you'd add "--abbrev-commit" to the diff-tree
> invocation, not rev-list!
> 
> So no, I cannot see a way in which "rev-list --abbrev" is useful without
> "--abbrev-commit". Which means that perhaps the former should imply the
> latter.

Maybe it should; maybe this patch is a good excuse to do so, and enforce
mutual exclusion of --abbrev-commit/--abbrev and --no-abbrev. Maybe it's
also a good time to add --abbrev-commit=<length>?

> 
> And as you noticed in your other patch, there is no way to abbreviate
> "--objects" output at all. I am not sure I have ever seen a good use for
> that. Though to be honest, I am not sure that "--abbrev" is really all
> that useful in the first place. Machine-readable output should never
> abbreviate, and human-readable ones generally already do.
> 
> But at any rate, before making any behavior changes it may make sense to
> think about how they'd interact with "rev-list --objects" abbreviation,
> if it were to be added.
> 
> As for the patch itself:
> 
> > diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> > index 9f31837d30..6ae0087b01 100644
> > --- a/builtin/rev-list.c
> > +++ b/builtin/rev-list.c
> > @@ -49,8 +49,8 @@ static const char rev_list_usage[] =
> >  "    --objects | --objects-edge\n"
> >  "    --unpacked\n"
> >  "    --header | --pretty\n"
> > -"    --abbrev=<n> | --no-abbrev\n"
> > -"    --abbrev-commit\n"
> > +"    --abbrev-commit [--abbrev=<n>]\n"
> > +"    --no-abbrev\n"
> 
> So --no-abbrev clears both --abbrev and --abbrev-commit. That sort of
> makes sense, though I did not expect it. But it also means that:
> 
>   --abbrev-commit [--abbrev=<n> | --no-abbrev]
> 
> is not right. Possibly:
> 
>   --abbrev-commit [--abbrev=<n>] | --no-abbrev
> 
> would show the interaction more clearly, but I don't have a strong
> opinion.

I did consider demonstrating it this way, but when both --abbrev-commit
and --no-abbrev are used together, we don't complain or reject the
invocation - which I would expect if the usage states the two options
are mutually exclusive.

I've been trying to think of good reasons not to enforce their mutual
exclusion, and the one I keep coming back to is that --no-abbrev might
be desired to override a git config'd abbreviation length - although I
didn't check to see whether we have one, maybe we would want one later.
And even in that case, I suppose that --abbrev-commit would not be
explicitly added to the call (because we'd infer from the config), or
that if it did need to be explicitly added (like if we need the user to
say they want abbreviation, but we want to use their configured
preferred length) then we could still reject the addition of
--no-abbrev.

So maybe it makes even more sense to take this patch as an opportunity
to make these options mutually exclusive... although that checking I
think would wind up in revision.c, and therefore widen the impact of
the change significantly.

From a practical standpoint, I guess you could consider --abbrev-commit
and --no-abbrev mutually exclusive, but since it's not enforced, I have
a little preference not to document it as though it is...

 - Emily

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH] rev-list: clarify --abbrev and --abbrev-commit usage
  2019-06-14 20:59   ` Emily Shaffer
@ 2019-06-14 21:27     ` Jeff King
  2019-06-14 22:56       ` Emily Shaffer
  0 siblings, 1 reply; 8+ messages in thread
From: Jeff King @ 2019-06-14 21:27 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: git

On Fri, Jun 14, 2019 at 01:59:50PM -0700, Emily Shaffer wrote:

> > So no, I cannot see a way in which "rev-list --abbrev" is useful without
> > "--abbrev-commit". Which means that perhaps the former should imply the
> > latter.
> 
> Maybe it should; maybe this patch is a good excuse to do so, and enforce
> mutual exclusion of --abbrev-commit/--abbrev and --no-abbrev. Maybe it's
> also a good time to add --abbrev-commit=<length>?

Hmm, yeah, I think that would reduce confusion quite a bit. If it were
"--abbrev-commit=<length>", then "--abbrev" would not be useful for
anything in rev-list. It would still work as a historical item, but we
would not need or want to advertise it in the usage at all. Good
suggestion.

> > is not right. Possibly:
> > 
> >   --abbrev-commit [--abbrev=<n>] | --no-abbrev
> > 
> > would show the interaction more clearly, but I don't have a strong
> > opinion.
> 
> I did consider demonstrating it this way, but when both --abbrev-commit
> and --no-abbrev are used together, we don't complain or reject the
> invocation - which I would expect if the usage states the two options
> are mutually exclusive.

Ah, I see. I don't consider "|" to indicate an exclusion to the point
that the options are rejected. Only that you wouldn't want to use both,
because one counteracts the other. So every "--no-foo" is mutually
exclusive with "--foo" in the sense that one override the other. But the
outcome is "last one wins", and not "whoops, we cannot figure out what
you meant". And that's what the original:

      --abbrev=<n> | --no-abbrev

before your patch was trying to say (and I suspect there are many other
cases of "|" with this kind of last-one-wins behavior).

> I've been trying to think of good reasons not to enforce their mutual
> exclusion, and the one I keep coming back to is that --no-abbrev might
> be desired to override a git config'd abbreviation length - although I
> didn't check to see whether we have one, maybe we would want one later.
> And even in that case, I suppose that --abbrev-commit would not be
> explicitly added to the call (because we'd infer from the config), or
> that if it did need to be explicitly added (like if we need the user to
> say they want abbreviation, but we want to use their configured
> preferred length) then we could still reject the addition of
> --no-abbrev.
>
> So maybe it makes even more sense to take this patch as an opportunity
> to make these options mutually exclusive... although that checking I
> think would wind up in revision.c, and therefore widen the impact of
> the change significantly.

You can configure core.abbrev, though I'm not sure if it ever requests
abbreviation itself, or if it simply sets the length when we do happen
to abbreviate based on command-line options.

But forgetting config for a moment, last-one-wins is useful even among
command line options. E.g., imagine an alias like this:

  [alias]
  mylog = git rev-list --abbrev-commit --pretty=oneline

It's nice if you can run "git mylog --no-abbrev" and have it do what you
expect, instead of complaining "you cannot use --abbrev-commit and
--no-abbrev together".

That's a toy example, but you can imagine more elaborate scripts that
set some default options, and allow arbitrary per-invocation options to
be appended.

-Peff

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH] rev-list: clarify --abbrev and --abbrev-commit usage
  2019-06-14 21:27     ` Jeff King
@ 2019-06-14 22:56       ` Emily Shaffer
  2019-06-19 21:21         ` Jeff King
  0 siblings, 1 reply; 8+ messages in thread
From: Emily Shaffer @ 2019-06-14 22:56 UTC (permalink / raw)
  To: Jeff King; +Cc: git

On Fri, Jun 14, 2019 at 05:27:14PM -0400, Jeff King wrote:
> On Fri, Jun 14, 2019 at 01:59:50PM -0700, Emily Shaffer wrote:
> 
> > > So no, I cannot see a way in which "rev-list --abbrev" is useful without
> > > "--abbrev-commit". Which means that perhaps the former should imply the
> > > latter.
> > 
> > Maybe it should; maybe this patch is a good excuse to do so, and enforce
> > mutual exclusion of --abbrev-commit/--abbrev and --no-abbrev. Maybe it's
> > also a good time to add --abbrev-commit=<length>?
> 
> Hmm, yeah, I think that would reduce confusion quite a bit. If it were
> "--abbrev-commit=<length>", then "--abbrev" would not be useful for
> anything in rev-list. It would still work as a historical item, but we
> would not need or want to advertise it in the usage at all. Good
> suggestion.

Given your comments below, I think rather than enforcing mutual
exclusion it makes more sense to enforce last-one-wins. But the thinking
is essentially the same.

> 
> > > is not right. Possibly:
> > > 
> > >   --abbrev-commit [--abbrev=<n>] | --no-abbrev
> > > 
> > > would show the interaction more clearly, but I don't have a strong
> > > opinion.
> > 
> > I did consider demonstrating it this way, but when both --abbrev-commit
> > and --no-abbrev are used together, we don't complain or reject the
> > invocation - which I would expect if the usage states the two options
> > are mutually exclusive.
> 
> Ah, I see. I don't consider "|" to indicate an exclusion to the point
> that the options are rejected. Only that you wouldn't want to use both,
> because one counteracts the other. So every "--no-foo" is mutually
> exclusive with "--foo" in the sense that one override the other. But the
> outcome is "last one wins", and not "whoops, we cannot figure out what
> you meant". And that's what the original:
> 
>       --abbrev=<n> | --no-abbrev
> 
> before your patch was trying to say (and I suspect there are many other
> cases of "|" with this kind of last-one-wins behavior).

For what it's worth, in this case it's not last-one-wins - --no-abbrev
always wins:

  emilyshaffer@podkayne:~/git [master]$ g rev-list --abbrev-commit
  --no-abbrev --max-count=5 --pretty=oneline HEAD
  b697d92f56511e804b8ba20ccbe7bdc85dc66810 Git 2.22
  6ee1eaca3e996e69691f515742129645f453e0e8 Merge tag 'l10n-2.22.0-rnd3' of
    git://github.com/git-l10n/git-po
  0cdb8d2db2f39d1a29636975168c457d2dc0d466 Merge branch 'fr_review' of
    git://github.com/jnavila/git
  d0149200792f579151166a4a5bfae7e66c5d998b Merge branch 'master' of
    git://github.com/alshopov/git-po
  82eb147dbbbd0221980883e87ca7efd16a939a6f l10n: fr.po: Review French
    translation
  emilyshaffer@podkayne:~/git [master]$ g rev-list --no-abbrev
  --abbrev-commit --max-count=5 --pretty=oneline HEAD
  b697d92f56511e804b8ba20ccbe7bdc85dc66810 Git 2.22
  6ee1eaca3e996e69691f515742129645f453e0e8 Merge tag 'l10n-2.22.0-rnd3' of
    git://github.com/git-l10n/git-po
  0cdb8d2db2f39d1a29636975168c457d2dc0d466 Merge branch 'fr_review' of
    git://github.com/jnavila/git
  d0149200792f579151166a4a5bfae7e66c5d998b Merge branch 'master' of
    git://github.com/alshopov/git-po
  82eb147dbbbd0221980883e87ca7efd16a939a6f l10n: fr.po: Review French
    translation
  emilyshaffer@podkayne:~/git [master]$ g rev-list --abbrev-commit
  --max-count=5 --pretty=oneline HEAD
  b697d92f56 Git 2.22
  6ee1eaca3e Merge tag 'l10n-2.22.0-rnd3' of
    git://github.com/git-l10n/git-po
  0cdb8d2db2 Merge branch 'fr_review' of git://github.com/jnavila/git
  d014920079 Merge branch 'master' of git://github.com/alshopov/git-po
  82eb147dbb l10n: fr.po: Review French translation

> 
> > I've been trying to think of good reasons not to enforce their mutual
> > exclusion, and the one I keep coming back to is that --no-abbrev might
> > be desired to override a git config'd abbreviation length - although I
> > didn't check to see whether we have one, maybe we would want one later.
> > And even in that case, I suppose that --abbrev-commit would not be
> > explicitly added to the call (because we'd infer from the config), or
> > that if it did need to be explicitly added (like if we need the user to
> > say they want abbreviation, but we want to use their configured
> > preferred length) then we could still reject the addition of
> > --no-abbrev.
> >
> > So maybe it makes even more sense to take this patch as an opportunity
> > to make these options mutually exclusive... although that checking I
> > think would wind up in revision.c, and therefore widen the impact of
> > the change significantly.
> 
> You can configure core.abbrev, though I'm not sure if it ever requests
> abbreviation itself, or if it simply sets the length when we do happen
> to abbreviate based on command-line options.
> 
> But forgetting config for a moment, last-one-wins is useful even among
> command line options. E.g., imagine an alias like this:
> 
>   [alias]
>   mylog = git rev-list --abbrev-commit --pretty=oneline
> 
> It's nice if you can run "git mylog --no-abbrev" and have it do what you
> expect, instead of complaining "you cannot use --abbrev-commit and
> --no-abbrev together".
> 
> That's a toy example, but you can imagine more elaborate scripts that
> set some default options, and allow arbitrary per-invocation options to
> be appended.

This makes a lot more sense than the scenarios I was imagining. Thanks.

I think a good solution here is to go and add --abbrev-commit=<n>
without breaking support for --abbrev=<n>; I'm a little more worried
about changing --no-abbrev to last-one-wins but I'll take a crack at it
and see what the test suite says. While I'm at it, I'll check for
last-one-wins with multiple instances of --abbrev[-commit]=<n>.

Having done so, I'll also change the documentation here in rev-list to:
 --abbrev-commit[=<n>] [--abbrev=<n>] | --no-abbrev

Sounds like a fun bit of low hanging fruit to me. Hoping to have a short
turnaround. :)

 - Emily

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH] rev-list: clarify --abbrev and --abbrev-commit usage
  2019-06-14 22:56       ` Emily Shaffer
@ 2019-06-19 21:21         ` Jeff King
  2019-06-19 22:09           ` Emily Shaffer
  0 siblings, 1 reply; 8+ messages in thread
From: Jeff King @ 2019-06-19 21:21 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: git

On Fri, Jun 14, 2019 at 03:56:54PM -0700, Emily Shaffer wrote:

> > Ah, I see. I don't consider "|" to indicate an exclusion to the point
> > that the options are rejected. Only that you wouldn't want to use both,
> > because one counteracts the other. So every "--no-foo" is mutually
> > exclusive with "--foo" in the sense that one override the other. But the
> > outcome is "last one wins", and not "whoops, we cannot figure out what
> > you meant". And that's what the original:
> > 
> >       --abbrev=<n> | --no-abbrev
> > 
> > before your patch was trying to say (and I suspect there are many other
> > cases of "|" with this kind of last-one-wins behavior).
> 
> For what it's worth, in this case it's not last-one-wins - --no-abbrev
> always wins:

Ah, thanks for pointing that; I hadn't noticed. That _is_ unlike most of
the rest of Git. I'm tempted to say it's a bug and should be fixed, but
I worry slightly that it could have an unexpected effect.

> I think a good solution here is to go and add --abbrev-commit=<n>
> without breaking support for --abbrev=<n>; I'm a little more worried
> about changing --no-abbrev to last-one-wins but I'll take a crack at it
> and see what the test suite says. While I'm at it, I'll check for
> last-one-wins with multiple instances of --abbrev[-commit]=<n>.

I think --abbrev-commit=<n> sounds safe enough, though I worry it may
get a bit complicated because we'd presumably want to fall back to the
<n> from --abbrev=<n>. I'll see how your patch turns out. :)

I like the idea of changing --no-abbrev to last-one-wins, as above, but
the test suite may not give us that much confidence. These kinds of
cases are often not well-covered, and we're really worried about the
wider world of random scripts people have grown over the last 10 years.
Of course if the test suite does break horribly that might give us extra
caution, but I'm not sure "the test suite does not break" gives us much
confidence.

> Having done so, I'll also change the documentation here in rev-list to:
>  --abbrev-commit[=<n>] [--abbrev=<n>] | --no-abbrev

Yeah, that makes sense.

-Peff

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH] rev-list: clarify --abbrev and --abbrev-commit usage
  2019-06-19 21:21         ` Jeff King
@ 2019-06-19 22:09           ` Emily Shaffer
  0 siblings, 0 replies; 8+ messages in thread
From: Emily Shaffer @ 2019-06-19 22:09 UTC (permalink / raw)
  To: Jeff King; +Cc: git

On Wed, Jun 19, 2019 at 05:21:59PM -0400, Jeff King wrote:
> On Fri, Jun 14, 2019 at 03:56:54PM -0700, Emily Shaffer wrote:
> 
> > > Ah, I see. I don't consider "|" to indicate an exclusion to the point
> > > that the options are rejected. Only that you wouldn't want to use both,
> > > because one counteracts the other. So every "--no-foo" is mutually
> > > exclusive with "--foo" in the sense that one override the other. But the
> > > outcome is "last one wins", and not "whoops, we cannot figure out what
> > > you meant". And that's what the original:
> > > 
> > >       --abbrev=<n> | --no-abbrev
> > > 
> > > before your patch was trying to say (and I suspect there are many other
> > > cases of "|" with this kind of last-one-wins behavior).
> > 
> > For what it's worth, in this case it's not last-one-wins - --no-abbrev
> > always wins:
> 
> Ah, thanks for pointing that; I hadn't noticed. That _is_ unlike most of
> the rest of Git. I'm tempted to say it's a bug and should be fixed, but
> I worry slightly that it could have an unexpected effect.
> 
> > I think a good solution here is to go and add --abbrev-commit=<n>
> > without breaking support for --abbrev=<n>; I'm a little more worried
> > about changing --no-abbrev to last-one-wins but I'll take a crack at it
> > and see what the test suite says. While I'm at it, I'll check for
> > last-one-wins with multiple instances of --abbrev[-commit]=<n>.
> 
> I think --abbrev-commit=<n> sounds safe enough, though I worry it may
> get a bit complicated because we'd presumably want to fall back to the
> <n> from --abbrev=<n>. I'll see how your patch turns out. :)
> 
> I like the idea of changing --no-abbrev to last-one-wins, as above, but
> the test suite may not give us that much confidence. These kinds of
> cases are often not well-covered, and we're really worried about the
> wider world of random scripts people have grown over the last 10 years.
> Of course if the test suite does break horribly that might give us extra
> caution, but I'm not sure "the test suite does not break" gives us much
> confidence.

I think at that point, based on what I've seen on the list, there's a
tendency to include a config option to enable the new behavior. And I
think that might pass the tipping point of "wasted effort" for me.
The change isn't really enabling anything that was impossible before
(last-one-wins is handy, but it's legacy public behavior now for
--abbrev). So I'm kind of starting to lean towards doing nothing at all.
It's a lot of work, for a not-very-functional change, which needs to be
specially configured to even happen... So maybe I'll save my time and
work on actual bugs instead. :)

> 
> > Having done so, I'll also change the documentation here in rev-list to:
> >  --abbrev-commit[=<n>] [--abbrev=<n>] | --no-abbrev
> 
> Yeah, that makes sense.
> 
> -Peff

Thanks for the input, all. I think I'll drop this.

 - Emily

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2019-06-19 22:09 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-13 22:15 [RFC PATCH] rev-list: clarify --abbrev and --abbrev-commit usage Emily Shaffer
2019-06-14 16:09 ` Junio C Hamano
2019-06-14 16:18 ` Jeff King
2019-06-14 20:59   ` Emily Shaffer
2019-06-14 21:27     ` Jeff King
2019-06-14 22:56       ` Emily Shaffer
2019-06-19 21:21         ` Jeff King
2019-06-19 22:09           ` Emily Shaffer

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).