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

Keep empty directories so that Maven builds without errors. #107

Merged
merged 1 commit into from Dec 8, 2012

Conversation

doctorlard
Copy link

No description provided.

@peterdietz
Copy link
Member

This is much cleaner than Pull Request #105. This empty file acts as a placeholder so that git tracks that there is a directory in place, so that maven can successfully package.

Its probably better that this be committed, than just a have everyone who checks out 1.8.x have to figure out why the build is broken.

A tiny note, I've changed to personally liking the name of the place-holder file be named .gitkeep, as opposed to .gitignore, but I don't think it makes a difference here.

(.gitignore should contain rules for what git should ignore, .gitkeep is invented for the simple purpose of tracking content, and I don't know if git changes its processing rules when it encountered a .gitignore with no rules...)

@helix84
Copy link
Member

helix84 commented Oct 22, 2012

Hi Peter, I'm trying in vain to keep the discussion abou this in one place. There are now 2 pull requests and 1 thread on -devel. The -devel thread has the widest reach, so that's probably the best place. Also, I detailed my concerns there.

@doctorlard
Copy link
Author

@joao-de-melo
Copy link
Member

Accept this pull request* Yes or no? Votes?
My vote: -1

@doctorlard
Copy link
Author

I vote +1 just to make the branch sane when people check it out of git. Who's going to be checking it out of Subversion? Even if they do, they get the empty directories anyway. I don't see a good reason NOT to fix this (it's a bug, after all).

@helix84
Copy link
Member

helix84 commented Dec 7, 2012

The discussions seem to have ceased. For the record, I'm also -1 on this change.

This pull request received 2 -1 votes, from Joao Melo and me and a +1 vote from Peter Dietz. Please note that 1 -1 is enough to refuse the code change.

Jonathan, please don't take offense in this decision, I understand the problem and it's not easy to solve it properly at this time. I hope it won't discourage you from contributing to DSpace in the future.

@helix84 helix84 closed this Dec 7, 2012
@tdonohue
Copy link
Member

tdonohue commented Dec 7, 2012

I'm unclear why folks are voting -1...could we get an explanation on why you feel that this change would be a bad idea?

I honestly think it seems harmless to add these empty directories into the old 1_8_x branch in GitHub. But, maybe I'm missing something?

Obviously though, we would NOT be releasing another 1.8.x version...so, this would only fix things for folks who checkout the 1_8_x branch directly, and wouldn't help anyone who uses the 1.8.2 tag.

@joao-de-melo
Copy link
Member

Tim, my vote resides on your words:

"We cannot fix the Git history now. We announced that the DSpace/DSpace repo on GitHub is official and there have been many forks. If we decided to rewrite the history, all of those forks would have to be thrown away and that's just not viable."

@tdonohue
Copy link
Member

tdonohue commented Dec 7, 2012

Those are helix84's words actually...and I don't think they are accurate in terms of this pull request.

As far as I can tell, this pull request (#107) just ADDS some .gitignore files to the existing DSpace 1_8_x branch. This wouldn't cause a history rewrite -- anyone who had forked us & is using the 1_8_x branch would just get those changes in their next "git pull".

Or at least that is how I understand it? Am I missing something in this pull request? I just don't see how the Git/GitHub history would even be affected by this change.

@tdonohue
Copy link
Member

tdonohue commented Dec 7, 2012

I was just assuming all we'd do here is add these .gitignore files to the existing 1_8_x branch (which is all this Pull Request looks to do). So, there'd be no history rewrite involved. We'd just fix this issue for people who checkout the 1_8_x branch directly. (But, to be clear, we would NOT be rewriting history to correct the 1.8.0, 1.8.1. or 1.8.2 tags -- those tags would all still be missing the empty directories)

@joao-de-melo
Copy link
Member

Oh sorry, i've deleted my own comment unintentionally.
But, if there is no history rewrite, i clearly change my vote to +1 (merge it).
Sorry for the misunderstood.

The previous deleted comment:
I just assumed that the procedure would be to rewrite the whole history, as it was some pre-defined procedure for old releases. If it only requires to merge it into 1_8_x, there's no problem at all.

@tdonohue
Copy link
Member

tdonohue commented Dec 7, 2012

I'll reopen this ticket then, and wait for response from helix84. Need to see if he also is willing to change his -1 vote before we proceed.

@tdonohue tdonohue reopened this Dec 7, 2012
@helix84
Copy link
Member

helix84 commented Dec 7, 2012

It's unfortunate that this conversation is spread in more than one place. Here's my reasoning in the original dspace-devel thread: http://sourceforge.net/mailarchive/message.php?msg_id=29987818

The misunderstood part is probably that this proposed solution is not about rewriting history, but about adding a non-systemic solution (i.e. confusion).

@doctorlard
Copy link
Author

To be honest, I do find this fairly discouraging. First of all, I don't see the harm in using the github discussion interface to discuss a github pull request. I find the email list awful to use I'm afraid - its Sourceforge archives browse interface is worse than useless without any apparent way to search, and the subject headers usually contain [dspace-devel] DuraSpace Jira before I get to the topic which is too long for my email client (okay that's a minor point! :-)

Secondly, I don't understand why we're talking about rewriting history, or how fixing a bug for folks checking out DSpace 1.8 code in git is at all confusing, or why there is a perception that such a change is somehow "non-systematic". If it were, perhaps the system in question needs review.

I'd say the principle of least surprise applies here, but it doesn't really matter, because the point of git and github is that this commit is now published and can be referred to, so people can manually merge it themselves if they want to be able to build DSpace 1.8 successfully, having checked out the 1.8 branch.

Besides, perhaps a better fix would be to have Maven attempt to create the directories if they don't exist.

@helix84
Copy link
Member

helix84 commented Dec 8, 2012

I don't really care where the disussion is as long as it is in one place. The first pull request being a bad merge, it seemed reasonable to discuss it somewhere else (plus extend the audience).

I seem to be the only one left opposing this, but I'm not here to be a jackass, so I'll back out. It doesn't mean I stopped thinking it's a bad idea, but I don't want to be hostile. And it's a rather small issue to make a fuss about. So I'll merge it.

I don't see any advantage in having Maven fix it vs. this solution. Both would solve the problem only from this commit onwards.

I do hope you still feel welcome and I'm looking forward to your future contributions.

helix84 added a commit that referenced this pull request Dec 8, 2012
Keep empty directories so that Maven builds without errors.
@helix84 helix84 merged commit 5816e75 into DSpace:dspace-1_8_x Dec 8, 2012
hardyoyo pushed a commit to hardyoyo/DSpace that referenced this pull request Mar 9, 2018
* Update news-xmlui.xml

Changed paragraph text.

* news-xmlui.xml

Changed paragraph text
hardyoyo pushed a commit to hardyoyo/DSpace that referenced this pull request Nov 28, 2018
* Update news-xmlui.xml

Changed paragraph text.

* news-xmlui.xml

Changed paragraph text
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.

None yet

5 participants