bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: juri@linkov.net,  rpluim@gmail.com,  57400@debbugs.gnu.org,  ane@iki.fi
>> Date: Thu, 13 Oct 2022 22:05:52 +0000
>> 
>> > Also, there's a passive tense here...
>> 
>> (This is getting embarrassing...)
>
> No need, it takes time to develop sensitivity to this stuff, and all
> of us, including myself, make these mistakes from time to time.

I appreciate hearing this.

>> I've rewritten the entire thing, how does this sound:
>> 
>>   "Translate COMMIT string into symbolic form.
>> A symbolic form is any revision that is not expressed in using
>> SHA-1 object name.  If the optional argument FORCE is non-nil,
>> this might include revision specifications like \"master~8\" (the
>> 8th parent of the commit that \"master\" is currently pointing
>> to).  If it is not possible to determine a symbolic commit, the
>> function returns nil."
>
> This is much better, but still leaves some obvious questions
> unanswered.
>
>>   Translate COMMIT string into symbolic form.
>
> What is a "COMMIT string"?  I guess you mean the commit ID, and the
> "string" part is just to indicate that its Lisp data type is a string?
> If so, we usually say something like
>
>   Translate Git revision descriptor of COMMIT, a string, to a symbolic form.

Is this perhaps not too long?  Would "Translate revision string COMMIT
to a symbolic form." be sufficient, especially as this is actually just
an internal function that wasn't marked as such (the Git history
indicates that this function, with this documentation string has been
around since the initial revision of the file in 2007)?

>>                     If the optional argument FORCE is non-nil,
>> this might include revision specifications like \"master~8\" (the
>> 8th parent of the commit that \"master\" is currently pointing
>> to).
>
> This begs the question: what kind of COMMIT strings are acceptable if
> FORCE is nil or omitted?  If we only accept SHA-1 hashes then, this
> should perhaps be mentioned in the first sentence.  But from reading
> the (unhelpful) man page of "git name-rev" (which leads down the
> rabbit hole to "git rev-parse"), it is my understanding that this will
> accept _any_ revision descriptor in any form.  

Yes, right.  And in absence of any restriction "COMMIT" should be
understood to be a SHA-1 reference or a symbolic reference, right?

>                                                So now I wonder why
> accepting something like "master~8" needs a special knob: it's just
> one of the forms supported by "git name-rev", isn't it?  So maybe you
> don't even need the additional argument and don't have to document it?

That is an open debate, the function is currently only used in vc-git.el
and is never invoked with the optional argument.  I've only added it
because it might be that it could be useful at some point in the future.

> But if there _are_ valid reasons not to accept the likes of
> "master~8", they should be in the doc string.  For example:
>
>   By default, COMMIT strings of the form "master~8" are rejected,
>   because <describe the reason here>, but if FORCE is non-nil, they
>   are allowed.

I guess it is difficult to come up with a "valid reason", the motivation
is that I wanted to have some way to ensure that
`vc-git-working-revision' only returns a symbolic form iff a branch or
tag is pointing to the working revision.  If you think it is preferable,
I could also invert the argument and make it into something like
"no-relative" or even pull the check into `vc-git-working-revision'.

> If "master~8" (or in general SOMETHING~N) is not the only form that is
> rejected, the description should include the other forms as well,
> perhaps as examples.
>
> (And note that the text I proposed above fixed yet another
> inconsistency: COMMIT is first described as a "revision" and "string"
> and then as "revision specification"; a good doc string should use the
> same consistent terminology throughout.)

Good point.

>>       If it is not possible to determine a symbolic commit, the
>> function returns nil.
>
> Again, for consistency, it is better to say
>
>   If it is not possible to determine the symbolic form of COMMIT, the
>   function returns nil.
>
> because the first sentence talked about converting COMMIT into a
> symbolic form.

Done.

> Thanks, and keep up the good work.

Thank you for your effort and detail.