git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] send-email: add proper default sender

From
Felipe Contreras <felipe.contreras@gmail.com>
Date
Nov 15, 2012, 01:41 UTC
Message-ID
<CAMP44s3vYP6zR6sKxgVSoZUySxcAuR7MioMeT1WDAnU0PtZKGA@mail.gmail.com>
In-Reply-To
<20121115000726.GA16910@sigill.intra.peff.net>
On Thu, Nov 15, 2012 at 1:07 AM, Jeff King <peff@peff.net> wrote:
Show 36 quoted lines
> On Tue, Nov 13, 2012 at 09:35:18PM +0100, Felipe Contreras wrote:
>
>> > Yes, dying would be a regression, in that you would have to configure
>> > your name via the environment and re-run rather than type it at the
>> > prompt. You raise a good point that for people who _could_ take the
>> > implicit default, hitting "enter" is working fine now, and we would lose
>> > that.  I'd be fine with also just continuing to prompt in the implicit
>> > case.
>> >
>> > But that is a much smaller issue to me than having send-email fail to
>> > respect environment variables and silently use user.*, which is what
>> > started this whole discussion. And I agree it is worth considering as a
>> > regression we should avoid.
>>
>> It might be smaller, I don't think so. A hypothetical user that was
>> relying on GIT_AUTHOR for whatever reason can switch to 'git
>> send-email --from' (which is much easier) when they notice the
>> failure, the same way somebody relying on fqdn would. The difference
>> is that people with fqdn do exist, and they might be relying on this.
>>
>> Both are small issues, that I agree with.
>>
>> But the point is that you seem to be very adamant about _my_
>> regressions, and not pay much attention about yours.
>
> Really? I mentioned initially the possibility of dying instead of
> prompting. You raised the point that it would regress a certain use
> case. And then what happened? I said above "you raise a good point[...].
> I'd be fine with also just continuing to prompt[...]. I agree it is
> worth considering as a regression we should avoid". And then I sent out
> a patch series which does not have the regression.
>
> In other words, my suggestion was a bad one, and once it was pointed
> out, I did not pursue it.  If you want to call that "not paying much
> attention", feel free. But I'd much rather you point out problems in my
> actual patch series.

But that I meant that when I introduce a regression it's like I'm killing all that is good and sacred about git, and when you do it's everything but that.

Yes, you sent a new patch. So did I.
Show 11 quoted lines
>> The second patch doesn't have this issue. It does change the behavior
>> of 'git commit', yeah, but I think that's a benefit.
>
> Changing "git commit" is even something I would entertain. It would be a
> regression for some people, but at least it buys us something (increased
> safety against people making bogus commits and failing to notice the
> warning). I'm undecided on whether that is worth it or not.
>
> But when you presented it, as far as I could tell the change in behavior
> to "git commit" was accidental (which is why I pointed it out in
> response).

How could it be accidental if I said this: "Not only will this fix 'git send-email', but it will also fix 'git commit'".

> And as it was in the middle of a discussion about whether
> regressions matter,
That was not the discussion at all.

You can't say that all regressions are the same, and if I say "regression X doesn't matter", that means ALL regressions don't matter. That's a hasty generalization.

Show 5 quoted lines
> If you want to seriously propose changing the behavior of "git commit",
> I think the best thing would be to make a real patch, laying out the
> pros and cons in the commit message, and post it. I would not be
> surprised if the other list participants have stopped reading our thread
> at this point, and the idea is going otherwise unnoticed.
I would, if I saw any chance in it actually going through.
Show 11 quoted lines
>> Or:
>>
>> 4. Just stop prompting
>>
>> I already sent a patch for 4. with all the details of why nobody (or
>> very few, if any) would be affected negatively.
>
> If doing (2) were really hard, that might be worth considering. But it's
> not. I already did it. So I don't see how this is an attractive option,
> unless my series is so unpalatable that we would rather accept a
> regression.

A matter of opinion. I think that series introduces way too much code for a very very small gain that eventually would probably disappear.

Show 10 quoted lines
>> >   [1/6]: ident: make user_ident_explicitly_given private
>> >   [2/6]: ident: keep separate "explicit" flags for author and committer
>> >   [3/6]: var: accept multiple variables on the command line
>> >   [4/6]: var: provide explicit/implicit ident information
>> >   [5/6]: Git.pm: teach "ident" to query explicitness
>> >   [6/6]: send-email: do not prompt for explicit repo ident
>>
>> I think this adds a lot of code that nobody would use.
>
> A lot of code? It is mostly refactoring,
Patch #1 and #3 are refactoring, the rest are not.
> which IMHO makes the resulting
> code cleaner, and it increases the utility of "git var", and our test
> coverage. If you have review comments, then by all means, respond to the
> series.

I don't have any comments, except that I don't think all that code is needed. And why would I bother commenting there, if my opinion will be ignored?

Show 15 quoted lines
>> > I do not necessarily agree on "git commit". Moreover, I feel like it is
>> > a separate issue. My series above _just_ implements the "do not prompt
>> > when explicit" behavior. It does not deal with git-commit at all, nor
>> > does it address the author/committer fallback questions. Those can
>> > easily go on top.
>>
>> Yes, at the cost of adding a lot of code. If we end up agreeing that
>> the changes to 'git commit' are desirable (which I hope at some point
>> we will), then this code would be all for nothing.
>
> If we are going to change "git commit" immediately, then I agree there
> is not much point merging my series. But even if we do change it, will
> we do so immediately? Will there be a deprecation period? If so, then my
> series helps send-email in the meantime. And it's already written, so
> you do not even have to do anything.
Yes, but it still adds a lot of code.
Show 30 quoted lines
>> I want clarify that this is merely a disagreement to at which level
>> should we worry about regressions. On one side of the spectrum you
>> have projects like GNOME, who don't have any problem breaking the
>> user-experience from one release to the next, I'm not proposing
>> anything like that. On the other side I think it's you, because I
>> don't recall encountering anybody with such an extreme position of
>> never introducing a regression ever if there's absolutely no evidence
>> that anybody is using certain feature.
>
> I don't think that's a fair characterization of my position. I am fine
> with introducing a regression if there is a large benefit to it, and
> especially if the regression is mutually exclusive with the benefit. For
> example, look at IDENT_STRICT. We used to allow broken email addresses
> in commits, and it was _me_ who pushed forward the change to disallow
> it. That potentially regressed people who would rather have junk in the
> commit objects than configure their identity (e.g., because they are
> creating commits on the backend of some automated process). But we
> discussed it, and the breakage was worth the increased safety for normal
> users. We could not have it both ways, since the safety came at the
> expense of switching the default.
>
> But with this topic, we had a too-safe default (a safety prompt that was
> sometimes overkill). We can have our cake and eat it, too: drop the
> prompt for the overkill cases, but leave the other cases untouched. And
> that is what I tried to do in my series.  Note that this _still_
> regresses certain use cases. What if I have configured my user.email,
> but I am expecting send-email to prompt me so I can put in some other
> random value. But we can't improve the prompting and leave that case
> there; they are mutually exclusive. But IMHO, the benefit outweighs the
> possibility of breakage.
That's true, so we will be introducing a regression regardless.
Now, if we go with this patch:
http://article.gmane.org/gmane.comp.version-control.git/209660

Instead of your patch series, who will get hurt? Hint: I already answered that question.

Cheers.
-- 
Felipe Contreras
Previous: Jeff KingNext: Jeff King
Message 38 of 76 in “send-email: add proper default sender”
  1. send-email: add proper default senderFelipe Contreras, Nov 11, 2012
  2. Ramkumar RamachandraNov 11, 2012
  3. Felipe ContrerasNov 11, 2012
  4. Jeff KingNov 12, 2012
  5. Felipe ContrerasNov 12, 2012
  6. Jeff KingNov 13, 2012
  7. Jeff KingNov 13, 2012
  8. Junio C HamanoNov 13, 2012
  9. Felipe ContrerasNov 13, 2012
  10. Jeff KingNov 13, 2012
  11. Jeff KingNov 13, 2012
  12. Felipe ContrerasNov 13, 2012
  13. Jeff KingNov 13, 2012
  14. Felipe ContrerasNov 13, 2012
  15. Felipe ContrerasNov 13, 2012
  16. Jeff KingNov 13, 2012
  17. Felipe ContrerasNov 13, 2012
  18. Jeff KingNov 13, 2012
  19. 1/6 ident: make user_ident_explicitly_given privateJeff King, Nov 13, 2012
  20. Jonathan NiederNov 14, 2012
  21. Jeff KingNov 14, 2012
  22. 2/6 ident: keep separate "explicit" flags for author and committerJeff King, Nov 13, 2012
  23. 3/6 var: accept multiple variables on the command lineJeff King, Nov 13, 2012
  24. Jonathan NiederNov 14, 2012
  25. Jeff KingNov 14, 2012
  26. 4/6 var: provide explicit/implicit ident informationJeff King, Nov 13, 2012
  27. Jonathan NiederNov 14, 2012
  28. Jeff KingNov 14, 2012
  29. 5/6 Git.pm: teach "ident" to query explicitnessJeff King, Nov 13, 2012
  30. Jonathan NiederNov 14, 2012
  31. Jeff KingNov 14, 2012
  32. 6/6 send-email: do not prompt for explicit repo identJeff King, Nov 13, 2012
  33. Jonathan NiederNov 14, 2012
  34. Jeff KingNov 14, 2012
  35. Jeff KingNov 14, 2012
  36. Felipe ContrerasNov 13, 2012
  37. Jeff KingNov 15, 2012
  38. Felipe ContrerasNov 15, 2012
  39. Jeff KingNov 15, 2012
  40. Felipe ContrerasNov 15, 2012
  41. 0/8 loosening "sender" prompt in send-emailJeff King, Nov 15, 2012
  42. 1/8 test-lib: allow negation of prerequisitesJeff King, Nov 15, 2012
  43. Jonathan NiederNov 15, 2012
  44. Jeff KingNov 15, 2012
  45. Jonathan NiederNov 15, 2012
  46. 2/8 t7502: factor out autoident prerequisiteJeff King, Nov 15, 2012
  47. Jonathan NiederNov 15, 2012
  48. 3/8 ident: make user_ident_explicitly_given staticJeff King, Nov 15, 2012
  49. Jonathan NiederNov 15, 2012
  50. 4/8 ident: keep separate "explicit" flags for author and committerJeff King, Nov 15, 2012
  51. Jonathan NiederNov 15, 2012
  52. 5/8 var: accept multiple variables on the command lineJeff King, Nov 15, 2012
  53. Jonathan NiederNov 15, 2012
  54. 6/8 var: provide explicit/implicit ident informationJeff King, Nov 15, 2012
  55. 7/8 Git.pm: teach "ident" to query explicitnessJeff King, Nov 15, 2012
  56. Jonathan NiederNov 15, 2012
  57. 8/8 send-email: do not prompt for explicit repo identJeff King, Nov 15, 2012
  58. Felipe ContrerasNov 15, 2012
  59. Jeff KingNov 15, 2012
  60. Felipe ContrerasNov 15, 2012
  61. Jeff KingNov 15, 2012
  62. Jeff KingNov 15, 2012
  63. Jeff KingNov 15, 2012
  64. Junio C HamanoNov 15, 2012
  65. Jeff KingNov 15, 2012
  66. Junio C HamanoNov 16, 2012
  67. Jeff KingNov 16, 2012
  68. Felipe ContrerasNov 16, 2012
  69. Jeff KingNov 16, 2012
  70. Junio C HamanoNov 16, 2012
  71. Jonathan NiederNov 15, 2012
  72. Junio C HamanoNov 13, 2012
  73. Jeff KingNov 13, 2012
  74. Junio C HamanoNov 13, 2012
  75. Erik Faye-LundNov 13, 2012
  76. Jeff KingNov 13, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.