Skip to content

Fixed unavailable link in Documentation/howto/new-command.txt along… - #647

Open
kyagi wants to merge 1 commit into
git:masterfrom
kyagi:fix-unavailable-link
Open

Fixed unavailable link in Documentation/howto/new-command.txt along…#647
kyagi wants to merge 1 commit into
git:masterfrom
kyagi:fix-unavailable-link

Conversation

@kyagi

@kyagi kyagi commented Sep 17, 2019

Copy link
Copy Markdown

… with the changeset history.

Signed-off-by: Kazuo Yagi [email protected]

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 ([email protected]) for code submissions, code reviews, and
bug reports. Nevertheless, you can use GitGitGadget (https://gitgitgadget.github.io/)
to conveniently send your Pull Requests commits to our mailing list.

Please read the "guidelines for contributing" linked above!

@kyagi

kyagi commented Sep 17, 2019

Copy link
Copy Markdown
Author

This is a rehash of #430 , on the advice of @dscho

@dscho

dscho commented Sep 17, 2019

Copy link
Copy Markdown
Member

Please 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. It would be good to spend at least one paragraph on explaining the rationale, rather than a rather terse oneliner. Further, the first line of the commit message should be a short summary and use the imperative ("docs: fix the link to api-builtin" or something like that).

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

@dscho

dscho commented Oct 9, 2019

Copy link
Copy Markdown
Member

@kyagi gentle ping?

@dscho

dscho commented Oct 22, 2019

Copy link
Copy Markdown
Member

@kyagi another gentle ping?

@kyagi

kyagi commented Oct 23, 2019

Copy link
Copy Markdown
Author

I'm at a loss as to what to do...

@dscho

dscho commented Oct 23, 2019

Copy link
Copy Markdown
Member

I'm at a loss as to what to do...

I tried to point out here what I think needs to be done...

@dscho

dscho commented Nov 8, 2019

Copy link
Copy Markdown
Member

Column per line? You mean char per line?

No. There are many Unicode characters out there which occupy more than one column.

@dscho

dscho commented Nov 22, 2019

Copy link
Copy Markdown
Member

@kyagi so... does #647 (comment) not clarify what I think you will want to do?

@dscho

dscho commented Dec 9, 2019

Copy link
Copy Markdown
Member

@kyagi is it really unclear what needs to be done?

The first line of the commit message should look like this:

docs: fix the link to api-builtin

And the commit message should not focus on what the patch does, the diff is really eloquent enough about this. A much more interesting aspect that is totally unanswered in this entire PR, and that would be a fine question to answer in the commit message, is why this is a change you (and really, the Git developers) want. This needs to be answered. Currently, there is no answer to this question.

If you need inspiration for good commit messages, just have a look at Git's commit history, it is full of excellent examples to follow.

@dscho

dscho commented Jan 25, 2020

Copy link
Copy Markdown
Member

@kyagi gentle ping?

@abhishekkumar2718

Copy link
Copy Markdown
Contributor

@dscho

Can I take over this PR? Lost half hour figuring where did the missing api-builtin.txt go!

@dscho

dscho commented Feb 26, 2020

Copy link
Copy Markdown
Member

@abhishekkumar2718 you will have to ask @kyagi, not me...

@abhishekkumar2718

abhishekkumar2718 commented Feb 27, 2020

Copy link
Copy Markdown
Contributor

@kyagi Can I work on this or help you out with the patch?

I'm at a loss as to what to do...

Beginning with what Johannes said, ensure the commit message focuses on why? We need to make changes as described in this commit because documentation was moved from api-builtin.txt to builtin.h.

Further, the first line of the commit message should be a short summary and use the imperative.

First line of commit message is the title of commit and briefly explains the changes within. It should additionally be in the imperative mood since we are saying if applied, this commit will <title>.

The sentence if applied, this commit will fixed unavailable link ... history does not make sense but if applied, this commit will fix the link to api-builtin makes perfect sense.

So, overall your commit message might look like:

docs: fix the link to api-builtin

ec14d4e moved documentation from api-builtin.txt to builtin.h. This
patch updates new-command.txt to reflect this change.

Notice how I have wrapped at 72 columns. This helps people who use old-style 80 columns per line terminals be able to read the entire message.

Nitpick for Johannes, it's better to wrap at 72 columns instead of 76 due to the right-hand margin. 1

@dscho

dscho commented Feb 27, 2020

Copy link
Copy Markdown
Member

Nitpick for Johannes, it's better to wrap at 72 columns instead of 76 due to the right-hand margin. 1

It is better, but not required. If you care to look at my commit messages, you will see that I do use the 72 columns rule myself.

As to going forward: if @kyagi does not reply, or does not want to update this PR, you could just send an alternative patch yourself. In that case, I would suggest linking to this here PR for reference.

@kyagi

kyagi commented Mar 2, 2020

Copy link
Copy Markdown
Author

@abhishekkumar2718 It's my pleasure to have you work on this patch. Thank you.

@kyagi

kyagi commented Mar 2, 2020

Copy link
Copy Markdown
Author

@abhishekkumar2718 Or you can send an alternative patch as Johannes said. It's up to you.

@kyagi
kyagi force-pushed the fix-unavailable-link branch from e86e787 to f9119fb Compare March 2, 2020 16:09
@kyagi

kyagi commented Mar 2, 2020

Copy link
Copy Markdown
Author

Let me hang on a minute. How about this commit message?

documentation: fix the stale link to api-builtin.txt

Users can not find the specified path which has become unavailable
through the past changesets.

@abhishekkumar2718

Copy link
Copy Markdown
Contributor

documentation: fix the stale link to api-builtin.txt

It's longer than 50 characters. Maybe replace 'documentation' with 'doc'.

Users can not find the specified path which has become unavailable
through the past changesets.

'... past changesets.' is a bit vague.

It would be helpful if you were more specific like the commit hash or title.

Reviewers save time since they don't have to refer back to your PR here to find out which changesets to be exact.

@kyagi

kyagi commented Mar 4, 2020

Copy link
Copy Markdown
Author

I wonder if it needs to be specific. IMHO, commit hashes does not necessarily matter in this case because diff is convincing enough. Rather, a series of SHA-1 hashes of the old change sets might make it difficult for readers to understand the purpose of this change. @dscho What do you think?

@dscho

dscho commented Mar 4, 2020

Copy link
Copy Markdown
Member

I think that @abhishekkumar2718's advice is pretty sound.

@kyagi
kyagi force-pushed the fix-unavailable-link branch from f9119fb to 9536c65 Compare March 7, 2020 05:39
@dscho

dscho commented Apr 13, 2020

Copy link
Copy Markdown
Member

@kyagi looks good. Wanna /submit?

@kyagi

kyagi commented Apr 14, 2020

Copy link
Copy Markdown
Author

/submit

@gitgitgadget-git

Copy link
Copy Markdown

Error: User kyagi is not permitted to use GitGitGadget

@kyagi

kyagi commented Apr 14, 2020

Copy link
Copy Markdown
Author

Yes, please.

@dscho

dscho commented Apr 14, 2020

Copy link
Copy Markdown
Member

/allow

@gitgitgadget-git

Copy link
Copy Markdown

User kyagi is now allowed to use GitGitGadget.

@kyagi

kyagi commented Apr 14, 2020

Copy link
Copy Markdown
Author

/submit

@gitgitgadget-git

Copy link
Copy Markdown

Submitted as [email protected]

@gitgitgadget-git

Copy link
Copy Markdown

On the Git mailing list, Jonathan Nieder wrote (reply to this):

(+Emily Shaffer, author of the related MyFirstContribution tutorial)
Hi,

Kazuo Yagi wrote:

> From: Kazuo Yagi <[email protected]>
> Subject: doc: fix the stale link to api-builtin.txt
>
> ec14d4e had moved documentation from api-builtin.txt to builtin.h.

Micronits:
- use "git log -1 --format=reference"
- changes due to previous commits go in the simple past tense, like
  "Commit ec14d4e moved [...]"

> This patch updates new-command.txt to reflect that change.

nit: describe what your patch does as though you are giving orders
to the codebase to change. Focus on what benefit you are trying to
bring about.

> Signed-off-by: Kazuo Yagi <[email protected]>
> ---
[...]
> along with the changeset history.

This part didn't make it into the commit message, but I think you
intended it to do so.  Putting that all together would make

	new-command doc: fix stale link to api-builtin.txt

	ec14d4ecb55 (builtin.h: take over documentation from api-builtin.txt,
	2017-08-02) moved the documentation for Git's builtin API to the
	header file. Update the "new command" how-to doc to reflect that
	change.

	While we're here, add a historical note to builtin.h.

[...]
> --- a/Documentation/howto/new-command.txt
> +++ b/Documentation/howto/new-command.txt
> @@ -1,13 +1,13 @@
>  From: Eric S. Raymond <[email protected]>
>  Abstract: This is how-to documentation for people who want to add extension
> - commands to Git.  It should be read alongside api-builtin.txt.
> + commands to Git.  It should be read alongside builtin.h.

Makes sense.

[...]
>  How to integrate new subcommands
>  ================================
>  
>  This is how-to documentation for people who want to add extension
> -commands to Git.  It should be read alongside api-builtin.txt.
> +commands to Git.  It should be read alongside builtin.h.

Likewise.  It's probably worth pointing to Documentation/MyFirstContribution
as well, which is a tutorial covering this subject in more detail.

(Actually, would it make sense to incorporate the information from
howto/new-command.txt into that page and then to retire the old
new-command doc?)

[...]
> @@ -48,7 +48,7 @@ binary); this organization makes it easy for people reading the code
>  to find things.
>  
>  See the CodingGuidelines document for other guidance on what we consider
> -good practice in C and shell, and api-builtin.txt for the support
> +good practice in C and shell, and builtin.h for the support
>  functions available to built-in commands written in C.

Most support functions are part of other APIs, so this was a strange
pointer in the first place.  But the change makes sense.

It's also a bit odd that this doc doesn't mention SubmittingPatches.

[...]
> --- a/builtin.h
> +++ b/builtin.h
> @@ -92,6 +92,31 @@
>   *
>   * The return value from `cmd_foo()` becomes the exit status of the
>   * command.
> + *
> + * Changeset History
> + * -----------------
> + *
> + * The following describes how the documentation has finally been placed
> + * in this file, over the related changesets.

*puzzled* Why is this information being added to the builtin.h file?
What is the reader trying to do when they read it?

Thanks and hope that helps,
Jonathan

> + *
> + * +-----------------+ *OLD LINK*  +-----------------+
> + * | api-builtin.txt | <~~~~~~~~~~ | api-command.txt |
> + * +-----------------+             +-----------------+
> + *    |                               ~  *  |
> + *    | deleted,                     ~  N   | moved and renamed from
> + *    | contents is taken over      ~  E    | Documentation/technical/
> + *    | by builtin.h               ~  W     | to
> + *    | (this file)               ~         | Documentation/howto/
> + *    |                          ~ L        |
> + *    |                         ~ I         |
> + *    v                        ~ N          v
> + * +-----------+              ~ K  +-----------------+
> + * | builtin.h | <~~~~~~~~~~~~ *   | new-command.txt |
> + * +-----------+                   +-----------------+
> + *
> + * ---> moved to(or renamed to)
> + * ~~~> refers to
> + *
>   */
>  
>  #define DEFAULT_MERGE_LOG_LEN 20

@gitgitgadget-git

Copy link
Copy Markdown

On the Git mailing list, Junio C Hamano wrote (reply to this):

Jonathan Nieder <[email protected]> writes:

>> + * Changeset History
>> + * -----------------
>> + *
>> + * The following describes how the documentation has finally been placed
>> + * in this file, over the related changesets.
>
> *puzzled* Why is this information being added to the builtin.h file?
> What is the reader trying to do when they read it?

Thanks for an excellent review, but you are being a bit too subtle
and/or diplomatic here.

A good rule of thumb to use when judging if a comment is appropriate
to have in the tracked data is if it talks about what used to be the
case in order to explain why it is in the current shape.  Often,
such description is useless for people going forward starting from
the current codebase, and is better described in the log message,
and I think that this is a prime example.  As an explanation to
justify why it is good to refer to builtin.h from the current
documentation that teaches what needs to be done to add a new
command, instead of api-builtin.txt, it is valuable to know how the
description of the API used to support builtin commands moved over
time from place to place (preferrably with references to the commits
that did so), and it belongs to the log message of this commit that
updates the reference to api-builtin.txt to builtin.h.

@gitgitgadget-git

Copy link
Copy Markdown

There is an issue in commit e18d57845e6745fc4a31b4e03fb0929f7fba02e6:
Commit not signed off

@kyagi
kyagi force-pushed the fix-unavailable-link branch 3 times, most recently from 92520b8 to 51a67b9 Compare May 20, 2020 16:10
ec14d4e had moved documentation from api-builtin.txt to builtin.h.
This patch updates new-command.txt to reflect that change.

The following describes how the documentation has finally been placed
in this file, over the related changesets.

+-----------------+ *OLD LINK*  +-----------------+
| api-builtin.txt | <~~~~~~~~~~ | api-command.txt |
+-----------------+             +-----------------+
   |                               ~  *  |
   | deleted,                     ~  N   | moved and renamed from
   | contents is taken over      ~  E    | Documentation/technical/
   | by builtin.h               ~  W     | to
   | (this file)               ~         | Documentation/howto/
   |                          ~ L        |
   |                         ~ I         |
   v                        ~ N          v
+-----------+              ~ K  +-----------------+
| builtin.h | <~~~~~~~~~~~~ *   | new-command.txt |
+-----------+                   +-----------------+

---> moved to(or renamed to)
~~~> refers to

Signed-off-by: Kazuo Yagi <[email protected]>
@kyagi
kyagi force-pushed the fix-unavailable-link branch from 51a67b9 to 0a863e6 Compare May 20, 2020 16:13
derrickstolee pushed a commit to derrickstolee/git that referenced this pull request May 31, 2024
I noticed the `osx-gcc` job failing in git#647 so I found this upstream fix
from @peff. Merging now to unblock PR builds in `microsoft/git`.
derrickstolee pushed a commit to derrickstolee/git that referenced this pull request Jun 19, 2024
I noticed the `osx-gcc` job failing in git#647 so I found this upstream fix
from @peff. Merging now to unblock PR builds in `microsoft/git`.
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