Skip to content

Add git dashes separator to some "log" and "diff" commands#23606

Merged
zeripath merged 2 commits into
go-gitea:mainfrom
wxiaoguang:fix-git-cmd-dash
Mar 26, 2023
Merged

Add git dashes separator to some "log" and "diff" commands#23606
zeripath merged 2 commits into
go-gitea:mainfrom
wxiaoguang:fix-git-cmd-dash

Conversation

@wxiaoguang

@wxiaoguang wxiaoguang commented Mar 21, 2023

Copy link
Copy Markdown
Contributor

Reference: #22578 (comment)

Credits to @tdesveaux , thank you very much for catching the problem. If you'd like to open a PR, feel free to replace this one.

Git reports fatal errors for ambiguous arguments:

fatal: ambiguous argument 'refs/a...refs/b': unknown revision or path not in the working tree.
        Use '--' to separate paths from revisions, like this:
        'git <command> [<revision>...] -- [<file>...]'

So the -- separator is necessary in some cases.

@wxiaoguang

wxiaoguang commented Mar 21, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Hmm, sorry for that I misread a message, wait for the author first.

@wxiaoguang wxiaoguang closed this Mar 21, 2023
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 21, 2023
@tdesveaux

tdesveaux commented Mar 21, 2023

Copy link
Copy Markdown
Contributor

I don't mind at all that you created the PR! You even added comments, so that's always good.
Just a question as to why you used .AddArguments("--") instead of .AddDashesAndList()

@wxiaoguang

wxiaoguang commented Mar 22, 2023

Copy link
Copy Markdown
Contributor Author

Thank you very much. Then I would re-open this PR.

why you used .AddArguments("--") instead of .AddDashesAndList()

I guess AddArguments("--") looks clearer and matches the comment (which also contains --), actually either is fine.

In history, we ever removed some "really redundant" dashes, like #22756, remove the dashes , at that time, the removal was correct (git check-attr --stdin [-z] [-a | --all | <attr>...]).

So I think if we need to add dashes to git command, we could make them more explicit to help future developers.

@wxiaoguang wxiaoguang reopened this Mar 22, 2023
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 22, 2023
@lunny lunny added the type/bug label Mar 22, 2023
@lunny lunny added this to the 1.20.0 milestone Mar 22, 2023
@silverwind silverwind added the outdated/backport/v1.19 This PR should be backported to Gitea 1.19 label Mar 22, 2023
zeripath
zeripath approved these changes Mar 26, 2023
View reviewed changes

@zeripath zeripath left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are there other places where these -- have been removed in the recent refactorings?

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 26, 2023
@zeripath zeripath merged commit 0df81b9 into go-gitea:main Mar 26, 2023
@GiteaBot GiteaBot mentioned this pull request Mar 26, 2023
Merged
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Mar 26, 2023
@wxiaoguang @GiteaBot
Add git dashes separator to some "log" and "diff" commands (go-gitea#…
442bcd5 
…23606)

Reference:
go-gitea#22578 (comment)

Credits to @tdesveaux , thank you very much for catching the problem. If
you'd like to open a PR, feel free to replace this one.

Git reports fatal errors for ambiguous arguments:

```
fatal: ambiguous argument 'refs/a...refs/b': unknown revision or path not in the working tree.
        Use '--' to separate paths from revisions, like this:
        'git <command> [<revision>...] -- [<file>...]'
```

So the `--` separator is necessary in some cases.
@GiteaBot GiteaBot added the backport/done All backports for this PR have been created label Mar 26, 2023
@wxiaoguang

wxiaoguang commented Mar 26, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Are there other places where these -- have been removed in the recent refactorings?

No, the only one is that git check-attr --stdin [-z] [-a | --all | <attr>...], which I have confirmed.

This PR related old code was the same as before. I didn't change logic during refactoring.

Hmm, sorry that I found that it might be a regression, so I will re-check #21535 (update: not a regression)

@wxiaoguang wxiaoguang deleted the fix-git-cmd-dash branch March 26, 2023 18:38
@wxiaoguang

wxiaoguang commented Mar 26, 2023

Copy link
Copy Markdown
Contributor Author

OK, double confirmed, not a regression. The code was that long ago:

See the old GetCompareInfo and GetDiffNumChangedFiles

https://github.com/wxiaoguang/gitea/blame/bdfd751af88c5bdb70dbdfb4f7f607f6fbf77896/modules/git/repo_compare.go#L90

https://github.com/wxiaoguang/gitea/blame/bdfd751af88c5bdb70dbdfb4f7f607f6fbf77896/modules/git/repo_compare.go#L140

zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 27, 2023
@zjjhot
Merge remote-tracking branch 'upstream/main'
5bd354a 
* upstream/main: (23 commits)
  Fix project card preview select and template select (go-gitea#23684)
  [skip ci] Updated translations via Crowdin
  Add git dashes separator to some "log" and "diff" commands (go-gitea#23606)
  Add Simplified Chinese translate for oauth2-provider (go-gitea#23713)
  Fix incorrect `toggle` buttons (go-gitea#23676)
  Fine tune more downdrop settings, use SVG for labels, improve Repo Topic Edit form (go-gitea#23626)
  Allow new file and edit file preview if it has editable extension (go-gitea#23624)
  [skip ci] Updated translations via Crowdin
  Clean some legacy files and move some build files (go-gitea#23699)
  Remove row clicking from notification table (go-gitea#22695)
  Describe Gitea's purpose more accurately (go-gitea#23698)
  [skip ci] Updated translations via Crowdin
  ensure go/bin path exists when copying hugo bin into it (go-gitea#23692)
  Create commit status when event is `pull_request_sync` (go-gitea#23683)
  Add `deps-docs` command to makefile (go-gitea#23686)
  Fix incorrect package doc link (go-gitea#23679)
  Improve indices for `action` table (go-gitea#23532)
  Clarify that Gitea requires JavaScript (go-gitea#23677)
  Use data-tooltip-content for tippy tooltip (go-gitea#23649)
  Add aria attributes to interactive time tooltips. (go-gitea#23661)
  ...
6543 pushed a commit that referenced this pull request Apr 4, 2023
@GiteaBot @wxiaoguang
Add git dashes separator to some "log" and "diff" commands (#23606) (#…
d752f0d 
…23720)

Backport #23606 by @wxiaoguang

Reference:
#22578 (comment)

Credits to @tdesveaux , thank you very much for catching the problem. If
you'd like to open a PR, feel free to replace this one.

Git reports fatal errors for ambiguous arguments:

```
fatal: ambiguous argument 'refs/a...refs/b': unknown revision or path not in the working tree.
        Use '--' to separate paths from revisions, like this:
        'git <command> [<revision>...] -- [<file>...]'
```

So the `--` separator is necessary in some cases.

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@lunny lunny lunny approved these changes

+1 more reviewer

@zeripath zeripath zeripath approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. outdated/backport/v1.19 This PR should be backported to Gitea 1.19 type/bug

Projects

None yet

Milestone

1.20.0

Development

Successfully merging this pull request may close these issues.

6 participants

@wxiaoguang @tdesveaux @lunny @zeripath @silverwind @GiteaBot