git-apply.txt: correct description of --cached
diff mbox series

Message ID 20200810110338.52203-1-ray@ameretat.dev
State New
Headers show
Series
  • git-apply.txt: correct description of --cached
Related show

Commit Message

Raymond E. Pasco Aug. 10, 2020, 11:03 a.m. UTC
The blurb for "--cached" says it implies "--index", but in reality
"--cached" and "--index" are distinct modes with different behavior.

Remove the sentence "This implies `--index`." to make the description
accurate.

Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
---
 Documentation/git-apply.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Junio C Hamano Aug. 10, 2020, 4:18 p.m. UTC | #1
"Raymond E. Pasco" <ray@ameretat.dev> writes:

> The blurb for "--cached" says it implies "--index", but in reality
> "--cached" and "--index" are distinct modes with different behavior.
>
> Remove the sentence "This implies `--index`." to make the description
> accurate.
>
> Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
> ---
>  Documentation/git-apply.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
> index b9aa39000f..373a9354b5 100644
> --- a/Documentation/git-apply.txt
> +++ b/Documentation/git-apply.txt
> @@ -72,7 +72,7 @@ OPTIONS
>  --cached::
>  	Apply a patch without touching the working tree. Instead take the
>  	cached data, apply the patch, and store the result in the index
> -	without using the working tree. This implies `--index`.
> +	without using the working tree.

The updated text is not wrong per-se, but I have a feeling that this
is barking up a wrong tree.  The implication is probably referring
to the fact that "--index" does certain verification and "--cached"
does the same (i.e. the patch must be applicable to what is in the
index).  We may want to update the description for both options.

How about simplifying them like this, perhaps?

 Documentation/git-apply.txt | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index b9aa39000f..92b5f0ae22 100644
--- a/Documentation/git-apply.txt
+++ b/Documentation/git-apply.txt
@@ -58,21 +58,18 @@ OPTIONS
 --check::
 	Instead of applying the patch, see if the patch is
 	applicable to the current working tree and/or the index
-	file and detects errors.  Turns off "apply".
+	file and detects errors.  Turns off `--apply`.
 
 --index::
-	When `--check` is in effect, or when applying the patch
-	(which is the default when none of the options that
-	disables it is in effect), make sure the patch is
-	applicable to what the current index file records.  If
-	the file to be patched in the working tree is not
-	up to date, it is flagged as an error.  This flag also
-	causes the index file to be updated.
+	Apply the patch to both the contents in the index and in the
+	working tree.  It is an error if the patched file in the
+	working tree is not up to date.
 
 --cached::
-	Apply a patch without touching the working tree. Instead take the
-	cached data, apply the patch, and store the result in the index
-	without using the working tree. This implies `--index`.
+	Apply the patch only to the contents in the index but not to
+	the working tree.  It is OK if the contents in the index
+	and in the working tree are different, as the latter is
+	never looked at.
 
 --intent-to-add::
 	When applying the patch only to the working tree, mark new
Phillip Wood Aug. 12, 2020, 1:32 p.m. UTC | #2
On 10/08/2020 17:18, Junio C Hamano wrote:
> "Raymond E. Pasco" <ray@ameretat.dev> writes:
> 
>> The blurb for "--cached" says it implies "--index", but in reality
>> "--cached" and "--index" are distinct modes with different behavior.
>>
>> Remove the sentence "This implies `--index`." to make the description
>> accurate.
>>
>> Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
>> ---
>>  Documentation/git-apply.txt | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
>> index b9aa39000f..373a9354b5 100644
>> --- a/Documentation/git-apply.txt
>> +++ b/Documentation/git-apply.txt
>> @@ -72,7 +72,7 @@ OPTIONS
>>  --cached::
>>  	Apply a patch without touching the working tree. Instead take the
>>  	cached data, apply the patch, and store the result in the index
>> -	without using the working tree. This implies `--index`.
>> +	without using the working tree.
> 
> The updated text is not wrong per-se, but I have a feeling that this
> is barking up a wrong tree.  The implication is probably referring
> to the fact that "--index" does certain verification and "--cached"
> does the same (i.e. the patch must be applicable to what is in the
> index).  We may want to update the description for both options.
> 
> How about simplifying them like this, perhaps?

I think this is clearer, I've got one comment below

> 
>  Documentation/git-apply.txt | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
> index b9aa39000f..92b5f0ae22 100644
> --- a/Documentation/git-apply.txt
> +++ b/Documentation/git-apply.txt
> @@ -58,21 +58,18 @@ OPTIONS
>  --check::
>  	Instead of applying the patch, see if the patch is
>  	applicable to the current working tree and/or the index
> -	file and detects errors.  Turns off "apply".
> +	file and detects errors.  Turns off `--apply`.
>  
>  --index::
> -	When `--check` is in effect, or when applying the patch
> -	(which is the default when none of the options that
> -	disables it is in effect), make sure the patch is
> -	applicable to what the current index file records.  If
> -	the file to be patched in the working tree is not
> -	up to date, it is flagged as an error.  This flag also
> -	causes the index file to be updated.
> +	Apply the patch to both the contents in the index and in the
> +	working tree.  It is an error if the patched file in the
> +	working tree is not up to date.

I wonder if it would be clearer to say "This option requires the index
entry for the patched file to match the working tree". Saying "if the
patched file in the working tree is not up to date" does not say up to
date with what and one could argue that it is the index that is out of
date rather than the working tree if they don't match.

Best Wishes

Phillip

>  --cached::
> -	Apply a patch without touching the working tree. Instead take the
> -	cached data, apply the patch, and store the result in the index
> -	without using the working tree. This implies `--index`.
> +	Apply the patch only to the contents in the index but not to
> +	the working tree.  It is OK if the contents in the index
> +	and in the working tree are different, as the latter is
> +	never looked at.
>  
>  --intent-to-add::
>  	When applying the patch only to the working tree, mark new
>
Phillip Wood Aug. 12, 2020, 1:59 p.m. UTC | #3
On 10/08/2020 17:18, Junio C Hamano wrote:
> "Raymond E. Pasco" <ray@ameretat.dev> writes:
> 
>> The blurb for "--cached" says it implies "--index", but in reality
>> "--cached" and "--index" are distinct modes with different behavior.
>>
>> Remove the sentence "This implies `--index`." to make the description
>> accurate.
>>
>> Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
>> ---
>>   Documentation/git-apply.txt | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
>> index b9aa39000f..373a9354b5 100644
>> --- a/Documentation/git-apply.txt
>> +++ b/Documentation/git-apply.txt
>> @@ -72,7 +72,7 @@ OPTIONS
>>   --cached::
>>   	Apply a patch without touching the working tree. Instead take the
>>   	cached data, apply the patch, and store the result in the index
>> -	without using the working tree. This implies `--index`.
>> +	without using the working tree.
> 
> The updated text is not wrong per-se, but I have a feeling that this
> is barking up a wrong tree.  The implication is probably referring
> to the fact that "--index" does certain verification and "--cached"
> does the same (i.e. the patch must be applicable to what is in the
> index).  We may want to update the description for both options.
> 
> How about simplifying them like this, perhaps?

I think this is clearer, I've got one comment below

>   Documentation/git-apply.txt | 19 ++++++++-----------
>   1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
> index b9aa39000f..92b5f0ae22 100644
> --- a/Documentation/git-apply.txt
> +++ b/Documentation/git-apply.txt
> @@ -58,21 +58,18 @@ OPTIONS
>   --check::
>   	Instead of applying the patch, see if the patch is
>   	applicable to the current working tree and/or the index
> -	file and detects errors.  Turns off "apply".
> +	file and detects errors.  Turns off `--apply`.
>   
>   --index::
> -	When `--check` is in effect, or when applying the patch
> -	(which is the default when none of the options that
> -	disables it is in effect), make sure the patch is
> -	applicable to what the current index file records.  If
> -	the file to be patched in the working tree is not
> -	up to date, it is flagged as an error.  This flag also
> -	causes the index file to be updated.
> +	Apply the patch to both the contents in the index and in the
> +	working tree.  It is an error if the patched file in the
> +	working tree is not up to date.

I wonder if it would be clearer to say "This option requires the index 
entry for the patched file to match the working tree". Saying "if the 
patched file in the working tree is not up to date" does not say up to 
date with what and one could argue that it is the index that is out of 
date rather than the working tree if they don't match.

Best Wishes

Phillip

>   --cached::
> -	Apply a patch without touching the working tree. Instead take the
> -	cached data, apply the patch, and store the result in the index
> -	without using the working tree. This implies `--index`.
> +	Apply the patch only to the contents in the index but not to
> +	the working tree.  It is OK if the contents in the index
> +	and in the working tree are different, as the latter is
> +	never looked at.
>   
>   --intent-to-add::
>   	When applying the patch only to the working tree, mark new
>
Junio C Hamano Aug. 12, 2020, 7:23 p.m. UTC | #4
Phillip Wood <phillip.wood123@gmail.com> writes:

>>  --index::
>> -	When `--check` is in effect, or when applying the patch
>> -	(which is the default when none of the options that
>> -	disables it is in effect), make sure the patch is
>> -	applicable to what the current index file records.  If
>> -	the file to be patched in the working tree is not
>> -	up to date, it is flagged as an error.  This flag also
>> -	causes the index file to be updated.
>> +	Apply the patch to both the contents in the index and in the
>> +	working tree.  It is an error if the patched file in the
>> +	working tree is not up to date.
>
> I wonder if it would be clearer to say "This option requires the index
> entry for the patched file to match the working tree".

Perhaps.  But "the index entry to match the working tree" leaves the
definition of what is to "match" open to interpretation, so it may
need to be further tightened.

In the olden days, everybody knew "up to date", used to describe the
state of a file in the working tree, is a technical term with a
specific meaning (i.e. the contents has not changed since it was
added to the index, and also cached stat information in the index is
fresh), and that is why the original description used that wording.
But I agree that we should be able to express this without resorting
to a term of art.

    An error is raised if the file in the working tree being patched
    has contents different from what is registered in the index.

captures most of it, but still misses the "cached stat information
also must match" part.
Raymond E. Pasco Aug. 12, 2020, 8:52 p.m. UTC | #5
On Wed Aug 12, 2020 at 3:23 PM EDT, Junio C Hamano wrote:
> An error is raised if the file in the working tree being patched
> has contents different from what is registered in the index.
>
> captures most of it, but still misses the "cached stat information
> also must match" part.

As a point of pedantry, it checks the preimages (don't want docs readers
to be worried it might somehow touch something and *then* error out).

I'll see if I can think of a succinct wording while I'm updating some of
the other patches.

Patch
diff mbox series

diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index b9aa39000f..373a9354b5 100644
--- a/Documentation/git-apply.txt
+++ b/Documentation/git-apply.txt
@@ -72,7 +72,7 @@  OPTIONS
 --cached::
 	Apply a patch without touching the working tree. Instead take the
 	cached data, apply the patch, and store the result in the index
-	without using the working tree. This implies `--index`.
+	without using the working tree.
 
 --intent-to-add::
 	When applying the patch only to the working tree, mark new