I haven't read ahead in the series yet, but I presume you're making
this code extra generic because you plan to support additional `fixup`
options (such as `reword`), but I wonder if the cognitive overhead is
warranted or you could get by with something simpler, such as:
if (skip_prefix(msg, "amend:", &arg) ||
skip_prefix(msg, "reword:", &arg)) {
...
}
You still need to compute "len" because you'd want to tell between
--fixup="HEAD^{/^area: string}" and --fixup=bogus:HEAD (the latter
would want to say "no such variant 'bogus' for --fixup", but the
colon in the former is not the end of the name of variant.
So, skip_prefix() would not buy us much, I guess.
But the use of starts_with() in the original patch is bogus, I
think. fixup_message[] by the time the comparison is made is
NULL terminated at where the colon was originally, so we should be
doing !strcmp() to reject "--fixup=amendo:HEAD~2" with "no, 'amendo'
is not a valid variant name for --fixup option".
Also, am I misreading when I think that the use of starts_with() could
be replaced with a simple strcmp() since you've already inserted a
'\0' immediately after the final alphabetic character?
You still need to compute "len" because you'd want to tell between
--fixup="HEAD^{/^area: string}" and --fixup=bogus:HEAD (the latter
would want to say "no such variant 'bogus' for --fixup", but the
colon in the former is not the end of the name of variant.
I see what you mean. I vaguely recall quickly scanning over that
earlier discussion about ":" being otherwise legitimate when embedded
in the argument as you demonstrate, but didn't think about it when
reading this code. Perhaps the comment which this code adds:
* As `amend` suboption contains only alpha
* character. So check if first non alpha
* character in fixup_message is ':'.
could be extended a bit to mention that briefly since, without it, the
significance of "alpha-only characters followed by colon" is not
immediately obvious.
You still need to compute "len" because you'd want to tell between
--fixup="HEAD^{/^area: string}" and --fixup=bogus:HEAD (the latter
would want to say "no such variant 'bogus' for --fixup", but the
colon in the former is not the end of the name of variant.
I see what you mean. I vaguely recall quickly scanning over that
earlier discussion about ":" being otherwise legitimate when embedded
in the argument as you demonstrate, but didn't think about it when
reading this code. Perhaps the comment which this code adds:
* As `amend` suboption contains only alpha
* character. So check if first non alpha
* character in fixup_message is ':'.
could be extended a bit to mention that briefly since, without it, the
significance of "alpha-only characters followed by colon" is not
immediately obvious.
Okay, I will reword it and make it more clear regarding the main
reason for choosing this way.
I haven't read ahead in the series yet, but I presume you're making
this code extra generic because you plan to support additional `fixup`
options (such as `reword`), but I wonder if the cognitive overhead is
warranted or you could get by with something simpler, such as:
if (skip_prefix(msg, "amend:", &arg) ||
skip_prefix(msg, "reword:", &arg)) {
...
}
You still need to compute "len" because you'd want to tell between
--fixup="HEAD^{/^area: string}" and --fixup=bogus:HEAD (the latter
would want to say "no such variant 'bogus' for --fixup", but the
colon in the former is not the end of the name of variant.
So, skip_prefix() would not buy us much, I guess.
Yes, I also agree.
But the use of starts_with() in the original patch is bogus, I
think. fixup_message[] by the time the comparison is made is
NULL terminated at where the colon was originally, so we should be
doing !strcmp() to reject "--fixup=amendo:HEAD~2" with "no, 'amendo'
is not a valid variant name for --fixup option".
I am not sure about this because we used the starts_with() so that it can
support the _any_ prefix of `amend` or `reword` i.e to make all below
like combinations possible :
--fixup=a:HEAD~2
--fixup=am:HEAD~2
So, I am not sure if we need to replace it with !strcmp and work for
the specified prefix only ?
quoted
Also, am I misreading when I think that the use of starts_with() could
be replaced with a simple strcmp() since you've already inserted a
'\0' immediately after the final alphabetic character?
Correct.
Same reason for using the starts_with() applies for this part also
after inserting the '\0'. So, I think we can keep starts_with() ?
From: Eric Sunshine <hidden> Date: 2021-03-04 00:23:26
On Wed, Mar 3, 2021 at 2:41 AM Charvi Mendiratta [off-list ref] wrote:
On Tue, 2 Mar 2021 at 04:02, Junio C Hamano [off-list ref] wrote:
quoted
But the use of starts_with() in the original patch is bogus, I
think. fixup_message[] by the time the comparison is made is
NULL terminated at where the colon was originally, so we should be
doing !strcmp() to reject "--fixup=amendo:HEAD~2" with "no, 'amendo'
is not a valid variant name for --fixup option".
I am not sure about this because we used the starts_with() so that it can
support the _any_ prefix of `amend` or `reword` i.e to make all below
like combinations possible :
--fixup=a:HEAD~2
--fixup=am:HEAD~2
So, I am not sure if we need to replace it with !strcmp and work for
the specified prefix only ?
Hmm, I see. I didn't follow whatever discussion led to the decision to
use this sort of prefix matching, but I have to wonder if it is a good
idea. Was the idea that it behave similarly to sequencer commands in
`git rebase --interactive` which are often abbreviated to a single
letter? I personally would feel much more comfortable requiring a
full-word match for `amend` and `reword` at initial implementation.
That leaves the door open to later loosening it to do prefix-matching
if enough people request such a feature, whereas starting with
prefix-matching closes that door since we can never later tighten it
to require full words.
Anyhow, if the decision is to keep this behavior, then it almost
certainly deserves an in-code comment explaining the sort of
prefix-matching it's doing since it's otherwise too easy for readers
to be fooled as Junio and I were by not noticing that you had reversed
the arguments to starts_with().
On Wed, 3 Mar 2021 at 13:27, Eric Sunshine [off-list ref] wrote:
[...]
quoted
I am not sure about this because we used the starts_with() so that it can
support the _any_ prefix of `amend` or `reword` i.e to make all below
like combinations possible :
--fixup=a:HEAD~2
--fixup=am:HEAD~2
So, I am not sure if we need to replace it with !strcmp and work for
the specified prefix only ?
Hmm, I see. I didn't follow whatever discussion led to the decision to
use this sort of prefix matching, but I have to wonder if it is a good
idea. Was the idea that it behave similarly to sequencer commands in
`git rebase --interactive` which are often abbreviated to a single
letter?
I personally would feel much more comfortable requiring a
full-word match for `amend` and `reword` at initial implementation.
That leaves the door open to later loosening it to do prefix-matching
if enough people request such a feature, whereas starting with
prefix-matching closes that door since we can never later tighten it
to require full words.
Anyhow, if the decision is to keep this behavior, then it almost
certainly deserves an in-code comment explaining the sort of
prefix-matching it's doing since it's otherwise too easy for readers
to be fooled as Junio and I were by not noticing that you had reversed
the arguments to starts_with().