Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for the git-diff manual #580

Closed
wants to merge 2 commits into from
Closed

Conversation

Splarv
Copy link

@Splarv Splarv commented Mar 1, 2019

Git diff can work with a tree in the form git diff tree..tree too, only
the form git diff commit...commit can't accept a tree instead of a commit.

Also added usefull example about using a tree with git diff.

Thanks for taking the time to contribute to Git! Please be advised that the
Git community does not use github.com for their contributions. Instead, we use
a mailing list (git@vger.kernel.org) for code submissions, code reviews, and
bug reports. Nevertheless, you can use submitGit to conveniently send your Pull
Requests commits to our mailing list.

Please read the "guidelines for contributing" linked above!

Splarv and others added 2 commits March 1, 2019 17:10
Git diff can work with a tree in the form git diff tree..tree too, only
the form git diff commit...commit can't accept a tree instead of a commit.

Also added usefull example about using a tree with git diff.
@@ -81,7 +81,7 @@ in the last form, that use the "\..." notation, can be any
<tree>. For instance, if you want to make diffs against an empty
tree, you can create a tag pointing to the empty tree:

'git tag' empty 4b825dc642cb6eb9a060e54bf8d69288fbee4904
'git tag' empty $('git hash-object' -t tree /dev/null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of see this as a pointless change, at least for two reasons. When we move away from the SHA-1 hash and adopt a different one, it is likely that we'd still understand an old-style object, like an empty tree with known object name, so it would be plausible that the given example would keep working after the transition. Also, the proposed change makes an assumption that the object payload for an empty tree will stay to be a zero-length string when we move away from SHA-1 hash, which is not warranted. The time we have to break backward compatibility is when we would make other changes, too.

<tree>.
in the last form, that use the "\..." notation, can be any
<tree>. For instance, if you want to make diffs against an empty
tree, you can create a tag pointing to the empty tree:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd stop at fixing the original that implied ".." cannot be used to say "..." cannot, and would not add the empty-tree example at all.

While comparing with an empty tree and making it handy to do so with a tag is not technically incorrect per-se, checking a working tree for whitespace breakages by comparing everything with an empty tree is something we definitely do not want to encourage.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, what form is use to create empty tree is just a matter of taste. I also more like the first form.

What about the empty tree example. I added this for two reason. First, I can't think of another reason to use a tree-ish with git diff. Second, the diff with empty tree is a real problem on what some people looking for solution (for various reasons):
https://stackoverflow.com/questions/14564034/creating-a-git-diff-from-nothing/54946189#54946189
https://stackoverflow.com/questions/9765453/is-gits-semi-secret-empty-tree-object-reliable-and-why-is-there-not-a-symbolic/54945479#54945479
And the solutions are not simple nor obvious. So I think it will be helpful to mention it.

But well, if you get it only without empty tree example, I'll agree. What will be better: create new pull request or rebase this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will be better: create new pull request or rebase this?

First of all, Git's contributions are not accepted via PRs (instead it wants the patches to be sent to the Git mailing list)... but there is a way to use PRs to generate these patches.

So what you want to do is to rebase this to master, adjust according to the reviewers comments (if you disagree with them, you will have to find ways to convince them, preferably in the form of good commit messages).

It would be furthermore a good idea to change the format of the first line of the commit message to the suggested format (see https://git-scm.com/docs/SubmittingPatches#describe-changes): start with an area prefix (in your case, probably "user manual: "), keep it to 50 characters if possible, use the imperative form and do not use a full stop at the end of the first line.

Please also make sure that your commit message focuses on answering the question "why?" more than on "how?", and that it wraps at <= 76 columns per line. Also do make sure to add your sign off.

Finally send the patch to the mailing list for review. You can use GitGitGadget, submitGit or send it manually.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be furthermore a good idea to change the format of the first line of the commit message to the suggested format (see https://git-scm.com/docs/SubmittingPatches#describe-changes): start with an area prefix (in your case, probably "user manual: "), keep it to 50 characters if possible, use the imperative form and do not use a full stop at the end of the first line.

Please also make sure that your commit message focuses on answering the question "why?" more than on "how?", and that it wraps at <= 76 columns per line. Also do make sure to add your sign off.

I can't undestand. The patch is just path, here it is:

--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -77,8 +77,16 @@ two blob objects, or changes between two files on disk.

 Just in case you are doing something exotic, it should be
 noted that all of the <commit> in the above description, except
-in the last two forms that use ".." notations, can be any
-<tree>.
+in the last form, that use the "\..." notation, can be any
+<tree>. For instance, if you want to make diffs against an empty
+tree, you can create a tag pointing to the empty tree:
+
+'git tag' empty $('git hash-object' -t tree /dev/null)
+
+And use it for the 'git diff', for instance, to check a working tree
+for whitespaces:
+
+'git diff' --check empty

 For a more complete list of ways to spell <commit>, see
 "SPECIFYING REVISIONS" section in linkgit:gitrevisions[7].

Where is here a commit message?

@dscho
Copy link
Member

dscho commented Jul 4, 2019

@Splarv gentle ping?

@Splarv
Copy link
Author

Splarv commented Jul 4, 2019

@Splarv gentle ping?

Yep. I am busy now by my job, I'll do recommended steps later.

@dscho
Copy link
Member

dscho commented Sep 23, 2019

@Splarv maybe now is a better time than in July?

@dscho
Copy link
Member

dscho commented Oct 10, 2019

@Splarv or maybe now? Or maybe you are no longer interested in this patch? If so, please close the PR.

@Splarv
Copy link
Author

Splarv commented Oct 10, 2019

@Splarv or maybe now? Or maybe you are no longer interested in this patch? If so, please close the PR.

Yep, may be in a weekend.

@dscho
Copy link
Member

dscho commented Oct 25, 2019

@Splarv or maybe now? Or maybe you are no longer interested in this patch? If so, please close the PR.

Yep, may be in a weekend.

Or maybe we can re-read https://github.com/git/git/pull/580/files#r262316324 and concur that the change is not actually a good idea?

@Splarv
Copy link
Author

Splarv commented Oct 30, 2019

Or maybe we can re-read https://github.com/git/git/pull/580/files#r262316324 and concur that the change is not actually a good idea?

Disagree. The problem is and the only solution is to dig on stackoverflow. So, IMHO, it is worth be mentioned in documentation.

@dscho
Copy link
Member

dscho commented Oct 30, 2019

it is worth be mentioned in documentation.

Worth enough to address the problems I pointed out? (The commit messages still lack Signed-off-by: footers, for starters, and in particular one of the commits has a seriously lacking commit message...).

@Splarv
Copy link
Author

Splarv commented Oct 30, 2019

I sent this patch to the mailing list, as you directed me.

@Splarv Splarv closed this Oct 30, 2019
@dscho
Copy link
Member

dscho commented Oct 30, 2019

I sent this patch to the mailing list, as you directed me.

No you did not. Absolutely not. I mentioned at least twice that you need to add your sign-off, yet look at this:

https://public-inbox.org/git/232BC77E-F22E-469A-8839-6A1553A75B55@ya.ru/

It does not have a Signed-off-by: line!

It also does not wrap the commit message at <=76 columns per line, it does not follow the convention of starting the commit subject with an area (docs(diff): mention empty trees and --check would have been a substantial improvement), the there must be "..." instead of "..". part of the commit message is totally devoid of any supporting argument, and the commit message never talks about any concrete problem the patch tries to solve, let alone about any compelling reason why it needs to be fixed.

All of these issues could have been addressed very easily, prior to submission to the Git mailing list. That was the reason why I spent so much time trying to help you, but I can only offer assistance, there also has to be a willingness to accept it, and to act on it.

@dscho
Copy link
Member

dscho commented Oct 30, 2019

I sent this patch to the mailing list, as you directed me.

No you did not. Absolutely not.

@Splarv to clarify: you did not send it as I directed you. I tried to direct you to fixing the issues first, before sending anything to the mailing list.

@Splarv
Copy link
Author

Splarv commented Oct 31, 2019

I think I still can't understand what are you talking about. A "patch" is a simple result from diff or git diff. There are not a commit message or electronic sign. As you said

First of all, Git's contributions are not accepted via PRs (instead it wants the patches to be sent to the Git mailing list)...

And so I made git diff and sent this patch to the mailing list.

@dscho
Copy link
Member

dscho commented Oct 31, 2019

A "patch" is a simple result from diff or git diff.

Git does not accept patches without commit messages. See https://git-scm.com/docs/SubmittingPatches#describe-changes and the sections following that one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants