[v3,1/2] git-merge-file doc: drop "-file" from argument placeholders

Commit Message

brian m. carlson Nov. 1, 2023, 7:24 p.m. UTC

From: Martin Ågren <[e-mail]>

`git merge-file` takes three positional arguments. Each of them is
documented as `<foo-file>`. In preparation for teaching this command to
alternatively take three object IDs, make these placeholders a bit more
generic by dropping the "-file" parts. Instead, clarify early that the
three arguments are filenames. Even after the next commit, we can afford
to present this file-centric view up front and in the general
discussion, since it will remain the default one.

Signed-off-by: Martin Ågren <[e-mail]>
Signed-off-by: brian m. carlson <[e-mail]>
---
 Documentation/git-merge-file.txt | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

Comments

Junio C Hamano Nov. 1, 2023, 11:53 p.m. UTC | #1

"brian m. carlson" <[e-mail]> writes:

> From: Martin Ågren <[e-mail]>
>
> `git merge-file` takes three positional arguments. Each of them is
> documented as `<foo-file>`. In preparation for teaching this command to
> alternatively take three object IDs, make these placeholders a bit more

Minor nit.  Don't we want to say "three blob object names"?  Unless
we plan to grow this feature into accepting three tree object names,
that is.

Martin Ågren Nov. 2, 2023, 8:53 a.m. UTC | #2

On Thu, 2 Nov 2023 at 00:53, Junio C Hamano <[e-mail]> wrote:
>
> "brian m. carlson" <[e-mail]> writes:
>
> > From: Martin Ågren <[e-mail]>
> >
> > `git merge-file` takes three positional arguments. Each of them is
> > documented as `<foo-file>`. In preparation for teaching this command to
> > alternatively take three object IDs, make these placeholders a bit more
>
> Minor nit.  Don't we want to say "three blob object names"?  Unless
> we plan to grow this feature into accepting three tree object names,
> that is.

Hmm, yeah. Or just "three non-filename arguments". I do wonder: doesn't
this mean that the second patch could/should possibly move away from the
notion of "object ID"/`--object-id`? (That's not trying to shift any
blame from one patch to the other, that's my honest reaction.)

Ah, yes, I thought I recognized this. Quoting your response [1] to v2:

> I briefly thought about suggesting --blob-id instead of --object-id
> simply because you'd never want to feed it trees and commits, but
> the error message from read_mmblob() the users would get mentions
> 'blob' to signal that non-blob objects are unwelcome, so the name of
> the optionwould be OK as-is.

Maybe you having a similar reaction a second time makes this smell a bit
more?

[1] https://lore.kernel.org/git/xmqqo7ge2xzl.fsf@gitster.g/


Martin

brian m. carlson Nov. 2, 2023, 9:18 a.m. UTC | #3

On 2023-11-02 at 08:53:36, Martin Ågren wrote:
> On Thu, 2 Nov 2023 at 00:53, Junio C Hamano <[e-mail]> wrote:
> >
> > "brian m. carlson" <[e-mail]> writes:
> >
> > > From: Martin Ågren <[e-mail]>
> > >
> > > `git merge-file` takes three positional arguments. Each of them is
> > > documented as `<foo-file>`. In preparation for teaching this command to
> > > alternatively take three object IDs, make these placeholders a bit more
> >
> > Minor nit.  Don't we want to say "three blob object names"?  Unless
> > we plan to grow this feature into accepting three tree object names,
> > that is.
> 
> Hmm, yeah. Or just "three non-filename arguments". I do wonder: doesn't
> this mean that the second patch could/should possibly move away from the
> notion of "object ID"/`--object-id`? (That's not trying to shift any
> blame from one patch to the other, that's my honest reaction.)

Not specifying an option would make this ambiguous.  What if I have a
file named "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391"?  Is that the
empty blob, or is it that file?  Normally we have ways to disambiguate
this, but those don't work here because of the positional arguments.

> Ah, yes, I thought I recognized this. Quoting your response [1] to v2:
> 
> > I briefly thought about suggesting --blob-id instead of --object-id
> > simply because you'd never want to feed it trees and commits, but
> > the error message from read_mmblob() the users would get mentions
> > 'blob' to signal that non-blob objects are unwelcome, so the name of
> > the optionwould be OK as-is.
> 
> Maybe you having a similar reaction a second time makes this smell a bit
> more?

I think the name is fine.  We don't typically use the phrase "blob ID"
anywhere, but we do say "object ID".  We'd need to say "--blob", but
I'm not sure that's an improvement, and I fear it may be less
understandable.

Martin Ågren Nov. 2, 2023, 9:29 a.m. UTC | #4

On Thu, 2 Nov 2023 at 10:18, brian m. carlson
<[e-mail]> wrote:
>
> On 2023-11-02 at 08:53:36, Martin Ågren wrote:

> > Hmm, yeah. Or just "three non-filename arguments". I do wonder: doesn't
> > this mean that the second patch could/should possibly move away from the
> > notion of "object ID"/`--object-id`? (That's not trying to shift any
> > blame from one patch to the other, that's my honest reaction.)
>
> Not specifying an option would make this ambiguous.  What if I have a
> file named "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391"?  Is that the
> empty blob, or is it that file?  Normally we have ways to disambiguate
> this, but those don't work here because of the positional arguments.

I didn't intend to suggest dropping the new option altogether, sorry for
being unclear. I meant moving from `--object-id` to something else, such
as `--blob` that you mention. Completely agreed that something explicit
is needed here and that heuristics aren't possible.

> I think the name is fine.  We don't typically use the phrase "blob ID"
> anywhere, but we do say "object ID".  We'd need to say "--blob", but
> I'm not sure that's an improvement, and I fear it may be less
> understandable.

Agreed. Thanks.

Martin

Junio C Hamano Nov. 2, 2023, 4:28 p.m. UTC | #5

Martin Ågren <[e-mail]> writes:

> Maybe you having a similar reaction a second time makes this smell a bit
> more?

Not at all.  I am perfectly OK with --object-*, not --blob-*, as the
end-user facing option name.  I however strongly prefer to see our
log messages record the thought behind the design accurately in
order to help future developers when they wonder what our intention
was back when the commit was created.

In this case, I want to see that we tell our future selves "even
though we named the option 'object', we plan to support blobs and
nothing else, at least for now".

Thanks.

Patch

diff --git a/Documentation/git-merge-file.txt b/Documentation/git-merge-file.txt
index 7e9093fab6..bf0a18cf02 100644
--- a/Documentation/git-merge-file.txt
+++ b/Documentation/git-merge-file.txt
@@ -11,19 +11,20 @@  SYNOPSIS
 [verse]
 'git merge-file' [-L <current-name> [-L <base-name> [-L <other-name>]]]
 	[--ours|--theirs|--union] [-p|--stdout] [-q|--quiet] [--marker-size=<n>]
-	[--[no-]diff3] <current-file> <base-file> <other-file>
+	[--[no-]diff3] <current> <base> <other>
 
 
 DESCRIPTION
 -----------
-'git merge-file' incorporates all changes that lead from the `<base-file>`
-to `<other-file>` into `<current-file>`. The result ordinarily goes into
-`<current-file>`. 'git merge-file' is useful for combining separate changes
-to an original. Suppose `<base-file>` is the original, and both
-`<current-file>` and `<other-file>` are modifications of `<base-file>`,
+Given three files `<current>`, `<base>` and `<other>`,
+'git merge-file' incorporates all changes that lead from `<base>`
+to `<other>` into `<current>`. The result ordinarily goes into
+`<current>`. 'git merge-file' is useful for combining separate changes
+to an original. Suppose `<base>` is the original, and both
+`<current>` and `<other>` are modifications of `<base>`,
 then 'git merge-file' combines both changes.
 
-A conflict occurs if both `<current-file>` and `<other-file>` have changes
+A conflict occurs if both `<current>` and `<other>` have changes
 in a common segment of lines. If a conflict is found, 'git merge-file'
 normally outputs a warning and brackets the conflict with lines containing
 <<<<<<< and >>>>>>> markers. A typical conflict will look like this:
@@ -36,8 +37,8 @@  normally outputs a warning and brackets the conflict with lines containing
 
 If there are conflicts, the user should edit the result and delete one of
 the alternatives.  When `--ours`, `--theirs`, or `--union` option is in effect,
-however, these conflicts are resolved favouring lines from `<current-file>`,
-lines from `<other-file>`, or lines from both respectively.  The length of the
+however, these conflicts are resolved favouring lines from `<current>`,
+lines from `<other>`, or lines from both respectively.  The length of the
 conflict markers can be given with the `--marker-size` option.
 
 The exit value of this program is negative on error, and the number of
@@ -62,7 +63,7 @@  OPTIONS
 
 -p::
 	Send results to standard output instead of overwriting
-	`<current-file>`.
+	`<current>`.
 
 -q::
 	Quiet; do not warn about conflicts.