Re: [PATCH 1/3] builtin/fetch.c: Add pretty_url() and print_url()
On Wed, Dec 18, 2013 at 7:18 PM, Tom Miller wrote: > On Wed, Dec 18, 2013 at 3:47 PM, Junio C Hamano wrote: >> Tom Miller writes: >> >>> In order to fix branchname DF conflicts during `fetch --prune`, the way >>> the header is output to the screen needs to be refactored. Here is an >>> exmaple of the output with the line in question denoted by '>': >>> >>> $ git fetch --prune --dry-run upstream From https://github.com/git/git >>> a155a5f..5512ac5 maint -> upstream/maint >>> d7aced9..7794a68 master -> upstream/master >>> 523f7c4..3e57c29 next -> upstream/next >>>+ 462f102...0937cdf pu -> upstream/pu (forced update) >>> e24105a..5d352bc todo -> upstream/todo >>>* [new tag] v1.8.5.2 -> v1.8.5.2 >>>* [new tag] v1.8.5.2 -> v1.8.5.2 >>> >>> pretty_url(): >>> This function when passed a transport url will anonymize the transport >>> of the url. It will strip a trailing '/'. It will also strip a trailing >>> '.git'. It will return the newly formated url for use. I do not believe >>> there is a need for stripping the trailing '/' and '.git' from a url, >>> but it was already there and I wanted to make as little changes as >>> possible. >> >> OK. I tend to agree that stripping the trailing part is probably >> not a good idea and we would want to remove that but that definitely >> should be done as a separate step, or even as a separate series on >> top of this one. > > I think that removing the trailing part will greatly reduce the complexity > to the point were it is unnecessary to have pretty_url(). My goal with > extracting this function is to isolate the complexity of formatting the > url to a single spot. I am thinking along the lines of the following > commit order: > > 1. Remove the "remove trailing part" > 2. Add print_url() > 3. Always print url when pruning > 4. Reverse order of prune and fetch > >>> print_url(): >>> This function will convert a transport url to a pretty url using >>> pretty_url(). Then it will print out the pretty url to stderr as >>> indicated above in the example output. It uses a global variable >>> named "gshown_url' to prevent this header for being printed twice. >> >> Gaah. What is that 'g' doing there? Please don't do that >> meaningless naming. > > I am not familiar with C conventions and I was trying to stay consistent. > I saw other global variables starting with 'g' and made an assumption. > It will use the original name in the upcoming patches. > >> I do not think the change to introduce such a global variable >> belongs to this refactoring step. The current caller can decide >> itself if it called that function, and if you are going to introduce >> new callers in later steps, they can coordinate among themselves, >> no? > > I agree, there is no reason for introducing it in this step. Thanks for > pointing that out. After working on this some more and realizing there is more work to be done on the "fetch --prune should prune before fetching" issue. Also, seeing Jeff's response opened my eyes even more. I believe you are correct. The "trailing parts" piece should be split off into another patch set. I think it would make sense to add the "fetch --prune should print the header url" to that patch set. Should I submit those patches as a separate thread or reply to this thread with just those patches? -- Tom Miller jacker...@gmail.com -- 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: [PATCH/WIP] Repair DF conflicts during fetch.
On Fri, Nov 29, 2013 at 1:07 PM, Thomas Rast wrote:
> Tom Miller writes:
>
>> When a DF conflict occurs during a fetch, --prune should be able to fix
>> it. When fetching with --prune, the fetching process happens before
>> pruning causing the DF conflict to persist and report an error. This
>> patch prunes before fetching, thus correcting DF conflicts during a
>> fetch.
>>
>> Signed-off-by: Tom Miller
>> ---
>> builtin/fetch.c | 10 +-
>> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> Good catch.
>
> I can't comment on the correctness of the patch right now, but here's a
> test you could steal. It just reproduces what you describe, and I did
> verify that it confirms the fix ;-)
>
> diff --git i/t/t5510-fetch.sh w/t/t5510-fetch.sh
> index 5d4581d..a981125 100755
> --- i/t/t5510-fetch.sh
> +++ w/t/t5510-fetch.sh
> @@ -614,4 +614,18 @@ test_expect_success 'all boundary commits are excluded' '
> test_bundle_object_count .git/objects/pack/pack-${pack##pack
> }.pack 3
> '
>
> +test_expect_success 'branchname D/F conflict resolved by --prune' '
> + git branch dir/file &&
> + git clone . prune-df-conflict &&
> + git branch -D dir/file &&
> + git branch dir &&
> + (
> + cd prune-df-conflict &&
> + git fetch --prune &&
> + git rev-parse origin/dir >../actual
> + ) &&
> + git rev-parse dir >expect &&
> + test_cmp expect actual
> +'
> +
> test_done
>
>
> --
> Thomas Rast
> t...@thomasrast.ch
Thanks, I appreciate the test. I have added it and gave credit via a
"Tested-by" section. I have been looking into adding a pruning header
to "fix" the output, but that is just the first solution I've been able to
come up with. I believe before I have an elegant solution I'll have to
read the code more carefully and brush up on my C.
Thanks,
Tom Miller
PS. I apologize for the duplicate message the mailing list rejected my
first for not being plaintext only.
--
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
