Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Replace the confusing --work-dir option by two options (that actually work)#156

Merged
dscho merged 2 commits intogitgitgadget:mastergitgitgadget/gitgitgadget:masterfrom
dscho:git-git-workdirdscho/gitgitgadget:git-git-workdirCopy head branch name to clipboard
Dec 10, 2019
Merged

Replace the confusing --work-dir option by two options (that actually work)#156
dscho merged 2 commits intogitgitgadget:mastergitgitgadget/gitgitgadget:masterfrom
dscho:git-git-workdirdscho/gitgitgadget:git-git-workdirCopy head branch name to clipboard

Conversation

@dscho
Copy link
Member

@dscho dscho commented Nov 22, 2019 •
edited
Loading

--work-dir did not actually do anything.

Besides, we need two options, one to override the current working directory as the one to be used as GitGitGadget worktree, and one to override the config setting in that worktree that points to the git.git working directory.

This PR makes it so. It is based on #165.

@dscho dscho requested a review from webstech November 22, 2019 11:46
@dscho dscho mentioned this pull request Nov 22, 2019
Merged
@dscho dscho mentioned this pull request Nov 22, 2019
Closed
@webstech
Copy link
Contributor

webstech commented Nov 23, 2019

I feel really bad about this. I actually prefer the change you made earlier which did not change the purpose of --work-dir. That would position getCIHelper() to pass commander.workDir as part of the change for #140.

If you prefer this change, I can approve but I suspect the other change was your original intention.

@dscho
Copy link
Member Author

dscho commented Nov 23, 2019

I actually prefer the change you made earlier which did not change the purpose of --work-dir.

From my analysis, it looks as if --work-dir does not actually do anything. Or do you find any effect of passing that option to misc-helper?

@dscho
Copy link
Member Author

dscho commented Nov 24, 2019

I actually prefer the change you made earlier which did not change the purpose of --work-dir.

From my analysis, it looks as if --work-dir does not actually do anything. Or do you find any effect of passing that option to misc-helper?

Argh. I managed to forget about #140, where you suggested to make it do something: to set the GitGitGadget working directory.

So now I understand why we need this working directory: to be able to find res/WELCOME.md.

Given that we need to pass two working directories to CIHelper, the option name --work-dir looks awfully ambiguous and confusing... I think I will want to clarify the purposes by dropping the --work-dir option, and by introducing --gitgitgadget-work-dir and --git-git-work-dir instead.

@webstech what do you think about this idea?

@webstech
Copy link
Contributor

webstech commented Nov 24, 2019

Argh. I managed to forget about #140, where you suggested to make it do something: to set the GitGitGadget working directory.

So now I understand why we need this working directory: to be able to find res/WELCOME.md.

The code is expecting to run in a GitGitGadget working directory. I think that is okay.

Given that we need to pass two working directories to CIHelper, the option name --work-dir looks awfully ambiguous and confusing... I think I will want to clarify the purposes by dropping the --work-dir option, and by introducing --gitgitgadget-work-dir and --git-git-work-dir instead.

What is being passed as --work-dir is really a config location. From that config, the git work dir is extracted and GitGitGadget obtains the SMTP info. CIHelper passes "." as the config location to GitGitGadget at the moment and #140 is intended to change that. So, the clarification is that --work-dir is a git config location. I don't think it is used for anything else.

@webstech webstech mentioned this pull request Dec 9, 2019
Merged
@dscho
ci-helper: clarify attribute name
da63d1c 
The `gggConfig` attribute does not, in fact, point to the config, but to
a directory where to look for the Git config containing the
GitGitGadget-related settings.

Let's reflect that by renaming the attribute so that it says "Dir".

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho dscho changed the title Make --work-dir _actually_ do something Replace the confusing --work-dir option by two options (that actually work) Dec 9, 2019
@dscho
Copy link
Member Author

dscho commented Dec 9, 2019

@webstech I rebased this on top of your PR that I just merged, reworked the patch substantially, and updated the PR's description. Could you have another look, please?

webstech
webstech previously approved these changes Dec 9, 2019
View reviewed changes
Copy link
Contributor

@webstech webstech left a comment

Choose a reason for hiding this comment

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

Thanks. This is much clearer and more usable for testing.

Can you explain the rational for specifying commander.gitgitgadgetWorkDir || "." on line 53 versus defaulting the value in commander? I see similar code in git.ts with workDir?: string that could be workDir = "*". Just trying to understand since I am more inclined to code defaults.

Thanks for clarifying the variable meanings in ci-helper.ts. ;-)

@dscho
misc-helper: replace --work-dir by two separate options
0142722 
First of all, this fixes the bug where passing `--work-dir` to the
script does exactly nothing at all.

Further, the option name was outright confusing: GitGitGadget accesses
_two_ worktrees, the git.git clone (needed to interact with the commits
and the notes stored at https://github.com/gitgitgadget/git) and the
GitGitGadget worktree that is expected to contain a Git config with
further information (and it also contains the `res/WELCOME.md` message
we show new users).

The confusion was pointed out by Chris Webster in
gitgitgadget#148 (comment)

Let's replace the `--work-dir` option by two options:

--git-work-dir:

	This points to the git.git working directory to work with (it
	can actually be a bare clone, it does not require a worktree at
	all).

--gitgitgadget-work-dri:

	This option points to the worktree to use for the `CIHelper`: it
	contains the `res/WELCOME.md` file we show to prospective
	GitGitGadget users, and it also contains the Git config where we
	store settings such as the SMTP configuration for sending the
	patch series to the Git mailing list.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho
Copy link
Member Author

dscho commented Dec 9, 2019

Can you explain the rational for specifying commander.gitgitgadgetWorkDir || "." on line 53 versus defaulting the value in commander? I see similar code in git.ts with workDir?: string that could be workDir = "*".

The explanation is simple: I am not really used to concept of default values in Typescript ;-)

I just fixed the gitgitgadgetGitDir issue.

@dscho dscho merged commit a0deba2 into gitgitgadget:master Dec 10, 2019
@dscho dscho deleted the git-git-workdir branch December 10, 2019 21:51
@dscho
Copy link
Member Author

dscho commented Dec 10, 2019

@webstech thank you for your patient reviews!

@weekly-digest weekly-digest bot mentioned this pull request Dec 15, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@webstech webstech webstech approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dscho @webstech
Morty Proxy This is a proxified and sanitized view of the page, visit original site.