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

elogv: accept PORTAGE_LOGDIR #21

Closed
wants to merge 1 commit into from

Conversation

parona-source
Copy link

https://bugs.gentoo.org/901961
https://bugs.gentoo.org/668538

Unsure how to attribute this one.

(The patch is created from Ninpo,
with help from [Arfrever] and sam_)

Manually applied and tested it works.

@thesamesam
Copy link
Member

Why no Bug: or Closes: tags?

As for attribution: Thanks-to, I suppose.

@thesamesam
Copy link
Member

thesamesam commented Oct 6, 2023

Also, @sping, please merge w/ rebase rather than merge commits (see https://www.gentoo.org/glep/glep-0066.html#id6, we broadly apply this to other repos as well). I just usually pick up PRs with pram but you can do it with normal git fetch and such too.

Thank you both for giving some love to elogv!

Copy link
Member

@hartwork hartwork left a comment

Choose a reason for hiding this comment

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

@parona-source thanks for this! One small thing:

log_dir seems like a suboptimal variable name in this context since it's a name/key/attribute to look up, not a directory. Almost any other name is likely better. Maybe env_var or logdir_var or var_name or …? Would be great, thanks! 🙏

@parona-source parona-source force-pushed the portage_logdir branch 2 times, most recently from 6642570 to 641ff32 Compare October 6, 2023 20:26
@parona-source
Copy link
Author

@thesamesam @hartwork addressed everything I hope

@thesamesam
Copy link
Member

Please switch the first bare bug link to 'Bug: https://bugs.gentoo.org/668538' (rather than just a bare link), then remove blank lines between the trailers, then lgtm

Bug: https://bugs.gentoo.org/668538
Closes: https://bugs.gentoo.org/901961
Co-authored-by: josef.95 <josef64@posteo.org>
Thanks-To: Sam James <sam@gentoo.org>
Thanks-To: Arfrever Frehtes Taifersar Arahesis <arfrever.fta@gmail.com>
Thanks-To: Ninpo <ninpo@gap.la>
Signed-off-by: Alfred Wingate <parona@protonmail.com>
@hartwork
Copy link
Member

hartwork commented Oct 6, 2023

@thesamesam I'm not a fan of that request. The Git history is perfectly readable with the merge requests and it's been like that for years in this repository.

Screenshot_20231006_222323

It could hardly be less of a problem and it does have semantic value: The pull request scope is directly visible. This is not some 2k commits per second repository. I understand the the value of rebase merges in some Git repositires but here? I don't get why this should change now and be dictated by a policy that was made for a largily different scenario, the main ebuild repository 🤷

@thesamesam
Copy link
Member

I'm just telling you what Gentoo normally does. I'm not dictating anything, it could well've been that you either had no preference or weren't aware. If you really prefer them, then so be it, it doesn't make any difference to me if it's a repo where you're doing all the work. You need not shrug :)

@hartwork
Copy link
Member

hartwork commented Oct 6, 2023

@thesamesam PS: Without the merge commits I will have to re-write all commit messages to contain Signed-off-by: <me> lines also. It requires not-plain-Git tooling that would not be needed at all otherwise. For little to no value.

@thesamesam
Copy link
Member

git am -sS, git rebase -sS

@hartwork
Copy link
Member

hartwork commented Oct 6, 2023

git am -sS, git rebase -sS

@thesamesam I knew the amend one, -s with rebase seems to be merge strategy though? Found --signoff now, good to know, thanks!

I'm just telling you what Gentoo normally does. I'm not dictating anything

Okay, I did read "please merge w/ rebase rather than merge commits" and "we broadly apply this to other repos as well" quite wrong then, probably because of your position in Gentoo and the reference to "we", it sounded like "sping, get in order, rule 15 over here" to me. Thanks for the clarification.

, it could well've been that you either had no preference or weren't aware. If you really prefer them, then so be it, it doesn't make any difference to me if it's a repo where you're doing all the work. You need not shrug :)

Thanks.

@hartwork
Copy link
Member

hartwork commented Oct 6, 2023

Please switch the first bare bug link to 'Bug: https://bugs.gentoo.org/668538' (rather than just a bare link), then remove blank lines between the trailers, then lgtm

@thesamesam I believe that's all done/satisfied now, let me try merge this with rebase and see if GitHub still considers it merged (I think it should)…

@parona-source thanks!

@hartwork
Copy link
Member

hartwork commented Oct 6, 2023

[..] let me try merge this with rebase and see if GitHub still considers it merged (I think it should)…

@thesamesam the experiment failed apparently, despite e2b9632 on master GitHub does not consider this pull request merged. And git cherry agrees and does not consider the commits the same, even when both sides have the Signed-off-by line. Isn't that weird? Given…

The equivalence test is based on the diff, after removing whitespace and line numbers. git-cherry therefore detects when commits have been "copied" by means of git-cherry-pick(1), git-am(1) or git-rebase(1).

…in man git-cherry — any idea what I may be missing?

To reproduce this very case locally:

# git checkout -b one c5dab19631d0813c558404107dcdafdefc9ce944

# git checkout -b two c5dab19631d0813c558404107dcdafdefc9ce944^ && git cherry-pick c5dab19631d0813c558404107dcdafdefc9ce944

# git cherry -v one two
- 2d..temporary..bc elogv: accept PORTAGE_LOGDIR  # should be empty, no?

# git cherry -v two one
- c5..temporary..44 elogv: accept PORTAGE_LOGDIR

# git --version
git version 2.42.0

@hartwork hartwork closed this Oct 6, 2023
@hartwork
Copy link
Member

hartwork commented Oct 6, 2023

(Merged at e2b9632 despite being closed)

@hartwork
Copy link
Member

hartwork commented Oct 6, 2023

@parona-source anything more needed before elogv 0.8.3?

@parona-source
Copy link
Author

I don't have anything particular in mind. I might look at the open feature request at some point, but I can't promise any ETA.

So its fine in my view to release.

@parona-source parona-source deleted the portage_logdir branch October 6, 2023 21:36
@hartwork
Copy link
Member

hartwork commented Oct 6, 2023

@parona-source good, thanks!

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

3 participants