Remove `git lfs clone` command from man by gtsiolis · Pull Request #3301 · git-lfs/git-lfs

gtsiolis · 2018-10-04T11:08:17Z

The warning you get when you run git lfs clone is a bit misleading (see 47fa852#r30762174). This will remove the git lfs clone command from man (see 47fa852#r30753241). @PastelMobileSuit could you review this? 🍄

docs/man/git-lfs.1.ronn

PastelMobileSuit

Thanks for putting this together, it looks great! ✨

If you want to remove git lfs checkout as well I'm all for that, as that's also been deprecated! @ttaylorr are you on-board with that change as well?

gtsiolis · 2018-10-04T17:12:12Z

Thanks @PastelMobileSuit! I've also removed git lfs checkout in f5a42ec. @ttaylorr let me know if you're ok with this. Otherwise I can revert the additional change.

ttaylorr · 2018-10-05T01:22:01Z

If you want to remove `git lfs checkout` as well I'm all for that, as that's also been deprecated! @ttaylorr are you on-board with that change as well?

I don't think that we should remove 'git lfs checkout' in the same way that you have removed 'git lfs clone' here. It's known that we won't be adding any new features or supporting 'git lfs clone' in the future, but @bk2204 has just made some improvements, and marked the command as no longer deprecated in 35ba22d (docs: add --to and related options to docs, 2018-10-02). That all said, I think that what you've got here looks great. I think that all of: $ git lfs clone --help $ git lfs help clone $ man git-lfs-clone should still work, and the way that you've made the changes here ensures that that is the case. This'll allow users to learn about the 'git lfs clone' command between now and the time that we remove it from the client (circa v3.0.0), should they need it for any exotic reason. Eventually, we'll remove the clone command entirely (and thusly drop its manpage, too), but I think that this is the right change for now.

ttaylorr · 2018-10-05T01:22:49Z

@ttaylorr let me know if you're ok with this. Otherwise I can revert the adittional change.

Can you please drop this change (instead of reverting it, so as not to add additional commits to the history)? Thank you!

gtsiolis · 2018-10-05T07:59:25Z

@ttaylorr thanks for the meticulous response. Yes, all git lfs clone related commands (including the man git-lfs-clone command) should work as before. Also, I agree to not removing git lfs checkout since it's no longer deprecated. However, should we remove the WARNING message or the whole msg in the following part instead?

git-lfs/commands/command_checkout.go

Lines 38 to 44 in b8f9b11

    
           msg := []string{ 
        
           	"WARNING: 'git lfs checkout' is deprecated and will be removed in v3.0.0.", 
        
           	"'git checkout' has been updated in upstream Git to have comparable speeds", 
        
           	"to 'git lfs checkout'.", 
        
           } 
        
           fmt.Fprintln(os.Stderr, strings.Join(msg, "\n"))

Can you please drop this change (instead of reverting it, so as not to add additional commits to the history)? Thank you!

I shouldn't have used the word revert in this repository because I actually meant to say remove or even better reset --hard. I've dropped the change and updated the pull request title. 😅

ttaylorr · 2018-10-05T14:00:18Z

However, should we remove the `WARNING` message or the whole `msg` in the following part? https://github.com/git-lfs/git-lfs/blob/b8f9b11a2a55289fb77418fef87fa98b04d66510/commands/command_checkout.go#L38-L44

@bk2204 can you take a look at this?

ttaylorr · 2018-10-05T14:04:19Z

@gtsiolis Thanks again for your wonderful contribution!

gtsiolis · 2018-10-05T14:07:00Z

Thanks @ttaylorr @PastelMobileSuit! 🍉

bk2204 · 2018-10-05T14:12:00Z

Sure. I'll put in a pull request to undeprecate it.

gtsiolis · 2018-10-05T14:38:03Z

@bk2204 I've pushed some changes in gtsiolis@a411779. Let me know if you think this is ok to submit a pull request. If this is more complex than this, feel free to take this over.

I've also noticed the git lfs clone warning is also used in a test in t/t-clone-deprecated.sh Maybe remove this file, too? /cc @ttaylorr

bk2204 · 2018-10-05T14:56:45Z

@gtsiolis Sorry, I didn't notice your comment in time. I just put in a pull request with identical changes at #3303.

gtsiolis · 2018-10-05T14:59:48Z

No problem @bk2204! I'm glad we made identical changes.

gtsiolis · 2018-10-05T23:42:20Z

I've also noticed the git lfs clone warning is also used in a test in t/t-clone-deprecated.sh Maybe remove this file, too?

@bk2204 what do you think of this?

bk2204 · 2018-10-09T14:16:38Z

I think as long as the command itself still exists, it would be better to keep the test for the deprecated message. Of course, when we remove the command in v3.0.0, we can remove the test for the deprecated warning.

gtsiolis · 2018-10-09T14:25:10Z

@bk2204 you're right, I completely forgot the command is still there. 😅

bk2204 · 2018-10-09T16:01:28Z

Not a problem. Glad you're thinking about ways to keep things tidy.

mloskot · 2018-11-14T10:23:47Z

Apologies if this is inappropriate to 'hijack' the PR, but I think this is very related.

Given that

git lfs clone has been deprecated
git clone has been improved

is this suggestion from the Tutorial still valid or should it be updated as well?

A clone will typically be slower than optimal because git clone only supports downloading
each file in serial. To achieve a faster parallel batch download, temporaily disable
git lfs then fetch files in batch
GIT_LFS_SKIP_SMUDGE=1 git clone https://github.com/username/my_lfs_repo.git destination_dir

Perhaps, for common users, the recommended way to install-configure-clone is simply to:

git lfs install [--local] --force --skip-smudge
git clone

Then, no more fiddling with git config --global filter.lfs or GIT_LFS_SKIP_SMUDGE=1 is required. Correct?

ttaylorr · 2018-11-14T23:04:06Z

is this suggestion from the Tutorial still valid or should it be updated as well?

Ah, this should certainly be updated.

bk2204 · 2018-11-15T15:05:56Z

I've fixed that text so it's now accurate. Thanks for pointing it out!

mloskot · 2018-11-15T16:10:37Z

Thank you!

Add a number of newer and plumbing commands to the main Git LFS man page, if they were not listed there, except for git-lfs-clone(1) which is deprecated per 67723dd and #3301.

Remove git lfs clone command from man

67723dd

gtsiolis commented Oct 4, 2018

View reviewed changes

docs/man/git-lfs.1.ronn Show resolved Hide resolved

gtsiolis referenced this pull request Oct 4, 2018

docs/man/git-lfs.1.ronn: update casing and missing commands

47fa852

PastelMobileSuit approved these changes Oct 4, 2018

View reviewed changes

gtsiolis changed the title ~~Remove git lfs clone command from man~~ Remove deprecated commands from man Oct 4, 2018

gtsiolis force-pushed the remove-git-lfs-clone-command-from-man branch from f5a42ec to 67723dd Compare October 5, 2018 07:55

gtsiolis changed the title ~~Remove deprecated commands from man~~ Remove git lfs clone command from man Oct 5, 2018

ttaylorr merged commit 20f8acd into git-lfs:master Oct 5, 2018

ttaylorr added this to the v2.6.0 milestone Oct 5, 2018

gtsiolis deleted the remove-git-lfs-clone-command-from-man branch October 5, 2018 14:08

Conversation

gtsiolis commented Oct 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

PastelMobileSuit left a comment

Choose a reason for hiding this comment

Uh oh!

gtsiolis commented Oct 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ttaylorr commented Oct 5, 2018 via email

Uh oh!

ttaylorr commented Oct 5, 2018 via email

Uh oh!

gtsiolis commented Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ttaylorr commented Oct 5, 2018 via email

Uh oh!

ttaylorr commented Oct 5, 2018

Uh oh!

gtsiolis commented Oct 5, 2018

Uh oh!

bk2204 commented Oct 5, 2018

Uh oh!

gtsiolis commented Oct 5, 2018

Uh oh!

bk2204 commented Oct 5, 2018

Uh oh!

gtsiolis commented Oct 5, 2018

Uh oh!

gtsiolis commented Oct 5, 2018

Uh oh!

bk2204 commented Oct 9, 2018

Uh oh!

gtsiolis commented Oct 9, 2018

Uh oh!

bk2204 commented Oct 9, 2018

Uh oh!

mloskot commented Nov 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ttaylorr commented Nov 14, 2018

Uh oh!

bk2204 commented Nov 15, 2018

Uh oh!

mloskot commented Nov 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gtsiolis commented Oct 4, 2018 •

edited

Loading

gtsiolis commented Oct 4, 2018 •

edited

Loading

gtsiolis commented Oct 5, 2018 •

edited

Loading

mloskot commented Nov 14, 2018 •

edited

Loading