* Re: [PATCH 3/4] utf8: die if failed to re-encoding
From: Junio C Hamano @ 2012-02-21 17:36 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1329834292-2511-3-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Return value NULL in this case means "no conversion needed", which is
> not quite true when conv == -1.
Doing this only when producing new commits to avoid spreading damage might
be a good idea.
But utf8.c::reencode_string() is sufficiently deep in the call-chains to
make me suspect that the codepaths this change affects are not limited to
creation ones. If this also forbids readers from resurrecting salvageable
bits while reading (imagine your commit had "encodign vscii" but your log
message was in English, except only your name had letters outside ASCII
that I cannot locally convert to utf-8 for viewing), I do not think it is
an acceptable change.
^ permalink raw reply
* Re: [PATCHv3] git-p4: add initial support for RCS keywords
From: Eric Scouten @ 2012-02-21 17:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Pete Wyckoff, Luke Diamand, git
In-Reply-To: <7vobssgkt6.fsf@alter.siamese.dyndns.org>
On Tue, Feb 21, 2012 at 09:25, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Scouten <eric@scouten.com> writes:
>
>>> > r'\$(Id|Header|Author|Date|DateTime|Change|File|Revision)[^$]*\$'
>>>
>>> Still no ":"? Won't that match too much?
>>
>>> Fix the colon thing at least, then happy to add my Acked-By.
>>
>> No, that would be an incorrect change. The colon is added by P4 when
>> it expands the keyword pattern, but it is *not* part of the pattern
>> required by P4 to trigger a keyword expansion.
>>
>> http://kb.perforce.com/article/54/using-rcs-keywords
>
> I have this suspicion that both Pete and your last sentence is correct,
> but the regexp in the patch and your "would be an incorrect change" are
> wrong.
>
> I am not a P4 expert, but I would be very surprised if P4 expands "$Ida$"
> as if it is "$Id$" or "$Id: old expansion$", which the regexp would match.
>
> Wouldn't it be more like this?
>
> \$ # begins with a dollar, followed by...
> ( Id | Header | ... ) # one of these keywords, followed by ...
> ( :[^$]+ )? # possibly an old expansion, followed by
> \$ # another dollar sign
Good catch. Yes, you're probably right.
--
Eric Scouten :: software developer, photographer :: Poulsbo, WA (near Seattle)
http://ericscouten.com :: click for Flickr, Facebook, Twitter, LinkedIn links
^ permalink raw reply
* Re: Patchset NTLM-Authentication
From: Junio C Hamano @ 2012-02-21 18:02 UTC (permalink / raw)
To: Schmidt, Marco, Thomas Rast; +Cc: git, avarab
In-Reply-To: <8762f05n9q.fsf_-_@thomas.inf.ethz.ch>
Thomas Rast <trast@inf.ethz.ch> writes:
> This mostly parallels http.authAny which was introduced in b8ac923
> (Add an option for using any HTTP authentication scheme, not only
> basic, 2009-11-27). http.authAny was removed, and its feature
> unconditionally enabled, in 525ecd2 (Remove http.authAny, 2009-12-28).
> However the reasoning of the latter does not apply here because XXXX.
Thanks, Thomas.
I think this paragraph is essential, especially the XXXX part, if we were
to accept the proposed change and keep the new configuration. Otherwise
we won't know what to do when somebody proposes to unconditionally enable
this ;-)
If it turns out that we can set CURLOPT_PROXYAUTH always to CURLAUTH_ANY
without compromising security, then an explanation why this does not have
to be optional, similar to what justified 525ecd2, needs to be there
instead, and the patch needs to be tweaked to drop the configuration bits.
Marco, I extracted your patch in the attachment and took a look at it
before composing the above response.
- Your log message seems to be indented by two spaces for some strange
reason;
- it does not have any justification like the example Thomas gave
you; and
- it also is missing your S-o-b.
Care to re-roll one more time?
^ permalink raw reply
* Re: [PATCH 2/4] Do attempt pretty print in ASCII-incompatible encodings
From: Jeff King @ 2012-02-21 18:21 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1329834292-2511-2-git-send-email-pclouds@gmail.com>
On Tue, Feb 21, 2012 at 09:24:50PM +0700, Nguyen Thai Ngoc Duy wrote:
> We rely on ASCII everywhere. We print "\n" directly without conversion
> for example. The end result would be a mix of some encoding and ASCII
> if they are incompatible. Do not do that.
>
> In theory we could convert everything to utf-8 as intermediate medium,
> process process process, then convert final output to the desired
> encoding. But that's a lot of work (unless we have a pager-like
> converter) with little real use. Users can just pipe everything to
> iconv instead.
I'm not sure why we bother checking this. Using non-ASCII-superset
encodings is broken, yes, but are people actually doing that? I assume
that the common one is utf-16, and anybody using it will experience
severe breakage immediately. So are people actually doing this? Are
there actually encodings that will cause subtle breakage that we want to
catch?
-Peff
^ permalink raw reply
* Re: [PATCH 4/4] Only re-encode certain parts in commit object, not the whole
From: Jeff King @ 2012-02-21 18:25 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1329834292-2511-4-git-send-email-pclouds@gmail.com>
On Tue, Feb 21, 2012 at 09:24:52PM +0700, Nguyen Thai Ngoc Duy wrote:
> Commit object has its own format, which happens to be in ascii, but
> not really subject to re-encoding.
>
> There are only four areas that may be re-encoded: author line,
> committer line, mergetag lines and commit body. Encoding of tags
> embedded in mergetag lines is not decided by commit encoding, so leave
> it out and consider it binary.
Is this worth the effort? Yes, re-encoding the ASCII bits of the commit
object is unnecessary. But do we actually handle encodings that are not
ASCII supersets? IOW, I could see the point if this is making it
possible to hold utf-16 names and messages in your commits (though why
you would want to do so is beyond me...). But my understanding is that
this is horribly broken anyway by other parts of the code. And even
looking at your code below:
> +static char *reencode_commit(const char *buffer,
> + const char *out_enc, const char *in_enc)
> +{
> + struct strbuf out = STRBUF_INIT;
> + struct strbuf buf = STRBUF_INIT;
> + char *reencoded, *s, *e;
> +
> + strbuf_addstr(&buf, buffer);
> +
> + s = strstr(buf.buf, "\nauthor ");
> + assert(s != NULL);
Wouldn't this assert trigger in the presence of encodings which
contain ASCII NUL (e.g., wide encodings like utf-16)?
Is there an encoding you have in mind which would be helped by this?
-Peff
^ permalink raw reply
* Re: Patchset NTLM-Authentication
From: Daniel Stenberg @ 2012-02-21 19:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Schmidt, Marco, Thomas Rast, git, avarab
In-Reply-To: <7vfwe4gj4n.fsf@alter.siamese.dyndns.org>
On Tue, 21 Feb 2012, Junio C Hamano wrote:
> If it turns out that we can set CURLOPT_PROXYAUTH always to CURLAUTH_ANY
> without compromising security, then an explanation why this does not have to
> be optional, similar to what justified 525ecd2, needs to be there instead,
> and the patch needs to be tweaked to drop the configuration bits.
Allow me to provide some libcurl info on this!
Setting it to ANY will unconditionally cause an extra roundtrip which you can
avoid if you know what auth type the proxy wants and you set it at once. With
ANY set, libcurl will first "probe" the proxy to figure out which type to use
and then go on and actually do it in a second request (and possibly even a
third request in some cases).
It can actually be seen as a security _improvement_ in some cases where for
example Basic auth (user+password sent as plain text) can be avoided in
preference to a more secure mechanism, but I think that's a rather rare case
for git.
IMO, if ANY is considered fine for normal host authentication I think it could
be considered fine for proxy authentication as well.
--
/ daniel.haxx.se
^ permalink raw reply
* Re: git status: small difference between stating whole repository and small subdirectory
From: Junio C Hamano @ 2012-02-21 19:16 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy
Cc: Thomas Rast, Jeff King, Piotr Krukowiecki, Git Mailing List
In-Reply-To: <CACsJy8DE86qzA1=GiKZFRCt5aH8X4iMyDvfrhnqwmbq52szhHg@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> I'm aware that Jeff's tackling at lower level, which retains
> cache-tree for many more cases.
>
> But this patch seems simple and safe
> to me, and in my experience this case happens quite often (or maybe I
> tend to keep my index clean). Junio, any chance this patch may get in?
I do not think we are talking about a duplicated effort here.
By definition, the change to hook into unpack_trees() and making sure we
invalidate all the necessary subtrees in the cache cannot give you a cache
tree that is more populated than what you started with. And the train of
thought in Peff's message is to improve this invalidation---we currently
invalidate everything ;-)
Somebody has to populate the cache tree fully when we _know_ the index
matches a certain tree, and adding a call to prime_cache_tree() in
strategic places is a way to do so. The most obvious is write-tree, but
there are a few other existing codepaths that do so.
Because prime_cache_tree() by itself is a fairly expensive operation that
reads all the trees recursively, its benefits need to be evaluated. It
should to happen only in an operation that is already heavy-weight, is
likely to have read all the trees and have many of them in-core cache, and
also relatively rarely happens compared to "git add" so that the cost can
be amortised over time, such as "reset --(hard|mixed)".
Switching branches is likely to fall into that category, but that is just
my gut feeling. I would feel better at night if somebody did a benchmark
;-)
One thing we do not currently do anywhere that _might_ be of merit is to
make a call to cache_tree_update() instead of prime_cache_tree() when we
already know that only a very small subpart of the cache-tree is invalid
and it is cheaper to repair it by rehashing only a small portion of the
index than to re-prime the entire cache tree with prime_cache_tree().
^ permalink raw reply
* Re: [PATCH] Ignore SIGPIPE when running a filter driver
From: Johannes Sixt @ 2012-02-21 19:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jehan Bing, git, peff, jrnieder
In-Reply-To: <7vsji5jgtv.fsf@alter.siamese.dyndns.org>
Am 20.02.2012 23:11, schrieb Junio C Hamano:
> Jehan Bing <jehan@orb.com> writes:
>> @@ -360,12 +361,16 @@ static int filter_buffer(int in, int out, void *data)
>> if (start_command(&child_process))
>> return error("cannot fork to run external filter %s", params->cmd);
>>
>> + sigchain_push(SIGPIPE, SIG_IGN);
>> +
>> write_err = (write_in_full(child_process.in, params->src, params->size) < 0);
>> if (close(child_process.in))
>> write_err = 1;
>> if (write_err)
>> error("cannot feed the input to external filter %s", params->cmd);
>>
>> + sigchain_pop(SIGPIPE);
>> +
>
> Thanks.
>
> I think this is OK on a POSIX system where this function is run by
> start_async() which is implemented with a forked child process.
>
> I do not now if it poses a issue on Windows, though. Johannes, any
> comments?
I do not expect the change to cause a problem on Windows.
-- Hannes
^ permalink raw reply
* Re: [PATCH 0/8 v6] diff --stat: use the full terminal width
From: Junio C Hamano @ 2012-02-21 20:10 UTC (permalink / raw)
To: Zbigniew Jędrzejewski-Szmek; +Cc: git, Michael J Gruber, pclouds, j.sixt
In-Reply-To: <4F436C5D.7070606@in.waw.pl>
Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:
> This seem overly complex. A nice property to have would be
> "if the window is wide enough so there's enough space for full
> filenames, the graph part scales monotonically with the change count".
> (If there's filename truncation, than there just isn't enough space
> for everything and the graph may be compressed. But otherwise, if we
> have two graphs which do not end at the edge of the screen, and the
> second one is wider than the first one, then without looking at the
> change counts we know that the second one has more changes).
>
> For this property to be satisfied, the graph_width limit would have to
> be independent of the filename width.
>
> So maybe it should be ...
Sorry, the desired property I would understand, but that does not click
with your "have to be independent" conclusion, so I do not have comment on
the "maybe it should be..." part.
The resolution requirement may want to set a "desired lower limit" for the
width of the graph, but it is only "desired" because it is possible that
you have to bust the limit if you have three files with 1, 9999 and 10000
changed lines and your terminal is only 200 columns wide.
The current code caps name part to 50/80, but allows the graph to use more
when you have only shorter names. Perhaps you can follow the same logic
in the first part of your [7/8] (which needs to be separated to at least
in two pieces, as it conflates the "lift 50-column cap from the name width
and make it proportional to the term_width()" part and "but cap the graph
part to 40-column" part, that are separate topics)? Then we can try
different heuristics to find a better way to cap the length of the graph
on top?
^ permalink raw reply
* Re: [PATCH] completion: Allow dash as the first character for __git_ps1
From: Junio C Hamano @ 2012-02-21 20:40 UTC (permalink / raw)
To: Christian Hammerl; +Cc: git
In-Reply-To: <1329740273-5580-1-git-send-email-info@christian-hammerl.de>
Christian Hammerl <info@christian-hammerl.de> writes:
> If the argument for `__git_ps1` begins with a dash, `printf` tries to
> interpret it as an option which results in an error message.
> The problem is solved by adding '--' before the argument to tell
> `printf` to not interpret the following argument as an option.
> Adding '--' directly to the argument does not help because the argument
> is enclosed by double quotes.
>
> Signed-off-by: Christian Hammerl <info@christian-hammerl.de>
> ---
> contrib/completion/git-completion.bash | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 91c7acb..61ff152 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -313,7 +313,7 @@ __git_ps1 ()
> fi
>
> local f="$w$i$s$u"
> - printf "${1:- (%s)}" "$c${b##refs/heads/}${f:+ $f}$r$p"
> + printf -- "${1:- (%s)}" "$c${b##refs/heads/}${f:+ $f}$r$p"
> fi
> }
Hmm, so you may be doing something like PS1='\W$(__git_ps1 "-%s")'?
OK, and thanks.
^ permalink raw reply
* how do you review auto-resolved files
From: Neal Kreitzinger @ 2012-02-21 20:41 UTC (permalink / raw)
To: git
When git does a merges (merge/rebase/cherry-pick) it auto-resolves same-file
changes that do not conflict on the same line(s).
Technical Question: What are the recommended commands for reviewing the
files that auto-resolved after a "merge"?
It seems like the commands might be different depending on the type of
merge: git-merge, git-rebase, git-cherry-pick. I imagine there are three
steps to the "review auto-resolutions" procedure:
(1) Determine the list of files that were changed on both sides (same-file
edits) and which of those were auto-resolved during the merge. (Preferably
excluding those files that merge-conflicted since you already know how you
manually resolved those.)
(2) Review the auto-resolved files in full context to verify whether the
auto-resolutions are desirable.
(3) Manually remediate the merge-result (auto-resolution) or redo the
merge-of-that-file for any files with undesirable auto-resolutions. Perhaps
an edit of the auto-resolved file is sufficient for simple remediations, but
for more challenging remediations a manual redo of the merge-of-that-file
would be desired.
Please advise on the proven (tried and tested) ways that others are using to
verify/ensure that their auto-resolve results are correct.
Procedural/Philosophical Question: What are the pros and cons of
auto-resolved files?
Currently, we address the problem up-front instead of after-the-fact by
enforcing merge-conflicts on every same-file edit by means of a
"user-date-stamp" on "line 1" of every source file changed by performing
keyword expansion (# $User$ $Date$) in our pre-commit hook. I don't think
keyword expansion or forcing merge-conflicts for every same-file edit is a
common practice among git users. Therefore, this seems like somewhat of a
kludgey hack. Furthermore, I assume that all git users are somehow
reviewing their auto-resolutions. (There is no way I would assume that git
merged my same-file edits correctly. It's great that git
does-the-right-thing most-of-the-time, but that doesn't change the fact that
I still have to review everything for undesirable resolutions.)
In light of this, it seems that there is no advantage to letting git
auto-resolve same-file changes because the review process after-the-fact
would actually be more error-prone and tedious than just manually-merging
same-file edits up-front. If I force you to resolve merge-conflicts
up-front then I'm ensuring the merge-resolution is deliberate (and hopefully
intelligent). If I expect/assume you are going to review the
auto-resolutions after-the-fact then you can neglect this because you:
- have become complacent that git usually does-what-you-want so "you don't
really need to do it",
- are lazy and do it half-way,
- forget to do it,
- think "git magically does your work for you",
- don't know how to do it,
- don't even realize that anything auto-resolved or what auto-resolved,
- decide you don't have to do it because that is what testing if for,
- you think that your time is so valuable that an ounce-of-prevention on
your part is not worth a pound-of-cure on the part of others.
Please comment on the pros and cons of "manual-merge up-front for same-file
edits" vs. "review-and-remediate after-the-fact for auto-resolutions of
same-file edits".
Thanks in advance for your replies!
v/r,
neal
^ permalink raw reply
* Re: [PATCH v2] tag: refuse tag messages that contain NULs
From: Junio C Hamano @ 2012-02-21 20:47 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1329741483-22567-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> It's not about after those. It's about right before write_sha1_file().
> I wanted to catch all NULs no matter how they come. But yes the check
> should happen early to avoid wasting user's time (e.g. doing signing)
No, it is not about that. It is about checking _the input_. If we had
bugs in do_sign() that adds what we do not want, that is not a user's
fault and "a NUL byte in tag message not allowed" is an inappropriate
thing to give to the user.
And giving "We screwed up and added NUL that you cannot work around to
remove, sorry, you hit a bug." is not very useful.
> So how about this?
> ...
> diff --git a/sha1_file.c b/sha1_file.c
> index 88f2151..2fc8623 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2519,6 +2519,12 @@ int write_sha1_file(const void *buf, unsigned long len, const char *type, unsign
> char hdr[32];
> int hdrlen;
>
> + /* GIT_HASH_NUL is for the test suite to hash abitrary content */
> + if (!getenv("GIT_HASH_NUL") &&
> + (!strcmp(type, commit_type) || !strcmp(type, tag_type)) &&
> + memchr(buf, '\0', len))
> + return error("BUG: %s message contains NUL.", type);
> +
This is yucky. Is this really worth it?
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index e93ac73..8cb13e5 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -1269,4 +1269,8 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '
> test_must_fail git tag -v -s
> '
>
> +test_expect_success 'tag content contains NUL' '
> + test_must_fail git tag -F "$TEST_DIRECTORY"/t3900/UTF-16.txt utf16
> +'
> +
This is caught without the change to write_sha1_file(), isn't it? If so,
I would say we should drop that GIT_HASH_NUL hunk.
^ permalink raw reply
* Re: [PATCH] completion: Allow dash as the first character for __git_ps1
From: Christian Hammerl @ 2012-02-21 20:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vty2jgbsh.fsf@alter.siamese.dyndns.org>
Yes, kind of.
It is more like PS1='\W$(__git_ps1 "-[ %s ]")'. And i am fixing it after each update on my machine. ;-)
Junio C Hamano <gitster@pobox.com> schrieb:
>Christian Hammerl <info@christian-hammerl.de> writes:
>
>> If the argument for `__git_ps1` begins with a dash, `printf` tries to
>> interpret it as an option which results in an error message.
>> The problem is solved by adding '--' before the argument to tell
>> `printf` to not interpret the following argument as an option.
>> Adding '--' directly to the argument does not help because the
>argument
>> is enclosed by double quotes.
>>
>> Signed-off-by: Christian Hammerl <info@christian-hammerl.de>
>> ---
>> contrib/completion/git-completion.bash | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/contrib/completion/git-completion.bash
>b/contrib/completion/git-completion.bash
>> index 91c7acb..61ff152 100755
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -313,7 +313,7 @@ __git_ps1 ()
>> fi
>>
>> local f="$w$i$s$u"
>> - printf "${1:- (%s)}" "$c${b##refs/heads/}${f:+ $f}$r$p"
>> + printf -- "${1:- (%s)}" "$c${b##refs/heads/}${f:+ $f}$r$p"
>> fi
>> }
>
>Hmm, so you may be doing something like PS1='\W$(__git_ps1 "-%s")'?
>OK, and thanks.
--
Diese Nachricht wurde von meinem Android-Mobiltelefon mit K-9 Mail gesendet.
^ permalink raw reply
* Re: [PATCH] Ignore SIGPIPE when running a filter driver
From: Junio C Hamano @ 2012-02-21 20:58 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Jehan Bing, git, gitster, j.sixt, peff
In-Reply-To: <20120221030150.GA31737@burratino>
Jonathan Nieder <jrnieder@gmail.com> writes:
>> If a filter is not defined or if it fails, git behaves as if the filter
>> is a no-op passthru. However, if the filter exits before reading all
>> the content, and depending on the timing git, could be kill with
>> SIGPIPE instead.
>>
>> Ignore SIGPIPE while processing the filter to detect when it exits
>> early and fallback to using the unfiltered content.
>>
>> Signed-off-by: Jehan Bing <jehan@orb.com>
>
> For the benefit of the uninitiated ("how would ignoring an error help
> me detect an error?"): setting the SIGPIPE handler to SIG_IGN does not
> actually ignore the broken pipe condition but causes it to be reported
> as an I/O error, errno == EPIPE. That means instead of being killed
> by SIGPIPE, git gets to fall back to passthrough and report the
> filter's mistake.
Yes.
You could rephrase bit better to further clarify it, perhaps like this:
Ignore SIGPIPE when running a filter driver
If a filter is not defined or if it fails, git should behave as if the
filter is a no-op passthru.
However, if the filter exits before reading all the content, depending on
the timing, git could be killed with SIGPIPE when it tries to write to the
pipe connected to the filter.
Ignore SIGPIPE while processing the filter to give us a chance to check
the return value from a failed write, in order to detect and act on this
mode of failure in a more controlled way.
Signed-off-by: Jehan Bing <jehan@orb.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
although I think Jehan's original was already clear enough.
> So at least on POSIX-y platforms, this patch looks good to me. Thanks
> for writing it.
Thank you and Johannes for eyeballing and sanity checking.
Will queue.
^ permalink raw reply
* [PATCH] cherry-pick: No advice to commit if --no-commit
From: Phil Hord @ 2012-02-21 21:05 UTC (permalink / raw)
To: Git List, Junio C Hamano, Ramkumar Ramachandra, Jonathan Nieder,
Phil Hord
Cc: Phil Hord
When cherry-pick fails it offers a helpful hint about how to
proceed. The hint tells the user how to do the cleanup
needed by the conflicted cherry-pick and finish the job after
conflict resolution. In case of cherry-pick --no-commit, the
hint goes too far. It tells the user to finish up with
'git commit'. That is not what this git-cherry-pick was
trying to do in the first place.
Restrict the hint in case of --no-commit to avoid giving this
extra advice.
Also, add a test verifying the reduced hint for the --no-commit
version of cherry-pick.
Signed-off-by: Phil Hord <hordp@cisco.com>
---
sequencer.c | 12 +++++++-----
t/t3507-cherry-pick-conflict.sh | 14 ++++++++++++++
2 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 5fcbcb8..b7965e8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -123,7 +123,7 @@ static void write_cherry_pick_head(struct commit *commit, const char *pseudoref)
strbuf_release(&buf);
}
-static void print_advice(int show_hint)
+static void print_advice(int show_hint, struct replay_opts *opts)
{
char *msg = getenv("GIT_CHERRY_PICK_HELP");
@@ -138,10 +138,12 @@ static void print_advice(int show_hint)
return;
}
- if (show_hint)
+ if (show_hint) {
advise(_("after resolving the conflicts, mark the corrected paths\n"
- "with 'git add <paths>' or 'git rm <paths>'\n"
- "and commit the result with 'git commit'"));
+ "with 'git add <paths>' or 'git rm <paths>'"));
+ if (!opts->no_commit)
+ advise(_( "and commit the result with 'git commit'"));
+ }
}
static void write_message(struct strbuf *msgbuf, const char *filename)
@@ -423,7 +425,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
: _("could not apply %s... %s"),
find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
msg.subject);
- print_advice(res == 1);
+ print_advice(res == 1, opts);
rerere(opts->allow_rerere_auto);
} else {
if (!opts->no_commit)
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index ee1659c..0c81b3c 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -59,6 +59,20 @@ test_expect_success 'advice from failed cherry-pick' "
test_i18ncmp expected actual
"
+test_expect_success 'advice from failed cherry-pick --no-commit' "
+ pristine_detach initial &&
+
+ picked=\$(git rev-parse --short picked) &&
+ cat <<-EOF >expected &&
+ error: could not apply \$picked... picked
+ hint: after resolving the conflicts, mark the corrected paths
+ hint: with 'git add <paths>' or 'git rm <paths>'
+ EOF
+ test_must_fail git cherry-pick --no-commit picked 2>actual &&
+
+ test_i18ncmp expected actual
+"
+
test_expect_success 'failed cherry-pick sets CHERRY_PICK_HEAD' '
pristine_detach initial &&
test_must_fail git cherry-pick picked &&
--
1.7.9.267.gda172.dirty
^ permalink raw reply related
* Re: how do you review auto-resolved files
From: Junio C Hamano @ 2012-02-21 21:19 UTC (permalink / raw)
To: Neal Kreitzinger; +Cc: git
In-Reply-To: <ji0vik$e48$1@dough.gmane.org>
"Neal Kreitzinger" <neal@rsss.com> writes:
> When git does a merges (merge/rebase/cherry-pick) it auto-resolves same-file
> changes that do not conflict on the same line(s).
>
> Technical Question: What are the recommended commands for reviewing the
> files that auto-resolved after a "merge"?
Imagine that you are the maintainer of the mainline and are reviewing the
work made on a side branch that you just merged, but pretend that the
contribution came as a patch instead. How would you assess the damage to
your mainline?
You would use "git show --first-parent $commit" for that.
And then look at what the sideline wanted to do to the old baseline:
git log -p $commit^..$commit
which would, unless the person who worked on the side branch did a shoddy
job describing his work, explain what the side branch wanted to achieve
and also _how_ it wanted to achieve it.
And then re-read the first "git show" output with that knowledge, together
with the knowledge you have on your mainline codebase, and decide if the
solution used by the side branch is still valid. If it makes sense, you
are done. If the advance in your mainline since the side branch forked
invalidated some assumption the side branch made (e.g. a helper function
the side branch used has changed its meaning, a helper function the side
branch changed its meaning gained more callsite on the mainline, etc.),
you have a semantic conflict that you would need to address.
It is unclear what exactly you consider "auto-resolve" in your message, so
I'd refrain from commenting on the "Philosophical" part, at least for now.
^ permalink raw reply
* Re: [PATCH] completion: remote set-* <name> and <branch>
From: Philip Jägenstedt @ 2012-02-21 21:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, SZEDER Gábor, Felipe Contreras, Teemu Likonen
In-Reply-To: <7vvcn2lyw6.fsf@alter.siamese.dyndns.org>
On Mon, Feb 20, 2012 at 08:58, Junio C Hamano <gitster@pobox.com> wrote:
> Philip Jägenstedt <philip@foolip.org> writes:
>
>> Complete <name> only for set-url. For set-branches and
>> set-head, complete <name> and <branch> over the network,
>> like e.g. git pull already does.
>>
>> Signed-off-by: Philip Jägenstedt <philip@foolip.org>
>
> You addressed your patch to Shawn, who originally wrote this, but
>
> "git shortlog -n -s --no-merges --since=9.months pu contrib/completion"
>
> indicates that he no longer is involved in enhancing this script, and it
> has seen actions primarily from three people I Cc'ed this message to.
Thanks. Perhaps git-completion.bash should not say "Send all patches
to the current maintainer" and simply defer to SubmittingPatches?
>> contrib/completion/git-completion.bash | 12 +++++++++---
>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
>> index 1505cff..8e7abb6 100755
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -738,6 +738,9 @@ __git_complete_remote_or_refspec ()
>> {
>> local cur_="$cur" cmd="${words[1]}"
>> local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
>> + if [ "$cmd" = "remote" ]; then
>> + c=$((++c))
>> + fi
>
> I don't know about others, but auto-incrementing a variable and assigning
> the result to the same variable, while not wrong at all, hurts my brain.
>
> c=$(($c + 1))
>
> is far more readable and does not suggest there is any funky magic
> involved. Also it is a good habit to get into not to omit $ from
> variables inside arithmetic substitution, even though bash allows it and
> this script is meant to be consumed only by shells that understand this
> bash-ism.
>
> I do not know offhand if zsh groks it, but the point is that you do not
> have to worry about it if you write "$(($c+1))" instead of "$((c+1))".
CodingGuidlines suggests to follow local convention, which was
"c=$((++c))". This file also uses "++n", "i++" and "((i++))". I will
send a v2 patch that normalizes these, open to discussion of course.
--
Philip Jägenstedt
^ permalink raw reply
* Re: [PATCH] contrib: added git-diffall
From: Junio C Hamano @ 2012-02-21 21:51 UTC (permalink / raw)
To: Tim Henigan; +Cc: git
In-Reply-To: <1329785969-828-1-git-send-email-tim.henigan@gmail.com>
Tim Henigan <tim.henigan@gmail.com> writes:
> diff --git a/contrib/diffall/git-diffall b/contrib/diffall/git-diffall
> new file mode 100755
> index 0000000..ef01eda
> --- /dev/null
> +++ b/contrib/diffall/git-diffall
> @@ -0,0 +1,275 @@
> +#!/bin/bash -e
Does this have to be bash-only (iow, infested with bash-isms beyond
repair), or is you wrote this merely from inertia?
The following is only after a cursory scanning, so there may be other
things that needs fixing, but anyway:
- Don't use "which" in scripts. Its output is not machine parseable, and
exit code is not reliable, across platforms. It is only meant for
consumption by human who can read English (or natural language in the
current locale).
> + if test -z "$paths"
> + then
> + paths=$1
> + else
> + paths="$paths $1"
> + fi
Just a style tip; if you are going to let shell $IFS split this list
anyway, it is customary to write the above as
paths="$paths$1 "
> + git diff --name-only "$left"..."$right" -- $paths > "$tmp"/filelist
git diff ... -- $paths >"$tmp/filelist"
> +# Exit immediately if there are no diffs
> +if test ! -s "$tmp"/filelist
> +then
> + exit 0
> +fi
> +
> +if test -n "$copy_back" && test "$right_dir" != "working_tree"
> +then
> + echo "--copy-back is only valid when diff includes the working tree."
> + exit 1
> +fi
> +
> +# Create the named tmp directories that will hold the files to be compared
> +mkdir -p "$tmp"/"$left_dir" "$tmp"/"$right_dir"
mkdir -p "$tmp/$left_dir" "$tmp/$right_dir"
> + if test -n "$compare_staged"
> + then
> + ls_list=$(git ls-tree HEAD $name)
> + if test -n "$ls_list"
> + then
> + mkdir -p "$tmp"/"$left_dir"/"$(dirname "$name")"
> + git show HEAD:"$name" > "$tmp"/"$left_dir"/"$name"
> + fi
> + else
> + mkdir -p "$tmp"/"$left_dir"/"$(dirname "$name")"
> + git show :"$name" > "$tmp"/"$left_dir"/"$name"
> + fi
That's misleadingly indented. First I thought "in what case would we want
to switch the LHS between HEAD:$path and :$path when doing diff --cached?"
but the overindented four lines starting from the funny "fi" is about non
cached case.
> + fi
> +done < "$tmp"/filelist
> +
> +cd "$tmp"
> +LOCAL="$left_dir"
> +REMOTE="$right_dir"
> +
> +if test -n "$diff_tool"
> +then
> + export BASE
> + eval $diff_tool '"$LOCAL"' '"$REMOTE"'
> +else
> + run_merge_tool "$merge_tool" false
> +fi
> +
> +# This function is called on exit
> +cleanup () {
> + cd "$start_dir"
> +
> + # Copy files back to the working dir, if requested
> + if test -n "$copy_back" && test "$right_dir" = "working_tree"
> + then
> + git_top_dir=$(git rev-parse --show-toplevel)
> + find "$tmp/$right_dir" -type f|while read file; do
> + cp "$file" "$git_top_dir/${file#$tmp/$right_dir/}"
> + done
Why is this loop written in such a dense way? Everything else (except for
that misindented part) were almost to our CodingStyle and was fairly easy
to read, though.
> + fi
> +
> + # Remove the tmp directory
> + rm -rf "$tmp"
> +}
> +
> +trap cleanup EXIT
Does this even trigger? This is not Perl that parses and runs set-up code
before executing everything else, so I suspect this last line amounts to
the same thing as writing just
cleanup
without trap nor signal names.
If you are to set up temporary files or directories that you want to clean
up, a good discipline is to follow this order:
- define variable(s) to hold the temporary locations, e.g.
tmpdir=$(mktemp ...)
- set the trap before starting to use these temporary locations, e.g.
trap 'rm -rf "$tmpdir' 0 1 2 3 15
- and then start populating tmpdir and do whatever you want to do.
^ permalink raw reply
* [PATCH v2] completion: remote set-* <name> and <branch>
From: Philip Jägenstedt @ 2012-02-21 21:54 UTC (permalink / raw)
To: git
Cc: SZEDER Gábor, Felipe Contreras, Teemu Likonen,
Philip Jägenstedt
In-Reply-To: <CAKHWUkZjQYnf=LwS_RC-E_7gV73AzbWjxs33E5-FL25s6_qX4g@mail.gmail.com>
Complete <name> only for set-url. For set-branches and
set-head, complete <name> and <branch> over the network,
like e.g. git pull already does.
The style used for incrementing and decrementing variables was fairly
inconsistenty and was normalized to use ++x, or ((++x)) in contexts
where the former would otherwise be interpreted as a command. This is a
bash-ism, but for obvious reasons this script is already bash-specific.
Finally, remove out-of-date documentation for how to submit patches and
(silently) defer to Documentation/SubmittingPatches like all other code.
Signed-off-by: Philip Jägenstedt <philip@foolip.org>
---
contrib/completion/git-completion.bash | 42 +++++++++++++------------------
1 files changed, 18 insertions(+), 24 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 1505cff..9dac084 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -60,18 +60,6 @@
# per-repository basis by setting the bash.showUpstream config
# variable.
#
-#
-# To submit patches:
-#
-# *) Read Documentation/SubmittingPatches
-# *) Send all patches to the current maintainer:
-#
-# "Shawn O. Pearce" <spearce@spearce.org>
-#
-# *) Always CC the Git mailing list:
-#
-# git@vger.kernel.org
-#
if [[ -n ${ZSH_VERSION-} ]]; then
autoload -U +X bashcompinit && bashcompinit
@@ -395,7 +383,7 @@ __git_reassemble_comp_words_by_ref()
fi
# List of word completion separators has shrunk;
# re-assemble words to complete.
- for ((i=0, j=0; i < ${#COMP_WORDS[@]}; i++, j++)); do
+ for ((i=0, j=0; i < ${#COMP_WORDS[@]}; ++i, ++j)); do
# Append each nonempty word consisting of just
# word separator characters to the current word.
first=t
@@ -408,7 +396,7 @@ __git_reassemble_comp_words_by_ref()
# Attach to the previous token,
# unless the previous token is the command name.
if [ $j -ge 2 ] && [ -n "$first" ]; then
- ((j--))
+ ((--j))
fi
first=
words_[$j]=${words_[j]}${COMP_WORDS[i]}
@@ -416,7 +404,7 @@ __git_reassemble_comp_words_by_ref()
cword_=$j
fi
if (($i < ${#COMP_WORDS[@]} - 1)); then
- ((i++))
+ ((++i))
else
# Done.
return
@@ -738,6 +726,9 @@ __git_complete_remote_or_refspec ()
{
local cur_="$cur" cmd="${words[1]}"
local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
+ if [ "$cmd" = "remote" ]; then
+ ((++c))
+ fi
while [ $c -lt $cword ]; do
i="${words[c]}"
case "$i" in
@@ -755,7 +746,7 @@ __git_complete_remote_or_refspec ()
-*) ;;
*) remote="$i"; break ;;
esac
- c=$((++c))
+ ((++c))
done
if [ -z "$remote" ]; then
__gitcomp_nl "$(__git_remotes)"
@@ -788,7 +779,7 @@ __git_complete_remote_or_refspec ()
__gitcomp_nl "$(__git_refs)" "$pfx" "$cur_"
fi
;;
- pull)
+ pull|remote)
if [ $lhs = 1 ]; then
__gitcomp_nl "$(__git_refs "$remote")" "$pfx" "$cur_"
else
@@ -995,7 +986,7 @@ __git_find_on_cmdline ()
return
fi
done
- c=$((++c))
+ ((++c))
done
}
@@ -1006,7 +997,7 @@ __git_has_doubledash ()
if [ "--" = "${words[c]}" ]; then
return 0
fi
- c=$((++c))
+ ((++c))
done
return 1
}
@@ -1129,7 +1120,7 @@ _git_branch ()
-d|-m) only_local_ref="y" ;;
-r) has_r="y" ;;
esac
- c=$((++c))
+ ((++c))
done
case "$cur" in
@@ -2289,7 +2280,7 @@ _git_config ()
_git_remote ()
{
- local subcommands="add rename rm show prune update set-head"
+ local subcommands="add rename rm set-head set-branches set-url show prune update"
local subcommand="$(__git_find_on_cmdline "$subcommands")"
if [ -z "$subcommand" ]; then
__gitcomp "$subcommands"
@@ -2297,9 +2288,12 @@ _git_remote ()
fi
case "$subcommand" in
- rename|rm|show|prune)
+ rename|rm|set-url|show|prune)
__gitcomp_nl "$(__git_remotes)"
;;
+ set-head|set-branches)
+ __git_complete_remote_or_refspec
+ ;;
update)
local i c='' IFS=$'\n'
for i in $(git --git-dir="$(__gitdir)" config --get-regexp "remotes\..*" 2>/dev/null); do
@@ -2580,7 +2574,7 @@ _git_tag ()
f=1
;;
esac
- c=$((++c))
+ ((++c))
done
case "$prev" in
@@ -2633,7 +2627,7 @@ _git ()
--help) command="help"; break ;;
*) command="$i"; break ;;
esac
- c=$((++c))
+ ((++c))
done
if [ -z "$command" ]; then
--
1.7.5.4
^ permalink raw reply related
* Re: [PATCH] completion: remote set-* <name> and <branch>
From: Junio C Hamano @ 2012-02-21 22:23 UTC (permalink / raw)
To: Philip Jägenstedt
Cc: git, SZEDER Gábor, Felipe Contreras, Teemu Likonen
In-Reply-To: <CAKHWUkZjQYnf=LwS_RC-E_7gV73AzbWjxs33E5-FL25s6_qX4g@mail.gmail.com>
Philip Jägenstedt <philip@foolip.org> writes:
> Thanks. Perhaps git-completion.bash should not say "Send all patches
> to the current maintainer" and simply defer to SubmittingPatches?
I see you did this in your follow-up patch. Thanks.
>>> {
>>> local cur_="$cur" cmd="${words[1]}"
>>> local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
>>> + if [ "$cmd" = "remote" ]; then
>>> + c=$((++c))
>>> + fi
>>
>> I don't know about others, but auto-incrementing a variable and assigning
>> the result to the same variable, while not wrong at all, hurts my brain.
>>
>> c=$(($c + 1))
>>
>> is far more readable and does not suggest there is any funky magic
>> involved. Also it is a good habit to get into not to omit $ from
>> variables inside arithmetic substitution, even though bash allows it and
>> this script is meant to be consumed only by shells that understand this
>> bash-ism.
>>
>> I do not know offhand if zsh groks it, but the point is that you do not
>> have to worry about it if you write "$(($c+1))" instead of "$((c+1))".
>
> CodingGuidlines suggests to follow local convention, which was
> "c=$((++c))". This file also uses "++n", "i++" and "((i++))". I will
> send a v2 patch that normalizes these, open to discussion of course.
OK, it was my mistake to suggest $c++ in this file; it liberally uses
dollar-less variables, and I agree that it is a good idea to stick to that
local convention.
But I think you went too far in your follow-up patch to make the increment
and decrement uniform.
"i++" is so much easier on the eye unless you must use "++i" in order to
use the value of the incremented "i" in an expression, and the changes to
turn existing instances of free-standing "i++" to "++i" done only for the
side effect of incrementing the variables look totally backwards.
Although I do not deeply care. Just leaving the new one as you originally
wrote, i.e.
c=$((++c))
would have been easier to review for the area experts, I would think.
^ permalink raw reply
* Re: [PATCH] cherry-pick: No advice to commit if --no-commit
From: Jonathan Nieder @ 2012-02-21 22:23 UTC (permalink / raw)
To: Phil Hord; +Cc: Git List, Junio C Hamano, Ramkumar Ramachandra, Phil Hord
In-Reply-To: <1329858317-3066-1-git-send-email-hordp@cisco.com>
Hi Phil,
Phil Hord wrote:
> Signed-off-by: Phil Hord <hordp@cisco.com>
Thanks.
> +++ b/sequencer.c
[...]
> @@ -138,10 +138,12 @@ static void print_advice(int show_hint)
> return;
> }
>
> - if (show_hint)
> + if (show_hint) {
> advise(_("after resolving the conflicts, mark the corrected paths\n"
> - "with 'git add <paths>' or 'git rm <paths>'\n"
> - "and commit the result with 'git commit'"));
> + "with 'git add <paths>' or 'git rm <paths>'"));
> + if (!opts->no_commit)
> + advise(_( "and commit the result with 'git commit'"));
"cherry-pick --no-commit" was not about to commit, but the user might
have been. I think the hint is intended to convey that authorship
will be correctly preserved if the user continues with "git commit"
and no special -c option is necessary.
Could you say a little more about the motivation for this patch? For
example, did the existing message confuse someone, or was it grating
in the context of some particular workflow?
A smaller detail: splitting the message into two like this gives
translators less control over how to phrase the message and where to
wrap lines. Luckily that is easy to fix with
if (opts->no_commit)
advise(...);
else
advise(...);
which means more flexibility in phrasing the message with pertinent
advice for each case. ;-)
Ciao,
Jonathan
^ permalink raw reply
* Re: [PATCH] cherry-pick: No advice to commit if --no-commit
From: Junio C Hamano @ 2012-02-21 22:31 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Phil Hord, Git List, Ramkumar Ramachandra, Phil Hord
In-Reply-To: <20120221222049.GA31934@burratino>
Jonathan Nieder <jrnieder@gmail.com> writes:
>> + if (show_hint) {
>> advise(_("after resolving the conflicts, mark the corrected paths\n"
>> + "with 'git add <paths>' or 'git rm <paths>'"));
>> + if (!opts->no_commit)
>> + advise(_( "and commit the result with 'git commit'"));
>
> "cherry-pick --no-commit" was not about to commit, but the user might
> have been. I think the hint is intended to convey that authorship
> will be correctly preserved if the user continues with "git commit"
> and no special -c option is necessary.
Actually, an often used but perhaps neglected use case of "cherry-pick --no-commit"
is to use it as a substitute for
git show $that_commit | git apply [--index]
to lift the change from $that_commit and incorporate it into your
unrelated (from the point of view of the author of $that_commit) work.
The user would be doing "edit $this_file" and "edit $that_file" before or
after the "show | apply" aka "cherry-pick --no-commit", and it is merely
a type-saver, not having to re-type the change $that_commit introduced
relative to its parent.
So in that context, I can understand the suggestion to commit is at best
annoying ("I am still working on the current change. Why do you talk about
committing?") and at worst alarming and misleading ("Oh, am I to lose some
change if I do not commit right now?").
> A smaller detail: splitting the message into two like this gives
> translators less control over how to phrase the message and where to
> wrap lines. Luckily that is easy to fix with
>
> if (opts->no_commit)
> advise(...);
> else
> advise(...);
Yes, this pattern must be followed in the reroll.
Thanks for a review and comment.
^ permalink raw reply
* Re: Working with remotes with (too) many branches
From: Philip Jägenstedt @ 2012-02-21 22:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jonathan Nieder
In-Reply-To: <7v7gzipkt8.fsf@alter.siamese.dyndns.org>
On Sun, Feb 19, 2012 at 22:36, Junio C Hamano <gitster@pobox.com> wrote:
> Philip Jägenstedt <philip@foolip.org> writes:
>
>> 1. If I make a typo with remote set-branches, fetch will fail with
>> "fatal: Couldn't find remote ref refs/heads/typotopic" and not fetch
>> anything at all.
>
> At that point you can notice the earlier typo and remove or fix the fetch
> refspec you have.
>
> Alternatively, set-branches could run ls-remote against the remote and
> notice that there is no such branch over there. However, even if you got
> the branch name right when you did "set-branches", you would still see the
> same "Couldn't find" when the branch gets removed over there, so you would
> need a way to remove or fix the fetch refspec you have *anyway*.
>
> So, assuming that there is no easy way to remove one branch from the set
> of branches tracked from a given remote, it is much more important to add
> such a way. Checking against a typo when "set-branches" is run is "nicer
> to have" but lack of it is not a show-stopper.
>
> Wouldn't "git config --unset remote.origin.fetch '/typotopic'" be
> sufficient in the meantime even if a user fears "vi .git/config"?
Yeah, one can recover if one knows what config "set-branches" maps to,
but I'd like this to be less error-prone so that I can recommend it to
others suffering from branch explosion. Tab completion and
"set-branches --remove" is probably a good start.
>> 2. If I forget that I've previously worked with footopic and
>> set-branches --add it again, I'll get a duplicate line in my config.
>
> I do not know the duplicate hurts anything, but I agree that it would be
> more aescetically pleasing if "--add" noticed what you already had and
> avoided duplicates.
Right, it's not worthwhile by itself, but something to consider if
implementing --delete.
>> 3. When I don't care about footopic anymore, there's no clear way to
>> stop fetching it and remove refs/remotes/main/footopic.
>
> Isn't the lack of "set-branches --delete" the same as #1 above?
Kind of, several of these points can be solved by the same fixes.
> The
> latter would be "branch -r -d main/footopic" but I could imagine
> "set-branches --delete" would do that for you once implemented.
Letting "set-branches --delete" manage this per-branch would be nice, I agree.
Would it be wrong, though, if "fetch --prune" and "remote prune"
simply pruned all refs under refs/remotes/main/ that were not fetched?
It seems to me that if one starts out with all branches and then
"set-branches main footopic", one really does want all other refs to
go away on the next fetch.
>> 4. If set-branches --delete existed one could end up with no fetch
>> lines in the remote config, at which point fetch falls back to
>> fetching HEAD, instead of the expected nothing.
>
> Don't do that, then ;-)
>
> I could imagine a new preference "fetch.$remote.default=nothing" that
> causes "git fetch" to fail with "You have fetch.$remote.default=nothing
> set, so I am not fetching anything from there without any configured
> refspec". The default would be fetch.$remote.default=HEAD, I would guess.
>
> The preference can be set automatically when you use "set-branches"
> without "--add" for a given remote, as use of "set-branches" is a clear
> indication that you want to deviate from the built-in default behaviour
> when interacting with that remote.
That sounds like a good idea. To make sure that I understand the
notation, the actual config would look something like this:
[remote "origin"]
url = git://github.com/gitster/git.git
# no fetch, it was removed
[fetch "origin"]
default = nothing
This looks a bit odd to me, shouldn't the new setting file under
[remote "origin"]?
>> I'd appreciate feedback on these issues so that I don't waste time
>> trying to patch the wrong problems. Suggestions for an alternative
>> work flow is of course also most welcome!
>
> Admittedly I wouldn't use "set-branches" myself (it is far easier to tweak
> with "vi .git/config", and I wasn't involved in), but looking at the whole
> history of the feature with "git log --grep=set-branch builtin/remote.c",
> I have a feeling that not many people used, cut by its still-unrounded
> edges, and polished the subcommand by rounding them out yet, and it has a
> large room for improvement.
That was my impression as well.
> It would have saved you time to wait for a round-trip if you did the above
> "git log" yourself to find out from whom this subcommand came from, and
> looked at list archive to see if the original author is still active here
> (in this case he is), and Cc'ed him before posting the message I am
> responding to.
Thanks for the tip. (I did look at the original commit and search the
archives, but found no answers.)
--
Philip Jägenstedt
^ permalink raw reply
* Re: Working with remotes with (too) many branches
From: Jonathan Nieder @ 2012-02-21 22:43 UTC (permalink / raw)
To: Philip Jägenstedt; +Cc: Junio C Hamano, git, martin f. krafft
In-Reply-To: <CAKHWUkY_bEtKoPOiWFz6i2bp-tbekapROOaCYR0k5Ymh0HYMdA@mail.gmail.com>
Hi Philip,
Philip Jägenstedt wrote:
> It seems to me that if one starts out with all branches and then
> "set-branches main footopic", one really does want all other refs to
> go away on the next fetch.
I would expect set-branches to remove the remote-tracking branches
that are no longer relevant itself, actually.
Martin, does this seem sensible to you? If so, I'll be happy to look
into it in the next few days (or I'll be even happier someone else
gets to it first).
Hope that helps,
Jonathan
^ permalink raw reply
* Re: Working with remotes with (too) many branches
From: Junio C Hamano @ 2012-02-21 22:47 UTC (permalink / raw)
To: Philip Jägenstedt; +Cc: Junio C Hamano, git, Jonathan Nieder
In-Reply-To: <CAKHWUkY_bEtKoPOiWFz6i2bp-tbekapROOaCYR0k5Ymh0HYMdA@mail.gmail.com>
Philip Jägenstedt <philip@foolip.org> writes:
> This looks a bit odd to me, shouldn't the new setting file under
> [remote "origin"]?
Yeah, remote.$name.fetchDefault or something.
> Thanks for the tip. (I did look at the original commit and search the
> archives, but found no answers.)
Heh, it does say --delete # not implemented ;-)
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox