No arguments behaviour

[WalterSmuts]

Is there any reason to require a <revision> argument. I would expect the tool to default to the root without a limit.

Great tool BTW :)

[krobelus]

A better default would be @{upstream} but that's not perfect either (doesn't work during rebase or if tracking branch is not the true upstream).
We don't want to create fixups for commits that have already been merged because we don't want to change those.

[torbiak]

krobelus brings up some great points. Also, I wanted users to think about what git-autofixup is doing and choose a start revision intentionally, since the choice can significantly impact both how long it takes the tool to run (a blame has be generated for every commit) and what fixup commits it creates. I imagine the most common case is for people to be rewriting the history of their whole topic branch, but maybe you only want to distribute new changes across the most recent few commits. I haven't actually used git-autofixup much for several years, but when I did I selected more specific revisions often enough that I was never motivated to set a default or even make an alias in my shell for using @{upstream}.

Still, it might be worth adding @{upstream} as the default. It's kinda surprising nobody asked about it previously. Does everyone have a shell alias for using @{upstream}, or uses it though magit? @WalterSmuts, what have you been using as the start revision?

UPDATE: I made it sound like git blame has to be run for each commit in <rev>.., but actually it only has to be run for each relevant file. The given revision does affect how much work git blame does, though, since we can tell it to avoid searching past the given revision using a grafts file.

[WalterSmuts]

I support @krobelus 's suggestion of defaulting to @{upstream}.

@WalterSmuts, what have you been using as the start revision?

Previously I've used HEAD~10 (10 being somewhat arbitrary). My thought about defaulting to root was that there is still the stage during the interactive rebase where the programmer can inspect and discard the fixups that don't make sense. Generally I think any fixups earlier than @{upstream} will likely be to be discarded and just extra work during the interactive rebase stage so @{upstream} sounds like a good limit.

Might even be better to do the most common ancestor between HEAD and @{upstream}. This would support the case where @{upstream} is a parent of HEAD but wouldn't attempt to rebase if @{upstream} has moved via a git fetch. The thought here is that the user of git-autofixup likely did not mean to initiate work of rebasing unto the new upstream, possibly having to resolve merge conflicts, but rather wants to cleanup the fixups and only the fixups.

[krobelus]

let's say we're working on branch a with upstream is origin/b.
The default of @{upstream}.. means that we only consider commits as potential fixup targets if they are new on a.
Now if a git fetch finds that origin/b has been force-pushed, the commit range @{upstream}.. will also include the old upstream commits that our branch still includes (before a rebase).
The solution is to use git merge-base --fork-point @{upstream} HEAD, then we'll only target commits that are actually from a.

---

now for figuring out the upstream branch during interactive rebase, I've been using this logic:

upstream='@{upstream}'

gitdir=$(git rev-parse --git-dir)
if test -d "$gitdir"/rebase-merge; then
	branch=$(cat "$gitdir"/rebase-merge/head-name)
	branch=${branch#refs/heads/}
	remote=$(
		git config branch."$branch".remote ||
		git config branch."$branch".pushRemote ||
		git config remote.pushDefault ||
		echo origin
	)

	# By default the upstream branch has the same name.
	if [ "$remote" != . ]; then
		upstream="$remote/$branch"
	else
		upstream="$branch"
	fi

	merge=$(git config branch."$branch".merge)
	merge=${merge#refs/heads/}
	if [ "$remote" != . ]; then
		merge="$remote/$merge"
	fi
	if git rev-parse "$merge" >/dev/null 2>&1; then
		upstream=$merge
	fi
fi

and then pass git merge-base --fork-point "$upstream" HEAD to git-autofixup

I think the logic is correct; probably there are some outlandish scenarios it doesn't support

[torbiak]

@krobelus Cool, I never thought to use autofixup during an interactive rebase, but I can imagine it being quite useful. I'll translate your logic to perl.

[krobelus]

actually there's a simpler way:

upstream='@{upstream}'

gitdir=$(git rev-parse --git-dir)
if test -d "$gitdir"/rebase-merge; then
	branch=$(cat "$gitdir"/rebase-merge/head-name)
	branch=${branch#refs/heads/}
	upstream="$branch@{upstream}"
fi
merge_bases=$(git merge-base --all --fork-point "$upstream" HEAD 2>/dev/null)
if [ -n "$merge_bases" ]; then
    printf HEAD
    # in case there are multiple merge bases, we want to avoid targeting any of their commits
    printf " ^%s" $merge_bases
fi

Thanks for taking care of that!

[krobelus]

hmm so apparently merge-base --fork-point might not find any merge base (but merge-base does in my case).
So we might want it as fallback. The scenarios where --fork-point adds value are edge cases but it seems like a strict improvement.

[torbiak]

Yeah, good point. I saw that in the merge-base manpage, but I haven't gotten to writing a test for it yet.

[torbiak]

#28 covers almost all the cases I can think of. The one exception is that I doubt it's worth adding special handling for when there's multiple branch.<name>.merge values.

It'd be great to hear your guys' thoughts if anything doesn't work how you'd expect.