| Message ID | 4684d54aeb3e00c96ba581c824a04e47b7236db7.1660828108.git.git@grubix.eu (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1c8dfc3674fd9da7ded669b706b292ee2074db73 |
| Headers | show |
| Series | sequencer: clarify translations | expand |
On Thu, Aug 18 2022, Michael J Gruber wrote: > `error_resolve_conflict()` checks the untranslated action_name > parameter, so pass it as is. > > Suggested-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> > Signed-off-by: Michael J Gruber <git@grubix.eu> > --- > sequencer.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/sequencer.c b/sequencer.c > index 51d75dfbe1..8b32b239b9 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -537,7 +537,7 @@ static struct tree *empty_tree(struct repository *r) > static int error_dirty_index(struct repository *repo, struct replay_opts *opts) > { > if (repo_read_index_unmerged(repo)) > - return error_resolve_conflict(_(action_name(opts))); > + return error_resolve_conflict(action_name(opts)); > > error(_("your local changes would be overwritten by %s."), > _(action_name(opts))); > @@ -3753,7 +3753,7 @@ static int do_reset(struct repository *r, > init_checkout_metadata(&unpack_tree_opts.meta, name, &oid, NULL); > > if (repo_read_index_unmerged(r)) { > - ret = error_resolve_conflict(_(action_name(opts))); > + ret = error_resolve_conflict(action_name(opts)); > goto cleanup; > } Perhaps we should have the error_resolve_conflict() function take a "enum replay_action" instead? We could just do this more isolated change, but perhaps that "while-we're-at-it" would be acceptable to reduce the risk of running with this particular set of scissors. Then we could note in a comment in that function that we do not want to translate the string we'd get from action_name()...
Am Do., 18. Aug. 2022 um 17:02 Uhr schrieb Ævar Arnfjörð Bjarmason <avarab@gmail.com>: > > > On Thu, Aug 18 2022, Michael J Gruber wrote: > > > `error_resolve_conflict()` checks the untranslated action_name > > parameter, so pass it as is. > > > > Suggested-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> > > Signed-off-by: Michael J Gruber <git@grubix.eu> > > --- > > sequencer.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/sequencer.c b/sequencer.c > > index 51d75dfbe1..8b32b239b9 100644 > > --- a/sequencer.c > > +++ b/sequencer.c > > @@ -537,7 +537,7 @@ static struct tree *empty_tree(struct repository *r) > > static int error_dirty_index(struct repository *repo, struct replay_opts *opts) > > { > > if (repo_read_index_unmerged(repo)) > > - return error_resolve_conflict(_(action_name(opts))); > > + return error_resolve_conflict(action_name(opts)); > > > > error(_("your local changes would be overwritten by %s."), > > _(action_name(opts))); > > @@ -3753,7 +3753,7 @@ static int do_reset(struct repository *r, > > init_checkout_metadata(&unpack_tree_opts.meta, name, &oid, NULL); > > > > if (repo_read_index_unmerged(r)) { > > - ret = error_resolve_conflict(_(action_name(opts))); > > + ret = error_resolve_conflict(action_name(opts)); > > goto cleanup; > > } > > Perhaps we should have the error_resolve_conflict() function take a > "enum replay_action" instead? We could just do this more isolated > change, but perhaps that "while-we're-at-it" would be acceptable to > reduce the risk of running with this particular set of scissors. > > Then we could note in a comment in that function that we do not want to > translate the string we'd get from action_name()... Rather than setting out to do that, I'd retract 2/3/4 just to get 1 done, which was my original motivation ... or switch git to C again as I did for a while in the past ...
Michael J Gruber <git@grubix.eu> writes: > Rather than setting out to do that, I'd retract 2/3/4 just to get 1 > done, which was my original motivation ... or switch git to C again as > I did for a while in the past ... While I found 4/4 a bit questionable, these early three patches looked eminently sensible to me.
Hi Ævar, On Thu, 18 Aug 2022, Ævar Arnfjörð Bjarmason wrote: > On Thu, Aug 18 2022, Michael J Gruber wrote: > > > `error_resolve_conflict()` checks the untranslated action_name > > parameter, so pass it as is. > > > > Suggested-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> > > Signed-off-by: Michael J Gruber <git@grubix.eu> > > --- > > sequencer.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/sequencer.c b/sequencer.c > > index 51d75dfbe1..8b32b239b9 100644 > > --- a/sequencer.c > > +++ b/sequencer.c > > @@ -537,7 +537,7 @@ static struct tree *empty_tree(struct repository *r) > > static int error_dirty_index(struct repository *repo, struct replay_opts *opts) > > { > > if (repo_read_index_unmerged(repo)) > > - return error_resolve_conflict(_(action_name(opts))); > > + return error_resolve_conflict(action_name(opts)); > > > > error(_("your local changes would be overwritten by %s."), > > _(action_name(opts))); > > @@ -3753,7 +3753,7 @@ static int do_reset(struct repository *r, > > init_checkout_metadata(&unpack_tree_opts.meta, name, &oid, NULL); > > > > if (repo_read_index_unmerged(r)) { > > - ret = error_resolve_conflict(_(action_name(opts))); > > + ret = error_resolve_conflict(action_name(opts)); > > goto cleanup; > > } > > Perhaps we should have the error_resolve_conflict() function take a > "enum replay_action" instead? We could do that. We could also just delete the sequencer code. It's just that both are a bad idea. Ciao, Johannes
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> Perhaps we should have the error_resolve_conflict() function take a >> "enum replay_action" instead? > > We could do that. We could also just delete the sequencer code. It's just > that both are a bad idea. Sorry, but I do not quite understand this comment. You may think some parts of the sequencer code are a bad idea but I think overall it is eminently useful and usable enough that it does not make sense to "just delete the sequencer code"---if there are things we find bad ideas in there, we should fix them instead, no? In any case, can you keep the conversation more civil? I have to say that between you two, you may by no means be the only one who is unnecessarily abrasive, but if you do not understand why the other side suggests a solution you feel you do not like, you can ask more constructively why they think it is a good idea, without assuming that they are doing so only to block you. Or explain why you think it is a bad idea by showing the consequences of their solution, e.g. "there are 20 callsites, among which only 1 has the enum readily available so it would be a lot of churn to give the other 19 the enum, even though the error helper may become simpler with a single switch() statement if we allow it to take an enum." or something (I know this function is called only from very few places, so 1 out of 20 is a totally made-up reasoning that would not apply in this case, but you get the idea). Thanks.
Hi Junio, [Michael, I do not consider what I wrote below relevant for your patch series, you may ignore it if you want] On Fri, 19 Aug 2022, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > >> Perhaps we should have the error_resolve_conflict() function take a > >> "enum replay_action" instead? > > > > We could do that. We could also just delete the sequencer code. It's just > > that both are a bad idea. > > Sorry, but I do not quite understand this comment. I expected a seasoned reviewer to offer such a suggestion only after looking up (or remembering) how `error_resolve_conflict()` is defined, and where, and where its callers are. After all, many suggestions that come to mind during a review turn out to be a bad idea when considering them carefully, and if that can be determined before the mail is sent, everybody wins back some time. In this instance, `error_resolve_conflict()` is declared in `advice.h`. The suggestion to use a sequencer-specific data type there sounds... controversial. But okay, maybe there are good reasons to suggest that. Let's look at the callers. Two callers in `sequencer.c`. Okay, maybe it makes a bit more sense. But one caller in `advice.c`? Let's dig deeper. That caller in `advice.c` is `die_resolve_conflict()`, which is called in the built-ins `commit`, `merge-recursive`, `merge` and `pull`. Those callers have nothing to do with the sequencer, therefore it is a bad idea to suggest using a sequencer-specific data type in that call chain. From my perspective, that is enough to retire the suggestion. When I wrote what I wrote, I thought that it was a pretty quick thing to determine, so quick that I really expected to not see such a suggestion on the mailing list in the first place. In hindsight, I understand that you would have had to look at the code, and not just at the patch, to see this. And therefore it is probably not quite as obvious as I thought. I did not expect new contributors to be able to analyze this quickly, but a Git mailing list regular, yes. For my flippant response, I apologize. As for the suggestion I criticized: I stand by my assessment. It is not a good idea, and it was not necessary to send it out before doing a cursory sanity check. We want code contribution to have a high quality, and the code reviews should meet at least the same bar. Ciao, Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > ... We want code contribution to have a high quality, and the > code reviews should meet at least the same bar. I like that one. Ævar is not alone, but many of us often throw an unrelated "observation" into a review thread that is a total tangent. While I do not think it is necessarily a bad thing, because these tangential discussions often turn into separate idea that lead to improvements, we should learn to (1) mark a tangent clearly as such and (2) keep the quality of the tangent reasonably high. Thanks.
diff --git a/sequencer.c b/sequencer.c index 51d75dfbe1..8b32b239b9 100644 --- a/sequencer.c +++ b/sequencer.c @@ -537,7 +537,7 @@ static struct tree *empty_tree(struct repository *r) static int error_dirty_index(struct repository *repo, struct replay_opts *opts) { if (repo_read_index_unmerged(repo)) - return error_resolve_conflict(_(action_name(opts))); + return error_resolve_conflict(action_name(opts)); error(_("your local changes would be overwritten by %s."), _(action_name(opts))); @@ -3753,7 +3753,7 @@ static int do_reset(struct repository *r, init_checkout_metadata(&unpack_tree_opts.meta, name, &oid, NULL); if (repo_read_index_unmerged(r)) { - ret = error_resolve_conflict(_(action_name(opts))); + ret = error_resolve_conflict(action_name(opts)); goto cleanup; }
`error_resolve_conflict()` checks the untranslated action_name parameter, so pass it as is. Suggested-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Michael J Gruber <git@grubix.eu> --- sequencer.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)