Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Mon, Apr 15, 2013 at 3:28 PM, Junio C Hamano wrote: > * fc/remote-hg (2013-04-11) 21 commits > - remote-hg: activate graphlog extension for hg_log() > - remote-hg: fix bad file paths > - remote-hg: document location of stored hg repository > - remote-hg: fix bad state issue > - remote-hg: add 'insecure' option > - remote-hg: add simple mail test > - remote-hg: add basic author tests > - remote-hg: show more proper errors > - remote-hg: force remote push > - remote-hg: push to the appropriate branch > - remote-hg: update tags globally > - remote-hg: update remote bookmarks > - remote-hg: refactor export > - remote-hg: split bookmark handling > - remote-hg: redirect buggy mercurial output > - remote-hg: trivial test cleanups > - remote-hg: make sure fake bookmarks are updated > - remote-hg: fix for files with spaces > - remote-hg: properly report errors on bookmark pushes > - remote-hg: add missing config variable in doc > - remote-hg: trivial cleanups > > Rerolled. > > Waiting for comments. >From whom? And about this: http://mid.gmane.org/1365638832-9000-3-git-send-email-felipe.contre...@gmail.com I think it's a disservice to git users to not consider this a "cooking patch", specially since it's only the commit message somebody was worried about. But whatever, don't. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Felipe Contreras writes: > And about this: > http://mid.gmane.org/1365638832-9000-3-git-send-email-felipe.contre...@gmail.com What about it? Is that the one you said you are going to reroll? I do not recall the details of Peff's complaints, but re-reading the log message of the patch itself, seeing "correctly" twice is not satisfactory. As you very well know, a bug description that says "This does not correctly work!" and stops there is not as useful as a description that defines what "correct" behaviour is expected. If one of them said "update correctly to record what was pushed" or something like that, that should be sufficient. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Mon, Apr 15, 2013 at 01:28:53PM -0700, Junio C Hamano wrote: > [Graduated to "master"] > [...] > * jk/http-error-messages (2013-04-06) 9 commits > (merged to 'next' on 2013-04-11 at 7a03981) > + http: drop http_error function > + remote-curl: die directly with http error messages > + http: re-word http error message > + http: simplify http_error helper function > + remote-curl: consistently report repo url for http errors > + remote-curl: always show friendlier 404 message > + remote-curl: let servers override http 404 advice > + remote-curl: show server content on http errors > + http: add HTTP_KEEP_ERROR option > > Improve error reporting from the http transfer clients. I had not been keeping tabs on the progress of this topic, and was surprised to see it in master already. It hadn't gotten any comments, so I sort of assumed I would need to re-post to get interest. I don't mind, but... ...the tip of your current master does not currently pass the test suite[1]. I bisected the problem to "show server content on http errors" from the above topic, but haven't figure it out past that. I typically run "make test" before submitting, so I'm guessing it is an interaction with another topic that graduated around the same time (though it's also possible that I just failed to test after the last rebase). I'll investigate further, but it may be a few hours. -Peff [1] I know you always test master before pushing it out, but I suspect you do not run the GIT_TEST_HTTPD tests. The failures are in t5541 and t5551. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Mon, Apr 15, 2013 at 6:14 PM, Junio C Hamano wrote: > Felipe Contreras writes: > >> And about this: >> http://mid.gmane.org/1365638832-9000-3-git-send-email-felipe.contre...@gmail.com > > What about it? Is that the one you said you are going to reroll? At first, but then I changed my mind. > I do not recall the details of Peff's complaints, but re-reading the > log message of the patch itself, seeing "correctly" twice is not > satisfactory. As you very well know, a bug description that says > "This does not correctly work!" and stops there is not as useful as > a description that defines what "correct" behaviour is expected. The subject is: transport-helper: update remote helper namespace Clearly, that's the correct behavior. Why would anybody send a change that does something other than the correct behavior? --- When pushing, the remote namespace is updated correctly (e.g. refs/origin/master), but not the remote helper's (e.g. refs/testgit/origin/master). --- So it should be clear now: the remote namespace refs/origin/master is updated, but not the remote helper's namespace refs/testgit/origin/master, which is what I already said. I don't know what more do you expect. When you push 'refs/heads/master' to origin, you expect 'refs/remotes/origin/master' to point to the same commit, same with 'refs/testgit/origin/master', why would you expect to point somewhere else? > If one of them said "update correctly to record what was pushed" or > something like that, that should be sufficient. Sure, it's still under the definition of "cooking" in my mind. Anyway, I'm not feeling a great urge to resubmit this patch just because of the commit message, which I think it's perfectly fine. There's more interesting things work on. Personally I feel it's preferable to fix the actual issue that is already present rather than hold it because the commit message might or might not be enough for hypothetical point in the future. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On 16 April 2013 01:25, Jeff King wrote: > On Mon, Apr 15, 2013 at 01:28:53PM -0700, Junio C Hamano wrote: > > [Graduated to "master"] > > [...] > > * jk/http-error-messages (2013-04-06) 9 commits > > (merged to 'next' on 2013-04-11 at 7a03981) > > [...] > > ...the tip of your current master does not currently pass the test > suite[1]. > [...] > > [1] I know you always test master before pushing it out, but I suspect > you do not run the GIT_TEST_HTTPD tests. The failures are in t5541 > and t5551. Ah, that explains why the test suite passed here, I built a new version an hour ago from current master (v1.8.2.1-418-gaec3f77, 2013-04-15 12:45:15 -0700), and no errors were found. I build new gits almost every day for testing purposes (master, and next and maint very often) on several machines with different setups, and of course also to have the newest version. I'd like to run as many tests as possible. Is there any list of environment variables or make directives available to enable most of them? Regards, Øyvind -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Mon, Apr 15, 2013 at 07:25:32PM -0400, Jeff King wrote:
> On Mon, Apr 15, 2013 at 01:28:53PM -0700, Junio C Hamano wrote:
>
> > * jk/http-error-messages (2013-04-06) 9 commits
> [...]
> ...the tip of your current master does not currently pass the test
> suite[1]. I bisected the problem to "show server content on http
> errors" from the above topic, but haven't figure it out past that. I
> typically run "make test" before submitting, so I'm guessing it is an
> interaction with another topic that graduated around the same time
> (though it's also possible that I just failed to test after the last
> rebase).
This patch on top of jk/http-error-messages fixes it.
-- >8 --
Subject: [PATCH] http: set curl FAILONERROR each time we select a handle
Because we reuse curl handles for multiple requests, the
setup of a handle happens in two stages: stable, global
setup and per-request setup. The lifecycle of a handle is
something like:
1. get_curl_handle; do basic global setup that will last
through the whole program (e.g., setting the user
agent, ssl options, etc)
2. get_active_slot; set up a per-request baseline (e.g.,
clearing the read/write functions, making it a GET
request, etc)
3. perform the request with curl_*_perform functions
4. goto step 2 to perform another request
Breaking it down this way means we can avoid doing global
setup from step (1) repeatedly, but we still finish step (2)
with a predictable baseline setup that callers can rely on.
Until commit 6d052d7 (http: add HTTP_KEEP_ERROR option,
2013-04-05), setting curl's FAILONERROR option was a global
setup; we never changed it. However, 6d052d7 introduced in
option where some requests might turn off FAILONERROR. Later
requests using the same handle would have the option
unexpectedly turned off, which meant they would not notice
http failures at all.
This could easily be seen in the test-suite for the
"half-auth" cases of t5541 and t5551. The initial requests
turned off FAILONERROR, which meant it was erroneously off
for the rpc POST. That worked fine for a successful request,
but meant that we failed to react properly to the HTTP 401
(instead, we treated whatever the server handed us as a
successful message body).
The solution is simple: now that FAILONERROR is a
per-request setting, we move it to get_active_slot to make
sure it is reset for each request.
Signed-off-by: Jeff King
---
Hmph. I have no idea how this ever passed the tests, so I can only
assume that I screwed up in running them. I even recall considering this
issue while writing the patches, but I mixed up which of get_curl_handle
and get_active_slot it needed to be in when I did so.
http.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/http.c b/http.c
index 58c063c..48d4ff6 100644
--- a/http.c
+++ b/http.c
@@ -282,7 +282,6 @@ static CURL *get_curl_handle(void)
#endif
if (ssl_cainfo != NULL)
curl_easy_setopt(result, CURLOPT_CAINFO, ssl_cainfo);
- curl_easy_setopt(result, CURLOPT_FAILONERROR, 1);
if (curl_low_speed_limit > 0 && curl_low_speed_time > 0) {
curl_easy_setopt(result, CURLOPT_LOW_SPEED_LIMIT,
@@ -506,6 +505,7 @@ struct active_request_slot *get_active_slot(void)
curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, NULL);
curl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0);
curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);
+ curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);
if (http_auth.password)
init_curl_http_auth(slot->curl);
--
1.8.2.8.g44e4c28
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Tue, Apr 16, 2013 at 01:49:13AM +0200, Øyvind A. Holm wrote: > > [1] I know you always test master before pushing it out, but I suspect > > you do not run the GIT_TEST_HTTPD tests. The failures are in t5541 > > and t5551. > > Ah, that explains why the test suite passed here, I built a new > version an hour ago from current master (v1.8.2.1-418-gaec3f77, > 2013-04-15 12:45:15 -0700), and no errors were found. I build new gits > almost every day for testing purposes (master, and next and maint very > often) on several machines with different setups, and of course also > to have the newest version. I'd like to run as many tests as possible. > Is there any list of environment variables or make directives > available to enable most of them? I don't think there's a master list anywhere. The simplest thing is probably to run the whole suite and grep for skipped tests, each of which should usually explain their reasoning for the skip. Like: make test | grep '# skip' Note that this will turn up more than just environment variables to set. It will also turn up missing programs you might need to install to run the tests (e.g., we do not do subversion tests if svn is not installed). Which is a good thing if you are trying for complete test coverage. But it will also turn up useless things that are just fundamental to your platform (e.g., you cannot do both the case-sensitive and the case-insensitive filesystem tests). Most of the tests are on by default, unless necessary programs are missing or you explicitly disable them. GIT_TEST_HTTPD and GIT_TEST_GIT_DAEMON seem to be the exceptions. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Mon, Apr 15, 2013 at 8:30 PM, Jeff King wrote: > Subject: [PATCH] http: set curl FAILONERROR each time we select a handle > > Until commit 6d052d7 (http: add HTTP_KEEP_ERROR option, > 2013-04-05), setting curl's FAILONERROR option was a global > setup; we never changed it. However, 6d052d7 introduced in s/in/an/ > option where some requests might turn off FAILONERROR. Later > requests using the same handle would have the option > unexpectedly turned off, which meant they would not notice > http failures at all. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
n Mon, Apr 15, 2013 at 4:28 PM, Junio C Hamano wrote: > * jn/gitweb-install-doc (2013-04-15) 1 commit > - gitweb/INSTALL: Simplify description of GITWEB_CONFIG_SYSTEM > > Reword gitweb configuration instrutions. > > Will merge to 'next'. While the re-worded text is easier on the eyes it fails to note why we don't just dump our idiosyncratic way of doing things and just make the system-wide defaults act individually. This information is useful to system administrators, as it explains what is actually going on. -- -Drew Northup -- "As opposed to vegetable or mineral error?" -John Pescatore, SANS NewsBites Vol. 12 Num. 59 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Felipe Contreras writes: > So it should be clear now: the remote namespace refs/origin/master is > updated, but not the remote helper's namespace > refs/testgit/origin/master, which is what I already said. I don't know > what more do you expect. When you push 'refs/heads/master' to origin, > you expect 'refs/remotes/origin/master' to point to the same commit, > same with 'refs/testgit/origin/master', why would you expect to point > somewhere else? Let me play somebody who comes later and wonders about this exchange three months down the road... You mention three refs/ here. Do they live in the same repository? Any Git person is expected to know refs/heads/master, which is "my local branch I have worked on and I am pushing". It also is easy to guess what "refs/remotes/origin/master" is, even though we are not talking about a usual Git remote. It is to keep track of the remote behind the helper we are pushing into, and is updated to pretend as if we fetched immediately from the place we just pushed. The latter being in sync with what we pushed is something that can naturally be expected. Now, what is this third "refs/testgit/origin/master" thing? Is it expected to always be the same as "refs/remotes/origin/master"? If that is the case, why do we even need such a redundant information in the first place? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Mon, Apr 15, 2013 at 11:12 PM, Junio C Hamano wrote: > Felipe Contreras writes: > >> So it should be clear now: the remote namespace refs/origin/master is >> updated, but not the remote helper's namespace >> refs/testgit/origin/master, which is what I already said. I don't know >> what more do you expect. When you push 'refs/heads/master' to origin, >> you expect 'refs/remotes/origin/master' to point to the same commit, >> same with 'refs/testgit/origin/master', why would you expect to point >> somewhere else? > > Let me play somebody who comes later and wonders about this exchange > three months down the road... > > You mention three refs/ here. Do they live in the same repository? > Any Git person is expected to know refs/heads/master, which is "my > local branch I have worked on and I am pushing". It also is easy to > guess what "refs/remotes/origin/master" is, even though we are not > talking about a usual Git remote. It is to keep track of the remote > behind the helper we are pushing into, and is updated to pretend as > if we fetched immediately from the place we just pushed. The latter > being in sync with what we pushed is something that can naturally be > expected. > > Now, what is this third "refs/testgit/origin/master" thing? Is it > expected to always be the same as "refs/remotes/origin/master"? If > that is the case, why do we even need such a redundant information > in the first place? Answering that question is beyond the scope of the commit message. The purpose of the commit message is not to educate people about the current design of transport-helpers, we have Documentation/gitremote-helpers.txt for that. We also have discussions in the mailing list. The untold answer is 'if you have to ask, you don't understand this code', but if you must, the short answer is: because it doesn't work otherwise. Yes, in theory not using a remote helper ref namespace should work, but it doesn't, and I sent patches to try to fix this behavior, but those, along with this particular patch, where ignored. So I gave up on those patches that tried to fix the behavior for more corner cases and tried to push the simplest one I could find, still finding trouble. I tried to document that in t/t5801-remote-helpers.sh, although to be honest, the tests that pass can be hardly be considered proper behavior. This is of course, for import/export, which are the only operations I'm familiar with (maybe the namespace makes sense for push/fetch operations. So basically I'm just tired of explaining the same things over and over again, and if you try to explain every single detail, I think any reasonable person would reach the conclusion that the design doesn't really make sense. But the only person that seems to be trying to explain it and improve it seems to be me. The commit message is no place to explain all these subtleties, it would be huge and would need to refer to plenty of mails in the mailing list. I could send the whole patch series I have, and then it might become clearer why not having the refspec doesn't work, but then ,the chances of this patch not getting through would be higher, as the last time I sent such series, it didn't go through. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Felipe Contreras writes:
> Clearly, that's the correct behavior. Why would anybody send a change
> that does something other than the correct behavior?
Along the same lines, why would anyone write broken code? Nobody does,
right?
If anyone reads that commit message in more than a few weeks, then it's
because some of the code is *broken*. So the reader is investigating a
situation where there must be a flaw somewhere, and trying to pin down
the source. Having access to the thinking behind each commit means s/he
can more easily verify whether that thinking was correct and still
applies.
And your commit messages do nothing towards that end.
A cursory look^W^Wreview of the messages in fc/remote-hg:
remote-hg: fix bad file paths
Mercurial allows absolute file paths, and Git doesn't like that.
Only describes the problem; no reasoning as to what the chosen solution
is or why it is correct. (I can at least infer the former from the
code, but not the latter.)
remote-hg: show more proper errors
When cloning or pushing fails, we don't want to show a stack-trace.
So what do we show?
It also seems that you do not actually use the import you add, or do
you?
remote-hg: force remote push
Ideally we shouldn't do this, as it's not recommended in mercurial
documentation, but there's no other way to push multiple bookmarks (on
the same branch), which would be the behavior most similar to git.
At the same time, add a configuration option for the people that don't
want to risk creating new remote heads.
This one, for a change, says what it does but doesn't say what problem
it fixes.
I'll refrain from commenting on all the one-line messages, and just
point at this one:
remote-hg: trivial test cleanups
In $DAYJOB the advice is to avoid "trivial" (and similarly "obvious"):
either it *is* trivial, in which case you don't need to point that out,
or you're just trying to handwave over the fact that it's not. Like
this:
git_clone () {
- hg -R $1 bookmark -f -r tip master &&
git clone -q "hg::$PWD/$1" $2
}
Not knowing the code I can only conjecture, but surely there was a
reason that the hg call lived in a function called git_clone? And
surely there must be a good reason why it is no longer needed?
My personal favorite however is this one:
remote-bzr: improve tag handling
revision_history() is deprecated and doesn't do what we want (revno
instead of dotted_revno?).
I don't even know how to parse that question mark. Does it actually ask
a question? Does it mean to imply, by the intonation suggested by a
question mark, "how could anyone ever have been so silly as to use a
revno instead of a dotted_revno"?
By the way, it's easy to find similarly helpful messages in git.git in
the old days. One that I remember stumbling across was:
Add the --color-words option to the diff options family
With this option, the changed words are shown inline. For example,
if a file containing "This is foo" is changed to "This is bar", the diff
will now show "This is " in plain text, "foo" in red, and "bar" in green.
How could it not be obvious how it achieves this to anyone who has read
the ~170 lines of code it adds?
Luckily *that* code was correct and feature-complete right from the
start, so nobody ever had to actually read it to figure out what's going
on.
But that was back in 2006. I should think that git.git has improved
since; when I wrote my first patches in 2008, I was impressed with the
readable history and extensive reviews.
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Jeff King writes:
> The solution is simple: now that FAILONERROR is a
> per-request setting, we move it to get_active_slot to make
> sure it is reset for each request.
>
> Signed-off-by: Jeff King
> ---
> Hmph. I have no idea how this ever passed the tests, so I can only
> assume that I screwed up in running them. I even recall considering this
> issue while writing the patches, but I mixed up which of get_curl_handle
> and get_active_slot it needed to be in when I did so.
Thanks.
> http.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/http.c b/http.c
> index 58c063c..48d4ff6 100644
> --- a/http.c
> +++ b/http.c
> @@ -282,7 +282,6 @@ static CURL *get_curl_handle(void)
> #endif
> if (ssl_cainfo != NULL)
> curl_easy_setopt(result, CURLOPT_CAINFO, ssl_cainfo);
> - curl_easy_setopt(result, CURLOPT_FAILONERROR, 1);
>
> if (curl_low_speed_limit > 0 && curl_low_speed_time > 0) {
> curl_easy_setopt(result, CURLOPT_LOW_SPEED_LIMIT,
> @@ -506,6 +505,7 @@ struct active_request_slot *get_active_slot(void)
> curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, NULL);
> curl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0);
> curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);
> + curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);
> if (http_auth.password)
> init_curl_http_auth(slot->curl);
Thanks
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Tue, Apr 16, 2013 at 4:59 AM, Thomas Rast wrote: > Felipe Contreras writes: > >> Clearly, that's the correct behavior. Why would anybody send a change >> that does something other than the correct behavior? > > Along the same lines, why would anyone write broken code? Nobody does, > right? Yes, I should change the subject to: transport-helper: update remote helper namespace, because that's exactly the thing we DON'T want to do, the purpose of this patch is to mess up everything Suree. I'm willing and knowingly introducing a change that goes diametrically opposite to what we want. > If anyone reads that commit message in more than a few weeks, then it's > because some of the code is *broken*. That is irrelevant. Junio said the correct behavior was not described, when if fact it clearly is. Whether or not the patch has a bug in it is irrelevant to the fact that the correct behavior is described or not. > So the reader is investigating a > situation where there must be a flaw somewhere, and trying to pin down > the source. Having access to the thinking behind each commit means s/he > can more easily verify whether that thinking was correct and still > applies. Sure, and where is the thinking not clear? The remote helper ref is not updated, so we do update it. How is that not clear? > And your commit messages do nothing towards that end. Oh, it does. You just don't understand how remote-helper works. > A cursory look^W^Wreview of the messages in fc/remote-hg: [skipping irrelevant comments] I'm sorry, did you actually hit an issue that required to look at the commit message to understand where the issue came from? No? Then I won't bother with hypotheticals. If you want to waste your time, by all means, rewrite all my commit messages with essays that nobody will ever read. I'm not going to do that for some hypothetical case that will never happen. I'm not going to waste my time. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Felipe Contreras writes: > Sure, and where is the thinking not clear? The remote helper ref is > not updated, so we do update it. How is that not clear? Sure, between "leaving it untouched, keeping the stale value" and "updating it to match what was pushed", everybody would know you mean the latter when you say "correctly update". There is no third option "updating it to match a random commit that is related to but is not exactly the same as what was pushed" to be correct. What I felt unclear was _why_ both of these two (remote and testgit) have to get updated. In other words, "correctly update it" because "without doing so, these bad things X, Y and Z will happen". -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Tue, Apr 16, 2013 at 2:19 PM, Junio C Hamano wrote: > Felipe Contreras writes: > >> Sure, and where is the thinking not clear? The remote helper ref is >> not updated, so we do update it. How is that not clear? > > Sure, between "leaving it untouched, keeping the stale value" and > "updating it to match what was pushed", everybody would know you > mean the latter when you say "correctly update". There is no third > option "updating it to match a random commit that is related to but > is not exactly the same as what was pushed" to be correct. > > What I felt unclear was _why_ both of these two (remote and testgit) > have to get updated. In other words, "correctly update it" because > "without doing so, these bad things X, Y and Z will happen". The bad thing that would happen is that it won't be up-to-date. If you don't know what an outdated ref causes, then you don't know what transport-helper does with it, and if you don't know that, why are you bothering trying to review this patch? Is the purpose of a patch to educate people? Here it goes. The remote helper ref is going to be used to tell fast-export which refs to negate (e.g. ^refs/testgit/origin/master), so that extra commits are not generated, which the remote helper should ignore anyway, because it should already have marks for those. So doing two consecutive pushes, would push the commits twice. It's worth noting this is the first time anybody asks what is the negative effect of this not getting fixed. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Tue, Apr 16, 2013 at 3:48 PM, Felipe Contreras wrote: > Here it goes. The remote helper ref is going to be used to tell > fast-export which refs to negate (e.g. ^refs/testgit/origin/master), > so that extra commits are not generated, which the remote helper > should ignore anyway, because it should already have marks for those. > So doing two consecutive pushes, would push the commits twice. > > It's worth noting this is the first time anybody asks what is the > negative effect of this not getting fixed. Yes, but what is noteworthy about it is that you did not include that in your commit message to begin with. This is the commit message request from Documentation/SubmittingPatches: The body should provide a meaningful commit message, which: . explains the problem the change tries to solve, iow, what is wrong with the current code without the change. . justifies the way the change solves the problem, iow, why the result with the change is better. . alternate solutions considered but discarded, if any. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Tue, Apr 16, 2013 at 3:04 PM, Felipe Contreras wrote: > On Tue, Apr 16, 2013 at 4:59 AM, Thomas Rast wrote: >> A cursory look^W^Wreview of the messages in fc/remote-hg: > > [skipping irrelevant comments] > > I'm sorry, did you actually hit an issue that required to look at the > commit message to understand where the issue came from? No? Then I > won't bother with hypotheticals. > > If you want to waste your time, by all means, rewrite all my commit > messages with essays that nobody will ever read. I'm not going to do > that for some hypothetical case that will never happen. I'm not going > to waste my time. This is not a hypothetical. Almost every time I bisect a regression in git.git, I find the commit message tells me exactly why the commit did what it did and what the expected result was. I find this to be amazingly useful. Do I need to show you real instances of that happening? No. I promise it did, though. Of course, 99% of the commit messages may never be useful to me or anyone else. But we do not eschew them altogether. The 1% I have to rely on are nearly always helpful and clear, and that is the part I care about. If you will not waste your time to write a decent commit message, why do you waste our time asking us to review and accept ill-defined patches? Here, of course, I use the royal "us" as I do not review your patches. I do not know why that is; I suppose you patch things outside of my interests, but it may also be that your patches are simply incomprehensible by design. Phil -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Tue, Apr 16, 2013 at 5:34 PM, Phil Hord wrote: > On Tue, Apr 16, 2013 at 3:48 PM, Felipe Contreras > wrote: >> Here it goes. The remote helper ref is going to be used to tell >> fast-export which refs to negate (e.g. ^refs/testgit/origin/master), >> so that extra commits are not generated, which the remote helper >> should ignore anyway, because it should already have marks for those. >> So doing two consecutive pushes, would push the commits twice. >> >> It's worth noting this is the first time anybody asks what is the >> negative effect of this not getting fixed. > > Yes, but what is noteworthy about it is that you did not include that > in your commit message to begin with. This is the commit message > request from Documentation/SubmittingPatches: And yet, nobody asked for that. Anyway, drop the patch then. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Phil Hord writes: > ... Almost every time I bisect a regression > in git.git, I find the commit message tells me exactly why the commit > did what it did and what the expected result was. I find this to be > amazingly useful. > ... > Of course, 99% of the commit messages may never be useful to me or > anyone else. But we do not eschew them altogether. The 1% I have to > rely on are nearly always helpful and clear, and that is the part I > care about. It is nice to be appreciated for our efforts by somebody every once in a while ;-) Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Mon, Apr 15, 2013 at 01:28:53PM -0700, Junio C Hamano wrote: > Here are the topics that have been cooking. Commits prefixed with > '-' are only in 'pu' (proposed updates) while commits prefixed with > '+' are in 'next'. > [...] > -- > [New Topics] > [...] > * lf/read-blob-data-from-index (2013-04-15) 3 commits > (merged to 'next' on 2013-04-15 at 09f92c6) > + convert.c: Remove duplicate code > + Add size parameter to read_blob_data_from_index_path() > + Add public function read_blob_data_from_index_path() > > Reduce duplicated code between convert.c and attr.c. > > Will merge to 'master'. Not sure if you care but the commit messages of these are all wrong now that you squashed your API fix into the first commit. They all refer to read_blob_data_from_index_path() instead of read_blob_data_from_index() and most of the details mentioned in the first commit of this series no longer apply... Just saying :) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Junio C Hamano writes:
> * jc/add-2.0-delete-default (2013-03-08) 3 commits
> - git add ... defaults to "-A"
> (merged to 'next' on 2013-04-05 at 199442e)
> + git add: start preparing for "git add ..." to default to "-A"
> + builtin/add.c: simplify boolean variables
>
> In Git 2.0, "git add pathspec" will mean "git add -A pathspec". If
> you did this in a working tree that tracks dir/lost and dir/another:
>
> $ rm dir/lost
> $ edit dir/another
> $ git add dir
>
> The last step will not only notices and records updated
> dir/another, but also notices and records the removal of dir/lost
> in the index.
>
> Start training the users for this change to say --no-all when they
> want to ignore the removal to smooth the transition hump.
>
> Will merge to 'master' the early bits and cook the rest in 'next' until Git
> 2.0.
The warning triggers in some cases where it shouldn't, relating to
submodules:
$ git submodule add gito...@git.csa.inf.ethz.ch:domjudge.git domjudge
Adding existing repo at 'domjudge' to the index
warning: In Git 2.0, 'git add ...' will also update the
index for paths removed from the working tree that match
the given pathspec. If you want to 'add' only changed
or newly created paths, say 'git add --no-all ...' instead.
It also seems to hint that the problem is with giving a 'pathspec', but
in fact in the case of a "proper" pathspec (that isn't an existing path)
it does *not* trigger, even though it probably should:
$ git ls-files
foo
$ rm foo
$ git add 'f*'
$ git status
# On branch master
# Changes not staged for commit:
# (use "git add/rm ..." to update what will be committed)
# (use "git checkout -- ..." to discard changes in working directory)
#
# deleted:foo
#
no changes added to commit (use "git add" and/or "git commit -a")
$ git add -A 'f*'
$ git status
# On branch master
# Changes to be committed:
# (use "git reset HEAD ..." to unstage)
#
# deleted:foo
#
# Untracked files not listed (use -u option to show untracked files)
That's of course assuming that you want to unconditionally make -A the
default.
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Lukas Fleischer writes: > Not sure if you care but the commit messages of these are all wrong now > that you squashed your API fix into the first commit. They all refer to > read_blob_data_from_index_path()... Ouch; thanks for noticing. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Thomas Rast writes: > The warning triggers in some cases where it shouldn't, relating to > submodules: > > $ git submodule add gito...@git.csa.inf.ethz.ch:domjudge.git domjudge > Adding existing repo at 'domjudge' to the index > warning: In Git 2.0, 'git add ...' will also update > the index for paths removed from the working tree that match > the given pathspec. If you want to 'add' only changed > or newly created paths, say 'git add --no-all ...' instead. Good one. So "add" used internally there needs to say --no-add? > It also seems to hint that the problem is with giving a 'pathspec', but > in fact in the case of a "proper" pathspec (that isn't an existing path) > it does *not* trigger, even though it probably should: We have seen users who explicitly say: git add dir after removing dir/del and adding dir/ins got surprised that we do not notice removal of dir/del without "add -A". And it is fairly straight-forward to check and warn for such a case. > That's of course assuming that you want to unconditionally make -A the > default. I thought what the warning text says is what we decided to do eventually. Not "unconditionally", but "with ", but not "only with pathspec that exactly matches an existing path". It appears that we would better discuss and decide such details further, so let's revert the "warn early" bits from master and kick the topic back. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Junio C Hamano writes:
> Thomas Rast writes:
>
>> The warning triggers in some cases where it shouldn't, relating to
>> submodules:
>>
>> $ git submodule add gito...@git.csa.inf.ethz.ch:domjudge.git domjudge
>> Adding existing repo at 'domjudge' to the index
>> warning: In Git 2.0, 'git add ...' will also update
>> the index for paths removed from the working tree that match
>> the given pathspec. If you want to 'add' only changed
>> or newly created paths, say 'git add --no-all ...' instead.
>
> Good one. So "add" used internally there needs to say --no-add?
I think the logic in git-add needs to learn about submodules. The same
warning later trigger when you later say 'git add submoduledir', even
though that obviously doesn't walk inside the submodule.
>> It also seems to hint that the problem is with giving a 'pathspec', but
>> in fact in the case of a "proper" pathspec (that isn't an existing path)
>> it does *not* trigger, even though it probably should:
>
> We have seen users who explicitly say:
>
> git add dir
>
> after removing dir/del and adding dir/ins got surprised that we do
> not notice removal of dir/del without "add -A". And it is fairly
> straight-forward to check and warn for such a case.
I can see that problem, but along the same lines, why shouldn't I have
an expectation that when I say 'git add "*.py"' it removes stuff that I
have removed? That's what I tried to show with the f?o example.
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Thomas Rast writes: > I can see that problem, but along the same lines, why shouldn't I have > an expectation that when I say 'git add "*.py"' it removes stuff that I > have removed? You _should_ have that expectation. If it does not remove with the code that has been prepared for 2.0 (that is a bit beyond 'next'), then it is a big problem, but I think it does remove the removed python source without "-A", as long as you give a pathspec "*.py" (with quotes around it) that match it. I think it is just the warning code avoiding extra complexity and overhead, if you are talking about not getting warning in the pre-2.0 step that is in 'next'. Patches are very much welcomed, especially the ones that come before I get around to it ;-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Junio C Hamano writes: > Thomas Rast writes: > >> I can see that problem, but along the same lines, why shouldn't I have >> an expectation that when I say 'git add "*.py"' it removes stuff that I >> have removed? > > You _should_ have that expectation. > > If it does not remove with the code that has been prepared for 2.0 > (that is a bit beyond 'next'), then it is a big problem, but I think > it does remove the removed python source without "-A", as long as > you give a pathspec "*.py" (with quotes around it) that match it. > > I think it is just the warning code avoiding extra complexity and > overhead, if you are talking about not getting warning in the > pre-2.0 step that is in 'next'. Patches are very much welcomed, > especially the ones that come before I get around to it ;-) I took a brief look at the code, and as you said "add" needs to know about submodules, and the best fix looks to me to take the same approach Jonathan came up with to de-noise the "add -u/-A" topic. That is, to scan the working tree to actually see if we would record removals to the index in 2.0, but not remove them in this current version, and give the warning when the differences in the behaviours matter. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Tue, Apr 16, 2013 at 5:45 PM, Phil Hord wrote: > On Tue, Apr 16, 2013 at 3:04 PM, Felipe Contreras > wrote: >> On Tue, Apr 16, 2013 at 4:59 AM, Thomas Rast wrote: >>> A cursory look^W^Wreview of the messages in fc/remote-hg: >> >> [skipping irrelevant comments] >> >> I'm sorry, did you actually hit an issue that required to look at the >> commit message to understand where the issue came from? No? Then I >> won't bother with hypotheticals. >> >> If you want to waste your time, by all means, rewrite all my commit >> messages with essays that nobody will ever read. I'm not going to do >> that for some hypothetical case that will never happen. I'm not going >> to waste my time. > > This is not a hypothetical. Almost every time I bisect a regression > in git.git, I find the commit message tells me exactly why the commit > did what it did and what the expected result was. I find this to be > amazingly useful. Do I need to show you real instances of that > happening? No. I promise it did, though. Yes please. Show me one of the instances where you hit a bisect with any of the remote-hg commits mentioned above by Thomas Rast. > Of course, 99% of the commit messages may never be useful to me or > anyone else. But we do not eschew them altogether. The 1% I have to > rely on are nearly always helpful and clear, and that is the part I > care about. And how do you know this will be part of the 1%? You don't. How many times have you tracked regressions in transport helper's import/export functionality? How many times in remote-hg? How many times has *anybody* done so? > If you will not waste your time to write a decent commit message, why > do you waste our time asking us to review and accept ill-defined > patches? Because it *fixes a problem*. And a commit essay doesn't fix any, because nobody will ever go back in history and wonder, hey, what is up with this commit. If somebody does, then I will accept that commit essays are always a must. But it won't happen. > Here, of course, I use the royal "us" as I do not review > your patches. I do not know why that is; I suppose you patch things > outside of my interests, but it may also be that your patches are > simply incomprehensible by design. Yeah, but that's the thing, if you don't understand the code the patches are changing, then how can you know the commit message is sufficient to figure things out when a regression is found? You don't. You can't. Let's face the truth, you are advocating for stopping progress on the name that something might happen sometime in the feature, although most likely won't. When in reality, it just won't. And you are not saying "it would be nice to have full commit essay", you are saying: "without a commit essay this patch should NOT be merged", even more "without a commit essay this patch should NOT be considered a cooking patch". I think the commit message is fine, you don't. So YOU go ahead and write the proper one. If you don't, all you are doing is being an impediment to progress. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Wed, Apr 17, 2013 at 11:14:42AM -0700, Junio C Hamano wrote: > > I think it is just the warning code avoiding extra complexity and > > overhead, if you are talking about not getting warning in the > > pre-2.0 step that is in 'next'. Patches are very much welcomed, > > especially the ones that come before I get around to it ;-) > > I took a brief look at the code, and as you said "add" needs to know > about submodules, and the best fix looks to me to take the same > approach Jonathan came up with to de-noise the "add -u/-A" topic. > > That is, to scan the working tree to actually see if we would record > removals to the index in 2.0, but not remove them in this current > version, and give the warning when the differences in the behaviours > matter. Yeah, I had the same thought, as this warning has been bugging me for the last day or two. The worst part about it is that I finally trained myself to type "git add ." to silence the _other_ warning, and now it triggers this one. :) -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Felipe Contreras writes: > And how do you know this will be part of the 1%? You don't. How many > times have you tracked regressions in transport helper's import/export > functionality? How many times in remote-hg? How many times has > *anybody* done so? The last point makes it all the more important to have a good history [*1*]. An area that no developer rarely touches with a little user base can stay dormant for a long time, and when people do need to hunt for an ancient bug or to enhance the existing feature to support a new use case without breaking the old use case, the original author may not be around, lost interest, or no longer uses his own creation. The code left behind tells us what the author thought was the best way to solve his problem, but it does not clearly define what the problem he tried to solve was, within what constraint he had to find a solution for it, and why he thought that the solution was the best (or sometimes "only") one. Log and in-code comments are to explain such things that are beyond how the code works and what it does. [Footnote] *1* In this message, I am not judging if the depth of your writing for the particular change is deep enough. It depends on how well the reader knows the area, and there is no single right answer to that question. Incidentally that is why we tend to err on the more descriptive side. The next person your commit will help may not know the area as well as you do and has to figure things out on his own. You are helping him by being descriptive. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Jeff King writes:
> Yeah, I had the same thought, as this warning has been bugging me for
> the last day or two. The worst part about it is that I finally trained
> myself to type "git add ." to silence the _other_ warning, and now it
> triggers this one. :)
So here is the "reworked" one on top of what is in 'next'.
It introduces a bit of conflict with the "add -u/-A" topic, so I am
not ready to push out the integration result yet.
-- >8 --
Subject: [PATCH] git add: rework the logic to warn "git add ..."
default change
The earlier logic to warn against "git add subdir" that is run
without "-A" or "--no-all" was only to check any given
exactly spells a directory name that (still) exists on the
filesystem. This had number of problems:
* "git add '*dir'" (note that the wildcard is hidden from the
shell) would not trigger the warning.
* "git add '*.py'" would behave differently between the current
version of Git and Git 2.0 for the same reason as "subdir", but
would not trigger the warning.
* "git add dir" for a submodule "dir" would just update the index
entry for the submodule "dir" without ever recursing into it, and
use of "-A" or "--no-all" would matter. But the logic only
checks the directory-ness of "dir" and gives an unnecessary
warning.
Rework the logic to detect the case where the behaviour will be
different in Git 2.0, and issue a warning only when it matters.
Even with the code before this warning, "git add subdir" will have
to traverse the directory in order to find _new_ files the index
does not know about _anyway_, so we can do this check without adding
an extra pass to find if matches any removed file.
This essentially updates the "add_files_to_cache()" public API to
"update_files_in_cache()" API that is internal to "git add", because
with the "--all" option, the function is no longer about "adding"
paths to the cache, but is also used to remove them.
There are other callers of the former from "checkout" (used when
"checkout -m" prepares the temporary tree that represents the local
modifications to be merged) and "commit" ("commit --include" that
picks up local changes in addition to what is in the index). Since
ADD_CACHE_IGNORE_ERRORS (aka "--no-all") is not used by either of
them, once dust settles after Git 2.0 and the warning becomes
unnecessary, we may want to unify these two functions again.
Signed-off-by: Junio C Hamano
---
builtin/add.c | 64 +++
1 file changed, 38 insertions(+), 26 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index f8f6c9e..4242bce 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -26,6 +26,9 @@ static int take_worktree_changes;
struct update_callback_data {
int flags;
int add_errors;
+
+ /* only needed for 2.0 transition preparation */
+ int warn_add_would_remove;
};
static int fix_unmerged_status(struct diff_filepair *p,
@@ -49,6 +52,17 @@ static int fix_unmerged_status(struct diff_filepair *p,
return DIFF_STATUS_MODIFIED;
}
+static void warn_add_would_remove(const char *path)
+{
+ warning(_("In Git 2.0, 'git add ...' will also update the\n"
+ "index for paths removed from the working tree that match\n"
+ "the given pathspec. If you want to 'add' only changed\n"
+ "or newly created paths, say 'git add --no-all ...'"
+ " instead.\n\n"
+ "'%s' would be removed from the index without --no-all."),
+ path);
+}
+
static void update_callback(struct diff_queue_struct *q,
struct diff_options *opt, void *cbdata)
{
@@ -70,6 +84,10 @@ static void update_callback(struct diff_queue_struct *q,
}
break;
case DIFF_STATUS_DELETED:
+ if (data->warn_add_would_remove) {
+ warn_add_would_remove(path);
+ data->warn_add_would_remove = 0;
+ }
if (data->flags & ADD_CACHE_IGNORE_REMOVAL)
break;
if (!(data->flags & ADD_CACHE_PRETEND))
@@ -81,20 +99,27 @@ static void update_callback(struct diff_queue_struct *q,
}
}
-int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
+static void update_files_in_cache(const char *prefix, const char **pathspec,
+ struct update_callback_data *data)
{
- struct update_callback_data data;
struct rev_info rev;
init_revisions(&rev, prefix);
setup_revisions(0, NULL, &rev, NULL);
init_pathspec(&rev.prune_data, pathspec);
rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
rev.diffopt.format_callback = update_callback;
- data.flags = flags;
- data.add_errors = 0;
- rev.diffopt.format_callback_data = &da
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Wed, Apr 17, 2013 at 6:56 PM, Junio C Hamano wrote: > Felipe Contreras writes: > >> And how do you know this will be part of the 1%? You don't. How many >> times have you tracked regressions in transport helper's import/export >> functionality? How many times in remote-hg? How many times has >> *anybody* done so? > > The last point makes it all the more important to have a good > history [*1*]. An area that no developer rarely touches with a little > user base can stay dormant for a long time, and when people do need > to hunt for an ancient bug or to enhance the existing feature to > support a new use case without breaking the old use case, the > original author may not be around, lost interest, or no longer uses > his own creation. You are going in circles, I said such situation was *HYPOTHETICAL*, Phil Hord said it wasn't, and now you are bringing back more hypothetical examples, which I would gladly address, as soon as you accept they are HYPOTHETICAL. Now, how about you answer the questions about the *REAL* situations Phil Hord mentioned? * How many times have you tracked regressions in transport helper's import/export functionality? Hint: zero. * How many times in remote-hg? Hint: zero. * How many times has *anybody* done so? Hint: other than me, quite possibly zero. And then, before we consider this *hypothetical* situation, it might be worth noticing what commit this hypothetical person would hit if you do *not* apply this patch, and what the commit message says: --- remote-helpers: add support for an export command Signed-off-by: Junio C Hamano --- Yeah, well, glad you didn't apply my patch, wouldn't want to mess up the code that was clearly explained by that commit message. And before you rationalize the above commit, because maybe the functionality was described in the documentation, it wasn't: transport-helper.c | 132 ++--- 1 file changed, 120 insertions(+), 12 deletions(-) If you do apply my patch, it turns out even the shortest version of my commit message already gives more information to this *hypothetical* developer person. > [Footnote] > > *1* In this message, I am not judging if the depth of your writing > for the particular change is deep enough. It depends on how well > the reader knows the area, and there is no single right answer > to that question. > > Incidentally that is why we tend to err on the more descriptive > side. The next person your commit will help may not know the > area as well as you do and has to figure things out on his > own. You are helping him by being descriptive. I partially agree with this, but I think documenting the nuts and bolts of transport-helper would be better in done in code, documentation, tests, and mailing list analysis. And in all those respects, I believe I've done a more than adequate job. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Felipe Contreras writes: > * How many times have you tracked regressions in transport helper's > import/export functionality? > > Hint: zero. The real question to make the situation non-hypothetical would actually be "how many times did you track a regression that bisected down to *this particular commit*". Any regression that ends up on another commit is irrelevant. I guess you realize how stupid my argument is. But how is yours different? You do realize that your claim that nobody is ever going to bisect down to your commit is as hypothetical as other people's claim (if you think it is not, then try to point us a proof that nobody is ever going to need a good message in the future to understand what I mean). We're trying to make all the code and all the commits clean. It seems to be a consensus here that review is good. I see no reason to purposely make some commits less good than others based on the fact that they may not be used in the future. Search your favorite search engine for "broken window principle" to get more arguments in this direction. > * How many times has *anybody* done so? > > Hint: other than me, quite possibly zero. If you want to be the only developer, and avoid being disturbed by others, then why are you pushing your changes to git.git? Why are you even discussing on this list? -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Thu, Apr 18, 2013 at 2:44 AM, Matthieu Moy wrote: > Felipe Contreras writes: > >> * How many times have you tracked regressions in transport helper's >> import/export functionality? >> >> Hint: zero. > > The real question to make the situation non-hypothetical would actually > be "how many times did you track a regression that bisected down to > *this particular commit*". Any regression that ends up on another commit > is irrelevant. > > I guess you realize how stupid my argument is. But how is yours > different? I did not make any argument (stupid or otherwise), I made I claim; I won't waste my time with hypotheticals. > You do realize that your claim that nobody is ever going to > bisect down to your commit is as hypothetical as other people's claim > (if you think it is not, then try to point us a proof that nobody is > ever going to need a good message in the future to understand what I > mean). Yeah, they are both hypotheticals, the only difference is that your claim is very easy to prove; all you need is *ONE* example. But I'm very happy to withdraw my claim, as long as you withdraw your claim as well, and we go back to the default position: we don't know if anybody will every look at these commit messages again. > We're trying to make all the code and all the commits clean. It seems to > be a consensus here that review is good. I see no reason to purposely > make some commits less good than others based on the fact that they may > not be used in the future. You have to prove first that they are "less good", and the best way to do that is provide commit messages of your own, if you do that, they can be used instead, but if you don't, what do you propose to do? Drop the patches? > Search your favorite search engine for "broken window principle" to get > more arguments in this direction. More like broken windows hypothesis, which is not without its critics. >> * How many times has *anybody* done so? >> >> Hint: other than me, quite possibly zero. > > If you want to be the only developer, and avoid being disturbed by > others, then why are you pushing your changes to git.git? Why are you > even discussing on this list? Doesn't matter, it's still *HYPOTHETICAL* that anybody will every hit this in a bisect. Now, if you agree it's all hypothetical, the next rational thing to do is risk analysis: how likely is it to happen, and what would be the impact if it does? The answer to both questions is: close to *ZERO*. So, considering the nature of these patches (a remote-helper in the contrib area that is relatively new), and the active developers (me), I'd say it's much more important to get the fixes in, than to document every little quirk, detail and reasoning behind them. It's the balance I think it's best at this point, and it is my time, and it is my decision what I do with it. It might also help to compare oranges with oranges, and with regards to remote-hg transport helpers, I do believe the one in contrib/remote-helpers has the best commit messages: msysgit's remote-hg: --- commit 6bbd5365988d63780acc2ab407878eef8c19b47c Author: Sverre Rabbelier Date: Sun Aug 22 01:22:14 2010 -0500 git_remote_helpers: add fastimport library git_remote_helpers/fastimport/__init__.py | 0 git_remote_helpers/fastimport/commands.py | 469 + git_remote_helpers/fastimport/dates.py| 79 +++ git_remote_helpers/fastimport/errors.py | 182 + git_remote_helpers/fastimport/head_tracker.py | 47 +++ git_remote_helpers/fastimport/helpers.py | 88 + git_remote_helpers/fastimport/idmapfile.py| 65 + git_remote_helpers/fastimport/parser.py | 621 ++ git_remote_helpers/fastimport/processor.py| 222 +++ git_remote_helpers/setup.py | 3 +- 10 files changed, 1775 insertions(+), 1 deletion(-) --- gitifyhg: -- commit 4b364563cd705dc5e69082e6b80d304fe50b9c9c Author: Alex Sydell Date: Sat Mar 23 23:46:33 2013 -0700 Report correct (instead of unknown) hashes when importing refs into git gitifyhg/gitifyhg.py | 27 +-- gitifyhg/hgimporter.py | 20 ++-- gitifyhg/util.py | 44 test/test_push.py | 31 +++ 4 files changed, 94 insertions(+), 28 deletions(-) --- And of course, the best place to discuss the lack of good commit messages, is in the patches themselves, which are after all, sent to the mailing list for everyone to review. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Felipe Contreras wrote: > I think the commit message is fine, you don't. So YOU go ahead and > write the proper one. If you don't, all you are doing is being an > impediment to progress. Hey Felipe. Let's get a few things straightened out first: - We all act in our selfish interests, and write code to scratch our personal itches. I don't write code or commit messages for anyone else, and neither should you. - However, we're not working in isolation. We have this giant mailing list where we all post our patches. It's like a bazaar where we compete against other patches for developer attention and potential reviewers. In other words, it's a free market, and we're selling our product: if it fails to sell, will you blame the market or your product? I write clear code and beautiful commit messages exactly for this reason: I'm fighting for attention! - We have to learn to interoperate with others' code and conventions, if we want to be part of the community. That doesn't mean that we drown out our individuality, but it means that a our patch series has to conform to some minimal, loose, and evolving standard. Now, you can argue that many of the existing conventions are outdated (I do it all the time), but it cannot change overnight. Your influence on the community will show up over an extended period of time. - We are not an old enterprise who blame breakages on a few individuals, and fire them. We're a community where all of us are equally responsible for all parts of the code. I am as responsible for the remote-hg code in master as you are, as I had every opportunity to review it when the patch series came up on the list. I might have chosen not to, but that doesn't relieve me of responsibility. - We don't practice division of labour. There are no managers, "testing people", "documentation people", "code-writing people", "commit-message writing people" etc. Everyone has to do some portion of all these tasks, although we try to keep the boring work/ technical debt to a minimum. Don't ask other people to write commit messages for your code. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Thu, Apr 18, 2013 at 4:19 AM, Ramkumar Ramachandra wrote: > Felipe Contreras wrote: >> I think the commit message is fine, you don't. So YOU go ahead and >> write the proper one. If you don't, all you are doing is being an >> impediment to progress. > > Hey Felipe. Let's get a few things straightened out first: > > - We all act in our selfish interests, and write code to scratch our > personal itches. I don't write code or commit messages for anyone > else, and neither should you. > > - However, we're not working in isolation. We have this giant mailing > list where we all post our patches. It's like a bazaar where we > compete against other patches for developer attention and potential > reviewers. In other words, it's a free market, and we're selling our > product: if it fails to sell, will you blame the market or your > product? I write clear code and beautiful commit messages exactly for > this reason: I'm fighting for attention! Except the customers are not git developers, it's git users. Git developers rejecting patches because of the commit message is akin to distributors rejecting products because they don't like the transportation packages; they are only hurting themselves, by hurting their customers. > - We have to learn to interoperate with others' code and conventions, > if we want to be part of the community. That doesn't mean that we > drown out our individuality, but it means that a our patch series has > to conform to some minimal, loose, and evolving standard. Now, you > can argue that many of the existing conventions are outdated (I do it > all the time), but it cannot change overnight. Your influence on the > community will show up over an extended period of time. And the only way it can change is by discussing. The only one that gets bitten by fixes not getting merged are git users, not me. So if a discussion of a commit message impedes the merging of the commit, I don't get affected, but when we have agreed to disagree on what constitutes a good message, and the patch is still on hold, then there's a problem. > - We are not an old enterprise who blame breakages on a few > individuals, and fire them. We're a community where all of us are > equally responsible for all parts of the code. I am as responsible > for the remote-hg code in master as you are, as I had every > opportunity to review it when the patch series came up on the list. I > might have chosen not to, but that doesn't relieve me of > responsibility. I don't think so. Unless you added your Signed-off-by, you are not. > - We don't practice division of labour. There are no managers, > "testing people", "documentation people", "code-writing people", > "commit-message writing people" etc. Everyone has to do some portion > of all these tasks, although we try to keep the boring work/ technical > debt to a minimum. Don't ask other people to write commit messages > for your code. I am not. Neither should they ask me to write the commit messages they want. They can make *suggestions*, and I can reject them. When two persons have different ideas, often times both are wrong, and the middle-ground is best, but sometimes a person reaches the middle-ground, and sometimes one person was right from the start. But when everyone shares the *assumption* that there is never a commit message that is too long, you know the wrestling mat of ideas is rigged. I wonder if I should write a commit message as long as a book chapter for a one-liner, only to prove a point, but I'm honestly afraid that it would be committed as is. And remember what started the conversation; do you think a patch with a possibly incomplete commit message should not be merged to pu (proposed updates), shouldn't even be mentioned in the "what's cooking" mail, and thus shouldn't even be considered "cooking"? Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Felipe Contreras wrote: > Except the customers are not git developers, it's git users. Git > developers rejecting patches because of the commit message is akin to > distributors rejecting products because they don't like the > transportation packages; they are only hurting themselves, by hurting > their customers. Huh? I certainly don't develop for some "git users" I don't even know or care about. In this order of precedence, my customers are: 1. Me. 2. People who develop git.git, whom I have to cooperate with. 3. People who exercise git heavily like the linux.git community, as opposed to some little projects that operate using pull requests on GitHub. ... 137. People who incidentally choose to use git. 138. People who incidentally choose to use git, but aren't on Linux. I don't know if Junio or the others share this view, but this is how I personally operate and I'm very happy. And nobody is hurting anyone else. Someone wrote some code, and failed to sell it to the community. That's all happened. > The only one that gets bitten by fixes not getting merged are git > users, not me. So if a discussion of a commit message impedes the > merging of the commit, I don't get affected, but when we have agreed > to disagree on what constitutes a good message, and the patch is still > on hold, then there's a problem. I think the whole issue of whether your commit message conforms to some transcendental standard is orthogonal to the issue. Is your patch getting attention? Has it attracted reviewers, and turned up on the latest "What's cooking"? > I don't think so. Unless you added your Signed-off-by, you are not. Okay, so your view differs. > I am not. Neither should they ask me to write the commit messages they > want. They can make *suggestions*, and I can reject them. Ofcourse you have a right to reject suggestions. The question at the end of the day doesn't change: did you manage to get people to read your patch? > When two persons have different ideas, often times both are wrong, and > the middle-ground is best, but sometimes a person reaches the > middle-ground, and sometimes one person was right from the start. https://yourlogicalfallacyis.com/middle-ground :) > But when everyone shares the *assumption* that there is never a commit > message that is too long, you know the wrestling mat of ideas is > rigged. I wonder if I should write a commit message as long as a book > chapter for a one-liner, only to prove a point, but I'm honestly > afraid that it would be committed as is. I'm with you, and don't share that assumption. I'm not accusing you of writing commit messages that don't conform to some "transcendental standard" either: I didn't look at your patches in the first place, because the simple Signed-off-by: one-liner in the body didn't really make me want to read it. > And remember what started the conversation; do you think a patch with > a possibly incomplete commit message should not be merged to pu > (proposed updates), shouldn't even be mentioned in the "what's > cooking" mail, and thus shouldn't even be considered "cooking"? It's irrelevant what I think or others think. The point is that it wasn't mentioned. Now, why wasn't it mentioned? Is it because Junio and the community hate you, and are conspiring against getting your code merged? Or is it because it didn't catch anyone's eye, and Junio was waiting for it to happen (as always)? TL;DR version: Your goal in submitting a patch is to sell it to other people in the community. If enough people like your patch, it gets merged (but that is only the second step). Your goal is not to fix problems for some unknown "users", or argue about some "transcendental standard". Ofcourse the community shares some view about what a patch should look like, but you can mould those expectations gradually. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Thu, Apr 18, 2013 at 5:27 AM, Ramkumar Ramachandra wrote: > Felipe Contreras wrote: >> Except the customers are not git developers, it's git users. Git >> developers rejecting patches because of the commit message is akin to >> distributors rejecting products because they don't like the >> transportation packages; they are only hurting themselves, by hurting >> their customers. > > Huh? I certainly don't develop for some "git users" I don't even know > or care about. In this order of precedence, my customers are: > > 1. Me. > 2. People who develop git.git, whom I have to cooperate with. > 3. People who exercise git heavily like the linux.git community, as > opposed to some little projects that operate using pull requests on > GitHub. > ... > 137. People who incidentally choose to use git. > 138. People who incidentally choose to use git, but aren't on Linux. As they are for most open source developers on the planet, not me. I believe a project is nothing without its users. > And nobody is hurting anyone else. Someone wrote some code, and > failed to sell it to the community. That's all happened. If remote-hg wasn't available for users, they would be hurt; if stash wasn't available, if rebase --interactive didn't exist, if there was no msysgit, if it wasn't so fast, if the object model wasn't so simple and extensible; users would be hurt. And if users didn't have all these, there would be less users, and if there were less users, there would be less developers, and mercurial might have been more popular, and most repositories you have to work on would be in mercurial, and you might be developing mercurial right now. But I won't bother trying to convince you that no project is more important than its users (in the words of Linus Torvalds), because most people don't see the big picture. >> I don't think so. Unless you added your Signed-off-by, you are not. > > Okay, so your view differs. > >> I am not. Neither should they ask me to write the commit messages they >> want. They can make *suggestions*, and I can reject them. > > Ofcourse you have a right to reject suggestions. The question at the > end of the day doesn't change: did you manage to get people to read > your patch? No, at the end of the day what matters is: did the users benefit from this? The answer for this particular patch is no, and it's not my fault. >> When two persons have different ideas, often times both are wrong, and >> the middle-ground is best, but sometimes a person reaches the >> middle-ground, and sometimes one person was right from the start. > > https://yourlogicalfallacyis.com/middle-ground :) Yeah, but I didn't claim that, I said sometimes one person was right from the start, no middle-ground. >> But when everyone shares the *assumption* that there is never a commit >> message that is too long, you know the wrestling mat of ideas is >> rigged. I wonder if I should write a commit message as long as a book >> chapter for a one-liner, only to prove a point, but I'm honestly >> afraid that it would be committed as is. > > I'm with you, and don't share that assumption. s/everyone/almost everyone/ > I'm not accusing you > of writing commit messages that don't conform to some "transcendental > standard" either: I didn't look at your patches in the first place, > because the simple Signed-off-by: one-liner in the body didn't really > make me want to read it. > >> And remember what started the conversation; do you think a patch with >> a possibly incomplete commit message should not be merged to pu >> (proposed updates), shouldn't even be mentioned in the "what's >> cooking" mail, and thus shouldn't even be considered "cooking"? > > It's irrelevant what I think or others think. The point is that it > wasn't mentioned. Now, why wasn't it mentioned? Is it because Junio > and the community hate you, and are conspiring against getting your > code merged? Or is it because it didn't catch anyone's eye, and Junio > was waiting for it to happen (as always)? My abridged version of the story is: because Jeff King pointed to an area of improvement he wasn't even strongly attached to, I agreed to resubmit, Junio saw that, then I changed my mind, Junio probably didn't see that, and then he forgot about it. Then, since it's taboo to suggest that a concise commit message is fine, a discussion sprung. > TL;DR version: Your goal in submitting a patch is to sell it to other > people in the community. I disagree. > If enough people like your patch, it gets > merged (but that is only the second step). Your goal is not to fix > problems for some unknown "users", or argue about some "transcendental > standard". I disagree. > Ofcourse the community shares some view about what a patch > should look like, but you can mould those expectations gradually. If experience is any guide, doesn't look like that. But I've noticed that after many months that a patch has been sent people realize that it's more important to get the damn issu
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Since you disagreed with the rest, I'll only respond to this part: Felipe Contreras wrote: > But I won't bother trying to convince you that no project is more > important than its users (in the words of Linus Torvalds), because > most people don't see the big picture. I didn't say otherwise. What I'm saying is: my personal incentive to write code does not prioritize the supposed benefit of some unknown "user" somewhere on the planet above everything else. My personal incentive prioritizes me, and my immediate circle (ie. the git community). The benefit propagates outwards to extended circles until it reaches the people I care least about: incidental end-users. That's how people are connected: how can I care about distant unknown people I'm not connected to? The people in the outermost circles benefit the least, because they didn't get a say in the development. All they can do is write a rant about it on their blog, and hope that it gets fixed someday. You just ditched us, the inner circle of people who care about your work the most, and are instead trying to convince us that we're hurting some unknown hypothetical "users" by not merging your code immediately. If you think these users are more important to you than we are, then why are you posting your code on this mailing list? Start your own project that's focused on satisfying these users. It doesn't even need to be open source or have a community of reviewers, because all you care about are users. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Okay, one more segment needs to be responded to. Felipe Contreras wrote: > If remote-hg wasn't available for users, they would be hurt; if stash > wasn't available, if rebase --interactive didn't exist, if there was > no msysgit, if it wasn't so fast, if the object model wasn't so simple > and extensible; users would be hurt. And if users didn't have all > these, there would be less users, and if there were less users, there > would be less developers, and mercurial might have been more popular, > and most repositories you have to work on would be in mercurial, and > you might be developing mercurial right now. Flawed logic. A large number of users doesn't automatically imply good software with lots of features, or even a great development community. A great development community leads to great software. And great software leads to lots of users. Sure, there's a feedback loop pushing users to become developers; but it doesn't start with users of vaporware, leading to more developers joining the effort to turn that vaporware into a great product. Life doesn't begin with users. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Thu, Apr 18, 2013 at 6:31 AM, Ramkumar Ramachandra wrote: > Since you disagreed with the rest, I'll only respond to this part: > > Felipe Contreras wrote: >> But I won't bother trying to convince you that no project is more >> important than its users (in the words of Linus Torvalds), because >> most people don't see the big picture. > > I didn't say otherwise. What I'm saying is: my personal incentive to > write code does not prioritize the supposed benefit of some unknown > "user" somewhere on the planet above everything else. My personal > incentive prioritizes me, and my immediate circle (ie. the git > community). The benefit propagates outwards to extended circles until > it reaches the people I care least about: incidental end-users. If the people that matter most are given the worst prioritization, it means the prioritization is wrong. > That's how people are connected: how can I care about distant unknown > people I'm not connected to? It's called empathy. > The people in the outermost circles > benefit the least, because they didn't get a say in the development. > All they can do is write a rant about it on their blog, and hope that > it gets fixed someday. To the detriment of the project. > You just ditched us, the inner circle of people who care about your > work the most, and are instead trying to convince us that we're > hurting some unknown hypothetical "users" by not merging your code > immediately. The users are real, the developers that will look retroatcively to the commit message of this patch are not. > If you think these users are more important to you than we are, then > why are you posting your code on this mailing list? What other way is there for this code to reach the users? > Start your own > project that's focused on satisfying these users. Start a new project so I can include a patch that hasn't made it yet into the "what's cooking" in one week? That's ridiculous. > It doesn't even > need to be open source or have a community of reviewers, because all > you care about are users. Who said *all* that matters are the users? And even if somebody did, ultimately a closed source proprietary software doesn't benefit the users, so either way it has to be open and active to benefit the users. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Thu, Apr 18, 2013 at 6:46 AM, Ramkumar Ramachandra wrote: > Okay, one more segment needs to be responded to. > > Felipe Contreras wrote: >> If remote-hg wasn't available for users, they would be hurt; if stash >> wasn't available, if rebase --interactive didn't exist, if there was >> no msysgit, if it wasn't so fast, if the object model wasn't so simple >> and extensible; users would be hurt. And if users didn't have all >> these, there would be less users, and if there were less users, there >> would be less developers, and mercurial might have been more popular, >> and most repositories you have to work on would be in mercurial, and >> you might be developing mercurial right now. > > Flawed logic. > > A large number of users doesn't automatically imply good software with > lots of features, or even a great development community. A great > development community leads to great software. And great software > leads to lots of users. Sure, there's a feedback loop pushing users > to become developers; but it doesn't start with users of vaporware, > leading to more developers joining the effort to turn that vaporware > into a great product. > > Life doesn't begin with users. Nobody knows how life began, and it doesn't matter now, what matters is how life evolves. It doesn't matter if the chicken was first, or the egg, what matters is that if all the chickens and eggs are gone, there won't be more. Plenty of projects have died because they stopped caring about their users, and without users there's no new developers, and the old developers eventually move on, and all the literary quality of commit messages have no eyes to see it. I repeat: no project is more important than its users. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Wed, Apr 17, 2013 at 06:39:06PM -0700, Junio C Hamano wrote:
> Subject: [PATCH] git add: rework the logic to warn "git add ..."
> default change
>
> [...]
>
> Rework the logic to detect the case where the behaviour will be
> different in Git 2.0, and issue a warning only when it matters.
> Even with the code before this warning, "git add subdir" will have
> to traverse the directory in order to find _new_ files the index
> does not know about _anyway_, so we can do this check without adding
> an extra pass to find if matches any removed file.
Thanks, I think this is looking much better.
A few minor nits on the message itself:
> +static void warn_add_would_remove(const char *path)
> +{
> + warning(_("In Git 2.0, 'git add ...' will also update the\n"
> + "index for paths removed from the working tree that match\n"
> + "the given pathspec. If you want to 'add' only changed\n"
> + "or newly created paths, say 'git add --no-all ...'"
> + " instead.\n\n"
This wrapping looks funny in the actual output, due to the extra
"warning:" and the lack of newline before "instead":
warning: In Git 2.0, 'git add ...' will also update the
index for paths removed from the working tree that match
the given pathspec. If you want to 'add' only changed
or newly created paths, say 'git add --no-all ...' instead.
Wrapping it like this ends up with a much more natural-looking
paragraph:
warning: In Git 2.0, 'git add ...' will also update the
index for paths removed from the working tree that match the given
pathspec. If you want to 'add' only changed or newly created paths,
say 'git add --no-all ...' instead.
> + "'%s' would be removed from the index without --no-all."),
> + path);
I think mentioning the filename is a good thing; the original message
left me scratching my head and wondering "so, did you add it or not?".
I still think your "would be" is unnecessarily confusing, though. It is
"would be in Git 2.0 without --no-all, but we did not now". Which makes
sense when you think about it, but it took me a moment to parse.
Perhaps we can be more direct with something like:
warning: did not stage removal of 'foo'
or perhaps the present tense "not staging removal of..." would be
better.
I also think it makes sense to show every path that is affected, not
just the first one, to be clear about what was done (and what _would_
have been done in Git 2.0).
A patch with all of the suggestions together is below. I still think the
multi-line warning block looks ugly. I kind of like the way advise()
puts "hint:" on each line. I wonder if we should do the same here.
diff --git a/builtin/add.c b/builtin/add.c
index 4242bce..aae550a 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -52,15 +52,19 @@ static void warn_add_would_remove(const char *path)
return DIFF_STATUS_MODIFIED;
}
+static const char *add_would_remove_warning = N_(
+/* indent for "warning: " */
+ "In Git 2.0, 'git add ...' will also update the\n"
+"index for paths removed from the working tree that match the given\n"
+"pathspec. If you want to 'add' only changed or newly created paths,\n"
+"say 'git add --no-all ...' instead.\n");
+
static void warn_add_would_remove(const char *path)
{
- warning(_("In Git 2.0, 'git add ...' will also update the\n"
- "index for paths removed from the working tree that match\n"
- "the given pathspec. If you want to 'add' only changed\n"
- "or newly created paths, say 'git add --no-all ...'"
- " instead.\n\n"
- "'%s' would be removed from the index without --no-all."),
- path);
+ static int warned_once;
+ if (!warned_once++)
+ warning(_(add_would_remove_warning));
+ warning("did not stage removal of '%s'", path);
}
static void update_callback(struct diff_queue_struct *q,
@@ -84,10 +88,8 @@ static void update_callback(struct diff_queue_struct *q,
}
break;
case DIFF_STATUS_DELETED:
- if (data->warn_add_would_remove) {
+ if (data->warn_add_would_remove)
warn_add_would_remove(path);
- data->warn_add_would_remove = 0;
- }
if (data->flags & ADD_CACHE_IGNORE_REMOVAL)
break;
if (!(data->flags & ADD_CACHE_PRETEND))
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Jeff King writes:
> +static const char *add_would_remove_warning = N_(
> +/* indent for "warning: " */
> + "In Git 2.0, 'git add ...' will also update the\n"
> +"index for paths removed from the working tree that match the given\n"
> +"pathspec. If you want to 'add' only changed or newly created paths,\n"
> +"say 'git add --no-all ...' instead.\n");
> +
> static void warn_add_would_remove(const char *path)
> {
> - warning(_("In Git 2.0, 'git add ...' will also update the\n"
> - "index for paths removed from the working tree that match\n"
> - "the given pathspec. If you want to 'add' only changed\n"
> - "or newly created paths, say 'git add --no-all ...'"
> - " instead.\n\n"
> - "'%s' would be removed from the index without --no-all."),
> - path);
> + static int warned_once;
> + if (!warned_once++)
> + warning(_(add_would_remove_warning));
> + warning("did not stage removal of '%s'", path);
> }
Would "add --dry-run" say this, too?
> static void update_callback(struct diff_queue_struct *q,
> @@ -84,10 +88,8 @@ static void update_callback(struct diff_queue_struct *q,
> }
> break;
> case DIFF_STATUS_DELETED:
> - if (data->warn_add_would_remove) {
> + if (data->warn_add_would_remove)
> warn_add_would_remove(path);
> - data->warn_add_would_remove = 0;
> - }
> if (data->flags & ADD_CACHE_IGNORE_REMOVAL)
> break;
> if (!(data->flags & ADD_CACHE_PRETEND))
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Thu, Apr 18, 2013 at 10:51:12AM -0700, Junio C Hamano wrote:
> Jeff King writes:
>
> > +static const char *add_would_remove_warning = N_(
> > +/* indent for "warning: " */
> > + "In Git 2.0, 'git add ...' will also update the\n"
> > +"index for paths removed from the working tree that match the given\n"
> > +"pathspec. If you want to 'add' only changed or newly created paths,\n"
> > +"say 'git add --no-all ...' instead.\n");
> > +
> > static void warn_add_would_remove(const char *path)
> > {
> > - warning(_("In Git 2.0, 'git add ...' will also update the\n"
> > - "index for paths removed from the working tree that match\n"
> > - "the given pathspec. If you want to 'add' only changed\n"
> > - "or newly created paths, say 'git add --no-all ...'"
> > - " instead.\n\n"
> > - "'%s' would be removed from the index without --no-all."),
> > - path);
> > + static int warned_once;
> > + if (!warned_once++)
> > + warning(_(add_would_remove_warning));
> > + warning("did not stage removal of '%s'", path);
> > }
>
> Would "add --dry-run" say this, too?
It probably makes sense to continue to have the warning in the dry-run
case, but it may make sense to tweak it grammatically when we are in
dry-run mode. Saying "would stage removal" is technically correct, but I
think it is somewhat ambiguous: would git do it if we were not in a
--dry-run, or would git do it if it were Git 2.0?
Doing it as:
warning: not staging removal of '%s'
could work for both cases. Something like "not considering" (or another
synonym for "considering") might be even more accurate. It is not just
that we did not stage it; it is what we did not even consider it an item
for staging under the current rules.
Note that the "not staging" warnings may potentially be interspersed
with the normal dry-run output. I think that's OK. But another
alternative would be to collect the paths and then print:
warning: In Git 2.0, ...
The following deleted paths were not considered under the current
rule. Use "git add -A" to stage their removal now.
foo
bar
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Jeff King writes:
> could work for both cases. Something like "not considering" (or another
> synonym for "considering") might be even more accurate. It is not just
> that we did not stage it; it is what we did not even consider it an item
> for staging under the current rules.
Yes, "not considering" is much more sensible, while side-stepping
the dryrun issue. Or
warning("ignoring removal of '%s'")
> Note that the "not staging" warnings may potentially be interspersed
> with the normal dry-run output. I think that's OK.
As long as the top-text makes it clear what "not considering" (or
"ignoring") in the following text means, I think it is fine.
But I think we are doing users a disservice by listing tons of
paths. Where the difference of versions matters _most_ is when the
user has tons of removed paths in the working tree. Either with one
warning per path, or a block of collected paths at the end, we are
scrolling the more important part of the message up.
That was why I originally showed one path as an example and stopped
there. Perhaps it is a better solution to keep that behaviour and
rephrase the message to say that we ignored removal of paths like
this one '%s' you lost from the working tree but it will change in
Git 2.0 and you will better learn to use the --no-all option now.
The inter-topic conflicts between stages of three "add in Git 2.0"
topics is getting cumbersome even with the help from rerere, so I'd
like to merge their preparatory steps as I have them now to 'next'
and merge them down to 'master' first, and start applying tweaks
from there, or something.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Wed, Apr 17, 2013 at 2:50 PM, Felipe Contreras wrote: > On Tue, Apr 16, 2013 at 5:45 PM, Phil Hord wrote: >> On Tue, Apr 16, 2013 at 3:04 PM, Felipe Contreras >>> If you want to waste your time, by all means, rewrite all my commit >>> messages with essays that nobody will ever read. I'm not going to do >>> that for some hypothetical case that will never happen. I'm not going >>> to waste my time. >> >> This is not a hypothetical. Almost every time I bisect a regression >> in git.git, I find the commit message tells me exactly why the commit >> did what it did and what the expected result was. I find this to be >> amazingly useful. Do I need to show you real instances of that >> happening? No. I promise it did, though. > > Yes please. Show me one of the instances where you hit a bisect with > any of the remote-hg commits mentioned above by Thomas Rast. I made no such claim. In fact, I have never bisected to any remote-hg-related commit. I fail to see the relevance of this qualifier, though. P -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Thu, Apr 18, 2013 at 11:16:33AM -0700, Junio C Hamano wrote:
> Jeff King writes:
>
> > could work for both cases. Something like "not considering" (or another
> > synonym for "considering") might be even more accurate. It is not just
> > that we did not stage it; it is what we did not even consider it an item
> > for staging under the current rules.
>
> Yes, "not considering" is much more sensible, while side-stepping
> the dryrun issue. Or
>
>warning("ignoring removal of '%s'")
I like that much better than either of my suggestions.
> > Note that the "not staging" warnings may potentially be interspersed
> > with the normal dry-run output. I think that's OK.
>
> As long as the top-text makes it clear what "not considering" (or
> "ignoring") in the following text means, I think it is fine.
Agreed, and I think the current text is fine for that (though neither of
us is the best judge at this point of how a less familiar user would
interpret it).
> But I think we are doing users a disservice by listing tons of
> paths. Where the difference of versions matters _most_ is when the
> user has tons of removed paths in the working tree. Either with one
> warning per path, or a block of collected paths at the end, we are
> scrolling the more important part of the message up.
I'm not sure I agree. Even with a handful, it made me wonder why one was
mentioned and not others. That _could_ be cleared up by rewording (i.e.,
making it clear that this is an example, and there may be more). But
somehow listing them is what I would expect. Perhaps because it gives
the user a clue about what to do next; they ask themselves "did I want
those updated or not?".
In the orphaned-commit message when leaving a detached HEAD, we collect
the answer, say "you are leaving N commits", and show the first 5 five
of them, with an ellipsis at the end if we didn't show them all. Would
it makes sense to do that here?
Yet another alternative would be to print a warning for each path, but
hold the main warning for the end, so that it is the first thing the
user sees. That has the added bonus that regular "--dry-run" output
will not scroll it away, either.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Jeff King writes: >> But I think we are doing users a disservice by listing tons of >> paths. Where the difference of versions matters _most_ is when the >> user has tons of removed paths in the working tree. Either with one >> warning per path, or a block of collected paths at the end, we are >> scrolling the more important part of the message up. > > I'm not sure I agree. Even with a handful, it made me wonder why one was > mentioned and not others. That _could_ be cleared up by rewording (i.e., > making it clear that this is an example, and there may be more). But > somehow listing them is what I would expect. Perhaps because it gives > the user a clue about what to do next; they ask themselves "did I want > those updated or not?". > > In the orphaned-commit message when leaving a detached HEAD, we collect > the answer, say "you are leaving N commits", and show the first 5 five > of them, with an ellipsis at the end if we didn't show them all. Would > it makes sense to do that here? Because this is to help people who are _used_ to seeing "git add" not take the removals into account, I doubt that "Did I want those updated or not? Let me see the details of them." will be the question they will be asking [*1*]. I dunno. [Footnote] *1* "I know I didn't want to include these removals to the index, but I learned today that in later Git I should make myself more clear if I want to keep doing so; thanks for letting me know.", or "I've long been assuming that I have to say 'git add' and 'git rm' separately, but I learned today that I can say 'add --all', and in later Git I do not even have to; thanks for letting me know." are the two reactions I expected. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Thu, Apr 18, 2013 at 02:37:53PM -0700, Junio C Hamano wrote: > Because this is to help people who are _used_ to seeing "git add" > not take the removals into account, I doubt that "Did I want those > updated or not? Let me see the details of them." will be the > question they will be asking [*1*]. > > I dunno. > > > [Footnote] > > *1* "I know I didn't want to include these removals to the index, > but I learned today that in later Git I should make myself more > clear if I want to keep doing so; thanks for letting me know.", or > "I've long been assuming that I have to say 'git add' and 'git rm' > separately, but I learned today that I can say 'add --all', and in > later Git I do not even have to; thanks for letting me know." are > the two reactions I expected. I am expecting a reaction more like "Hmm, I never thought about it before. Does that make sense to me or not? Let me think about which paths it pertains to and decide". Which I admit is no more likely than the scenarios you outlined, but it is close to what I thought the first time I saw the warning. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Jeff King writes: > I am expecting a reaction more like "Hmm, I never thought about it > before. Does that make sense to me or not? Let me think about which > paths it pertains to and decide". Let's step back and re-review the main text. It currently says: In Git 2.0, 'git add ...' will also update the index for paths removed from the working tree that match the given pathspec. If you want to 'add' only changed or newly created paths, say 'git add --no-all ...' instead. This was written for the old "we may want to warn" logic that did not even check if we would be omitting a removal. The new logic will show the text _only_ when the difference matters, we have an opportunity to tighten it a lot, for example: You ran 'git add' with neither '-A (--all)' or '--no-all', whose behaviour will change in Git 2.0 with respect to paths you removed from your working tree. * 'git add --no-all ', which is the current default, ignores paths you removed from your working tree. * 'git add --all ' will let you also record the removals. The removed paths (e.g. '%s') are ignored with this version of Git. Run 'git status' to remind yourself what paths you have removed from your working tree. or something? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Thu, Apr 18, 2013 at 3:06 PM, Phil Hord wrote: > On Wed, Apr 17, 2013 at 2:50 PM, Felipe Contreras >> Yes please. Show me one of the instances where you hit a bisect with >> any of the remote-hg commits mentioned above by Thomas Rast. > > I made no such claim. In fact, I have never bisected to any > remote-hg-related commit. I fail to see the relevance of this > qualifier, though. Here, this is what you said: You: > Me: >> [skipping irrelevant comments] >> >> I'm sorry, did you actually hit an issue that required to look at the >> commit message to understand where the issue came from? No? Then I >> won't bother with hypotheticals. >> >> If you want to waste your time, by all means, rewrite all my commit >> messages with essays that nobody will ever read. I'm not going to do >> that for some hypothetical case that will never happen. I'm not going >> to waste my time. > > This is not a hypothetical. If something is not hypothetical, it's real, which means it actually happened, but then you said you never made the claim that it did. So what is it? Either it did happen, or it didn't; you cannot have your cake and eat it. If you are going to change your claims on the fly, and deny you ever made them, I don't see much point in discussing with you. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Thu, Apr 18, 2013 at 03:10:29PM -0700, Junio C Hamano wrote: > Jeff King writes: > > > I am expecting a reaction more like "Hmm, I never thought about it > > before. Does that make sense to me or not? Let me think about which > > paths it pertains to and decide". > > Let's step back and re-review the main text. Good idea. I was very caught up in the existing message and what it made me expect, and not in what we are trying to accomplish overall. > It currently says: > > In Git 2.0, 'git add ...' will also update the > index for paths removed from the working tree that match > the given pathspec. If you want to 'add' only changed > or newly created paths, say 'git add --no-all ...' > instead. > > This was written for the old "we may want to warn" logic that did > not even check if we would be omitting a removal. The new logic > will show the text _only_ when the difference matters, we have an > opportunity to tighten it a lot, for example: > > You ran 'git add' with neither '-A (--all)' or '--no-all', whose > behaviour will change in Git 2.0 with respect to paths you > removed from your working tree. > > * 'git add --no-all ', which is the current default, > ignores paths you removed from your working tree. > > * 'git add --all ' will let you also record the > removals. > > The removed paths (e.g. '%s') are ignored with this version of Git. > Run 'git status' to remind yourself what paths you have removed > from your working tree. > > or something? Yes, I like that much better. It reads more clearly than the original, and it is more obvious why we are mentioning the path at all. And I think the hint of "git status" is good. I had considered before that the user would simply run "git status" after the message to get more data, but I didn't want to rely on them knowing to do that. Actually mentioning it is a good solution. :) Thanks for pointing us in the right direction. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Junio C Hamano wrote: > You ran 'git add' with neither '-A (--all)' or '--no-all', whose > behaviour will change in Git 2.0 with respect to paths you > removed from your working tree. > > * 'git add --no-all ', which is the current default, > ignores paths you removed from your working tree. > > * 'git add --all ' will let you also record the > removals. > > The removed paths (e.g. '%s') are ignored with this version of Git. > Run 'git status' to remind yourself what paths you have removed > from your working tree. > > or something? That looks good. :) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Jonathan Nieder writes: > Junio C Hamano wrote: > >> You ran 'git add' with neither '-A (--all)' or '--no-all', whose >> behaviour will change in Git 2.0 with respect to paths you >> removed from your working tree. >> >> * 'git add --no-all ', which is the current default, >> ignores paths you removed from your working tree. >> >> * 'git add --all ' will let you also record the >> removals. >> >> The removed paths (e.g. '%s') are ignored with this version of Git. >> Run 'git status' to remind yourself what paths you have removed >> from your working tree. >> >> or something? > > That looks good. :) I think the direction may be good but the above is too tall to be the final version. of the message. Somebody good at phrasing needs to trim it down without losing the essense. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Thu, Apr 18, 2013 at 7:48 PM, Felipe Contreras wrote: > On Thu, Apr 18, 2013 at 3:06 PM, Phil Hord wrote: >> On Wed, Apr 17, 2013 at 2:50 PM, Felipe Contreras > >>> Yes please. Show me one of the instances where you hit a bisect with >>> any of the remote-hg commits mentioned above by Thomas Rast. >> >> I made no such claim. In fact, I have never bisected to any >> remote-hg-related commit. I fail to see the relevance of this >> qualifier, though. > > Here, this is what you said: > > You: >> Me: >>> [skipping irrelevant comments] >>> >>> I'm sorry, did you actually hit an issue that required to look at the >>> commit message to understand where the issue came from? No? Then I >>> won't bother with hypotheticals. >>> >>> If you want to waste your time, by all means, rewrite all my commit >>> messages with essays that nobody will ever read. I'm not going to do >>> that for some hypothetical case that will never happen. I'm not going >>> to waste my time. >> >> This is not a hypothetical. > > If something is not hypothetical, it's real, which means it actually > happened, but then you said you never made the claim that it did. So > what is it? My claim: I bisected to a commit whose commit message helped me deduce its entirety. Your fanciful interpretation, which I denied: "Show me one of the instances where you hit a bisect with any of the remote-hg commits mentioned above by Thomas Rast." I have never bisected to any commit related to remote-hg, and neither did I ever claim to. I do not know where you got such a ridiculous qualifier as this to append to my statement. > Either it did happen, or it didn't; It did. Where "it" is my actual claim, that I bisected to a commit whose commit message helped me deduce its entirety. But also, it didn't, where "it" is your preposterous interpretation of my interest and/or experiences with remote-hg commits. You seem only to want to argue, Felipe. I have neither time nor interest in pig-wrestling, myself. Phil -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Fri, Apr 19, 2013 at 10:25:46AM -0700, Junio C Hamano wrote: > Jonathan Nieder writes: > > > Junio C Hamano wrote: > > > >> You ran 'git add' with neither '-A (--all)' or '--no-all', whose > >> behaviour will change in Git 2.0 with respect to paths you > >> removed from your working tree. > >> > >> * 'git add --no-all ', which is the current default, > >> ignores paths you removed from your working tree. > >> > >> * 'git add --all ' will let you also record the > >> removals. > >> > >> The removed paths (e.g. '%s') are ignored with this version of Git. > >> Run 'git status' to remind yourself what paths you have removed > >> from your working tree. > >> > >> or something? > > > > That looks good. :) > > I think the direction may be good but the above is too tall to be > the final version. of the message. Somebody good at phrasing needs > to trim it down without losing the essense. Hmph. I actually like it as it is. It says: 1. Here's what triggered this warning (removed paths without -A). 2. Here is how you tell git what you want to do (--all/--no-all) 3. Here is how you get more information about what you wanted to do (mention one such path, point to "git status"). I would not want to cut out any of those three. You could perhaps cut out the bullet points in the middle, which reduces (2), but the user may be able to figure it out from the first sentence. However, I like the explicitness of those bullet points (and I prefer them to a wall of text which is more daunting to read). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Jeff King writes: > On Fri, Apr 19, 2013 at 10:25:46AM -0700, Junio C Hamano wrote: > >> Jonathan Nieder writes: >> >> > Junio C Hamano wrote: >> > >> >> You ran 'git add' with neither '-A (--all)' or '--no-all', whose >> >> behaviour will change in Git 2.0 with respect to paths you >> >> removed from your working tree. >> >> >> >> * 'git add --no-all ', which is the current default, >> >> ignores paths you removed from your working tree. >> >> >> >> * 'git add --all ' will let you also record the >> >> removals. >> >> >> >> The removed paths (e.g. '%s') are ignored with this version of Git. >> >> Run 'git status' to remind yourself what paths you have removed >> >> from your working tree. >> >> >> >> or something? >> > >> > That looks good. :) >> >> I think the direction may be good but the above is too tall to be >> the final version. of the message. Somebody good at phrasing needs >> to trim it down without losing the essense. > > Hmph. I actually like it as it is. It says: > > 1. Here's what triggered this warning (removed paths without -A). > > 2. Here is how you tell git what you want to do (--all/--no-all) > > 3. Here is how you get more information about what you wanted to do > (mention one such path, point to "git status"). > > I would not want to cut out any of those three. You could perhaps cut > out the bullet points in the middle, which reduces (2), but the user may > be able to figure it out from the first sentence. However, I like the > explicitness of those bullet points (and I prefer them to a wall of text > which is more daunting to read). I think we are saying the same thing. I do agree with you that these three points are "the essense" we do not want to lose. Shrinking the first paragraph and the last paragraph to less than three lines and each bullet point a single liner each was the kind of change I had in mind. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Fri, Apr 19, 2013 at 4:07 PM, Phil Hord wrote: > On Thu, Apr 18, 2013 at 7:48 PM, Felipe Contreras > wrote: >> If something is not hypothetical, it's real, which means it actually >> happened, but then you said you never made the claim that it did. So >> what is it? > > My claim: > >I bisected to a commit whose commit message helped me deduce its entirety. What a bold claim! That changes everything! Without your valuable input this discussion would have gone nowhere! This claim is absolutely worthless, nobody denies that somebody at some point in time did bisect a commit and a read it's commit message, and claiming what nobody denies makes as much sense as fighting the wind. Thanks for wasting all of our times. > Your fanciful interpretation, which I denied: > > "Show me one of the instances where you hit a bisect with > any of the remote-hg commits mentioned above by Thomas Rast." > > > I have never bisected to any commit related to remote-hg, and neither > did I ever claim to. I do not know where you got such a ridiculous > qualifier as this to append to my statement. That's *EXACTLY* the topic you replied to. Thomas Rast pointed to remote-hg commits, I asked if he hit an actual issue that required to look at the commit message, or such issue was hypothetical. Then you come along and say it isn't, Well, what isn't? If it's not the *EXACT* topic we were talking about. >> Either it did happen, or it didn't; > > It did. Where "it" is my actual claim, that I bisected to a commit > whose commit message helped me deduce its entirety. So "it" is absolutely unrelated to what we were talking about, and "it" was something nobody cared about, nor did help one iota to move the conversation forward. Thanks, thanks a lot. > But also, it didn't, where "it" is your preposterous interpretation of > my interest and/or experiences with remote-hg commits. > > You seem only to want to argue, Felipe. I have neither time nor > interest in pig-wrestling, myself. No, I don't want to argue, specially with people that either a) deny that they argued what they argued, or b) argue with pointless obvious claims about something that is not being discussed. Whether it's intellectual dishonesty, or plain madness, I'm not interested. Hitting an issue that required anybody to look at the commit messages of the commits that Thomas Rast mentioned, is a hypothetical situation. *Period*. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
[off-topic; what happened/happens to your series is entirely unrelated to the issue] Felipe Contreras wrote: > Nobody knows how life began, and it doesn't matter now, what matters > is how life evolves. It doesn't matter if the chicken was first, or > the egg, what matters is that if all the chickens and eggs are gone, > there won't be more. > > Plenty of projects have died because they stopped caring about their > users, and without users there's no new developers, and the old > developers eventually move on, and all the literary quality of commit > messages have no eyes to see it. I was a pure end-user of git until about Jan 2010. I was initially impressed with git because it behaved in a beautiful consistent manner. Then I dug in and found out that it had a beautiful codebase, excellent mailing list (content and conventions), and large development community. I could literally read through the commit messages and code with ease. I do bounce between a few projects, but always come back to git because nothing else fits the criterion. What I do not consider (as much as the other things) is the number-of-end-users. Then again, you would argue that I came across git only because of a large enough user-base. I agree with that, but you're practically idolizing user-base as the most important thing. My point is simple: yes, it's nice to have a big user base. We already do. Now, what's the point of pitching to end-users who only use the most basic functionality? Their inputs are likely to be useless (arising from misunderstandings) anyway. They're not going to be the next developers. And they're not going to help create what our next developer is looking for in us either (i.e. codebase, community). Our primary customers are each other, because that's how we get a tight community and great codebase. And because the next potential developer looks like one of us. That does _not_ mean: live only within the community. Everyone should have a healthy interaction with the outside world, otherwise they risk turning into researchers and suffering engineering myopia. And ofcourse not attract a large userbase. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Tue, Apr 23, 2013 at 1:49 PM, Ramkumar Ramachandra wrote: > My point is simple: yes, it's nice to have a big user base. We > already do. Now, what's the point of pitching to end-users who only > use the most basic functionality? Their inputs are likely to be > useless (arising from misunderstandings) anyway. They're not going to > be the next developers. And they're not going to help create what our > next developer is looking for in us either (i.e. codebase, community). That is your mistake right there. They *are* the next developers, you yourself came from there. We all did. In fact, this notion that there's a divide between users and developers is a myth; it's a continuum that follows the Pareto distribution. It happens in every healthy open source project. And this is not an assumption, I've measured it: http://felipec.wordpress.com/2011/11/21/no-project-is-more-important-than-its-users/ 70% of the commits in git.git come from people that have provided less than 6 patches. That 70% (maybe 80%, maybe 90%) would have never happened, if git didn't have a large enough user-base. I'm not idolizing the user-base, this project *is* the user-base, developers are users, and without users there's no project. Again, in the words of Linus: no project is more important than it's users. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
jc/add-2.0-delete-default (Re: What's cooking in git.git (Apr 2013, #05; Mon, 15))
> On Fri, Apr 19, 2013 at 10:25:46AM -0700, Junio C Hamano wrote:
>>> Junio C Hamano wrote:
You ran 'git add' with neither '-A (--all)' or '--no-all', whose
behaviour will change in Git 2.0 with respect to paths you
removed from your working tree.
* 'git add --no-all ', which is the current default,
ignores paths you removed from your working tree.
* 'git add --all ' will let you also record the
removals.
The removed paths (e.g. '%s') are ignored with this version of Git.
Run 'git status' to remind yourself what paths you have removed
from your working tree.
or something?
[...]
>> Somebody good at phrasing needs
>> to trim it down without losing the essense.
By the way, it was mentioned on IRC that the above is a bit odd for
a different reason: the option --no-all that maintains the old behavior
is not intuitively named.
How about something like this?
warning: "git add" run on path with files removed (e.g., '%s')
hint: use "git add --ignore-removals " to ignore removals
hint: or "git add --no-ignore-removals " to notice them
hint: --ignore-removals is the default but this will change soon
hint: see git-add(1) for details
Then the --ignore-removals option could be added using a patch like
the following.
Signed-off-by: Jonathan Nieder
---
Documentation/git-add.txt | 24 +++-
builtin/add.c | 28 +---
2 files changed, 40 insertions(+), 12 deletions(-)
diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index b0944e57..8607cf37 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -9,7 +9,8 @@ SYNOPSIS
[verse]
'git add' [-n] [-v] [--force | -f] [--interactive | -i] [--patch | -p]
- [--edit | -e] [--all | [--update | -u]] [--intent-to-add | -N]
+ [--edit | -e] [--update | -u] [--no-ignore-removals | -A]
+ [--intent-to-add | -N]
[--refresh] [--ignore-errors] [--ignore-missing] [--]
[...]
@@ -109,17 +110,22 @@ If no is given, the current version of Git
defaults to
and its subdirectories. This default will change in a future version
of Git, hence the form without should not be used.
+--ignore-removals::
+--no-ignore-removals::
-A::
--all::
- Update the index not only where the working tree has a file
- matching but also where the index already has an
- entry. This adds, modifies, and removes index entries to
- match the working tree.
+ Update the index only where the working tree has a file
+ matching . This adds and modifies index entries
+ to match the working tree but ignores removed files.
+
-If no is given, the current version of Git defaults to
-"."; in other words, update all files in the current directory
-and its subdirectories. This default will change in a future version
-of Git, hence the form without should not be used.
+This is currently the default. Git 2.0 will change the default
+to --no-ignore-removals.
++
+With --no-ignore-removals (and its historical synonyms `-A` and
+`--all`), if no is given, the current version of Git
+defaults to "."; in other words, update all files in the current
+directory and its subdirectories. This default will change in a future
+version of Git, hence the form without should not be used.
-N::
--intent-to-add::
diff --git a/builtin/add.c b/builtin/add.c
index ab1c9e8f..4a4e71ad 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -28,6 +28,14 @@ struct update_callback_data {
int add_errors;
};
+static int parse_opt_neg_tertiary(const struct option *opt, const char *arg,
+ int unset)
+{
+ int *target = opt->value;
+ *target = unset ? 1 : 2;
+ return 0;
+}
+
static int fix_unmerged_status(struct diff_filepair *p,
struct update_callback_data *data)
{
@@ -271,7 +279,8 @@ static const char ignore_error[] =
N_("The following paths are ignored by one of your .gitignore files:\n");
static int verbose = 0, show_only = 0, ignored_too = 0, refresh_only = 0;
-static int ignore_add_errors, addremove, intent_to_add, ignore_missing = 0;
+static int ignore_add_errors, intent_to_add, ignore_missing = 0;
+static int ignore_removals, addremove;
static struct option builtin_add_options[] = {
OPT__DRY_RUN(&show_only, N_("dry run")),
@@ -283,10 +292,12 @@ static struct option builtin_add_options[] = {
OPT__FORCE(&ignored_too, N_("allow adding otherwise ignored files")),
OPT_BOOLEAN('u', "update", &take_worktree_changes, N_("update tracked
files")),
OPT_BOOLEAN('N', "intent-to-add", &intent_to_add, N_("record only the
fact that the path will be added later")),
- OPT_BOOLEAN('A', "all", &addremove, N_("add changes from all tracked
and untracked fi
Re: jc/add-2.0-delete-default (Re: What's cooking in git.git (Apr 2013, #05; Mon, 15))
Jonathan Nieder writes: > How about something like this? > > warning: "git add" run on path with files removed (e.g., '%s') > hint: use "git add --ignore-removals " to ignore removals > hint: or "git add --no-ignore-removals " to notice them > hint: --ignore-removals is the default but this will change soon > hint: see git-add(1) for details > > Then the --ignore-removals option could be added using a patch like > the following. adding ignore-removals as a synonym (and keeping it) would be a good idea. We would still need to carry --all and --no-all that have been with us ever since we added "-A" option, though. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: jc/add-2.0-delete-default (Re: What's cooking in git.git (Apr 2013, #05; Mon, 15))
Junio C Hamano writes:
>> Then the --ignore-removals option could be added using a patch like
>> the following.
>
> adding ignore-removals as a synonym (and keeping it) would be a good
> idea.
>
> We would still need to carry --all and --no-all that have been with
> us ever since we added "-A" option, though.
The final step to turn "-A" the default will be held back until Git 2.0 release,
but I've inserted the following patch before that step.
I am thinking that it would be a good idea to merge up to this step
to 'master' tomorrow, and have you guys tweak it further on 'master'
with a patch like the one I am responding to, before the 1.8.3
final. We will have to tweak the 2.0 endgame version as we go but
that is outside 'next' for now, so it should be manageable.
-- >8 --
Subject: [PATCH] git add: rephrase the "removal will cease to be ignored"
warning
Now the logic to decide when to warn has been tightened, we know the
user is in a situation where the current and future behaviours will
be different. Spell out what happens with these two versions and
how to explicitly ask for the behaviour, and suggest "git status" as
a way to inspect the current status.
Signed-off-by: Junio C Hamano
---
builtin/add.c | 21 ++---
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index 4242bce..20f459a 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -52,15 +52,22 @@ static int fix_unmerged_status(struct diff_filepair *p,
return DIFF_STATUS_MODIFIED;
}
+static const char *add_would_remove_warning = N_(
+ "You ran 'git add' with neither '-A (--all)' or '--no-all', whose\n"
+"behaviour will change in Git 2.0 with respect to paths you removed from\n"
+"your working tree. Paths like '%s' that are\n"
+"removed are ignored with this version of Git.\n"
+"\n"
+"* 'git add --no-all ', which is the current default, ignores\n"
+" paths you removed from your working tree.\n"
+"\n"
+"* 'git add --all ' will let you also record the removals.\n"
+"\n"
+"Run 'git status' to check the paths you removed from your working tree.\n");
+
static void warn_add_would_remove(const char *path)
{
- warning(_("In Git 2.0, 'git add ...' will also update the\n"
- "index for paths removed from the working tree that match\n"
- "the given pathspec. If you want to 'add' only changed\n"
- "or newly created paths, say 'git add --no-all ...'"
- " instead.\n\n"
- "'%s' would be removed from the index without --no-all."),
- path);
+ warning(_(add_would_remove_warning), path);
}
static void update_callback(struct diff_queue_struct *q,
--
1.8.2.1-650-g3c8b519
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
