Skip to content

Implement some missing string functions #26

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

Closed
wants to merge 5 commits into from

Conversation

th-otto
Copy link
Contributor

@th-otto th-otto commented Oct 15, 2017

No description provided.

@mikrosk
Copy link
Member

mikrosk commented Oct 15, 2017

Are you sure you have intended to base it on all of that PR merges and unrelated commits? A PR should be as small as possible, ideally rebased against master. If it's based on some other PR (for a good reason), noting something like "please merge PR #XY first" is enough.

Among others, there's commit 143c212 which was explicitly rejected by @atic-atac in #15

@th-otto
Copy link
Contributor Author

th-otto commented Oct 15, 2017 via email

@mikrosk
Copy link
Member

mikrosk commented Oct 15, 2017

What I do before applying any PR is that I switch to master, pull, create a branch and start committing. For another PR I basically repeat the same process.

If/when I screw up, it's still very similar -- usually the mistake is to have master --> some branch --> maybe some other branch --> my code. So the most dumb-proof solution is just to switch to master, create something like branch_name_new, cherry pick given commits, switch to branch_name and reset its tip to branch_name_new's tip and force push. That way the PR gets fixed without anyone knowing. ;-)

@th-otto
Copy link
Contributor Author

th-otto commented Oct 15, 2017 via email

@mikrosk
Copy link
Member

mikrosk commented Oct 15, 2017

And you can only do a forced push as long as nobody else has already pulled the new objects.

But that doesn't matter -- it's your PR, your branch, your code. You can rebase it ten times and will be still OK because the reviewer get the look on your latest changes, not the mess which led there.

So only problem is how to get rid of those commits as part of this PRs, otherwise its not very obvious what is new and what not.

See above, feel free to rewrite your branch's history, totally fine.

@th-otto
Copy link
Contributor Author

th-otto commented Oct 16, 2017

Hmpf. That did not work. Now the commits are empty. New try.

@th-otto
Copy link
Contributor Author

th-otto commented Oct 16, 2017

Ok, that looks better ;)

@mikrosk
Copy link
Member

mikrosk commented Oct 16, 2017

But still not there. :) For instance the PR-4 merge commit drags in the time zone stuff again. What do you want to have is a history in your PR-stringfuncs branch like this:

b9040d2
1de0b34
3a0a845 (master's tip)

Nothing else (unless there's a good reason to base it on some other PR).

@th-otto
Copy link
Contributor Author

th-otto commented Oct 16, 2017

hm but i now have only
b9040d2
1de0b34
on that branch:

git rev-list --left-right master...PR-stringfuncs

b9040d2
1de0b34

@mikrosk
Copy link
Member

mikrosk commented Oct 16, 2017

... and did you push it? ;)
th-otto

@th-otto
Copy link
Contributor Author

th-otto commented Oct 16, 2017

$git status
Your branch is up-to-date with 'origin/PR-stringfuncs'.

They were already declared in wchar.h, but never implemented
only and conflicts with recent versions of bfd
Some packages (including gcc) fail to compile with
error: invalid conversion from 'int (*)(const char*, const stat*, int)' to 'int (*)(const char*, stat*, int)'
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.

2 participants