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 commit in gh-publish-only #332

Closed
wants to merge 2 commits into from
Closed

Conversation

abachma2
Copy link
Member

@abachma2 abachma2 commented Feb 16, 2024

When trying to publish changes made in from the last few PRs merged, I got an error message that

git commit -m "Generated master for `git log source -1 --pretty=short --abbrev-commit`" && git push --force upstream master
fatal: ambiguous argument 'source': both revision and filename
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

This change should fix this error, removing the ambiguity in the source branch and the source directory and allow us to publish the changes.

This -- remove abiguity for git between source the branch and source
the directory. This seemed easier than renaming the source directory.
@abachma2 abachma2 requested a review from gonuke February 16, 2024 16:25
@abachma2 abachma2 self-assigned this Feb 16, 2024
@gonuke
Copy link
Member

gonuke commented Feb 16, 2024

source should not be redundant at this point, so we should look at this more carefully. By design (if memory serves correctly), this make target should:

  • fetch the upstream
  • checkout the master branch (which should have no source directory)
  • reset the HEAD (which does something subtle but important - perhaps removing the source dir from the current working dir)
  • copy all the content from the BUILDDIR
  • git add all the copied content
  • delete the BUILDDIR
  • git commit

At the time of commit there should not be any source directory to cause ambiguity. If it is there, it can sometimes be an indication of earlier stages not functioning properly?

@abachma2
Copy link
Member Author

The steps in the Makefile are:

make clean
make html
git fetch $(GH_UPSTREAM_REPO)
git checkout -B $(GH_PUBLISH_BRANCH) $(GH_UPSTREAM_REPO)/$(GH_PUBLISH_BRANCH)
git reset HEAD
rsync -a $(BUILDDIR)/* .
rsync -a $(BUILDDIR)/.* .
git add -f `(cd $(BUILDDIR); find . -type f; cd ..)`
rm -rf  $(BUILDDIR)
git commit -m "Generated $(GH_PUBLISH_BRANCH) for `git log $(GH_SOURCE_BRANCH) -1 --pretty=short --abbrev-commit`" && git push --force $(GH_UPSTREAM_REPO) $(GH_PUBLISH_BRANCH)
	git checkout $(GH_SOURCE_BRANCH)

$(BUILDDIR) is ./gh-build, ${GH_PUBLISH_BRANCH) is master, $(GH_SOURCE_BRANCH) is source, and $(GH_UPSTREAM_REPO) is upstream

So it does do all of the steps you described. The master branch does have a directory names source. I don't remember having issues with this when we last updated the website back in 2021, so maybe this is an update in git?

@gonuke
Copy link
Member

gonuke commented Feb 16, 2024

I don't think it's related to changes in git. I recognize this failure from way back, and I'm not sure how/when the source directory ended up in our master branch. I don't think it's supposed to be there... I'm digging through old commits to try to understand (or figure out why I remember this incorrectly...)

@abachma2
Copy link
Member Author

Have you had time to look into/think about this any?

@gonuke
Copy link
Member

gonuke commented Feb 21, 2024

Have you had time to look into/think about this any?

I tried to figure out if we need/expect the source directory to be there in the final published version of the website. My recollection is that we should not need that directory and that it's not supposed to be there. However, when I look through old commits, that directory has been present for a very long time - I think.

I'm trying to figure out how to confirm this, however, because we force push to main when we publish, so if the source directory was added recently, it may appear to be long standing in the git logs.

I guess the real question is whether anything on the website links into that directory and whether it would break the website if that directory was gone?

@gonuke
Copy link
Member

gonuke commented Feb 21, 2024

I searched a recent local build of the website and didn't find any links that referred to this source folder.

@abachma2
Copy link
Member Author

I can always try to remove the source directory from the main branch, then try to build and publish to see if that fixes it. We shuold be able to undo any changes that does.

@gonuke
Copy link
Member

gonuke commented Feb 21, 2024

I'm pretty sure that deleting source and force pushing the master branch should be safe and consistent with what I thought was the right structure anyway.

@gonuke
Copy link
Member

gonuke commented Feb 21, 2024

P.S. We should rename master to main at some point

@munkm
Copy link

munkm commented Mar 21, 2024

@abachma2 how about we open another PR deleting the source folder from the master branch?

@munkm munkm mentioned this pull request Mar 21, 2024
@abachma2
Copy link
Member Author

I just tried:

  • checking out master branch
  • removing source directory
  • checking out source branch
  • building via make gh-publish
    I still got the fatal error described above, and the source branch reappeared in the master branch.

@gonuke
Copy link
Member

gonuke commented Mar 22, 2024

I just tried:

  • checking out master branch
  • removing source directory
  • checking out source branch
  • building via make gh-publish
    I still got the fatal error described above, and the source branch reappeared in the master branch.

It looks like this changed in #303 and maybe it was not quite correct...

@gonuke
Copy link
Member

gonuke commented Mar 22, 2024

I may have approved it without understanding carefully enough what was going on

@gonuke
Copy link
Member

gonuke commented Mar 22, 2024

See PRs #346 and #345 to repair this to its originally intended state.

@abachma2
Copy link
Member Author

The changes in #345 and #346 fixed what this PR addressed, so this PR is being closed.

@abachma2 abachma2 closed this Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants